Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Review: IsSwipeNavigationEnabled.md #1400

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

tofuandeve
Copy link
Contributor

This is a review for the new IsSwipeNavigationEnabled API.

@tofuandeve tofuandeve added the API Proposal Review WebView2 API Proposal for review. label Jun 14, 2021

# Description
The default value for `IsSwipeNavigationEnabled` is `TRUE`.
When this setting is set to `FALSE`, it disables the ability of the end users to use swiping gesture on touch input enabled devices to trigger navigations such as swipe left/right to go back/forward or pull to refresh current page.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Navigation and refresh are distinct, apps might want to disable one but enable the other. Why block out that scenario?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tofuandeve Do we have customers using pull to refresh? Do we expect this could be a problem in the future?

Also, is it implemented this way because chromium has just one setting to control both back/forward swipes and refresh swipes?

We might say that since pull to refresh is off by default and not exposed via our API currently, that if we receive feedback about it, we can add a IsPullToRefreshEnabled setting that is not impacted by the setting we're adding now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Navigation and refresh are distinct

That's why I asked the clarifying question later about whether "Refresh" is considered a navigation by WebView2. The answer is "Yes" (effectively "navigate to the same place you already are"), in which case saying just "navigation" is internally consistent with the rest of WebView2.

So there's this general concept of navigation in WebView2, with different flavors based on the effect on the navigation history.

  • Back: Pop off the navigation history.
  • Forward: Push onto the navigation history.
  • Refresh: No change to navigation history.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MikeHillberg Refresh is considered a navigation in WebView2. As @david-risney mentioned above, swipe left/right to go back/forward and pull to refresh are bundled up together under overscroll controler in Chromium. We currently have some customers asked to be able to disable both swipe and pull to refresh.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be too boolean properties though, which would make it possible to disable both or just one. I'm not trying to add features, but worried that someone will ask for only one and this API won't allow for it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So right now IsSwipeNavigationEnabled will disable PullToRefresh, and then maybe in the future it won't. Is that the kind of behavior breaking change we can't make?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@david-risney hm, I need your thoughts on this please?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull to refresh isn't in our API surface yet. Its off by default and the end dev needs to use a command line switch to enable it. Its not really discoverable and I'm not worried about high adoption of that feature. Regardless we can support compat for this in the future if we want to properly support pull to refresh we can add a bool CoreWebView2Settings.IsPullToRefreshEnabled (that defaults to false). We can have the command line switch work as is (IsSwipeNavigationEnabled affects it) and the new actual API we add IsPullToRefreshEnabled is independent of IsSwipeNavigationEnabled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry if I'm not following, but it sounds like you're still saying that IsSwipeNavigationEnabled today will affect PTR. But in the future we might change that behavior, it will no longer affect PTR, and if want to control PRT plesae use IsPullToRefreshEnabled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm saying PTR is disabled by default. Today if you use the command line to enable PTR you also need to set IsSwipeNavigationEnabled to true. If we add a new API to enable PTR, that new API doesn't have to require the command line or IsSwipeNavigationEnabled being set.

specs/IsSwipeNavigationEnabled.md Outdated Show resolved Hide resolved
specs/IsSwipeNavigationEnabled.md Outdated Show resolved Hide resolved

# Description
The default value for `IsSwipeNavigationEnabled` is `TRUE`.
When this setting is set to `FALSE`, it disables the ability of the end users to use swiping gesture on touch input enabled devices to trigger navigations such as swipe left/right to go back/forward or pull to refresh current page.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tofuandeve Do we have customers using pull to refresh? Do we expect this could be a problem in the future?

Also, is it implemented this way because chromium has just one setting to control both back/forward swipes and refresh swipes?

We might say that since pull to refresh is off by default and not exposed via our API currently, that if we receive feedback about it, we can add a IsPullToRefreshEnabled setting that is not impacted by the setting we're adding now.

@david-risney david-risney added the review completed WebView2 API Proposal that's been reviewed and now needs final update and push label Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Proposal Review WebView2 API Proposal for review. review completed WebView2 API Proposal that's been reviewed and now needs final update and push
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants