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

Add support for keyboard navigation for submenu item #12

Merged

Conversation

peterblazejewicz
Copy link
Contributor

@peterblazejewicz peterblazejewicz commented Nov 3, 2019

This changes minor details in the BlazoredSubMenu implemenation to
support:

  • keyboard based navigation (role and tabindex)
  • keyboard based interaction (Enter key down)

As the support for event propagation in Blazor is not yet there, the
implementation marks support for SPACE keyboard as TODO item (supporting
SPACE from code requires cancelling propagation of the event in the
handler).

The changes do not impact samples details.

Thanks!

blazor-menu

@chrissainty
Copy link
Member

Thanks for doing this @peterblazejewicz. I don't know if you've seen, but .NET Core Preview 2 is out and Blazor now supports stopping event propagation. Would you like to make the changes you suggested before I merge this?

@peterblazejewicz
Copy link
Contributor Author

Would you like to make the changes you suggested before I merge this?

Yes, let me review the docs. The best, if we could use the focused trap only for Space and Enter (I believe they discussed this as part of discussion on how to implement these new apis).
Will update and ping back,
Thanks!

This changes minor details in the BlazoredSubMenu implemenation to
support:
- keyboard based navigation (role and tabindex)
- keyboard based interaction (Enter key down)

Thanks!
@peterblazejewicz peterblazejewicz force-pushed the feat/keyboard-support-submenu branch from 81a8461 to 62534f5 Compare November 5, 2019 21:42
@peterblazejewicz
Copy link
Contributor Author

@chrissainty
So I've amended the PR, but limited changes to removing references to the event bubbling implementation details and changing to a simple solution, that seems to work for most use cases.
I think one should not use the current, opaque 'preventDefault' attributes, as this is not something desired here with the specific key handler. For example: I'd like to prevent default event when Spacebar has been pressed (as there is a scroll event associated with space bar being down) and catch the up to toggle the menu. I cannot just prevent all events, as this breaks supporting events like built-in browser shortcuts while there is a focus on the menu.
Let's revisit the details in the future.
Thanks!

@chrissainty
Copy link
Member

This is great, thanks @peterblazejewicz.

I haven't had a chance to test out the new event management bits in Preview 2 yet. But it sounds like they are a bit all or nothing which might not be great.

@chrissainty chrissainty merged commit 175d849 into Blazored:master Nov 5, 2019
@peterblazejewicz peterblazejewicz deleted the feat/keyboard-support-submenu branch November 5, 2019 23:20
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.

2 participants