-
Notifications
You must be signed in to change notification settings - Fork 46.4k
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
[Flare] Add more functionality to Scroll event responder #16036
Conversation
Details of bundled changes.Comparing: 5cb8f6f...8a59925 react-events
Generated by 🚫 dangerJS |
@@ -165,13 +230,19 @@ const ScrollResponder: ReactDOMEventResponder = { | |||
state.pointerType = pointerType; | |||
break; | |||
} | |||
case 'wheel': { | |||
state.pointerType = 'mouse'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mention that trackPad
fires both scroll
and wheel
events?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned it in the summary of this PR. Did you mean for me to add a comment somewhere too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the pointerType going to be correct here if trackPad is the source of the wheel event?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm missing something. There's no way to know if it's a trackpad or not. Mouse events can trigger wheel events too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK sounds like we'll just not have trackpad as a pointer type for scroll
This PR adds more functionality to the
Scroll
event responder. Specifically:onScrollDragStart
andonScrollDragEnd
onMomentumScrollStart
andonMomentumScrollEnd
Flow prop types.I looked into how we might implement the momentum scroll events and ultimately decided it wasn't worth the logic to support. The issue is that trackpad events fire
wheel
andscroll
for each bit of momentum and thus it's not possible to know if the user is doing this – or if it's a system setting (like on MacOS).Furthermore, working out the difference in delta between scrolls and mousewheels is one possibility (as other third party libraries try do), but the amount of code it requires and the complexity in handling edge cases makes it non-trivial. Adding support for touch based momentum is simpler but then this feature feels half-baked. I'd much rather leave it as a TODO for later work.