Implement viewport-width-based fast-click heuristic

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Wenson Hsieh]

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.

[Wenson Hsieh]

<rdar://problem/23267308>

[Wenson Hsieh]

Created attachment 264163 [details]
Patch

[Simon Fraser (smfr)]

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?

[Wenson Hsieh]

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.

[Wenson Hsieh]

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.

[Wenson Hsieh]

Committed r191644: <http://trac.webkit.org/changeset/191644>

[Wenson Hsieh]

*** Bug 150469 has been marked as a duplicate of this bug. ***

[Patrick H. Lauke]

Just wanted to mention how happy it makes me to see this. Great stuff!

[hexalys]

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.

[Wenson Hsieh]

(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.

[Chris Rebert]

The blog post: https://webkit.org/blog/5610/more-responsive-tapping-on-ios/

[hexalys]

(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.

[André Hänsel]

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?

[Jon Lee]

(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?

[André Hänsel]

Done: https://bugs.webkit.org/show_bug.cgi?id=153402