-
Notifications
You must be signed in to change notification settings - Fork 54
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
tofuandeve
wants to merge
4
commits into
main
Choose a base branch
from
api-swipe-navigation
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
# Background | ||
|
||
Swiping navigation on touch screen includes: | ||
1. Swipe left/right (swipe horizontally) to navigate to previous/next page in navigation history. | ||
1. Pull to refresh (swipe vertically) the current page. | ||
|
||
In response to customer requests, the WebView2 team has introduced this Swipe Navigation API which allows developers to disable or enable the the ability of end users to use swiping gesture to navigate in WebView via a setting. | ||
|
||
In this document we describe the new setting. We'd appreciate your feedback. | ||
|
||
# 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. | ||
Disabling/Enabling `IsSwipeNavigationEnabled` does not take effect until the next navigation. | ||
|
||
|
||
# Examples | ||
```cpp | ||
wil::com_ptr<ICoreWebView2> webView; | ||
void SettingsComponent::ToggleSwipeNavigationEnabled() | ||
{ | ||
// Get webView's current settings | ||
wil::com_ptr<ICoreWebView2Settings> coreWebView2Settings; | ||
CHECK_FAILURE(webView->get_Settings(&coreWebView2Settings)); | ||
|
||
wil::com_ptr<ICoreWebView2Settings6> coreWebView2Settings6; | ||
coreWebView2Settings6 = coreWebView2Settings.try_query<ICoreWebView2Settings6>(); | ||
if(coreWebView2Settings6) | ||
{ | ||
BOOL enabled; | ||
CHECK_FAILURE(coreWebView2Settings6->get_IsSwipeNavigationEnabled(&enabled)); | ||
CHECK_FAILURE(coreWebView2Settings6->put_IsSwipeNavigationEnabled(enabled ? FALSE : TRUE)); | ||
} | ||
} | ||
``` | ||
|
||
```c# | ||
private WebView2 _webView; | ||
void ToggleSwipeNavigationEnabled() | ||
{ | ||
var coreWebView2Settings = _webView.CoreWebView2.Settings; | ||
coreWebView2Settings.IsSwipeNavigationEnabled = !coreWebView2Settings.IsSwipeNavigationEnabled; | ||
} | ||
``` | ||
|
||
# Remarks | ||
This API only affects the decision whether to trigger navigations of the main html page when end users perform overscrolling motions by swiping and has no effect on the scrolling interaction used to explore the web content shown in WebView2 or the CSS overscroll-behavior property. | ||
|
||
# API Notes | ||
|
||
See [API Details](#api-details) section below for API reference. | ||
|
||
# API Details | ||
|
||
## Win32 C++ | ||
```cpp | ||
[uuid(45b1f964-f703-47ac-a19a-b589dd0c5559), object, pointer_default(unique)] | ||
interface ICoreWebView2Settings6 : ICoreWebView2Settings5 { | ||
/// The `IsSwipeNavigationEnabled` property enables or disables the ability of the | ||
/// end user to use swiping gesture on touch input enabled devices to | ||
/// navigate in WebView2. It defaults to `TRUE`. | ||
/// | ||
/// When this property is TRUE, then all configured navigation gestures are enabled: | ||
/// 1. Swiping left and right to navigate forward and backward is always configured. | ||
/// 2. Swiping down to refresh is off by default and not exposed via our API currently, | ||
/// it requires the --pull-to-refresh option to be included in the additional browser | ||
/// arguments to be configured. (See put_AdditionalBrowserArguments.) | ||
/// | ||
/// When set to `FALSE`, the end user cannot swipe to navigate or pull to refresh. | ||
/// This API only affects the overscrolling navigation functionality and has no | ||
/// effect on the scrolling interaction used to explore the web content shown | ||
/// in WebView2. | ||
/// | ||
/// Disabling/Enabling IsSwipeNavigationEnabled takes effect after the | ||
/// next navigation. | ||
/// | ||
/// \snippet SettingsComponent.cpp ToggleSwipeNavigationEnabled | ||
[propget] HRESULT IsSwipeNavigationEnabled([out, retval] BOOL* enabled); | ||
/// Set the `IsSwipeNavigationEnabled` property | ||
[propput] HRESULT IsSwipeNavigationEnabled([in] BOOL enabled); | ||
} | ||
``` | ||
|
||
## .NET and WinRT | ||
|
||
```c# | ||
namespace Microsoft.Web.WebView2.Core | ||
{ | ||
public partial class CoreWebView2Settings | ||
{ | ||
|
||
public bool IsSwipeNavigationEnabled { get; set; }; | ||
|
||
} | ||
} | ||
|
||
``` |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 useIsPullToRefreshEnabled
.There was a problem hiding this comment.
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.