Closed Bug 1449268 Opened 6 years ago Closed 6 years ago

Treat document-level touch event listeners as passive. (Chrome scrolling intervention)

Categories

(Core :: DOM: Events, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Performance Impact ?
Tracking Status
relnote-firefox --- 61+
firefox-esr52 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: cpeterson, Assigned: smaug)

References

Details

(4 keywords, Whiteboard: [geckoview:klar][webcompat])

Attachments

(2 files, 2 obsolete files)

touchstart and touchmove event listeners on body, document and window would be treated as passive by default, which means they cannot preventDefault.

Google implemented this touch event intervention in Chrome 56 to improve scrolling performance on mobile sites.

Google's announcement:
https://developers.google.com/web/updates/2017/01/scrolling-intervention

The Chrome bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=639227

The original proposal of the intervention:
https://github.com/WICG/interventions/issues/18

WebKit implemented this intervention in iOS 11.3 Beta, but it broke some sites apparently because WebKit does not support the recommended `touch-action: none` workaround for sites that want to preserve the pre-intervention behavior. We should make sure this case works correctly in Firefox.

https://bugs.webkit.org/show_bug.cgi?id=182521
Whiteboard: [geckoview:klar] → [qf][geckoview:klar]
Olli told me we "just" need to look at the target and change the default to be passive. He doesn't think it's a huge amount of work.
Priority: -- → P2
Assignee: nobody → bugs
Is there any telemetry we want to collect for this change, such as many touch event listeners are affected?
Summary: Should Firefox treat document-level touch event listeners as passive? (Chrome scrolling intervention) → Treat document-level touch event listeners as passive. (Chrome scrolling intervention)
Need to remove the default value for AddEventListenerOptions.passive and then based on the target set the default.

Spec bug https://github.com/w3c/touch-events/issues/74
Also need to ensure our touch-action handling works the way the intervention requires.
Attached patch wip (obsolete) — Splinter Review
Untested wip, which may require some touch-action: auto changes.
Blink patch was surprisingly large https://chromium.googlesource.com/chromium/src.git/+/e913a6698f089e11e89fcc04ee19d34dac82d30a%5E%21/
nm those touch-action stuff, in blink they were for usecounter.
Attached patch wip (obsolete) — Splinter Review
This even compiles :)


remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/4afc1af5afa5b12d98f3b2cb5e76ef0ae456d583
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=4afc1af5afa5b12d98f3b2cb5e76ef0ae456d583
remote: recorded changegroup in replication log in 0.132s

