Bug 197821 – [iOS] When running layout tests that tap in the same location, subsequent tests fail to fire click handlers

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Wenson Hsieh]

Among many other ways, this can be observed when removing the workarounds in these tests:

 • editing/selection/ios/clear-selection-after-tapping-on-element-with-no-click-handler.html
 • editing/selection/ios/persist-selection-after-tapping-on-element-with-click-handler.html

...and running them back to back.

[Radar WebKit Bug Importer]

<rdar://problem/50700512>

[Wenson Hsieh]

Created attachment 369668 [details]
Patch

[alan]

Comment on attachment 369668 [details]
Patch

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

> Source/WebKit/ChangeLog:11
> +        After r244775, when running back-to-back layout tests on iOS that simulate taps in the same location, the double
> +        tap gesture recognizer for recognizing double clicks ends up recognizing instead of the single tap gesture
> +        recognizer in the subsequent test. This means that click handlers in the subsequent test will fail to recognize,

How do the other double tap gesture recognizers deal with this? Where do they get reset?

> Source/WebKit/ChangeLog:15
> +        To avoid this, we reset the double click gesture recognizer when navigating; this has the additional effect of
> +        making it such that the second page doesn't end up observing a dblclick when the first click was only sent to

This should be taken care of layerTreeTransactionIdAtLastTouchStart -and if it doesn't, we need to figure out why.

[Wenson Hsieh]

Comment on attachment 369668 [details]
Patch

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

>> Source/WebKit/ChangeLog:11
>> +        recognizer in the subsequent test. This means that click handlers in the subsequent test will fail to recognize,
> 
> How do the other double tap gesture recognizers deal with this? Where do they get reset?

Great question. There are two competing single-touch double tap gesture recognizers that I think you're referring to: _doubleTapGestureRecognizer and _nonBlockingDoubleTapGestureRecognizer, both on WKContentView. It's easy to see why _doubleTapGestureRecognizer doesn't interfere with the synthetic click gesture recognizer — since the test page has a responsive viewport, fast clicking is enabled, so _doubleTapGestureRecognizer is disabled. Explaining _nonBlockingDoubleTapGestureRecognizer took a bit more spelunking...

So the _nonBlockingDoubleTapGestureRecognizer is only enabled when _doubleTapGestureRecognizer is disabled — in other words, it's the double tap gesture recognizer that is used when fast-clicking is allowed. The intent of this is to not get in the way of the synthetic click GR, so that clicks can be recognized right away without waiting for the normal DTGR to fail.

Note that the test runner always loads about:blank in between tests, and also note that about:blank doesn't have a viewport by default (i.e., fast clicking is disabled). This means that in-between tests, we will load about blank (thereby calling [_nonBlockingDoubleTapGestureRecognizer setEnabled:NO]), and then when we get to the test page, it has a responsive viewport, so we call [_nonBlockingDoubleTapGestureRecognizer setEnabled:YES]. This has the identical result to what I did in this patch, which is to reset the double tap gesture recognizer and allow the single tap to recognize. Observe that forcing fast-clicking to be always enabled (and therefore cause _nonBlockingDoubleTapGestureRecognizer to be always enabled) causes this same bug reproduce, even after removing the recent _doubleTapGestureRecognizerForDoubleClick.

[Wenson Hsieh]

Comment on attachment 369668 [details]
Patch

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

>> Source/WebKit/ChangeLog:15
>> +        making it such that the second page doesn't end up observing a dblclick when the first click was only sent to
> 
> This should be taken care of layerTreeTransactionIdAtLastTouchStart -and if it doesn't, we need to figure out why.

First, I actually misspoke here — the second page won't actually observe a dblclick. What I meant to write here is that *if* the tapped element on the second page happens to have a click handler as well as a dblclick handler, then that click handler will be fired with an event with a clickCount of 2.

That said, I don't quite understand why you think layerTreeTransactionIdAtLastTouchStart would prevent this from happening. Could you explain in a bit more detail?

Here's what I think: layerTreeTransactionIdAtLastTouchStart will be set to the transaction ID corresponding to the RLT transaction during the second tap, and so the double click GR will send this transaction ID to WebPage. This second ID is well after the first RLT commit after loading the page, so this check:

    lastLayerTreeTransactionId < WebFrame::fromCoreFrame(*frameRespondingToDoubleClick)->firstLayerTreeTransactionIDAfterDidCommitLoad()

...is false, and we don't bail. In any case, I think this is a different (but related) problem to the one here. Even if we were to make some change that caused us to bail here (e.g. by sending the transaction ID corresponding to the first "click" of the double click gesture), it wouldn't fix all of these iOS tests that are now flaky due to the new double click gesture recognizer.

[Darin Adler]

Comment on attachment 369668 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3767
> +    [_doubleTapGestureRecognizerForDoubleClick setEnabled:NO];
> +    [_doubleTapGestureRecognizerForDoubleClick setEnabled:YES];

This kind of mysterious magical code needs a brief "why" comment explaining what the benefit is of disabling and re-enabling in quick succession.

[Wenson Hsieh]

Comment on attachment 369668 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3767
>> +    [_doubleTapGestureRecognizerForDoubleClick setEnabled:YES];
> 
> This kind of mysterious magical code needs a brief "why" comment explaining what the benefit is of disabling and re-enabling in quick succession.

Good call — added a comment.

[Wenson Hsieh]

Created attachment 369749 [details]
Patch

[Wenson Hsieh]

Created attachment 369751 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 369751 [details]
Patch

Clearing flags on attachment: 369751

Committed r245268: <https://trac.webkit.org/changeset/245268>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.