WICG / interventions

A place for browsers and web developers to collaborate on user agent interventions.
Other
177 stars 28 forks source link

Default to passive: true on document level wheel/mousewheel event listeners #64

Closed sahel-sh closed 2 years ago

sahel-sh commented 5 years ago

Chromium/Blink would like to propose an intervention where the passive option for wheel/mousewheel event listeners added to Document level objects would be true by default. To opt out developers can explicitly specify passive: false when they want to prevent scrolling/zooming.

This intervention is the wheel equivalent of touch scroll intervention which is implemented by Chrome (since M56), Safari1, and Firefox2.

Our statistics show that:

See more details in Document Level Passive Wheel Event Listener Intervention


1: Tried on Safari 11 (IOS 11.4) 2: Tried on Firefox 61

hober commented 5 years ago

cc @smfr @grorg

sahel-sh commented 5 years ago

/cc @staktrace @smaug----

sahel-sh commented 5 years ago

/cc @ChumpChief

Since high precision touchpad scrolling on Edge is already not blocked on wheel events. Edge might be implicitly in favor of the intervention.

ChumpChief commented 5 years ago

Yes, Edge is in favor. I've seen too many analytics libraries throwing benign wheel listeners on the document :)

I'd guess most pages with document-level preventDefaults are also non-scrollable, since it's weird for a user to see overflowing content and a scrollbar but not be able to scroll with mousewheel. Maybe some "parallax" sites that eat the wheel to apply their own physics/animation? Even with this intervention these probably function approximately as expected if setting scrollTop in script wins over the scroll animation.

That said, I'm now observing that in Chrome the scroll animation wins over setting scrollTop in script. Is that expected? Is there a means for a developer to halt an in-progress scroll animation in Chrome?

http://jsfiddle.net/gcxof62e/

You might want to consider changing this behavior to match Edge/FF to further reduce the compat risk of the intervention.

P.S. Precision Touchpad doesn't fire any wheel events in Edge so it's perhaps not quite fair to compare against. But regardless this intervention seems good.

staktrace commented 5 years ago

I don't have any objections to this intervention either. It should be straightforward to implement in FF as well.

sahel-sh commented 5 years ago

That said, I'm now observing that in Chrome the scroll animation wins over setting scrollTop in script. Is that expected? Is there a means for a developer to halt an in-progress scroll animation in Chrome?

Since M65 Chrome is handling wheel events asynchronously: https://www.chromestatus.com/feature/5728708706959360 In http://jsfiddle.net/gcxof62e/ if the developer wants to set document.scrollingElement.scrollTop they should preventDefault the wheel event and specify that the event listener is blocking.

ChumpChief commented 5 years ago

Love the scroll latching and async events :)

This is a slightly different point though - regardless of whether a developer is setting scrollTop in response to the wheel event or in response to something else, it seems that developer cannot interrupt the animation.

In this specific case, the compatibility risk I'm highlighting is for document-level script takeover of scrolling (e.g. maybe Nicescroll would be an example). Since these are not specifying passive: false today, I suspect the script-driven scroll animation might fight with the native scroll animation for some strange visual effects. Whereas if setting scrollTop halts the native scroll animation like in Edge and FF, then I suspect the script-driven scroll animation would be able to take over from the native animation and function mostly as expected (maybe barring a frame or two of native scrolling animation), potentially partially mitigating that compatibility risk.

In any case, I'm in favor of the intervention. Just offering a suggestion in case issues are discovered on that type of construction.

sahel-sh commented 5 years ago

Love the scroll latching and async events :)

This is a slightly different point though - regardless of whether a developer is setting scrollTop in response to the wheel event or in response to something else, it seems that developer cannot interrupt the animation.

In this specific case, the compatibility risk I'm highlighting is for document-level script takeover of scrolling (e.g. maybe Nicescroll would be an example). Since these are not specifying passive: false today, I suspect the script-driven scroll animation might fight with the native scroll animation for some strange visual effects. Whereas if setting scrollTop halts the native scroll animation like in Edge and FF, then I suspect the script-driven scroll animation would be able to take over from the native animation and function mostly as expected (maybe barring a frame or two of native scrolling animation), potentially partially mitigating that compatibility risk.

In any case, I'm in favor of the intervention. Just offering a suggestion in case issues are discovered on that type of construction.

I understand your concern about the fight between script-driven scroll animation and the native scroll animation which is the current behavior of Chrome and Firefox (tried http://jsfiddle.net/gcxof62e/ on 60.2.0esr (64-bit)) However this intervention won't change/add any compatibility risks: Regardless of the value of the passive option, even in cases that it used to be false by default and this intervention changes it to be true, with async wheel events if the first wheel event is not prevented by default the rest of the wheel events in the current scroll sequence are not cancelable and the fight between script-driven scroll animation and the native scroll animation will happen anyway with our without this intervention.

pauljohnknight commented 5 years ago

Hi

Since this has been implemented it has broken a number of smooth-scroll plugins / libraries that remove the 'judder' created by a traditional mouse wheel when scrolling on Chrome.

How would you set explicitly set passive: false as mentioned in the original post on this thread via javascript, as a stand-alone function (to save me going through all the code of a plugin finding where best to place it)?

Any help would be wonderful.

Paul.

ghost commented 5 years ago

@RByers or whoever implemented this change , you studied web sites but did you study chrome extensions and chrome apps?

Chrome extensions share same zoom level across popup, background, options and all other HTML documents.

That means if a chrome extension is not able to prevent users from zooming in and out on certain pages then the zoom level for all pages gets messed up and UI across all documents in a chrome extension is broken, then their users start to complain about broken UI and give 1 star reviews and uninstall the extension or app.

Chrome extension developers prevent this issue by calling e.preventDefault() on zoom document level zoom events so the zoom level across all documents stays the same, but because this breaking change is shipped, all chrome extensions that relied on e.preventDefault() for preventing zoom events are now broken!

Because I work at a company that develops nothing but Chrome extensions I can tell you that all of our chrome extensions are broken because of this change, I'm not sure about how many chrome extensions currently shipped on chrome web store are affected by this change.

Also note that not only chrome extensions but many of the Google's own products such as Google AdWords, AdSens and other products that make use of Dart programming language and dart:html package and Angular Dart framework that call e.preventDefault() are all broken, because Dart at the moment does not support passive events, See this issue for more information: https://github.com/dart-lang/sdk/issues/26544

Above issue was opened on 26 May 2016 , but support for passive events is still not there as of 2019 this just shows it can take months, sometimes years to get these breaking changes to work well with the existing apps and extensions.

If you care so much about web developers going bankrupt because of slow web sites, then why not care about chrome extension developers going bankrupt because of messed up zoom levels.

Why there is no browser level API to change zoom levels so we don't have to use hacks like e.preventDefault();

I have to record tutorials on how to fix messed up zoom levels and show it to users that mail me that their UI is messed up.

But instead of fixing real issues we decide to create more non-issues like making events passive by default.

Node’s development direction favoured performance over usability and robustness, eventually backend developers got tired because of that and switched to better alternatives such as Golang.

I see chrome developers are also following a similar approach that favours raw performance over usability and robustness, and this discussing already shows how wrong that is :)

Never favour raw performance over usability and robustness.

In my opinion, robustness and usability is far more important than anything else, and because we are a small team we want our apps to be robust and at the end of the day we want to sleep well in the night rather than having to worry about some event handlers failing to do what they should be doing otherwise if this breaking change was not shipped.

grorg commented 5 years ago

Maybe I don't understand how this repository is supposed to work. Have Chrome/Blink + Firefox implemented this intervention? If so, why is this issue still open?

My specific question is whether WebKit should do it now, but my general question is how to know what the state of an intervention issue proposed here is.

bzbarsky commented 5 years ago

Firefox has not implemented this. It was discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=1526725 back in February.

sahel-sh commented 5 years ago

...

Chrome extension developers prevent this issue by calling e.preventDefault() on zoom document level zoom events so the zoom level across all documents stays the same, but because this breaking change is shipped, all chrome extensions that relied on e.preventDefault() for preventing zoom events are now broken! ... Above issue was opened on 26 May 2016 , but support for passive events is still not there as of 2019 this just shows it can take months, sometimes years to get these breaking changes to work well with the existing apps and extensions.

I am sorry that your extensions are negatively impacted by the intervention. However e.preventDefault() is not broken: If a document level wheel event listener needs to call e.preventDefault() for wheel events to keep the zoom level the same, it should get declared as blocking using the {passive:false} option. Please check this developer's guide for more details: https://developers.google.com/web/updates/2019/02/scrolling-intervention You can also file a bug using https://crbug.com/wizard and provide and example extension to get better guidance.

