Bug 195473 – [iOS] Implement a faster click detection that intercepts double-tap-to-zoom if possible

• Component: New Bugs
• Status: RESOLVED FIXED

[Dean Jackson]

[iOS] Implement a faster click detection that intercepts double-tap-to-zoom if possible

[Radar WebKit Bug Importer]

<rdar://problem/48718396>

[Dean Jackson]

Created attachment 364042 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Dean Jackson]

Created attachment 364058 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Daniel Bates]

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.

[Dean Jackson]

Thanks for the review Dan!

[Wenson Hsieh]

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

[Daniel Bates]

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.

[Wenson Hsieh]

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

[Wenson Hsieh]

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?

[Dean Jackson]

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.

[Dean Jackson]

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

[Dean Jackson]

Created attachment 364295 [details]
Patch

[Wenson Hsieh]

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

[Dean Jackson]

Committed r242757: <https://trac.webkit.org/changeset/242757>

[Ryan Haddad]

(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

[Ryan Haddad]

Rebaselined tests in https://trac.webkit.org/changeset/242774/webkit

[Truitt Savell]

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

[Ryosuke Niwa]

Maybe Node::renderRect doesn't handle `display: contents` properly?

[Truitt Savell]

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

[Dean Jackson]

Crasher tracked in https://bugs.webkit.org/show_bug.cgi?id=196035