-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
Chrome 73 breaks wheel events #14856
Comments
We’ve definitely considered adding support for passive events and that’s on our longer term roadmap, but we’d rather not rush such an important API addition because of the Chrome intervention timings. |
Do you mind creating a small repro case to verify it's broken now? |
Sure thing, here's a repro: https://codesandbox.io/s/6zn44nmjvn In Chrome stable / Safari / etc the box will move but the rest of the page will remain static. In Chrome 73, the entire page rubber bands as you scroll around (the box also moves). Since rubber banding only happens with a trackpad, make sure to test it with one. Also note all the red errors in the console due to the intervention. |
I have mixed feelings on it, but I wonder if this is the best approach. We don't need to feature check for event options support either because all scroll events are captured anyway so the object coercing to true is fine:
This at least keeps the React behavior consistent while passive events are being figured out, but definitely presses the need to add more configuration to events. |
For me, at least on Linux and ChromeOS, this is easier to reproduce with an actual mouse with a mousewheel (it could be that OSX emits mouse wheel events for trackpads). I added a linear gradient to the background to better illustrate what's going on (event.preventDefault() should prevent scrolling on the parent, but it doesn't): Here's a vanilla example that uses The solution could be to just pass |
I think this goes against the wishes of the Chrome team, but it's the only solution I can think of to maintain existing behavior while a passive event API is being figured out. |
Talking with @sophiebits: it sounds like we should hold off on making a change until we clamp down a passive event listener API. We shouldn't undermine changes Chrome believes are important for improving site performance. If they have found that most scroll/wheel event listeners should be passive, that extends to React apps as well. We're shouldn't contribute to making apps slower for most cases when they don't need to be. Until a passive event API is available to React, one way to work around this is to use a ref to attach a non-passive event listener. (Edit: this is a work in progress. See #15036). I'm not super familiar with using TypeScript with hooks, but I've done my best to form @blixt's example to use a ref to attach a passive listener: https://codesandbox.io/s/zzqxp1yvy3 I imagine others will come to the issue board confused about this change, so it'll be important to have a clear path forward for future issues. Are there ways to improve on the solution above? |
Chrome shouldn't be changing default options that have such a significant impact on usability. Every site that relies on default |
@byronwall I strongly believe the Chrome team thinks very carefully and thoughtfully about the changes they make, and I don't believe it is right to call into question what they should or should not be doing without the full holistic picture of the drive behind the change and the expected outcome. It might be nice to get direct input here from someone who can provide a larger context that can help drive the path forward in a healthy way. |
This comment has been minimized.
This comment has been minimized.
@catamphetamine Sorry to hide your comment, but it doesn't help us come to a resolution and I want to keep this issue focused for others coming to the React issue board that might be confused. The Chrome team didn't do this in a vacuum, and took the time to research this thoroughly, as linked in the original issue description (https://developers.google.com/web/updates/2019/02/scrolling-intervention):
Much like React, Chrome is in a position to improve user experience across a broad base of users. Both teams are aligned in this goal, even if coordinating on a change like this could have been smoother. Forcing wheel events to be impassive would create additional breaking changes for React users, while undermining performance improvements on the platform. Let's figure out the best API for passive event handlers in React moving forward. However in the interim, let's also come up with a good general purpose solution so that it's easier for React users to handle this change. |
Translated: we're comfortable breaking 2% of those sites which currently rely on default behavior... My main issue is with Chrome and not React. Having said that, it's sensible to "unbreak" React applications relying on previously default behavior while a firm API is proposed for declaring passive events. The push for potential performance gains should be balanced against backward compatibility. In this case Chrome has overstepped. Going against their grain until the full API is in place seems a minor transgression.
What breaking changes are there? Until a week ago this was the default behavior. It is still the default behavior for non-Chrome browsers. Forgive me if I am missing the larger picture here? |
@byronwall I can see how websites are broken anyway be it requiring the manual addition of |
This is a temporary fix I've been using in the load file import React from 'react';
import ReactDOM from 'react-dom';
import App from './pages/App';
const EVENTS_TO_MODIFY = ['touchstart', 'touchmove', 'touchend', 'touchcancel', 'wheel'];
const originalAddEventListener = document.addEventListener.bind();
document.addEventListener = (type, listener, options, wantsUntrusted) => {
let modOptions = options;
if (EVENTS_TO_MODIFY.includes(type)) {
if (typeof options === 'boolean') {
modOptions = {
capture: options,
passive: false,
};
} else if (typeof options === 'object') {
modOptions = {
passive: false,
...options,
};
}
}
return originalAddEventListener(type, listener, modOptions, wantsUntrusted);
};
const originalRemoveEventListener = document.removeEventListener.bind();
document.removeEventListener = (type, listener, options) => {
let modOptions = options;
if (EVENTS_TO_MODIFY.includes(type)) {
if (typeof options === 'boolean') {
modOptions = {
capture: options,
passive: false,
};
} else if (typeof options === 'object') {
modOptions = {
passive: false,
...options,
};
}
}
return originalRemoveEventListener(type, listener, modOptions);
};
ReactDOM.render(<App />, document.getElementById('root')); |
Is it possible we can leave the removeEventListener as is since it does only look at the
|
@kheyse-oqton It's probably fine, but it is generally best practice to add and remove event listeners with identical options. |
I have a issue when the listeners are parsed. I think some listeners are not eliminated from the document.body or something like this, because something is strange, at mousemove event, some functions are executed. These listeners are from stripe or braintree I guess and this running a xhr request on mousemove. When I remove your solution, it works well. |
@yspektor That's a great temporary solution, though when defining modOptions you probably ought to include passive before ...options (eg. { passive: false, ...options }). That way it is still possible for third party libraries to add and remove passive listeners to and from the document as needed. @AlexRatiu Try reversing the order of passive and ...options in the modOptions declaration (in both functions) and see if that solves your problem. If any third-party libraries add passive listeners to the document prior to the execution of your code, then they will be unable to remove them when needed at a later point. |
This Chrome update is causing us problems. I'm building a react component library that is intended to be used by the general public, so polluting the global scope by modifying native methods on the document object is probably not going to be a valid option for us. Unfortunately resorting to native listeners doesn't work for us either since some of our components are using portals and native events don't propagate up the virtual dom through portals the same way that React events do. Any suggestions? |
Recently chrome changed the behavior of wheel listeners to be passive by default. Passive listeners cannot cancel event propagation. When a wheel event propagates outside of the canvas it can drastically slow down the browser as the parent elements try to compute whether or not they can scroll. This change uses manual event listener management to add active listeners, which is inline w/ the behavior for chrome before version 73. Test plan: use mousewheel. Notice there are no console.warn messages about passive listeners anymore. Semver impact: this is semver patch. Bugfix only. Further reading: facebook/react#14856 facebook/react#6436
Ran into this problem with trying to prevent browser zoom on control + wheel and performing a scaling action in our app instead, since native events didn't fit our use case I ended up with the following solution.
The use effect will bind a non passive event that prevents browser zoom and cleans itself up on unmount, then leaves your react event listeners to do their thing. |
@patrickmccallum I like the simplicity of this solution. But won't it end up |
Yep, that's correct. It's valid in our use case since we want to be able to trigger an app zoom from anywhere on the page, but it would stop you from scrolling for example a list that overflows. To fix that you'd just attach an if statement to check if the ctrl key is down in the event in cancelWheel.
I haven't looked into it much, but this could actually now be fixed in React 17 as per this blog post. |
Ok, read this whole thread, didn't find what I was hoping to find: away to tell React to NOT use passive event on a specific element, or at least some way to tell React to bind a specific event on that element instead of on the root element (as a delegated event) So, I want to stop the page from scrolling while the wheel (mouse for that matter) is being used over a specific range input field ( https://stackoverflow.com/a/65795791/104380 Here's my copied answer from stackoverflow: const wheelTimeout = useRef()
const onWheel = e => {
// ... some code I needed ...
// while wheel is moving, do not release the lock
clearTimeout(wheelTimeout.current)
// flag indicating to lock page scrolling (setTimeout returns a number)
wheelTimeout.current = setTimeout(() => {
wheelTimeout.current = false
}, 300)
}
// block the body from scrolling (or any other element)
useEffect(() => {
const cancelWheel = e => wheelTimeout.current && e.preventDefault()
document.body.addEventListener('wheel', cancelWheel, {passive:false})
return () => document.body.removeEventListener('wheel', cancelWheel)
}, []) Where |
Let me summarize the issue:
Therefore, there are two solutions. Either React adds some way to control passive-ness of events (tracked in #6436). Or you use native event listeners for the range of use cases where you really need this. Since using native event listeners already works (and has always worked) I don't think there's anything actionable left in this issue. Whatever API React could theoretically provide, it won't give you the same amount of flexibility that using the browser API directly would give you. So it seems like a reasonable solution in the meantime. |
Incorrect,
https://developers.google.com/web/updates/2019/02/scrolling-intervention
Unrelated. We are using React to create non-root elements, so
You are doing the opposite of how web browsers work
Native event listeners work, so React should also work. |
Thank you, I'm aware of that. At the time the change was introduced, React was subscribing on the I understand your point of view but I hope you see where I'm going with this as well. If not, I'm not sure I can do much to convince you. But the workaround using native event listeners still works. |
EDIT: This part actually has already been discussed in the thread. I'm sorry to repeat.
So changing it back to non-passive won't break anything. It's a good chance to make the change.
The "workaround" requires users to search the issue tracker (and in "closed" category now), at least it should be in the documentation. |
It will undo the spirit of Chrome's performance-motivated change for the websites using React. Since a lot of React websites are full-screen apps that mount close to the root, I would expect the same performance drop that the change was originally meant to solve. I agree that Chrome's change was disruptive, but at least it has achieved its purpose. It seems backwards to undo that performance improvement now that people have already implemented workarounds (and the most common cases indeed don't need active wheel listeners).
Yes, it's entirely fair that this should be documented. We're working on new docs now so I'll make a note to mention this. Also happy to review a PR if someone wants to send one now. Another thing we could do is to provide a warning on the synthetic wheel event when you call |
As for discoverability, all three top results for "chrome wheel events preventdefault" are relevant so I don't expect that closing the issue has any impact on this. |
@yairEO That is a horrible, horrible hack. It makes me cry. That being said, it's the only solution that worked for me, so I have encapsulated it in a hook:
Usage:
|
This isn't always accurate:
The primary issue I had a year ago is that native event listeners do not actually work the way that you usually need them to when used by an ancestor of a component that is rendered inside a portal created by ReactDOM.createPortal(). In these cases a synthetic React event that is generated inside the portal will properly propagate up through React's virtual DOM and can thus be addressed by an ancestor of the portaled component using a React event listener. If that same ancestor attempts to use a native event listener instead it won't ever fire off since the native event bubbles up through the actual DOM tree instead of the virtual DOM tree. This ended up being a huge issue for the component library that I was working on at that time which resulted in us needing to scrap our original plan and settle for sub-optimal alternatives to what we were trying to accomplish. Prior to React adding in a way to control the passive-ness of events, is there anything else you could recommend to get around this particular problem? |
I might be "out of league" and I'm probably not seeing the big picture here, but can't we just have an API change? A general support for My idea would be to add support to accept arrays for event props.
This would give us fine-grained control over any event options, and it can keep the default behavior. |
Is there a solution yet? It's really important that React doesn't break existing browser functionality, and So if there's a proper/clean solution, please provide one. For the length of time that this issue has been ongoing, frankly I bloody well expect one. How hard can it be? Other event can be preventDefault'ed perfectly fine. |
So we changed from "make it correct, make it fast, make it pretty", to "make it fast, make it correct, make it pretty" now? Urgh... That's not the direction things should go in. Correctness is more important than performance, because you can make a default correct system fast by optimizing it, but it's ridiculous to make a default incorrect system fast, because the incorrect thing doesn't actually solve your problem. |
This happens to me on Firefox now as well. What a dumb gotcha. And the justification is just as dumb: "We don't need to fix the framework cuz you can use the escape hatch in the framework to make it work right!" Then why am I bothering with your framework? |
@gaearon Please reopen, since a proper fixes has not yet been provided. As long as this issue is closed, it might get overlooked or assumed to be solved by contributors. |
Replaces the mouse wheel action in 2.3. There are problems with preventing default on it: see facebook/react#14856 Using a keyboard shortcut seems better anyway since not everyone has a mouse wheel.
Sadly this timeout hack doesn't fully work on firefox for me. |
Similar to #8968, but for the
wheel
andmousewheel
events. They are now passive by default for root elements in Chrome 73 (currently beta) which means React apps that have custom scrolling/zooming behaviors will run into issues.The quick fix may be to manually add event listeners with
{passive: false}
but has the React team considered if this should be configurable for the React event handler?Blog post from the Chrome team here: https://developers.google.com/web/updates/2019/02/scrolling-intervention
The text was updated successfully, but these errors were encountered: