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

[react-events] Press responder simplification #16517

Closed
wants to merge 1 commit into from

Conversation

necolas
Copy link
Contributor

@necolas necolas commented Aug 20, 2019

NOTE: this stacks on top of #16479 which I'll merge once we've internally synced what is currently in master. Preview-review should only look at the second commit in this PR "Press responder simplification".

  • Remove 'pressRetentionOffset' so we don't have to perform layout measurements during moves.
  • Maintain the pressed state while a pointer is down and moving, even if it moves outside the target.
  • Determine whether to call 'onPress' based on whether the pointer was released over the target.

This aligns more closely with how the web natively deals with pointers and simplifies the responder.
We only need to call getBoundingClientRect for touch pointers. Further revisions could be made to
avoid target capturing for touch pointers with PointerEvents, which would leave the gBCR call only
for the fallback TouchEvents.

Demo: https://codesandbox.io/s/latest-responder-build-rz96j30rp

@sizebot
Copy link

sizebot commented Aug 20, 2019

Warnings
⚠️

Base commit is broken: 2559111

Generated by 🚫 dangerJS

const onPressMove = props.onPressMove;

if (isFunction(onPressMove)) {
dispatchEvent(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this mean that we fire pressmove even if our pointer might be outside the target?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep

'onPressChange',
'onPressStart',
'onPressChange',
'onPressMove',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This relates to my comment above, surely if you move out, then end should fire?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do that for events that aren't capturing the target. For now this is fine and consistent across pointers

@trueadm
Copy link
Contributor

trueadm commented Aug 21, 2019

Maintain the pressed state while a pointer is down and moving, even if it moves outside the target.

This is a big change internally right? Are the plans to move the pointer moving behaviour to a Tap responder – if so, ignore my comments.

@necolas
Copy link
Contributor Author

necolas commented Aug 21, 2019

This is just how web buttons work anyway. I don't think anyone is depending on how press move works. And there's no other option for touch events without measuring the hit rect every move which is not good for perf

Copy link
Contributor

@trueadm trueadm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All makes sense :)

* Remove 'pressRetentionOffset' so we don't have to perform layout measurements during moves.
* Maintain the pressed state while a pointer is down and moving, even if it moves outside the target.
* Determine whether to call 'onPress' based on whether the pointer was released over the target.

This aligns more closely with how the web natively deals with pointers and simplifies the responder.
We only need to call getBoundingClientRect for touch pointers. Further revisions could be made to
avoid target capturing for touch pointers with PointerEvents, which would leave the gBCR call only
for the fallback TouchEvents.
@necolas necolas force-pushed the react-events/press-redesign branch from 227da9e to 7779b31 Compare August 21, 2019 18:04
@necolas
Copy link
Contributor Author

necolas commented Aug 22, 2019

I'm going to abandon this and create a new responder – Tap – so we can start fresh and be more intentional about swapping existing product code from Press to Tap when we're happy with the implementation.

@necolas necolas closed this Aug 22, 2019
@necolas necolas deleted the react-events/press-redesign branch December 20, 2019 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants