Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[
web
] Add keyboard support to gesture handler #3035[
web
] Add keyboard support to gesture handler #3035Changes from 12 commits
c29fdc9
76e01e4
9f6d237
e0d5799
e866846
5564563
2a6e1f5
7b258d2
63c8153
1a923af
c0aa2c6
edcfff9
1573a90
3cd0886
044cf87
324090d
83fbebf
b76a0ef
76bf761
af07b9e
0a5855f
aff6119
a5c20e9
2c169a2
f72af6c
03e20fc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Have you tested a scenario where one view is focused, Enter is pressed down, tab is pressed to focus another view, and then Enter is lifted?
If I understand correctly, the first view would receive DOWN and CANCEL events, but the second one would only receive an UP event. If that's the case, have you tested how different handlers behave in that scenario?
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.
Yes, this cancelation logic handles the case you've mentioned.
Without it, it was possible to press
Enter
, switch focus withTab
and after that it was impossible to useEnter
again, and the previously pressed element stayed in apressed down
state.As for an element only receiving the
UP
event, I haven't observed any concerning behaviour.Afaik both
Gesture Handler
and native buttons handle such cases, but i'll fix this just to be safe.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.
As of 044cf87 added logic preventing calling UP and CANCEL when element is not pressed down.
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.
We are in
web
environment, can't we usegetBoundingClientRect
?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.
Yes, but please use
react-native-gesture-handler/src/web/tools/GestureHandlerDelegate.ts
Line 15 in acf996d
it uses
getBoundingClientRect
under the hood but abstracts away the 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.
Hey @m-bert and @j-piasecki thank you for your review.
I'm not sure if it's possible to use
measureView
insideKeyEventManager
, asmeasureView
is only present on theGestureHandlerWebDelegate
, which we don't have access to.I also don't think it would be possible to add access to it either, without creating a circular dependency, as
GestureHandlerWebDelegate
already importsKeyEventManager
, and to attain this methodKeyEventManager
would have to also importGestureHandlerWebDelegate
.Please let me know if it's really necessary to
measureView
instead ofgetBoundingClientRect
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'd go with
getBoundingClientRect
- we already use it in PointerEventManager