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

Touch/Wheel Event Passiveness in React 17 #19651

Closed
gaearon opened this issue Aug 19, 2020 · 7 comments · Fixed by #19654
Closed

Touch/Wheel Event Passiveness in React 17 #19651

gaearon opened this issue Aug 19, 2020 · 7 comments · Fixed by #19654

Comments

@gaearon
Copy link
Collaborator

gaearon commented Aug 19, 2020

Chrome did an "intervention" back in the day, breaking all React touch and wheel listeners which used e.preventDefault() (#14856) because React happened to attach them to the document.

In React 17, events are no longer attached to the document. This happens to "undo" Chrome's intervention (#6436 (comment)). However, since this functionality is already broken in 16 and we haven't patched it up, it seems like this would just be a performance regression for the majority of cases.

It seems like we have a few options:

  • In the spirit of Chrome's "fix", keep touch listeners passive by default. e.preventDefault() is broken, just like it got broken in 16 by Chrome.
    • In this case, it is still a pain point that React offers no native API to intentionally attach an active listener for the case where you want to use e.preventDefault().
  • Do nothing, in which case Chrome's intervention would effectively be undone for React apps. There is an argument that Chrome itself only felt comfortable doing it for the document level and not individual elements — and conceptually React users were putting their handlers on individual elements. Delegation is just an implementation detail.
  • Not use delegation for touch/wheel listeners at all (?)

Filing this to be resolved before 17 final.

@gaearon gaearon added Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Type: Discussion and removed Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Aug 19, 2020
@gaearon
Copy link
Collaborator Author

gaearon commented Aug 19, 2020

We briefly discussed with @sebmarkbage and since the goal of 17 is to be as close to 16 as possible (while fixing interop), we're going to emulate Chrome's behavior and treat these as passive by default when attaching to roots. This doesn't solve the usability issue but that's orthogonal to the 17 release and should not block it.

@koba04
Copy link
Contributor

koba04 commented Aug 19, 2020

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 19, 2020

Is there a definitive list of affected events?

I take it, we have:

  • touchstart
  • touchmove
  • wheel

https://docs.google.com/document/d/1II7oSIpd8pK91V5kEM3tDLKcIj398jOJn8Niqy6_loI/edit

Anything else?

@koba04
Copy link
Contributor

koba04 commented Aug 19, 2020

MDN has a list for this. (I'm not sure whether the list is definitive or not.)
https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#Browser_compatibility

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 19, 2020

Seems like it's just these four. (I excluded mousewheel because we don't use it.)

@mikesherov
Copy link

As far as the "do nothing" option is concerned: This seems like this is likely a bug in lighthouse (or otherwise it's a missing feature in react as per your first option).

The intervention claims to only effect window and document level events, for performance reasons.
If that's the case, then there isn't a negative performance implication for attaching mouse wheel and touch events to other elements. And if that's the case, then lighthouse shouldn't report it as a performance issue.

If that's not true, then indeed it's a missing react feature to not be able to mark an event as passive.

As far as "don't do delegation for effected events", I don't think that can address the problem.

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 19, 2020

The intervention claims to only effect window and document level events, for performance reasons.

This is correct.

If that's the case, then there isn't a negative performance implication for attaching mouse wheel and touch events to other elements.

In my understanding, there is still a performance implication. But doing it for direct bindings is just a more breaking change so they weren't able to do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants