-
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
[react-events] Tap responder #16628
[react-events] Tap responder #16628
Conversation
ReactDOM: size: 0.0%, gzip: -0.0% Details of bundled changes.Comparing: f512537...23cc4f2 react-dom
react-events
Generated by 🚫 dangerJS |
1655201
to
6a7c145
Compare
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'll consolidate the common logic within the different responder implementations now that all the tests pass.
6a7c145
to
9df4e0b
Compare
Using useKeyboard in user space may lead to inconsistent UX. I thought the point of builtin usePress was to avoid opinions here. There was a case with scrolling down on pressing space. Is it considered in new implementation? |
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.
Very thorough, I love it. Let's get this in once we've got keyboard updated to reflect the previous Press
:)
@TrySound We'll still provide |
9df4e0b
to
e59a72c
Compare
This is a partial replacement for the 'Press' responder: 1. `useTap` is scoped to pointers (no keyboard support). Our current thinking is that "responders" should be limited to working with pointers, and that they can be combined with 'useKeyboard' in user-space. For example, we might create a 'usePress' hook in user-space that combines 'useTap' with 'useKeyboard' to react to both pointers and keyboard interactions. 2. `useTap` cancels the gesture once the pointer moves over an element that is not within the responder target's subtree. This differs from `usePress` (and React Native), where the gesture remains active after the pointer exits the target's subtree and is restarted once the pointer reenters. One of the drawbacks with the `usePress` behavior is that it requires repeatedly measuring DOM elements (which can cause jank) to perform hit region tests. `useTap` avoids doing this and relies on `document.elementFromPoint` only to support the TouchEvent fallbacks. 3. `useTap` calls `onTapUpdate` when the active gesture's state changes, `onTapEnd` when the gesture successfully completes. and `onTapCancel` when it fails. There is no `onTap` callback. `usePress` did not explicitly report back when the gesture failed, and product developers were confused about the difference between `onPress` and `onPressEnd`. 4. `useTap` explicitly separates the PointerEvent implementation from the MouseEvent/TouchEvent fallback. 5. `useTap` has better unit test coverage . All pointer types and the fallback environment are tested. The shape of the gesture state object is also defined and tested.
f005593
to
c8aa125
Compare
Details of bundled changes.Comparing: c66edb9...cb40f04 react-events
|
if (type === 'pointermove' && activePointerId !== pointerId) { | ||
if ( | ||
type === 'pointermove' && | ||
activePointerId !== nativeEvent.pointerId |
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.
Why not extract pointerId above? You use it just below.
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.
The value is only used (and not null
) within the the PointerEvent branches.
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 added some comments.
Reduces the minified size most. Smaller percentage improvement for gzip.
c8aa125
to
cb40f04
Compare
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.
Looks like you addressed my changes. So I'm cool with this – please make sure we cover Firefox for Android though. I know it's not a large % of users, but it's relatively easy to guard against.
This is a partial replacement for the 'Press' responder:
useTap
is scoped to pointers (no keyboard support). Our current thinking isthat "responders" should be limited to working with pointers, and that they can
be combined with 'useKeyboard' in user-space. For example, we might create a
'usePress' hook in user-space that combines 'useTap' with 'useKeyboard' to react
to both pointers and keyboard interactions.
useTap
cancels the gesture once the pointer moves over an element that isnot within the responder target's subtree. This differs from
usePress
(andReact Native), where the gesture remains active after the pointer exits the
target's subtree and is restarted once the pointer reenters. One of the
drawbacks with the
usePress
behavior is that it requires repeatedly measuringDOM elements (which can cause jank) to perform hit region tests.
useTap
avoidsdoing this and relies on
document.elementFromPoint
only to support theTouchEvent fallbacks.
useTap
callsonTapUpdate
when the active gesture's state changes,onTapEnd
when the gesture successfully completes. andonTapCancel
when itfails. There is no
onTap
callback.usePress
did not explicitly report backwhen the gesture failed, and product developers were confused about the
difference between
onPress
andonPressEnd
.useTap
explicitly separates the PointerEvent implementation from theMouseEvent/TouchEvent fallback.
useTap
has better unit test coverage . All pointer types and the fallbackenvironment are tested. The shape of the gesture state object is also defined
and tested.
Demo