1356293 - Cancelable is not set to false for passive event listener

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Description

[Benoit Girard (:BenWa)]

Attachment: cancelable.html

Spec: https://www.w3.org/TR/uievents/#event-flow-default-cancel

Snippet:
document.body.addEventListener('wheel', function(e) {
  console.log(e.cancelable);
  e.preventDefault();
}, {passive: true});

This should return false. In Firefox this returns true but calling preventDefault does not cancel the event. This is implemented correctly in Chrome.

See attached testcase in Firefox vs. Chrome for fully working example.

Comment 1

[Benoit Girard (:BenWa)]

This might be worthwhile to include with web platform tests.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Whoever is dispatching wheel should check whether there are only passive listeners, at least in APZ context.
Of course nothing guarantees that browser should honor passive.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

But if browser does support it for the relevant event, cancelable should be false.

Comment 4

[Benoit Girard (:BenWa)]

See Bug 1383900 comment 10 for justification why this flag is useful. For large sites, this is used to track which event listeners are blocking async scrolling and to monitor regressions. It can also be used to prioritize which are the most important to fix. Site like Facebook can set performance goals on making sure that all scrolls are either non-cancelable (proxy detection for async scrolling) or that there's a good use case for cancelable events (non-async scrolling) that cannot be done via a web feature like scroll-boundary-behavior to block scroll chaining.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

I'd like to implement this but the implementation details are not totally clear to me. It sounds to me like:

- for events that are not "APZ aware events" [1] we should keep whatever the current behaviour is (cancelable depends on the type of event, probably)
- for APZ aware events, we need to check if any of the listeners on the entire target chain are non-passive. If so, then we keep whatever the current behaviour is (presumably cancelable=true)
- for APZ aware events that only has passive listeners, we force the event's cancelable property to false before we dispatch it anywhere

And presumably this code should live inside EventStateManager::PreHandleEvent, or something around there? smaug, does this sound reasonable?

[1] http://searchfox.org/mozilla-central/rev/51eae084534f522a502e4b808484cde8591cb502/dom/events/EventListenerManager.cpp#1794

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Why would that live in ESM? Shouldn't we check all that before dispatching the event.

Though, since EventDispatcher creates the path, perhaps from performance point of view we could check the existence of passive only listeners in EventTargetChainItem::GetEventTargetParent.
Only when aVisitor.mMayHaveListenerManager is true, we could try to access ELM and query the information from there.

Comment 7

