Bug 150604

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 Flags
Patch simon.fraser: review+

Description Wenson Hsieh 2015-10-27 14:22:30 PDT
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.
Comment 1 Wenson Hsieh 2015-10-27 14:23:27 PDT
<rdar://problem/23267308>
Comment 2 Wenson Hsieh 2015-10-27 14:49:18 PDT
Created attachment 264163 [details]
Patch
Comment 3 Simon Fraser (smfr) 2015-10-27 15:27:55 PDT
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 4 Wenson Hsieh 2015-10-27 15:40:38 PDT
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 5 Wenson Hsieh 2015-10-27 16:26:46 PDT
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.
Comment 6 Wenson Hsieh 2015-10-27 16:35:39 PDT
Committed r191644: <http://trac.webkit.org/changeset/191644>
Comment 7 Wenson Hsieh 2015-10-27 17:01:28 PDT
*** Bug 150469 has been marked as a duplicate of this bug. ***
Comment 8 Patrick H. Lauke 2015-10-28 00:19:54 PDT
Just wanted to mention how happy it makes me to see this. Great stuff!
Comment 9 hexalys 2015-12-15 13:48:08 PST
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.
Comment 10 Wenson Hsieh 2015-12-15 13:59:44 PST
(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.
Comment 11 Chris Rebert 2015-12-15 14:01:56 PST
The blog post: https://webkit.org/blog/5610/more-responsive-tapping-on-ios/
Comment 12 hexalys 2015-12-15 14:34:37 PST
(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.
Comment 13 André Hänsel 2016-01-15 21:00:07 PST
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?
Comment 14 Jon Lee 2016-01-19 00:12:41 PST
(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?
Comment 15 André Hänsel 2016-01-23 19:53:32 PST
Done: https://bugs.webkit.org/show_bug.cgi?id=153402