Bug 150248

Summary: Extend fast-clicking behavior to trigger on elements that have negligible zoom
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: WebKit Misc.Assignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, ggaren, jonlee, redux, sam, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 122212    
Attachments:
Description Flags
Test page for zoom-dependent fast clicking
none
work in progress v1
none
Patch simon.fraser: review+

Wenson Hsieh
Reported 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.
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
DetailsFormatted DiffDiff
Patch (31.59 KB, patch)
2015-10-17 23:48 PDT, Wenson Hsieh 		simon.fraser: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Wenson Hsieh
Comment 1 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.
Wenson Hsieh
Comment 2 2015-10-16 13:18:48 PDT
<rdar://problem/23140069>
Wenson Hsieh
Comment 3 2015-10-16 15:28:18 PDT
Created attachment 263340 [details] work in progress v1
WebKit Commit Bot
Comment 4 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.
Wenson Hsieh
Comment 5 2015-10-17 23:48:45 PDT
Created attachment 263412 [details] Patch
WebKit Commit Bot
Comment 6 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.
Simon Fraser (smfr)
Comment 7 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&
Wenson Hsieh
Comment 8 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.
Wenson Hsieh
Comment 9 2015-10-19 12:54:23 PDT
Committed r191309: <http://trac.webkit.org/changeset/191309>
Note You need to log in before you can comment on or make changes to this bug.