Bug 150248 - Extend fast-clicking behavior to trigger on elements that have negligible zoom
Summary: Extend fast-clicking behavior to trigger on elements that have negligible zoom
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks: 122212
  Show dependency treegraph
 
Reported: 2015-10-16 13:12 PDT by Wenson Hsieh
Modified: 2015-10-22 09:12 PDT (History)
8 users (show)

See Also:


Attachments
Test page for zoom-dependent fast clicking (1.60 KB, text/html)
2015-10-16 13:12 PDT, Wenson Hsieh
no flags Details
work in progress v1 (30.59 KB, patch)
2015-10-16 15:28 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (31.59 KB, patch)
2015-10-17 23:48 PDT, Wenson Hsieh
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2015-10-16 13:12:41 PDT
Created attachment 263313 [details]
Test page for zoom-dependent fast clicking

We will want websites with scalable viewports to also benefit from fast-click behavior when applicable. One idea is to use the double-tap zoom factor as a heuristic to determine whether an element should quickly fire a click when tapped or wait for the double tap gesture to fail. If double tapping on an element will not cause the viewport to zoom very much, we should make single taps on it trigger fast clicking, since double-tapping will offer little value.
Comment 1 Wenson Hsieh 2015-10-16 13:14:14 PDT
In regards to the test page: double tapping the top button should trigger zoom; double tapping the bottom button should fire clicks, since double-tap to zoom won't do much for us there anyways.
Comment 2 Wenson Hsieh 2015-10-16 13:18:48 PDT
<rdar://problem/23140069>
Comment 3 Wenson Hsieh 2015-10-16 15:28:18 PDT
Created attachment 263340 [details]
work in progress v1
Comment 4 WebKit Commit Bot 2015-10-16 15:30:45 PDT
Attachment 263340 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:741:  The parameter name "!" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:772:  The parameter name "!" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Wenson Hsieh 2015-10-17 23:48:45 PDT
Created attachment 263412 [details]
Patch
Comment 6 WebKit Commit Bot 2015-10-17 23:51:21 PDT
Attachment 263412 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:834:  The parameter name "!" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:877:  The parameter name "!" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Simon Fraser (smfr) 2015-10-19 11:14:59 PDT
Comment on attachment 263412 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=263412&action=review

> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:940
> +void WebPageProxy::disableDoubleTapGesturesUntilTapIsFinishedIfNecessary(uint64_t requestID, bool allowsDoubleTapZoom, const WebCore::FloatRect& targetRect, bool isReplacedElement, double minimumScale, double maximumScale)

The "IfNecessary" is a big vague. Does that just refer to the potential zoom level change?

> Source/WebKit2/WebProcess/WebPage/ViewGestureGeometryCollector.cpp:104
> +void ViewGestureGeometryCollector::computeZoomInformationForNode(Node* node, FloatPoint& origin, FloatRect& renderRect, bool& isReplaced, double& viewportMinimumScale, double& viewportMaximumScale)

Node&
Comment 8 Wenson Hsieh 2015-10-19 12:10:00 PDT
Comment on attachment 263412 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=263412&action=review

Thanks for the review!

>> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:940
>> +void WebPageProxy::disableDoubleTapGesturesUntilTapIsFinishedIfNecessary(uint64_t requestID, bool allowsDoubleTapZoom, const WebCore::FloatRect& targetRect, bool isReplacedElement, double minimumScale, double maximumScale)
> 
> The "IfNecessary" is a big vague. Does that just refer to the potential zoom level change?

Yes -- we could potentially bail from disabling double tap gestures if the expected change in zoom level is too much. However, I hesitated when naming it something more specific like "IfZoomingIsNegligible" since the touch-action: manipulation patch will also use this codepath, and in that case a significant amount of zoom level change is overridden by having touch-action: manipulation on the element, so I thought "IfNecessary" would better capture both cases.

>> Source/WebKit2/WebProcess/WebPage/ViewGestureGeometryCollector.cpp:104
>> +void ViewGestureGeometryCollector::computeZoomInformationForNode(Node* node, FloatPoint& origin, FloatRect& renderRect, bool& isReplaced, double& viewportMinimumScale, double& viewportMaximumScale)
> 
> Node&

Got it, fixed. Since we never expect node to be null here, the reference makes a lot more sense.
Comment 9 Wenson Hsieh 2015-10-19 12:54:23 PDT
Committed r191309: <http://trac.webkit.org/changeset/191309>