Summary: | Implement viewport-width-based fast-click heuristic | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||
Component: | WebKit Misc. | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | 18runt88, andre, benjamin, bruno, dpcalhoun, jonlee, redux, simon.fraser, webkit-bug-importer, webkit | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | iPhone / iPad | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 122212 | ||||||
Attachments: |
|
Description
Wenson Hsieh
2015-10-27 14:22:30 PDT
Created attachment 264163 [details]
Patch
Comment on attachment 264163 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264163&action=review Looks good but I would like to see a new version that removes some of the NSUserDefaults calls. > Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.h:231 > + double viewportWidth() const { return m_viewportWidth; } > + void setViewportWidth(double width) { m_viewportWidth = width; } This name here is too ambiguous I think. We use the term "viewport" in a number of ways, but this one specifically applies to the contents of the viewport meta tag, right? > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:3080 > + if ([[NSUserDefaults standardUserDefaults] boolForKey:@"WebKitFastClickingDisabled"]) > + return YES; I think you should cache this pref and not check it every time. > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewInternal.h:112 > +@property (nonatomic, readonly) BOOL _allowDoubleTapGestures; This would be better as _allowsDoubleTapGestures > Source/WebKit2/UIProcess/PageClient.h:297 > + virtual void disableDoubleTapGesturesDuringTapIfNotAlreadyDisabled(uint64_t requestID) = 0; Is the "IfNotAlreadyDisabled" useful? > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.h:186 > +- (BOOL)_singleTapMayModifyDoubleTapGesturesEnabled; This needs a new name that is easier to parse. > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:836 > + return [[NSUserDefaults standardUserDefaults] boolForKey:@"WebKitShowFastClickDebugTapHighlights"]; Not a fan of consulting prefs several times on every link tap. Should these be WK2 prefs? Comment on attachment 264163 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264163&action=review >> Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.h:231 >> + void setViewportWidth(double width) { m_viewportWidth = width; } > > This name here is too ambiguous I think. We use the term "viewport" in a number of ways, but this one specifically applies to the contents of the viewport meta tag, right? Yes -- good point. viewportMetaTagWidth would be a more appropriate name, especially since it can take on values like ViewportArguments::ValueDeviceWidth (a.k.a. -2) >> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:3080 >> + return YES; > > I think you should cache this pref and not check it every time. Sounds good -- I'll store the flag. >> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewInternal.h:112 >> +@property (nonatomic, readonly) BOOL _allowDoubleTapGestures; > > This would be better as _allowsDoubleTapGestures Renamed. >> Source/WebKit2/UIProcess/PageClient.h:297 >> + virtual void disableDoubleTapGesturesDuringTapIfNotAlreadyDisabled(uint64_t requestID) = 0; > > Is the "IfNotAlreadyDisabled" useful? It was originally "IfNecessary", but I thought that might be a bit too vague -- I'll change it back (I think it's important to keep the "IfNecessary" though, since we will ignore the message in the UI process if it's redundant, e.g. touch-action: manipulation in a device-width viewport). >> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.h:186 >> +- (BOOL)_singleTapMayModifyDoubleTapGesturesEnabled; > > This needs a new name that is easier to parse. Agreed -- maybe _mayDisableDoubleTapGesturesDuringSingleTap? >> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:836 >> + return [[NSUserDefaults standardUserDefaults] boolForKey:@"WebKitShowFastClickDebugTapHighlights"]; > > Not a fan of consulting prefs several times on every link tap. Should these be WK2 prefs? Will do. Comment on attachment 264163 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264163&action=review >>> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:836 >>> + return [[NSUserDefaults standardUserDefaults] boolForKey:@"WebKitShowFastClickDebugTapHighlights"]; >> >> Not a fan of consulting prefs several times on every link tap. Should these be WK2 prefs? > > Will do. (as in, will cache the values for now). If we decide to go with this heuristic, I'll make a new patch that puts all the relevant fast-clicking options into WKPreferences. Committed r191644: <http://trac.webkit.org/changeset/191644> *** Bug 150469 has been marked as a duplicate of this bug. *** Just wanted to mention how happy it makes me to see this. Great stuff! I am noting that the webkit blog post's language and this bug appear specific to `device-with` only in terms of width qualifying for the fastclick behavior. Does or should the fast-click optimization cover a 'custom-viewport-width < viewport-width' as well? If not already covered, I think that a viewport or 'width=300,initial-scale=1.0' should also assume the fast-tap behavior. (In reply to comment #9) > I am noting that the webkit blog post's language and this bug appear > specific to `device-with` only in terms of width qualifying for the > fastclick behavior. Does or should the fast-click optimization cover a > 'custom-viewport-width < viewport-width' as well? > > If not already covered, I think that a viewport or > 'width=300,initial-scale=1.0' should also assume the fast-tap behavior. Currently, we only support fast-clicking for scalable viewports at width=device-width. We experimented with extending this to width < device-width as well, but were concerned that the ability to fast-click would differ when changing device orientation, which would be confusing to some users. We are considering ways to further extend fast-clicking behavior to scalable web pages. One way would be to do what you described, but instead of using device-width, use the smaller of the two device dimensions (width vs. height) to make behavior consistent across orientations. The blog post: https://webkit.org/blog/5610/more-responsive-tapping-on-ios/ (In reply to comment #10) > (In reply to comment #9) > Currently, we only support fast-clicking for scalable viewports at > width=device-width. We experimented with extending this to width < > device-width as well, but were concerned that the ability to fast-click > would differ when changing device orientation, which would be confusing to > some users. OK, makes sense in that respect. It could however apply during a screen lock. Or Screen.lockOrientation() context whenever it gets implemented. I just noticed that on some of my pages the double-tap-and-hold gesture is broken. I use this gesture a lot because pinch-zoom is difficult (and dangerous if you're walking next to a drain :)) with one hand on large devices. While analyzing the issue I found that it seems to be only on pages that have width=device-width set, which led me to this bug. Shouldn't the zoom functionality not only be removed from clickable elements? So if a tap hits no element that is a link, has an onclick handler bound or a focus-sensitive pseudoclass - basically if a tap has no effect - the zoom should still work after a subsequent tap-and-hold? (In reply to comment #13) > I just noticed that on some of my pages the double-tap-and-hold gesture is > broken. I use this gesture a lot because pinch-zoom is difficult (and > dangerous if you're walking next to a drain :)) with one hand on large > devices. > > While analyzing the issue I found that it seems to be only on pages that > have width=device-width set, which led me to this bug. > > Shouldn't the zoom functionality not only be removed from clickable elements? > > So if a tap hits no element that is a link, has an onclick handler bound or > a focus-sensitive pseudoclass - basically if a tap has no effect - the zoom > should still work after a subsequent tap-and-hold? Could you file a new bug for this, and include an example showing the issue? |