Bug 184250

Summary: Unable to call event.preventDefault in dynamically added event listener
Product: WebKit Reporter: alexreardon
Component: New BugsAssignee: Tadeu Zagallo <tzagallo>
Status: RESOLVED FIXED    
Severity: Critical CC: andrew, bbmarelyo, benjamin, cdumez, commit-queue, desandrocodes, dino, eoconnor, ews-watchlist, fnlctrl, ggaren, herr.ernst, john.firebaugh, jonlee, mansing.choy, rniwa, simon.fraser, stoffeastrom, tsov, tudor, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari 11   
Hardware: All   
OS: iOS 11   
See Also: https://bugs.webkit.org/show_bug.cgi?id=185656
Attachments:
Description Flags
Patch
none
Patch for landing
none
Patch ggaren: review+, commit-queue: commit-queue-

Description alexreardon 2018-04-02 17:16:04 PDT
Hi there,

When you add an event listener (the parent listener) and you add another event listener inside of it (the child listener), the dynamically added child event listener is not able to use event.preventDefault() as the event is non cancelable. The dynamically added child listener will receive the event - but it is unable to use preventDefault(). This is true regardless of whether the parent event handler is a passive or capture handler (or both).

This is true for all the event types I tested.

Here a simple example that reproduces the issue:

- desktop: https://codepen.io/alexreardon/pen/eMKMmm
- mobile: https://codepen.io/alexreardon/pen/yKEKby

