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 keyboard behaviors to menubar #3220

Open
wants to merge 21 commits into
base: develop
Choose a base branch
from

Conversation

tespin
Copy link
Contributor

@tespin tespin commented Aug 15, 2024

Progress on #2933

Tasks:

  • Add controls for navigating the menubar via Up, Down, Home, Escape keys
  • Implement a way to move focus to a menuitem that matches a typed character (focus does not move if there is no match)
  • Mouse pointer events for nested submenus
  • Add unit tests
  • Focus styles should align with currently focused menu item
  • Test with mobile view

Changes:

  • added roving tab index for focus management of menubar items
  • moved language dropdown to left menu

Hi all! I'm using this PR to work on implementing keyboard behaviors for the editor's menubar. This will support the accessibility features implied by the recently-added ARIA attributes in #3191. Currently I've added a roving tab focus and changing the focused item with the left and right keys. The language drop down was also moved to the left side of the menu.

I'm referencing the implementation details for the menubar in a sandbox linked below. Would love any feedback if anyone has some!

References:

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

Copy link
Collaborator

@raclim raclim left a comment

Choose a reason for hiding this comment

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

Thanks so much for diving into this and for providing detailed references/explanations—this is really awesome to see so far and I feel like it's a great start to implementing keyboard accessibility! :)

I'm not entirely familiar with best practices, so I didn't realize that it's preferred to use Arrow Keys vs. Tab when navigating through a menu bar! I personally feel that once the linting warnings are addressed, this may be ready to go!

I'm also going to ping @calebfoss and @nickmcintyre, who have done accessibility work with the p5.js library, to see if they might have any thoughts!

@tespin
Copy link
Contributor Author

tespin commented Aug 23, 2024

thanks! yes i didn't realize arrow keys should be used either until reviewing the wai aria guide. i learned that you can tab to get into the menubar but once that component is focused, the expectation for keyboard users is that arrow keys would be used to get to the other items. would love to get others' thoughts. as a reference, the initial commits in this pr allow navigation with the left and right arrow keys:

menubar-left-right

i also just pushed some recent commits that begin to add functionality for up and down arrows. there's some work to be done; for example, when first opening a submenu, the 1st item doesn't get a focus outline for some reason; and the keyboard behaviors are not all there (pressing the left and right keys while in a submenu should close the current submenu and focus the next one, expanding if it is another submenu; additionally, the down and up keys should both open the submenu and focus the first or last element respectively). there are also lots of code changes so i'm sure a refactor would be helpful down the line.

menubar-up-down

i'll keep poking at it and drop an update here.

@nickmcintyre
Copy link
Member

Great work @tespin! And thanks for tagging me @raclim -- I'll do some research and testing this weekend and will share any suggestions next week.

@nickmcintyre
Copy link
Member

@tespin my apologies for the delay! Your upgrades work well overall. As you pointed out, the Left and Right Arrow keys behave differently in a submenu than suggested by WAI-ARIA:

When focus is in a submenu of an item in a menubar, performs the following 3 actions:

  1. Closes the submenu.
  2. Moves focus to the previous item in the menubar.
  3. If focus is now on a menuitem with a submenu, either: (Recommended) opens the submenu of that menuitem without moving focus into the submenu, or opens the submenu of that menuitem and places focus on the first item in the submenu.

Actions 1 and 3 seem particularly helpful. Do you have a sense of what changes are needed to implement them?

@tespin
Copy link
Contributor Author

tespin commented Sep 16, 2024

@nickmcintyre no worries, thanks for getting back to me!

i spent some time on it today. my idea was to add an onKeyDown to createDropdownHandlers in NavBar.jsx. i tested use with the down arrow (which WAI-ARIA says, if focus is on a menuitem with a submenu, the down arrow should open the submenu). the current submenu opens, but i'm having trouble when it comes to the left and right keys -- they expand the previously active menu rather than the current one.

p5-keyboard-test-01

i'm learning that it has to do with react's asynchronous updates but i'm not sure if i'm accessing the current state correctly.

Screenshot 2024-09-16 at 3 09 45 PM

should i not do a functional state update with prevState?

@lindapaiste
Copy link
Collaborator

@nickmcintyre no worries, thanks for getting back to me!

i spent some time on it today. my idea was to add an onKeyDown to createDropdownHandlers in NavBar.jsx. i tested use with the down arrow (which WAI-ARIA says, if focus is on a menuitem with a submenu, the down arrow should open the submenu). the current submenu opens, but i'm having trouble when it comes to the left and right keys -- they expand the previously active menu rather than the current one.

p5-keyboard-test-01 p5-keyboard-test-01

i'm learning that it has to do with react's asynchronous updates but i'm not sure if i'm accessing the current state correctly.

Screenshot 2024-09-16 at 3 09 45 PM should i not do a functional state update with `prevState`?

hmmm. A functional state update with prevState is correct. I think the problem is which menu item is triggering the event handler, and consequently what is the value of dropdown. Like if "Sketch" is focused and you press the right arrow key, it's the "Sketch" menu that handles the event. But we don't want to do anything with "Sketch", we want to shift the focus (and possibly open the menu) for the next item in the list, which is "Help", because we are navigating to the right. The way that I coded the nav bar does not make that easy because there is no awareness of what other items are in the menu. This might take some refactoring to implement.

@tespin
Copy link
Contributor Author

tespin commented Oct 9, 2024

hmmm. A functional state update with prevState is correct. I think the problem is which menu item is triggering the event handler, and consequently what is the value of dropdown. Like if "Sketch" is focused and you press the right arrow key, it's the "Sketch" menu that handles the event. But we don't want to do anything with "Sketch", we want to shift the focus (and possibly open the menu) for the next item in the list, which is "Help", because we are navigating to the right. The way that I coded the nav bar does not make that easy because there is no awareness of what other items are in the menu. This might take some refactoring to implement.

ah i see. i think i did have some issues implementing certain changes with the current component, so a refactor would make sense. if there isn't one already, i'll start putting together a separate issue to start a conversation on what a refactor might look like.

thanks for your input!

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.

4 participants