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

[web] Add keyboard support to gesture handler #3035

Merged
merged 26 commits into from
Aug 9, 2024

Conversation

latekvo
Copy link
Contributor

@latekvo latekvo commented Aug 7, 2024

Description

By default, all native android and web components support keyboard navigation and option to press elements with Enter or Spacebar 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

  • try navigating around the example app using just the following keys: 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

@latekvo
Copy link
Contributor Author

latekvo commented Aug 7, 2024

As of 5564563, it looks like Keyboard is fully functional on web - this includes both Gesture Handler and all related components like Pressable

@latekvo latekvo changed the title Add keyboard support to gesture handler [web] Add keyboard support to gesture handler Aug 7, 2024
@latekvo

This comment was marked as resolved.

@latekvo latekvo marked this pull request as ready for review August 7, 2024 14:45
src/web/tools/KeyEventManager.ts Outdated Show resolved Hide resolved
};

private dispatchEvent(event: KeyboardEvent, eventType: EventTypes) {
(event.target as unknown as View)?.measure?.(
Copy link
Contributor

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?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but please use

it uses getBoundingClientRect under the hood but abstracts away the implementation.

Copy link
Contributor Author

@latekvo latekvo Aug 8, 2024

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

Copy link
Contributor

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 Show resolved Hide resolved
src/web/tools/KeyEventManager.ts Outdated Show resolved Hide resolved
src/web/tools/KeyEventManager.ts Outdated Show resolved Hide resolved
};

private dispatchEvent(event: KeyboardEvent, eventType: EventTypes) {
(event.target as unknown as View)?.measure?.(
Copy link
Member

Choose a reason for hiding this comment

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

Yes, but please use

it uses getBoundingClientRect under the hood but abstracts away the implementation.

src/web/tools/KeyEventManager.ts Outdated Show resolved Hide resolved
private keyDownCallback = (event: KeyboardEvent): void => {
console.log(event.code);
if (this.cancelationKeys.indexOf(event.code) !== -1) {
this.dispatchEvent(event, EventTypes.CANCEL);
Copy link
Member

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?

Copy link
Contributor Author

@latekvo latekvo Aug 8, 2024

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@m-bert m-bert left a 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 😅

src/PointerType.ts Show resolved Hide resolved
src/web/tools/KeyEventManager.ts Outdated Show resolved Hide resolved
src/web/tools/KeyEventManager.ts Outdated Show resolved Hide resolved
src/web/tools/KeyEventManager.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@m-bert m-bert left a comment

Choose a reason for hiding this comment

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

LGTM

@latekvo latekvo merged commit 6d645fc into main Aug 9, 2024
2 checks passed
@latekvo latekvo deleted the @latekvo/add-keyboard-support-to-gesture-handler branch August 9, 2024 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants