-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix: Text component pressRetentionOffset issue on Windows #14596
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
base: main
Are you sure you want to change the base?
Conversation
bool hasMoveEventListeners = | ||
IsAnyViewInPathListeningToEvent(eventPathViews, facebook::react::ViewEvents::Offset::PointerMove) || | ||
IsAnyViewInPathListeningToEvent(eventPathViews, facebook::react::ViewEvents::Offset::PointerMoveCapture); | ||
if (hasMoveEventListeners) { |
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.
Do we need to remove the hasMoveEventListeners check here? Isn't there a perf cost to this (now all move events will always have to be fired and propagated to JS, even if no-one is listening?
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.
only applies to active touches that have already claimed responder status - not to all pointer moves so sending these extra events for ongoing interactions, which is actually a small subset of all pointer movements
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.
It looks like pressRetentionOffset isn't supported on Text in core. It is a property on Pressable, not view/text.
Does it work on Pressable already? -- Is this change even needed?
so Official react docs explicitly lists pressRetentionOffset as a prop for [Text] components, showing that the functionality core support : on both iOS and Android this is supported , making Windows inconsistent without it. |
Description
Text component pressRetentionOffset on Windows wasnt working as expected behaviour as it is on native components
Type of Change
New feature (non-breaking change which adds functionality)
Why
the current code wasn't dispatching pointer move events through the touch event system for active touches with responders. The touch event system is what RNW uses to track touch interactions.
Resolves [Add Relevant Issue Here]
#13837
What
1>When a component claims responder status (via onStartShouldSetResponder), the pointer is marked as an "active touch"
2>As that pointer moves across the screen, all move events are correctly dispatched to JavaScript
3> This allows the Pressability component in JavaScript to properly track the movement and apply the pressRetentionOffset logic
Screenshots
pressOffsetrect_fix.mp4
Testing
using playground
Changelog
yes
Add a brief summary of the change to use in the release notes for the next release.
Text component pressRetentionOffset on Windows is added : allowing touches to remain active within the expanded area while canceling them when moving beyond it.