Please not that the intervention for touchmove and touchend has been shipped in chrome 56, and implemented by Edge, Firefox, and Safari layer on. This new issue (opened on Dec 2018) is for wheel events.

sahel-sh commented 5 years ago

/cc @NavidZ for follow up since I am not working on this project any longer.

ghost commented 5 years ago

Passing {passive:false} to event listeners that call e.preventDefault() is easy only if the code is written in JavaScript.

Obviously, the event listeners are not broken but their default behaviour is changed causing them to indirectly break.

These days most web development is done using transpilers such as Dart, Elm, TypeScript, GWT, GopherJS to name a few.

Above transpilers care more about robustness and less about performance, because first they have to be correct before they can optimise, if the transpiled code is not correct then all the optimisations are useless.

JS transpilers are written with consideration of certain things to be true such as all event listeners receiving {passive:false} as defined by specifications. By breaking this assumption you have invalidated their work.

Some languages that transpile to JS are strongly typed and some of them did not have the positional or optional parameter passive defined in the function call to addEventListener, now they are confused whether the default value of this parameter be false to make old apps work, or should it be true to comply with the interventions. Languages that do not support optional parameters, when they will add passive parameter to the function call to addEventListener their analysis plugin will start to show errors where addEventListener is called.

It is not the static sites like CNN that you used in the demo that are affected, it is the SPAs, Chrome apps, Chrome Extensions, Electron apps, JavaScript transpilers that are severely affected, does your team take all the above into consideration when you implement breaking changes like the above?

If your team wants to go out off their way to to implement breaking changes that go against the spec and affect everybody on the web then does you team also go out off their way to make sure that the changes are as less painful as possible to implement and to be used by everybody?

Does your team inform library maintainers, and developers who work on popular transpilers before implementing a breaking change, for example: by creating issues?

Even the people who wrote transpilers did not know about the changes made to event listeners that were shipped in 2017, I had to go and inform them about the changes and why it is important to implement support for passive events across event listeners, this just shows even those who are affected are not informed about the changes.

When working with transpilers, stacktraces are sometimes not useful. Transpilers may log countless warnings to the console, so if calling e.preventDefault() worked previously but did not work recently and there are hundreds of warning in the console the chances are the warning thrown by calling e.preventDefault() will go unnoticed.

To mark event listeners as passive in a transpiled code first a developer has to wait for months for support for passive events to be implemented. because the developers who worked on transpilers were not informed about the changes. After that old event listeners can be updated.

In our use case, our chrome extensions are required to be manually reviewed by chrome team before they can be released to chrome web store. This review process can take up to 7 days.

So it takes quite a while for the developers to notice and adapt to the changes because of not being informed.

Looking at the previous breaking changes your team has made, your team does not care about robustness, all they care about is raw performance on mobile devices achieved at the expense rest of the web.

Refactoring all event listeners across all projects will definitely take some time but does your team give enough time for everybody to catch up and implement the change? In 2017 when you made changes to the touch events, Google's own projects were broken.

Also Chrome Canary browser is not available to Linux users which means a huge group of developers do not get the opportunity to test the changes and see them in effect, they do not see how these changes affect all the apps until they are shipped to production versions of Chrome. Linux users are the ones who are happy to test bleeding edge software give feedback and improve it as well, but Canary not being available which means they are left in the dark until changes are shipped in production version of Chrome.

NavidZ commented 5 years ago

It is not the static sites like CNN that you used in the demo that are affected, it is the SPAs, Chrome apps, Chrome Extensions, Electron apps, JavaScript transpilers that are severely affected, does your team take all the above into consideration when you implement breaking changes like the above?

We certainly considered all the URLs that this intervention might have broken. That will include all the sites including the ones with Javascript transpilers. We also worked with popular frameworks to get this handled on their side such as react.

Looking at the previous breaking changes your team has made, your team does not care about robustness, all they care about is raw performance on mobile devices achieved at the expense rest of the web.

Refactoring all event listeners across all projects will definitely take some time but does your team give enough time for everybody to catch up and implement the change? In 2017 when you made changes to the touch events, Google's own projects were broken.

We do our best in terms of consulting all developers and other browser vendors through GitHub issues, blogposts, console warnings, and whatnot. One of the problems with improving web in general is that there will be always a long tail of websites/usecases that might never go away. We just have to make a call and say if a particular legacy feature usage has gone lower than a very very small threshold that will be good enough to change to make the overall performance of the rest of the web better. It is like breaking a very small percentage of the usecases to get a better performance for the rest of the web. We understand being in that small portion of the population is frustrating but consider a lot of other interventions that you were not on that small portion and your app got the benefit without noticing anything.

