Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Specify addEventListener default passive hack for smoothscroll.js #1088

Closed
zcorpan opened this issue Jun 17, 2022 · 11 comments
Closed

Specify addEventListener default passive hack for smoothscroll.js #1088

zcorpan opened this issue Jun 17, 2022 · 11 comments

Comments

@zcorpan
Copy link
Member

zcorpan commented Jun 17, 2022

So, Chromium and WebKit also implement this hack for smoothscroll.js:
https://bugs.chromium.org/p/chromium/issues/detail?id=501568
https://github.com/WebKit/WebKit/blob/dd956d5e74249681ddf904e0bbe401f308b65e0f/Source/WebCore/page/Quirks.cpp#L929-L946

As far as I can tell, Gecko does not. https://searchfox.org/mozilla-central/source/dom/events/EventListenerManager.cpp#1593 (and no search results for "ssc_wheel")

Should we put this in the spec?

The use counter is currently at ~0.01% of page views. But scrolling the page doesn't work at all without the hack.
https://chromestatus.com/metrics/feature/timeline/popularity/2020

Originally posted by @zcorpan in #365 (comment)

@domenic said

IMO we should put that hack in the spec, although it could perhaps be done separably since it's such a ... unique ... hack.

@annevk
Copy link
Member

annevk commented Jun 17, 2022

cc @smaug---- @EdgarChen

@zcorpan
Copy link
Member Author

zcorpan commented Jun 17, 2022

As noted in https://bugs.chromium.org/p/chromium/issues/detail?id=501568

firefox doesn't support 'mousewheel' events, only the standard 'wheel' events

Removing support for mousewheel events from Chromium and WebKit seems like a possible alternative solution here.

Chromium has use counters for only mousewheel, only wheel, or mousewheel+wheel listeners being invoked (implementation)

Pretty high numbers... How come Firefox is able to not support the mousewheel event? I found this bug Consider supporting the legacy mousewheel event for interop which cites 3 webcompat.com issues, so they're taking some web compat hit by not supporting it.

In httparchive (total of 14,951,058 pages), there are 0 pages that hit the SmoothScrollJSInterventionActivated.

query
SELECT
  url
FROM
  `httparchive.pages.2022_06_09_mobile`
WHERE
  STRPOS(payload, r'SmoothScrollJSInterventionActivated') > 0

@zcorpan
Copy link
Member Author

zcorpan commented Jun 17, 2022

Hmm. I wonder if the SmoothScrollJSInterventionActivated is broken. The sample URLs chart in https://chromestatus.com/metrics/feature/timeline/popularity/2020 had a sudden cutoff in April 2019.

When looking for the pragma that would invoke it (("mousewheel", ssc_wheel)) in response_bodies, in the sample 10k dataset, I find 30 pages (0.3%, which is in line with the pre-April 2019 use counter numbers). Query results.

query
SELECT
  page,
  url
FROM
  `httparchive.sample_data.response_bodies_mobile_10k`
WHERE
  REGEXP_CONTAINS(body, r'\(\s*["\']mousewheel["\']\s*,\s*ssc_wheel\s*\)')

@zcorpan
Copy link
Member Author

zcorpan commented Jun 17, 2022

Ooooh, I finally get it. It's dead code! Since the intervention we're specifying for #365 already makes all mousewheel listeners on window passive (unless passive is specified), this hack does literally nothing. It can be removed.

@zcorpan
Copy link
Member Author

zcorpan commented Jun 17, 2022

@RByers
Copy link
Contributor

RByers commented Jun 17, 2022

Oh, very cool! Sorry none of us put that together Simon! I was going to chime in here to say that back when I was looking into this I found a LOT of UA sniffing treating Firefox differently, and so I did expect that we'd need to specify this hack just for browsers claiming to be WebKit. But removing it is, of course, even better. Thanks for filing the issue!

@RByers
Copy link
Contributor

RByers commented Jun 17, 2022

Note that the problematic listener gets installed on body, not window. But chromium makes document and body listeners passive by default too, so I assume that's what's getting specced?

@zcorpan
Copy link
Member Author

zcorpan commented Jun 17, 2022

Yes, all of window, document, root element and "the body element"

@dtapuska
Copy link

Yes this hack forged the path for the larger intervention proving it was doable. I've removed it from blink.

@domenic
Copy link
Member

domenic commented Jun 17, 2022

Really great find Simon!! I guess we can close this issue now :).

@zcorpan
Copy link
Member Author

zcorpan commented Jun 20, 2022

Yep. I've also filed an issue for WebKit: https://bugs.webkit.org/show_bug.cgi?id=241782

@zcorpan zcorpan closed this as completed Jun 20, 2022
webkit-commit-queue pushed a commit to karlcow/WebKit that referenced this issue Nov 20, 2023
https://bugs.webkit.org/show_bug.cgi?id=241782
rdar://95972172

Reviewed by Antti Koivisto.

In whatwg/dom#1088, Simon Pieters analyzed
the hack introduced by WebKit and Blink to determine if it was
necessary to implement it in Gecko, but during the investigation
he discovered that a part of the Quirk being dead code. Blink removed
this hack on June 2022.
https://bugs.chromium.org/p/chromium/issues/detail?id=1337217
It should be possible to remove it on WebKit as well.

* Source/WebCore/dom/EventTarget.cpp:
(WebCore::EventTarget::addEventListener):
* Source/WebCore/page/Quirks.cpp:
(WebCore::Quirks::shouldMakeEventListenerPassive):
* Source/WebCore/page/Quirks.h:

Canonical link: https://commits.webkit.org/270987@main
aperezdc pushed a commit to WebKit/WebKit that referenced this issue Jan 25, 2024
…gi?id=241782

    Remove Quirk dead code: passive mousewheel event for smoothscroll.js
    https://bugs.webkit.org/show_bug.cgi?id=241782
    rdar://95972172

    Reviewed by Antti Koivisto.

    In whatwg/dom#1088, Simon Pieters analyzed
    the hack introduced by WebKit and Blink to determine if it was
    necessary to implement it in Gecko, but during the investigation
    he discovered that a part of the Quirk being dead code. Blink removed
    this hack on June 2022.
    https://bugs.chromium.org/p/chromium/issues/detail?id=1337217
    It should be possible to remove it on WebKit as well.

    * Source/WebCore/dom/EventTarget.cpp:
    (WebCore::EventTarget::addEventListener):
    * Source/WebCore/page/Quirks.cpp:
    (WebCore::Quirks::shouldMakeEventListenerPassive):
    * Source/WebCore/page/Quirks.h:

    Canonical link: https://commits.webkit.org/270987@main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

5 participants