Assuming it compiles on all the platforms, feel free to take a binary from tryserver and test. (it will be Easter break here, so I'll take a look at this next week if no one else has done it before that)
Attachment #8963742 - Attachment is obsolete: true
I'm not too happy with this stuff, since it makes web platform less consistent and web devs don't seem to really like it. But given that Blink and webkit are shipping this, there isn't much point to fight against (anymore. I did try to fight back before Blink was shipping).

The reason for Optional<bool> in EventListenerManager::AddEventListener is that we want to do event type check only in AddEventListenerByType since that is where we get EventMessage and can do fast comparison.


remote: View your changes here:
remote:   https://hg.mozilla.org/try/rev/1e1073bb1a7a546c001460c79d23168e1b21bdc8
remote:   https://hg.mozilla.org/try/rev/a3ba3ec260920f0365fc3d398fb715d985a0c318
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3ba3ec260920f0365fc3d398fb715d985a0c318
remote: recorded changegroup in replication log in 0.102s
Attachment #8963760 - Attachment is obsolete: true
Attachment #8964908 - Flags: review?(bugmail)
Comment on attachment 8964908 [details] [diff] [review]
touch_default_passive.diff

Review of attachment 8964908 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed

::: dom/events/EventListenerManager.cpp
@@ +1392,2 @@
>      flags.mOnce = options.mOnce;
> +    if (options.mPassive.WasPassed()) {

Just to make sure I understand, options.mPassive is changing from a regular bool to a Optional<bool> because you remove the default value in the .webidl, is that correct?

Does flags.mPassive need to be initialized here anyway? It's not clear to me what the type of |options| is and whether it initializes its members to zeros or not, but if not then we might need to ensure flags.mPassive is set to something that's not $random_memory.

::: gfx/layers/apz/test/mochitest/helper_tap_default_passive.html
@@ +3,5 @@
> +<head>
> +  <meta charset="utf-8">
> +  <meta name="viewport" content="width=device-width; initial-scale=1.0">
> +  <title>Ensure APZ doesn't wait for passive listeners</title>
> +  <script type="application/javascript" src="apz_test_native_event_utils.js"></script>

I'm assuming this is effectively a straight copy of helper_tap_passive with the explicit passive flag removed. If there's any significant changes let me know.

::: gfx/layers/apz/test/mochitest/mochitest.ini
@@ +41,5 @@
>      helper_subframe_style.css
>      helper_tall.html
>      helper_tap.html
>      helper_tap_fullzoom.html
> +    helper_tap_default_passive.html

Move this up one to keep alpha order
Attachment #8964908 - Flags: review?(bugmail) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10)
> Comment on attachment 8964908 [details] [diff] [review]
> touch_default_passive.diff
> 
> Review of attachment 8964908 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments addressed
> 
> ::: dom/events/EventListenerManager.cpp
> @@ +1392,2 @@
> >      flags.mOnce = options.mOnce;
> > +    if (options.mPassive.WasPassed()) {
> 
> Just to make sure I understand, options.mPassive is changing from a regular
> bool to a Optional<bool> because you remove the default value in the
> .webidl, is that correct?
yes.

> 
> Does flags.mPassive need to be initialized here anyway?
No.
The flag is false by default
https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/dom/events/EventListenerManager.h#71


 It's not clear to me
> what the type of |options| is
I think you mean flags here, not options.

 and whether it initializes its members to
> zeros or not, but if not then we might need to ensure flags.mPassive is set
> to something that's not $random_memory.
Flags is just a normal C++ class with constructor explicitly initializing member variables.
And aOptions is coming from webidl bindings


> I'm assuming this is effectively a straight copy of helper_tap_passive with
> the explicit passive flag removed. If there's any significant changes let me
> know.
it is

> @@ +41,5 @@
> >      helper_subframe_style.css
> >      helper_tall.html
> >      helper_tap.html
> >      helper_tap_fullzoom.html
> > +    helper_tap_default_passive.html
> 
> Move this up one to keep alpha order
ops. I wonder why mach didn't complain
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/99e592730f87
Treat document-level touch event listeners as passive, r=kats
https://hg.mozilla.org/mozilla-central/rev/99e592730f87
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Are we implementing `touch-action: none` as well?

Are we throwing a console warning, similar to Chrome (see https://developers.google.com/web/updates/2017/01/scrolling-intervention#breakage_and_guidance)?
Flags: needinfo?(hkirschner)
(In reply to Andreas Bovens [:abovens] from comment #17)
> Are we implementing `touch-action: none` as well?

This is supported in Firefox since 36. I don't know about any special treatment that is needed for the intervention.

> Are we throwing a console warning, similar to Chrome (see
> https://developers.google.com/web/updates/2017/01/scrolling-
> intervention#breakage_and_guidance)?

We don't have a good policy yet for landing interventions, but coordinating console messages with them is a low hanging fruit. I filed bug 1449268, but I don't think it should block this work shipping:

Chrome released the intervention 1 year ago, which is plenty of time for developers to notice and fix any breakage by making their event listeners passive. Passive event listeners also don't impact other browsers, so those changes are not behind browser detection.

Numbers are a bit sparse for this:
- Passive event listeners jumped recently from 10 to 20 (% of page loads): https://www.chromestatus.com/metrics/feature/timeline/popularity/1417
- Not 100% sure about this measure, but it might be a counter for interventions (happening on less than 0.5 of page loads): https://www.chromestatus.com/metrics/feature/timeline/popularity/1683
Flags: needinfo?(hkirschner)
Needinfo-ing Mike to get this on his radar.

Chris, if you haven't done so already, could you please send a PI request to make sure we don't see any breakage from this change?
Flags: needinfo?(miket)
Flags: needinfo?(cpeterson)
Whiteboard: [qf][geckoview:klar] → [qf][geckoview:klar][webcompat]
(In reply to Panos Astithas [:past] (please ni?) from comment #19)
> Chris, if you haven't done so already, could you please send a PI request to
> make sure we don't see any breakage from this change?

Thanks for the reminder! I'll submit a request.
Flags: needinfo?(cpeterson)
Thanks for the info, Harald. The console warning is indeed not a blocker, but could be a nice-to-have.

Great to see this ship so soon!
Depends on: 1452751
Thanks!(In reply to Panos Astithas [:past] (please ni?) from comment #19)
> Needinfo-ing Mike to get this on his radar.

thanks!
Flags: needinfo?(miket)
Documentation updates completed:

https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#Improving_scrolling_performance_with_passive_listeners (substantially revised)

BCD update submitted for addEventListener() to include Firefox 61 as supporting this change

https://developer.mozilla.org/en-US/docs/Web/Events/touchstart updated

https://developer.mozilla.org/en-US/docs/Web/Events/touchmove updated

https://developer.mozilla.org/en-US/docs/Web/API/Touch_events#Browser_compatibility updated

https://developer.mozilla.org/en-US/Firefox/Releases/61 updated to list this change

Let me know if there are any issues; otherwise, this documentation is considered up to date.
Clearing webcompat? flag because this feature already shipped in Firefox 61 and its developer documentation has been updated (in comment 25).
Flags: webcompat?
See Also: → 1574223
Performance Impact: --- → ?
Whiteboard: [qf][geckoview:klar][webcompat] → [geckoview:klar][webcompat]
You need to log in before you can comment on or make changes to this bug.