-
-
Notifications
You must be signed in to change notification settings - Fork 980
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
#3035
Conversation
…tps://github.com/software-mansion/react-native-gesture-handler into @latekvo/add-keyboard-support-to-gesture-handler
As of 5564563, it looks like Keyboard is fully functional on |
web
] Add keyboard support to gesture handler
This comment was marked as resolved.
This comment was marked as resolved.
src/web/tools/KeyEventManager.ts
Outdated
}; | ||
|
||
private dispatchEvent(event: KeyboardEvent, eventType: EventTypes) { | ||
(event.target as unknown as View)?.measure?.( |
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 use getBoundingClientRect
?
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
measureView(): MeasureResult; |
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
inside KeyEventManager
, as measureView
is only present on the GestureHandlerWebDelegate
, 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 imports KeyEventManager
, and to attain this method KeyEventManager
would have to also import GestureHandlerWebDelegate
.
Please let me know if it's really necessary to measureView
instead of getBoundingClientRect
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
src/web/tools/KeyEventManager.ts
Outdated
}; | ||
|
||
private dispatchEvent(event: KeyboardEvent, eventType: EventTypes) { | ||
(event.target as unknown as View)?.measure?.( |
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
measureView(): MeasureResult; |
it uses getBoundingClientRect
under the hood but abstracts away the implementation.
src/web/tools/KeyEventManager.ts
Outdated
private keyDownCallback = (event: KeyboardEvent): void => { | ||
console.log(event.code); | ||
if (this.cancelationKeys.indexOf(event.code) !== -1) { | ||
this.dispatchEvent(event, EventTypes.CANCEL); |
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 with Tab
and after that it was impossible to use Enter
again, and the previously pressed element stayed in a pressed 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.
Overall looks good, please take a look at the comments. In the meantime I'm going to test that PR 😅
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.
LGTM
Description
By default, all native android and web components support keyboard navigation and option to press elements with
Enter
orSpacebar
keys.Gestures and components provided by Gesture Handler lack the latter option.
This PR adds keyboard support to the entirety of gesture handler, allowing it to replace mouse or finger navigation.
Test plan
Tab
,Shift-Tab
,Space
,Enter
Gallery
Expand
Screen.Recording.2024-08-07.at.14.54.11.mov
Screen.Recording.2024-08-07.at.14.57.05.mov