Remove the experimental scroll/zoom-based heuristics for fast-clicking and replace it with a heuristic based on viewport width and current zoom scale. If the viewport width is equal to the device width and the user has not zoomed yet (i.e. at initial scale) then fire fast clicks instead of double taps. In all other cases, perform the old behavior. To make this optimization useful for width = device-width websites with scalable viewports, we also make double-tap-to-zoom-out change the viewport scale to be the initial scale rather than the min scale.
<rdar://problem/23267308>
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?
Done: https://bugs.webkit.org/show_bug.cgi?id=153402