I'm still not sure about your particular usecase to dig further and suggest any other actions for you. But just as another workaround (not sure whether it applies to your usecase or not) you could try having your listener not on the document but in a more nested element. That way it is not going to be affected by this intervention until more frameworks add the support for event listener options.

ghost commented 5 years ago

I recorded a video that shows broken Chrome extensions because of shared zoom level:

https://youtu.be/3NdxRv3LGp8?t=86

Above video demonstrates how some popular chrome extensions that have more than 1,700,000+ combined users are affected by the shared zoom level and above intervention.

Here is my proposed solution: Zoom levels for popup, options, omnibox, and injected iframes of a chrome extensions must be set to 100% by default so there will be no need of zoom disablers.

Above issue can be prevented by using this Dart code :

/// returns whether zoom was disabled or not
bool _disableZoom() {
  if (_isZoomDisabled) {
    return false;
  }
  // disabling use of control +/- for zooming
  html.window.document.onKeyDown.listen((e) {
    if (e.ctrlKey || e.metaKey) {
      int c;
      if (e.which != null) {
        c = e.which;
      }
      if (e.keyCode != null) {
        c = e.keyCode;
      }
      print('c is ${c}');
      if (c == 187 || c == 189 || c == 107 || c == 109) {
        e.preventDefault();
        // ignore: cascade_invocations
        e.stopPropagation();
        print('propagation stopped');
      }
    }
  });
  // disabling use of mousewheel for zooming
  html.window.onMouseWheel.listen((e) {
    if (e.ctrlKey || e.metaKey) {
      e.preventDefault();
      // ignore: cascade_invocations
      e.stopPropagation();
    }
  });
  _markZoomAsDisabled();
  return true;
}

but this code is now invalidated by above intervention.

Now many chrome extension developers will be forced to release a fix, it is easy for us to release a fix, but it is not easy for others who are not aware of this intervention and shared zoom levels.

ghost commented 5 years ago

I made a bug report: https://bugs.chromium.org/p/chromium/issues/detail?id=957115

NavidZ commented 5 years ago

@drbcode as we fixed the bug you reported I assume now your use case is addressed. Is that fair enough or do you have other concerns?

ghost commented 5 years ago

@NavidZ No other concerns! Thanks for addressing the issue.

Now we will not get complaints from users about broken zoom levels.

I was very happy when the fix was implemented in Chrome :)

NavidZ commented 5 years ago

I'm glad to see your issue fixed. Please feel free to file bugs and be vocal when you seen something wrong and we can always look into them and discuss solutions and workarounds.

jabcreations commented 4 years ago

This should not be implemented. This breaks GUI fixes (Gecko is blatant GUI breaker and most GUI bugs/regressions are not fixed). This will make gecko even more unbearable to program with and make engines that do not implement this more cooperative for UX.

In example I've ensured that in competent browsers that the X and Y axis scrolling while hovering range type input elements adjusts the input's value. However in Blink/Chrome if the parent element has a scrollbar it will scroll losing focus thereby negating the user's intuition to use their scrollwheel.

You are literally advocating that that you know better than web authors and I can dig up two decades worth of almost entirely ignored bug reports to show Mozilla is only further alienating the people who actually know WTF we're doing.

Stop dictating what you think web authors should be "allowed" to do.

Stop blindly copying Google.

bokand commented 4 years ago

Web authors can still use non-passive listeners and call preventDefault by specifying passive: false in the event listener. This only changes defaults which will impact legacy pages that don't update. While that's a concern, the trade-off has been carefully considered.

foolip commented 3 years ago

What is the status of getting this behavior into a spec? In https://github.com/whatwg/dom/issues/365#issuecomment-787531650 it was noted that we have tests for this in WPT, but seemingly no spec, so I've renamed the test in https://github.com/web-platform-tests/wpt/pull/29039.

bokand commented 3 years ago

+@flackr

johannhof commented 2 years ago

(As noted in https://github.com/WICG/interventions/pull/72, we intend to archive this repository and are thus triaging and resolving all open issues)

As noted in #35 I think we can continue to track this work under https://github.com/whatwg/dom/issues/365, see https://github.com/whatwg/dom/issues/365#issuecomment-1098416958