Bug 211521

This could well be a dupe of bug 211179.

<rdar://problem/62942374>

Hi juwagn. There is no test attached for either this bug nor bug 203786. It would help a great deal if you could provide us with one. Thanks!

Created attachment 398666 [details]
reproducer requested

How to reproduce

Safari on iOS 13.4.1 open bug_reproducer.html

1. finger touch (touchstart fired)
2. finger remove (touchend fired)
3. - execute next steps immediately like by double click -
4. finger touch (NO touchstart fired)
5. finger MOVE (NO touchmove fired)
6. finger remove (NO touchend fired)
In this scenario 4-6. steps are not anymore fired.

Same way Android Chrome fires 4.-6. steps correctly, and even more double touch increases on Android Chrome amount of fired touchstart/touchend events always by 2 event sequences(2start and 2end) as expected. But iOS 13.4.1 Safari fires always only one event sequence (1start and 1end).
This is serious bug in detection of "double touch move" gesture in our application.

Forgot to add, use "white" area for touching and use always one finger only!

In my description above "move event" won't be fired anyway, I didn't insert the extended handling for doing this, but that doesn't matter anyway, compared to Android Chrome the "6. finger remove (NO touchend fired)" will be fired by Android Chrome with noticeable change while Safari fires nothing here.
Expected would be firing of "touchstart" "touchend" "touchmove" events same way as on all common browsers like Android Chrome, Android FireFox etc.

I use "passive": false on addEventListener, so i don't care about scrolling acceleration since no page scrolling used, so "passive": false is wished behavior and should stay .preventDefault() preventable event.

Thanks for the test case! I managed to track down the regression to r253267, the fix for bug 204664. We already fixed a regression related to this bug in bug 211179, but this seems to be another aspect of the original regression that wasn't dealt with yet.

I’ve begun to investigate this.

One interesting thing I noticed is that the bug only seems to reproduce if the viewport is not responsive; adding something like:

<meta name="viewport" content="width=device-width, initial-scale=1">

seems to make the problem go away (at least, until the user manually pinches to zoom in).

This seems like bad interaction between the relatively new deferring gesture recognizers (WKDeferringGestureRecognizer) and the blocking double tap gesture recognizer (used when fast-clicking is disabled).

Created attachment 398999 [details]
Patch

Comment on attachment 398999 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:719
> +- (NSArray<WKDeferringGestureRecognizer *> *)_deferringGestureRecognizers

I think this needs to go inside:

    #if ENABLE(IOS_TOUCH_EVENTS)

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:728
> +    NSMutableArray *deferringGestureRecognizers = [NSMutableArray arrayWithCapacity:3];
> +    if (_deferringGestureRecognizerForImmediatelyResettableGestures)
> +        [deferringGestureRecognizers addObject:_deferringGestureRecognizerForImmediatelyResettableGestures.get()];
> +    if (_deferringGestureRecognizerForDelayedResettableGestures)
> +        [deferringGestureRecognizers addObject:_deferringGestureRecognizerForDelayedResettableGestures.get()];
> +    if (_deferringGestureRecognizerForSyntheticTapGestures)
> +        [deferringGestureRecognizers addObject:_deferringGestureRecognizerForSyntheticTapGestures.get()];
> +    return deferringGestureRecognizers;

There are more efficient ways to do this than using an NSMutableArray. The simplest I can think of is to use a C array. Something like this:

    WKDeferringGestureRecognizer *recognizers[3];
    NSUInteger count = 0;
    auto add = [&] (const RetainPtr<WKDeferringGestureRecognizer>& recognizer) {
        if (recognizer)
            recognizers[count++] = recognizer.get();
    };
    add(_deferringGestureRecognizerForImmediatelyResettableGestures);
    add(_deferringGestureRecognizerForDelayedResettableGestures);
    add(_deferringGestureRecognizerForSyntheticTapGestures);
    return [[NSArray arrayWithObjects:recognizers count:count];

That’s more lines of code, but it always seems better to me not to use a mutable array.

Comment on attachment 398999 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:719
>> +- (NSArray<WKDeferringGestureRecognizer *> *)_deferringGestureRecognizers
> 
> I think this needs to go inside:
> 
>     #if ENABLE(IOS_TOUCH_EVENTS)

Oops! Yes, that’s correct.

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:728
>> +    return deferringGestureRecognizers;
> 
> There are more efficient ways to do this than using an NSMutableArray. The simplest I can think of is to use a C array. Something like this:
> 
>     WKDeferringGestureRecognizer *recognizers[3];
>     NSUInteger count = 0;
>     auto add = [&] (const RetainPtr<WKDeferringGestureRecognizer>& recognizer) {
>         if (recognizer)
>             recognizers[count++] = recognizer.get();
>     };
>     add(_deferringGestureRecognizerForImmediatelyResettableGestures);
>     add(_deferringGestureRecognizerForDelayedResettableGestures);
>     add(_deferringGestureRecognizerForSyntheticTapGestures);
>     return [[NSArray arrayWithObjects:recognizers count:count];
> 
> That’s more lines of code, but it always seems better to me not to use a mutable array.

I’ll give this a shot — thanks!

Created attachment 399022 [details]
For EWS

Committed r261480: <https://trac.webkit.org/changeset/261480>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399022 [details].