Bug 195473 - [iOS] Implement a faster click detection that intercepts double-tap-to-zoom if possible
Summary: [iOS] Implement a faster click detection that intercepts double-tap-to-zoom i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-08 10:59 PST by Dean Jackson
Modified: 2020-06-12 10:03 PDT (History)
8 users (show)

See Also:


Attachments
Patch (59.42 KB, patch)
2019-03-08 11:28 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.85 MB, application/zip)
2019-03-08 13:33 PST, EWS Watchlist
no flags Details
Patch (58.93 KB, patch)
2019-03-08 13:44 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.80 MB, application/zip)
2019-03-08 15:21 PST, EWS Watchlist
no flags Details
Patch (62.85 KB, patch)
2019-03-11 15:16 PDT, Dean Jackson
wenson_hsieh: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2019-03-08 10:59:31 PST
[iOS] Implement a faster click detection that intercepts double-tap-to-zoom if possible
Comment 1 Radar WebKit Bug Importer 2019-03-08 11:01:28 PST
<rdar://problem/48718396>
Comment 2 Dean Jackson 2019-03-08 11:28:47 PST
Created attachment 364042 [details]
Patch
Comment 3 EWS Watchlist 2019-03-08 13:33:05 PST
Comment on attachment 364042 [details]
Patch

Attachment 364042 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11430140

New failing tests:
http/tests/adClickAttribution/anchor-tag-attributes-validation.html
compositing/video/video-clip-change-src.html
http/tests/security/anchor-download-block-crossorigin.html
Comment 4 EWS Watchlist 2019-03-08 13:33:06 PST
Created attachment 364056 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 5 Dean Jackson 2019-03-08 13:44:31 PST
Created attachment 364058 [details]
Patch
Comment 6 EWS Watchlist 2019-03-08 15:21:17 PST
Comment on attachment 364058 [details]
Patch

Attachment 364058 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11431671

New failing tests:
http/tests/adClickAttribution/anchor-tag-attributes-validation.html
http/tests/security/anchor-download-block-crossorigin.html
Comment 7 EWS Watchlist 2019-03-08 15:21:18 PST
Created attachment 364083 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 8 Daniel Bates 2019-03-09 22:10:39 PST
Comment on attachment 364058 [details]
Patch

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

> Source/WebKit/UIProcess/ios/SmartMagnificationController.mm:90
> +void SmartMagnificationController::calculatePotentialZoomParameters(FloatPoint origin, FloatRect targetRect, bool fitEntireRect, double viewportMinimumScale, double viewportMaximumScale, double& targetScale)

origin is unused. What is going on here?

> Source/WebKit/UIProcess/ios/SmartMagnificationController.mm:92
> +    // FIXME: Much of this code can be shared with didCollectGeometryForSmartMagnificationGesture.

Is this really true? If so, why are you procrastinating? Certainly can extract the local and  adjustSmart...(), but almost seems like a cop out for not fixing adjust....() to return a std::tuple instead of using out-rags. As adjust...() is designed now I do not see much benefit to code sharing :/

> Source/WebKit/UIProcess/ios/SmartMagnificationController.mm:95
> +    adjustSmartMagnificationTargetRectAndZoomScales(!fitEntireRect, targetRect, minimumScale, maximumScale);

I know you didn’t add this function in this patch, but what a mess is the argument order. Doesn’t even match the implied argument order in its name (I.e. first arg should be the rect not a bool).

> Source/WebKit/UIProcess/ios/SmartMagnificationController.mm:101
> +        targetScale = [m_contentView _initialScaleFactor];

Ok as-is. Looking at this code makes me think the name of this function and its single out arg is not good. This function compute the target scale as its only “output”, why not return by value and name this function something like computeTargetScale()?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:232
>  static const float tapAndHoldDelay  = 0.75;

Ok as-is. I know not part of your patch, but it irks me that there looks like an extra space before =. Also, old style and static unnecessary. New hotness constexpr.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:235
> +static const double fasterTapSignificantZoomThreshold = 0.8;

Ok as-is. static is unnecessary. Moreover, this is the old style. New hotness, constexpr.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:618
> +static WTF::WallTime singleTapIdentificationTime;

Is this following the code style guide for naming?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1508
> +    double initialScale = [self _initialScaleFactor];

Auto?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:2210
> +    bool wantsGeometryResponse = _page->preferences().fasterClicksEnabled();

wants....Response? What kind of name is this?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:2270
> +    WTF::Seconds duration = WTF::WallTime::now() - singleTapIdentificationTime;

Auto?

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:819
> +        m_viewGestureGeometryCollector->computeZoomInformationForNode(*m_potentialTapNode, origin, renderRect, fitEntireRect, viewportMinimumScale, viewportMaximumScale);

Ok as-is. Don’t like the design of the called function.
Comment 9 Dean Jackson 2019-03-10 12:44:54 PDT
Thanks for the review Dan!
Comment 10 Wenson Hsieh 2019-03-10 17:53:58 PDT
Comment on attachment 364058 [details]
Patch

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

> Source/WebKit/Platform/Logging.h:55
> +    M(Interaction) \

Nit - this logging category seems a little...too broad :P Maybe we can use the existing ViewGestures category, which is currently only about swipe?

> Source/WebKit/UIProcess/WebPageProxy.messages.in:198
> +    PotentialTapHasGeometry(uint64_t requestID, WebCore::FloatPoint origin, WebCore::FloatRect renderRect, bool fitEntireRect, double viewportMinimumScale, double viewportMaximumScale)

The origin, rect, fitEntireRect, and min/max viewport scales here are also common to the DidCollectGeometryForSmartMagnificationGesture message in SmartMagnificationController. It seems worth pulling these pieces of information out into a SmartMagnificationInformation or SmartMagnificationParameters so we can share some code, and then rename PotentialTapHasGeometry into something like HandleSmartMagnificationInformationForPotentialTap?

>> Source/WebKit/UIProcess/ios/SmartMagnificationController.mm:90
>> +void SmartMagnificationController::calculatePotentialZoomParameters(FloatPoint origin, FloatRect targetRect, bool fitEntireRect, double viewportMinimumScale, double viewportMaximumScale, double& targetScale)
> 
> origin is unused. What is going on here?

The FloatPoints and FloatRects can also be const& here.

>> Source/WebKit/UIProcess/ios/SmartMagnificationController.mm:101
>> +        targetScale = [m_contentView _initialScaleFactor];
> 
> Ok as-is. Looking at this code makes me think the name of this function and its single out arg is not good. This function compute the target scale as its only “output”, why not return by value and name this function something like computeTargetScale()?

+1

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:618
>> +static WTF::WallTime singleTapIdentificationTime;
> 
> Is this following the code style guide for naming?

Seems...slightly strange to use a static global variable for this! What if there are two web views and you tap both at roughly the same time? (e.g. in Mail). Definitely wouldn't be terrible, considering this is only used for logging, but I would prefer that this is tied to the lifetime of the synthetic click gesture recognizer.

But it's also possible we don't need to keep track of these times at all, since we can deduce them from the timestamps of the associated log messages (though determining the duration of each "faster click" would be a bit more annoying).

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:2311
> +    WTF::Seconds duration = WTF::WallTime::now() - singleTapIdentificationTime;

Nit - I don't think the WTF:: should be necessary here...perhaps either:

auto duration = WallTime::now() - singleTapIdentificationTime;

or, just inline WallTime::now() - singleTapIdentificationTime into the logging code below.

> LayoutTests/ChangeLog:18
> +        * fast/events/ios/ipad/fast-click-double-tap-sends-click-on-insignificant-zoom-expected.txt: Added.

Is this test really specific to iPad, or can we make it run on iPhone simulator as well?

> LayoutTests/resources/ui-helper.js:104
> +                        }, 120);

This seems okay, but hard-coded delays like this have had a history of being flaky in automation :/ We might need some testing-only SPI in the future to the effect of "do this action after the current potential tap has been handled in the web process", and then use this SPI to synthesize the second click after the first potential tap was handled.

Also, if we do decide to go for this and see how it fares in automation, I think we can make this logic quite a bit simpler by leveraging other UIHelper functions:

return new Promise(async resolve => {
    await UIHelper.activateAt(x, y);
    await new Promise(resolveAfterDelay => setTimeout(resolveAfterDelay, 120));
    await UIHelper.activateAt(x, y);
    resolve();
});

(...or something similar...)
Comment 11 Daniel Bates 2019-03-10 19:04:54 PDT
Comment on attachment 364058 [details]
Patch

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

>>> Source/WebKit/UIProcess/ios/SmartMagnificationController.mm:90
>>> +void SmartMagnificationController::calculatePotentialZoomParameters(FloatPoint origin, FloatRect targetRect, bool fitEntireRect, double viewportMinimumScale, double viewportMaximumScale, double& targetScale)
>> 
>> origin is unused. What is going on here?
> 
> The FloatPoints and FloatRects can also be const& here.

-1; I disagree. I am not going to speak to the unused origin, but do *not* make targetRect a const& . What you have here, passing by value is more efficient given your usage, and is the right choice. @Wenson: Let me know if you want an explanation why.
Comment 12 Wenson Hsieh 2019-03-10 19:15:09 PDT
Comment on attachment 364058 [details]
Patch

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

>>>> Source/WebKit/UIProcess/ios/SmartMagnificationController.mm:90
>>>> +void SmartMagnificationController::calculatePotentialZoomParameters(FloatPoint origin, FloatRect targetRect, bool fitEntireRect, double viewportMinimumScale, double viewportMaximumScale, double& targetScale)
>>> 
>>> origin is unused. What is going on here?
>> 
>> The FloatPoints and FloatRects can also be const& here.
> 
> -1; I disagree. I am not going to speak to the unused origin, but do *not* make targetRect a const& . What you have here, passing by value is more efficient given your usage, and is the right choice. @Wenson: Let me know if you want an explanation why.

Oh, I missed that adjustSmartMagnificationTargetRectAndZoomScales mutates targetRect. In that case, I wonder if it should be an outparam instead. I had assumed from the order of arguments that it should be a read-only argument in this method.

I guess the origin can still be made const& or removed entirely (if it's really not needed).
Comment 13 Wenson Hsieh 2019-03-10 19:32:53 PDT
Comment on attachment 364058 [details]
Patch

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

>>>>> Source/WebKit/UIProcess/ios/SmartMagnificationController.mm:90
>>>>> +void SmartMagnificationController::calculatePotentialZoomParameters(FloatPoint origin, FloatRect targetRect, bool fitEntireRect, double viewportMinimumScale, double viewportMaximumScale, double& targetScale)
>>>> 
>>>> origin is unused. What is going on here?
>>> 
>>> The FloatPoints and FloatRects can also be const& here.
>> 
>> -1; I disagree. I am not going to speak to the unused origin, but do *not* make targetRect a const& . What you have here, passing by value is more efficient given your usage, and is the right choice. @Wenson: Let me know if you want an explanation why.
> 
> Oh, I missed that adjustSmartMagnificationTargetRectAndZoomScales mutates targetRect. In that case, I wonder if it should be an outparam instead. I had assumed from the order of arguments that it should be a read-only argument in this method.
> 
> I guess the origin can still be made const& or removed entirely (if it's really not needed).

Never mind, passing targetRect by value does seem like the right choice here :/

>> Source/WebKit/UIProcess/ios/SmartMagnificationController.mm:95
>> +    adjustSmartMagnificationTargetRectAndZoomScales(!fitEntireRect, targetRect, minimumScale, maximumScale);
> 
> I know you didn’t add this function in this patch, but what a mess is the argument order. Doesn’t even match the implied argument order in its name (I.e. first arg should be the rect not a bool).

I also find this helper function a bit clumsy, but isn't it the case that we order arguments such that outparams go after other inputs to the function?
Comment 14 Dean Jackson 2019-03-11 11:47:38 PDT
Comment on attachment 364058 [details]
Patch

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

>> Source/WebKit/Platform/Logging.h:55
>> +    M(Interaction) \
> 
> Nit - this logging category seems a little...too broad :P Maybe we can use the existing ViewGestures category, which is currently only about swipe?

Good idea. I removed this and used ViewGestures.

>> Source/WebKit/UIProcess/WebPageProxy.messages.in:198
>> +    PotentialTapHasGeometry(uint64_t requestID, WebCore::FloatPoint origin, WebCore::FloatRect renderRect, bool fitEntireRect, double viewportMinimumScale, double viewportMaximumScale)
> 
> The origin, rect, fitEntireRect, and min/max viewport scales here are also common to the DidCollectGeometryForSmartMagnificationGesture message in SmartMagnificationController. It seems worth pulling these pieces of information out into a SmartMagnificationInformation or SmartMagnificationParameters so we can share some code, and then rename PotentialTapHasGeometry into something like HandleSmartMagnificationInformationForPotentialTap?

Will do.

>>>>>> Source/WebKit/UIProcess/ios/SmartMagnificationController.mm:90
>>>>>> +void SmartMagnificationController::calculatePotentialZoomParameters(FloatPoint origin, FloatRect targetRect, bool fitEntireRect, double viewportMinimumScale, double viewportMaximumScale, double& targetScale)
>>>>> 
>>>>> origin is unused. What is going on here?
>>>> 
>>>> The FloatPoints and FloatRects can also be const& here.
>>> 
>>> -1; I disagree. I am not going to speak to the unused origin, but do *not* make targetRect a const& . What you have here, passing by value is more efficient given your usage, and is the right choice. @Wenson: Let me know if you want an explanation why.
>> 
>> Oh, I missed that adjustSmartMagnificationTargetRectAndZoomScales mutates targetRect. In that case, I wonder if it should be an outparam instead. I had assumed from the order of arguments that it should be a read-only argument in this method.
>> 
>> I guess the origin can still be made const& or removed entirely (if it's really not needed).
> 
> Never mind, passing targetRect by value does seem like the right choice here :/

We might use origin in the future if we decide to take double-tap-to-scroll into account.

>>> Source/WebKit/UIProcess/ios/SmartMagnificationController.mm:101
>>> +        targetScale = [m_contentView _initialScaleFactor];
>> 
>> Ok as-is. Looking at this code makes me think the name of this function and its single out arg is not good. This function compute the target scale as its only “output”, why not return by value and name this function something like computeTargetScale()?
> 
> +1

OK.

>>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:618
>>> +static WTF::WallTime singleTapIdentificationTime;
>> 
>> Is this following the code style guide for naming?
> 
> Seems...slightly strange to use a static global variable for this! What if there are two web views and you tap both at roughly the same time? (e.g. in Mail). Definitely wouldn't be terrible, considering this is only used for logging, but I would prefer that this is tied to the lifetime of the synthetic click gesture recognizer.
> 
> But it's also possible we don't need to keep track of these times at all, since we can deduce them from the timestamps of the associated log messages (though determining the duration of each "faster click" would be a bit more annoying).

I removed the static variable and replaced it with the WebView pointer address in the logging. People will have to do arithmetic themselves.

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:2311
>> +    WTF::Seconds duration = WTF::WallTime::now() - singleTapIdentificationTime;
> 
> Nit - I don't think the WTF:: should be necessary here...perhaps either:
> 
> auto duration = WallTime::now() - singleTapIdentificationTime;
> 
> or, just inline WallTime::now() - singleTapIdentificationTime into the logging code below.

Has been removed.

>> LayoutTests/ChangeLog:18
>> +        * fast/events/ios/ipad/fast-click-double-tap-sends-click-on-insignificant-zoom-expected.txt: Added.
> 
> Is this test really specific to iPad, or can we make it run on iPhone simulator as well?

The viewport is different on iPhone, so it's hard to make a test that can zoom the same on both devices.

>> LayoutTests/resources/ui-helper.js:104
>> +                        }, 120);
> 
> This seems okay, but hard-coded delays like this have had a history of being flaky in automation :/ We might need some testing-only SPI in the future to the effect of "do this action after the current potential tap has been handled in the web process", and then use this SPI to synthesize the second click after the first potential tap was handled.
> 
> Also, if we do decide to go for this and see how it fares in automation, I think we can make this logic quite a bit simpler by leveraging other UIHelper functions:
> 
> return new Promise(async resolve => {
>     await UIHelper.activateAt(x, y);
>     await new Promise(resolveAfterDelay => setTimeout(resolveAfterDelay, 120));
>     await UIHelper.activateAt(x, y);
>     resolve();
> });
> 
> (...or something similar...)

Yeah - great suggestion. Fixed.
Comment 15 Dean Jackson 2019-03-11 14:29:00 PDT
Comment on attachment 364058 [details]
Patch

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

>>> Source/WebKit/UIProcess/ios/SmartMagnificationController.mm:95
>>> +    adjustSmartMagnificationTargetRectAndZoomScales(!fitEntireRect, targetRect, minimumScale, maximumScale);
>> 
>> I know you didn’t add this function in this patch, but what a mess is the argument order. Doesn’t even match the implied argument order in its name (I.e. first arg should be the rect not a bool).
> 
> I also find this helper function a bit clumsy, but isn't it the case that we order arguments such that outparams go after other inputs to the function?

Changed this to return a tuple, and updated the call sites. The name changes as well.

>>>> Source/WebKit/UIProcess/ios/SmartMagnificationController.mm:101
>>>> +        targetScale = [m_contentView _initialScaleFactor];
>>> 
>>> Ok as-is. Looking at this code makes me think the name of this function and its single out arg is not good. This function compute the target scale as its only “output”, why not return by value and name this function something like computeTargetScale()?
>> 
>> +1
> 
> OK.

BTW - The initial reason for this was because we could be scrolling instead of zooming, so we might need multiple out parameters.

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:235
>> +static const double fasterTapSignificantZoomThreshold = 0.8;
> 
> Ok as-is. static is unnecessary. Moreover, this is the old style. New hotness, constexpr.

Fixed both.

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:2210
>> +    bool wantsGeometryResponse = _page->preferences().fasterClicksEnabled();
> 
> wants....Response? What kind of name is this?

Changed to requestingMagnificationInformation
Comment 16 Dean Jackson 2019-03-11 15:16:37 PDT
Created attachment 364295 [details]
Patch
Comment 17 Wenson Hsieh 2019-03-11 16:17:54 PDT
Comment on attachment 364295 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:815
> +void WebPageProxy::potentialTapAtPosition(const WebCore::FloatPoint& position, bool requestingMagnificationInformation, uint64_t& requestID)

Nit - is it intended that requestMagnificationInformation in WKContentView becomes requestingMagnificationInformation in WebPageProxy? I think it might be better to use a consistent name for the "should I request magnification info" flag (maybe the name could just be shouldRequestMagnificationInformation).

> LayoutTests/resources/ui-helper.js:79
> +        console.assert(this.isIOS());

Nit - Generally speaking, UIHelper methods should work cross-platform if at all possible. We should probably use eventSender to make it work in the non-iOS case (or at least replace the console.assert(this.isIOS()) with a FIXME to implement it elsewhere).
Comment 18 Dean Jackson 2019-03-11 16:43:26 PDT
Committed r242757: <https://trac.webkit.org/changeset/242757>
Comment 19 Ryan Haddad 2019-03-11 22:09:43 PDT
(In reply to Build Bot from comment #6)
> Comment on attachment 364058 [details]
> Patch
> 
> Attachment 364058 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: https://webkit-queues.webkit.org/results/11431671
> 
> New failing tests:
> http/tests/adClickAttribution/anchor-tag-attributes-validation.html
> http/tests/security/anchor-download-block-crossorigin.html
These tests are failing on trunk macOS bots now that this change has landed. It looks like the line number for console messages changed from 108 to 133:
https://build.webkit.org/results/Apple%20High%20Sierra%20Release%20WK2%20(Tests)/r242766%20(10017)/results.html
Comment 20 Ryan Haddad 2019-03-11 22:19:02 PDT
Rebaselined tests in https://trac.webkit.org/changeset/242774/webkit
Comment 21 Truitt Savell 2019-03-19 15:12:35 PDT
Looks like the changes in https://trac.webkit.org/changeset/242757/webkit

caused this test fast/images/imagemap-in-shadow-tree.html

to become a constant crash on iOS debug. History:
http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fimages%2Fimagemap-in-shadow-tree.html

crash log:
https://build.webkit.org/results/Apple%20iOS%2012%20Simulator%20Debug%20WK2%20(Tests)/r243159%20(2861)/fast/images/imagemap-in-shadow-tree-crash-log.txt

I was able to reproduce this failure locally using 
run-webkit-tests fast/images/imagemap-in-shadow-tree.html --iterations 10 --ios-simulator

the test crashes on 242757 but not on 242756
Comment 22 Ryosuke Niwa 2019-03-19 15:22:05 PDT
Maybe Node::renderRect doesn't handle `display: contents` properly?
Comment 23 Truitt Savell 2019-03-19 15:45:29 PDT
This test is also crashing: http/tests/download/area-download.html

History:
http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fdownload%2Farea-download.html
Comment 24 Dean Jackson 2019-03-20 14:58:42 PDT
Crasher tracked in https://bugs.webkit.org/show_bug.cgi?id=196035