Able to correctly use event.preventDefault() in child listener on:
- Chrome
- Firefox
- Edge
- Internet Explorer 11
- Android: Chrome mobile 
- Android: Firefox (which I think just uses chrome under the hood)
- Safari 10.x (I was unable to test 11.2 as I could not find 

Not able to use event.preventDefault() in child listener on:
- Safari 11.3 on mac 
- Safari 11.3 on iOS 11.3 (as well as the other browsers that use the native browser such as firefox and chrome)
- Safari on iOS 11.2.6 (it looks like there was a configuration where event.preventDefault() was working. I think it had to do with touch-action: manipulation but I am yet to confirm)

This is important to us because we use event.preventDefault() on a touchmove event to prevent native scrolling on mobile. I can 

Work around:

You need to add an event handler in a non dynamic way (ie at startup) and then you can use event.preventDefault() as expected.

Impact:

A lot of drag and drop libraries and other tools use event.preventDefault() on touchmove to opt out of native scrolling. Right now this is not possible without adding your own touchmove listener before a touchstart
Comment 1 alexreardon 2018-04-02 17:58:18 PDT
A bit of context, upgrading from iOS 11.2 to iOS 11.3 caused react-beautiful-dnd to break: 
https://github.com/atlassian/react-beautiful-dnd/issues/413
Comment 2 alexreardon 2018-04-02 22:16:36 PDT
It looks like this is also effecting shopify/draggable
https://github.com/Shopify/draggable/issues/198
Comment 3 Radar WebKit Bug Importer 2018-04-03 10:28:10 PDT
<rdar://problem/39145092>
Comment 4 Max Hoffmann 2018-04-03 17:11:26 PDT
I can confirm this for:

- Safari 11.3 on Mac OS X 10.13.3
- Safari 11.3 on iOS 11.3

Any drag and drop implementation for touch relies on calling `touchMoveEvent.preventDefault()` to cancel native scrolling. I could imagine most dnd solutions out there adding `touchmove` as child listeners.

The current workaround suggested by alexreardon@gmail.com works well, but won't work when dynamically adding `touchmove` events to other documents, like iframes.
Comment 5 Dean Jackson 2018-04-10 20:58:22 PDT
In the linked examples, I see the main problem being that we explicitly mark mousemove events as being not cancelable:

MouseEvent::create

bool isCancelable = eventType != eventNames().mousemoveEvent && !isMouseEnterOrLeave;

Whereas the spec says it should be:

https://www.w3.org/TR/uievents/#event-type-mousemove

However, it looks like we've implemented it that way since 2013, so this wouldn't be a recent regression.

For touch events, it's been similarly unchanged since 2014.

I think I'll have to make a different test case.
Comment 6 Dean Jackson 2018-04-10 21:07:34 PDT
Yes, the touchmove is not cancelable either.

I was worried this might have something to do with making event listeners passive by default, but the examples here are explicitly setting to be not-passive.
Comment 7 Dean Jackson 2018-04-10 21:09:38 PDT
I can confirm it is only happening to event listeners that are added inside event handlers. It also doesn't matter if the once option is true or false.
Comment 8 Ernst 2018-04-11 01:30:17 PDT
The bug that dynamically added event listeners (i.e. event listeners added in event listeners) can't preventDefault exists since iOS 10: 
http://www.openradar.me/28359335
http://www.openradar.me/28479522
Comment 9 stoffeastrom 2018-04-13 04:09:49 PDT
It's affecting my gestures library also.

I can confirm it has been working a long time until iOS 11.3!
Comment 10 Tadeu Zagallo 2018-05-14 09:25:49 PDT
I think I finally understood how the behaviour actually changed. Dynamic touchmove listeners haven't been cancelable since iOS 10, so I couldn't find a minimal repro case at first, which was confusing, since the events *were* cancelable in the example provided here. What I realised was that the only reason the example worked in the first place was that there was already a touchmove listener on the document, added by React. So, before iOS 11.3, a touchmove event was cancelable as long as there was already one touchmove listener in the region of the touch. Since iOS 11.3, touchmove events cannot be prevented by dynamic listeners, regardless of whether there was already another listener. I have not yet identified what cause this regression though, but I'm working on that now that I have reduced the example.
Comment 11 Tadeu Zagallo 2018-05-14 14:33:46 PDT
I can see now how making event listeners passive by default affected all the drag-and-drop libraries: Even though the event listener was explicitly setting passive to false, its behaviour depended on the event listener that React had added to the document first, as I mentioned in my previous comment. That event listener was being added to the document, and did not explicitly set passive to false, so it was being treated as a passive listener (handled asynchronously), which in turn caused the dynamically added listener to also be handled asynchronously. I could verify that by modifying the React's listener to pass passive: false and that restored the original behaviour of the drag-and-drop. I'll have a patch soon.
Comment 12 alexreardon 2018-05-14 14:39:12 PDT
Is the patch to resume the pre 11.3 behaviour - or to fix the ability to enable calling event.preventDefault in a dynamically added event listener?
Comment 13 Tadeu Zagallo 2018-05-14 14:42:54 PDT
(In reply to alexreardon from comment #12)
> Is the patch to resume the pre 11.3 behaviour - or to fix the ability to
> enable calling event.preventDefault in a dynamically added event listener?

It will make touch events cancelable from dynamically added event listeners.
Comment 14 alexreardon 2018-05-14 14:44:45 PDT
Yay! 🎉
Comment 15 Tadeu Zagallo 2018-05-15 08:49:06 PDT
Created attachment 340412 [details]
Patch
Comment 16 Geoffrey Garen 2018-05-15 10:34:48 PDT
Comment on attachment 340412 [details]
Patch

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

r=me with comments

> LayoutTests/fast/events/touch/ios/touchmove-cancelable-after-touchstart.html:77
> +        // NOTE: removing this assertion will cause the test to fail, the event listener region is not immediately updated
> +        shouldBe("event.target", "square");

Let's clarify this comment:

(1) Let's say "For some reason, if we don't include this shouldBe line, the first few touchmove events we see will be passive (not cancelable)".

(2) Let's link to a bug report that says we should fix that.

(3) If all you need to work around that bug is a change to the DOM, then can we just do some kind of no-op DOM change? That will make the workaround clearer.

> LayoutTests/fast/events/touch/ios/touchmove-cancelable-after-touchstart.html:79
> +        window.addEventListener("touchmove", touchmoveEventHandler, { passive: false, once: true });
> +    }, { once: true });

I think it's a little racy for the web author to specify touchstart as passive. There's no guarantee that touchmove will be added before the user is done touching!

So, I think this test case would be a little better if we specified passive: false on the touchstart handler -- and it would still test our ability to treat the new touchmove handler as non-passive (synchronous).
Comment 17 Tadeu Zagallo 2018-05-15 13:57:42 PDT
Created attachment 340433 [details]
Patch for landing
Comment 18 Geoffrey Garen 2018-05-15 16:04:04 PDT
Comment on attachment 340433 [details]
Patch for landing

r=me
Comment 19 WebKit Commit Bot 2018-05-15 16:50:24 PDT
Comment on attachment 340433 [details]
Patch for landing

Clearing flags on attachment: 340433

Committed r231821: <https://trac.webkit.org/changeset/231821>
Comment 20 WebKit Commit Bot 2018-05-15 16:50:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Simon Fraser (smfr) 2018-05-15 19:47:25 PDT
Comment on attachment 340433 [details]
Patch for landing

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

> Source/WebKit/ChangeLog:9
> +        The tracking type for touch events were only update on touchstart, which meant that event

"were only update" -> "was only updated"
Comment 22 Tadeu Zagallo 2018-05-16 15:13:10 PDT
Reopening to attach new patch.
Comment 23 Tadeu Zagallo 2018-05-16 15:13:13 PDT
Created attachment 340528 [details]
Patch
Comment 24 EWS Watchlist 2018-05-16 15:14:35 PDT
Attachment 340528 [details] did not pass style-queue:


ERROR: Source/WebKit/ChangeLog:168:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: LayoutTests/ChangeLog:146:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 Geoffrey Garen 2018-05-16 15:16:02 PDT
Comment on attachment 340528 [details]
Patch

r=me
Comment 26 WebKit Commit Bot 2018-05-16 15:18:26 PDT
Comment on attachment 340528 [details]
Patch

Rejecting attachment 340528 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 340528, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog is not at the top of the file.

Full output: http://webkit-queues.webkit.org/results/7702520
Comment 27 Geoffrey Garen 2018-05-17 09:38:40 PDT
Committed r231907: <https://trac.webkit.org/changeset/231907>
Comment 28 alexreardon 2018-08-05 17:22:17 PDT
Thank you everyone for your efforts on this. 

Has the fix been released in Safari yet? How could I find this out?
Comment 29 alexreardon 2018-08-05 17:28:24 PDT
I can see that it has not yet been released on Safari 11.1.2: https://codepen.io/alexreardon/pen/eMKMmm
Comment 30 alexreardon 2018-08-05 17:38:20 PDT
It looks like it is now working for Safari on mobile: https://codepen.io/alexreardon/pen/yKEKby
Comment 31 Ryosuke Niwa 2018-08-05 17:39:05 PDT
The fix should be included in the iOS 12 betas.
Comment 32 alexreardon 2018-08-05 17:41:40 PDT
Great, thanks
Comment 33 fnlctrl 2018-08-19 18:52:24 PDT
Just tried the codepen https://codepen.io/alexreardon/pen/yKEKby on iOS 12.0 beta with Safari 12 on a real device, and the bug persists.
Comment 34 Tadeu Zagallo 2018-08-20 09:21:58 PDT
There's another bug related to this one that hasn't been fixed yet, in which only the first few touches are not cancelable from dynamically added touch handlers: https://bugs.webkit.org/show_bug.cgi?id=185656. In this case, since both events are handled with `once: true`, this happens most of the time. The patch for that bug has not been landed yet.
Comment 35 Andrew 2018-09-06 15:20:30 PDT
This is still broken on tests with iOS 12 public beta.