[Benoit Girard (:BenWa)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> - for events that are not "APZ aware events" [1] we should keep whatever the
> current behaviour is (cancelable depends on the type of event, probably)

I don't think it should make a difference if something is APZ aware since it's mostly an internal implementation detail and I don't see anything in the spec to back this notion.

Shouldn't it strickly be based on if there's any non-passive listener that would cause this event to become cancelable:
"""
In some cases this problem can be mitigated by specifying the event to be cancelable only when there is at least one non-passive listener. For example, non-passive TouchEvent listeners must block scrolling, but if all listeners are passive then scrolling can be allowed to start in parallel by making the TouchEvent uncancelable (so that calls to preventDefault() are ignored). So code dispatching an event is able to observe the absence of non-passive listeners, and use that to clear the cancelable property of the event being dispatched. 
"""
https://dom.spec.whatwg.org/#dom-event-cancelable

I think it would be odd for the value of this flag to change for a site if pref is change to (un)make an event APZ aware. It should strictly be based on if the site has a proper non-passive event listeners or not for that event's listeners. On the flip side if it's implemented this way it would also a site to check if the user has APZ enabled.

I guess the question is if Gecko wants to permit calling preventDefault on a passive event listeners that's not APZ-aware and thus where Gecko doesn't benefit in blocking preventDefault on this event. I think the answer should be yes for forward compatibility reasons. Otherwise you risk site-compat issue when you make an event APZ-aware, such as keydown, because a site had a passive keydown listener with a preventDefault.

But for the use cases I described in comment 10, I think sites are only interested in monitoring APZ aware events so it doesn't impact the use case either way.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

(In reply to Benoit Girard (:BenWa) from comment #7)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> > - for events that are not "APZ aware events" [1] we should keep whatever the
> > current behaviour is (cancelable depends on the type of event, probably)
> 
> I don't think it should make a difference if something is APZ aware

Yeah your comment makes sense. So we don't need to check for APZ-aware-ness. So now the steps boil down to this:

- when dispatching an event, check if any of the listeners on the entire target chain are non-passive. If there are only passive listeners, force the event's cancelable property to false before we dispatch it anywhere

(In reply to Olli Pettay [:smaug] from comment #6)
> Why would that live in ESM? Shouldn't we check all that before dispatching
> the event.

Putting this at the call site of every event doesn't make sense; it makes more sense to put it inside the event dispatching mechanism somewhere. I was guessing ESM but EventDispatcher::DispatchEvent looks more appropriate.

> Though, since EventDispatcher creates the path, perhaps from performance
> point of view we could check the existence of passive only listeners in
> EventTargetChainItem::GetEventTargetParent.
> Only when aVisitor.mMayHaveListenerManager is true, we could try to access
> ELM and query the information from there.

This sounds reasonable, I think.

Comment 9

[Ryan Hunt [:rhunt]]

I took a quick stab at implementing this. There are a couple of try failures.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d746a17ef4b32f7419549b5b3d9367d419c88ae6

I might come back to this in the future, but I'm working on other things right now.

Comment 10

[Vincent Hilla [:vhilla]]

There are wpts in /dom/events/non-cancelable-when-passive/*, see Bug 1778162

In addition to what is stated more generally in the dom spec, also see cancelability in the touch events spec.

Comment 11

[Vincent Hilla [:vhilla]]

I updated the patch and from :rhunt and limited it to touch and wheel events. Otherwise, some mochitests fail and I worry about web compatibility:

const et = new EventTarget();
et.addEventListener('test', (e) => console.log(e.cancelable), {passive: true});
et.dispatchEvent(new Event('test', {cancelable: true}));

this will log true in Blink and current Gecko.

As today is my last day at Mozilla, I won't be able to finish this. I'll attach the patch here and hope it is useful for someone in the future. Some things that could be considered:

• Maybe replace the PreventDefaultFromPassiveListenerWarning here with an error that is always printed, as in Chrome
• Possibly include more than touch and wheel events in the patch
• Convert above code into a wpt to ensure other events are not marked uncancelable
• Think about performance and code maintainability aspects

Comment 12

[Vincent Hilla [:vhilla]]

Attachment: Bug 1356293 - Make touch and wheel events without non-passive listeners uncancelable. r=#dom-core

According to the spec https://w3c.github.io/touch-events/#cancelability,
the touchstart, touchend and touchmove events should be uncancelable if
ther eare no non-passive listeners.

Comment 13

[Vincent Hilla [:vhilla]]

test_continuous_wheel_events.html is failing. It seems, sendWheelAndPaint dispatches the wheel event first to the parent process before it is dispatched in the content. As there are no non-passive listeners in the parent, the event will incorrectly be set to uncancelable.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Perhaps all the events targeted to not-in-same-docshell-tree targets should avoid this new behavior.
Since I assume the same issue might apply to other events too.

Comment 15

[Vincent Hilla [:vhilla]]

I'm unsure, does it suffice to check BrowserParent::GetFrom(aEvent->mTarget) as done here? That would resolve the test failures.

EventStateManager::HandleCrossProcessEvent makes it seem to me it's not enough to only consider aEvent->mTarget. And I don't have a good understanding of when events are propagated to other docshells.

Is the opposite situation to test_continuous_wheel_events.html possible, i.e. an event originates in the docshell containing the target, is set uncancelable there, propagates to a remote docshell, and encounters a non-passive listener?

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

I guess it should be enough to check BrowserParent::GetFrom(aEvent->mOriginalTarget), since in case the docshells are in the same process, all the relevant EventTargets are in the event path and the code in the patch should notice if there are non-passive listeners.

BrowserParent::GetFrom() is a tiny bit slow, so perhaps call that only in the parent process? (It returns always false in content processes)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c57d28db5a72
Make touch and wheel events without non-passive listeners uncancelable. r=dom-core,edgar

Comment 18

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/52211 for changes under testing/web-platform/tests

Comment 19

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/c57d28db5a72

Comment 20

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot