Closed Bug 1449268 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
DOM: Events
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Project Flags:
Performance Impact ?
Tracking Flags:
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)

touch_default_passive.diff
(deleted), patch
kats
: review+
Details | Diff | Splinter Review
touch_default_passive_2.diff
(deleted), patch
Details | Diff | Splinter Review
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) (deleted) — — 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) (deleted) — — 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
Attached patch touch_default_passive.diff (deleted) — — Splinter Review
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: 7 years ago
status-firefox61: affected → fixed
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.
Keywords: dev-doc-needed → dev-doc-complete
Clearing webcompat? flag because this feature already shipped in Firefox 61 and its developer documentation has been updated (in comment 25).
Flags: webcompat?
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.

Attachment

General

Creator:
Created:
Updated:
Size: