WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
197821
[iOS] When running layout tests that tap in the same location, subsequent tests fail to fire click handlers
https://bugs.webkit.org/show_bug.cgi?id=197821
Summary
[iOS] When running layout tests that tap in the same location, subsequent tes...
Wenson Hsieh
Reported
2019-05-11 22:35:10 PDT
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.
Attachments
Patch
(7.45 KB, patch)
2019-05-11 22:59 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Patch
(7.60 KB, patch)
2019-05-13 11:13 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Patch
(7.60 KB, patch)
2019-05-13 11:14 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2019-05-11 22:40:05 PDT
<
rdar://problem/50700512
>
Wenson Hsieh
Comment 2
2019-05-11 22:59:54 PDT
Comment hidden (obsolete)
Created
attachment 369668
[details]
Patch
alan
Comment 3
2019-05-12 07:33:03 PDT
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 4
2019-05-12 15:15:32 PDT
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 5
2019-05-12 15:48:59 PDT
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 6
2019-05-13 09:20:40 PDT
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 7
2019-05-13 11:11:24 PDT
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
Comment 8
2019-05-13 11:13:33 PDT
Comment hidden (obsolete)
Created
attachment 369749
[details]
Patch
Wenson Hsieh
Comment 9
2019-05-13 11:14:32 PDT
Created
attachment 369751
[details]
Patch
WebKit Commit Bot
Comment 10
2019-05-13 19:25:40 PDT
Comment on
attachment 369751
[details]
Patch Clearing flags on attachment: 369751 Committed
r245268
: <
https://trac.webkit.org/changeset/245268
>
WebKit Commit Bot
Comment 11
2019-05-13 19:25:41 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug