Bug 215368

Summary: iOS: Scrolling and touch events sporadically stop working after navigating
Product: WebKit Reporter: Tim Horton <thorton>
Component: New BugsAssignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, bdakin, hi, megan_gardner, simon.fraser, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Tim Horton 2020-08-11 03:53:09 PDT 
iOS: Scrolling and touch events sporadically stop working after navigating
Comment 1 Tim Horton 2020-08-11 03:53:57 PDT 
Created attachment 406372 [details]
Patch
Comment 2 Tim Horton 2020-08-11 03:53:59 PDT 
<rdar://problem/65801531>
Comment 3 Tim Horton 2020-08-11 03:55:55 PDT 
Created attachment 406373 [details]
Patch
Comment 4 Wenson Hsieh 2020-08-11 07:47:43 PDT 
Comment on attachment 406373 [details]
Patch

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

> LayoutTests/fast/events/touch/ios/touch-event-stall-after-navigating-with-pending-asynchronous-touch-start.html:19
> +                await new Promise((resolve) => { testRunner.runUIScriptImmediately(`
> +                    uiController.doubleTapAtPoint(${x}, ${y}, .01, function() {
> +                        uiController.uiScriptComplete();
> +                    });`, resolve) });

Nit - the indentation looks a bit off here.
Comment 5 Simon Fraser (smfr) 2020-08-11 10:25:31 PDT 
Comment on attachment 406373 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/EventDispatcher.cpp:179
> +void EventDispatcher::takeQueuedTouchEventsForPage(const WebPage& webPage, TouchEventQueue& destinationQueue)

Why doesn't this just return a TouchEventQueue?
Comment 6 Devin Rousso 2020-08-11 10:48:23 PDT 
Comment on attachment 406373 [details]
Patch

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

> LayoutTests/fast/events/touch/ios/touch-event-stall-after-navigating-with-pending-asynchronous-touch-start.html:16
> +                await new Promise((resolve) => { testRunner.runUIScriptImmediately(`

NIT: why is this `Promise` needed?
Comment 7 Tim Horton 2020-08-11 13:16:11 PDT 
Comment on attachment 406373 [details]
Patch

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

>> Source/WebKit/WebProcess/WebPage/EventDispatcher.cpp:179
>> +void EventDispatcher::takeQueuedTouchEventsForPage(const WebPage& webPage, TouchEventQueue& destinationQueue)
> 
> Why doesn't this just return a TouchEventQueue?

🤷‍♂️ probably predates `move`

Not going to fix it now, but it's a good point.

>> LayoutTests/fast/events/touch/ios/touch-event-stall-after-navigating-with-pending-asynchronous-touch-start.html:16
>> +                await new Promise((resolve) => { testRunner.runUIScriptImmediately(`
> 
> NIT: why is this `Promise` needed?

You are right! It's not (and I don't totally understand why the test works anymore, but it does).

>> LayoutTests/fast/events/touch/ios/touch-event-stall-after-navigating-with-pending-asynchronous-touch-start.html:19
>> +                    });`, resolve) });
> 
> Nit - the indentation looks a bit off here.

Devin said the same thing, but it looks right to me‽ I am confused :) Maybe getting rid of the promise will help.
Comment 8 Tim Horton 2020-08-11 13:17:45 PDT 
Created attachment 406410 [details]
Patch
Comment 9 EWS 2020-08-11 14:04:15 PDT 
Committed r265515: <https://trac.webkit.org/changeset/265515>

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