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

feat(navbar keyboard navigation): optionally navigate to submenu items using the Tab and Enter keys. #3482

Merged

Conversation

ddouglasz
Copy link
Contributor

@ddouglasz ddouglasz commented Apr 5, 2024

Follow up to navbar keyboard navigation.

Summary

Optionally navigate to submenu items using the Tab and Enter keys. This way, we can choose to navigate through the submenu of a menu item that has a submenu or skip the submenu and move to the next menu item.
Some context can be found in the github conversation here.

Copy link

changeset-bot bot commented Apr 5, 2024

🦋 Changeset detected

Latest commit: 4e07032

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 37 packages
Name Type
@commercetools-frontend/application-shell Minor
@commercetools-frontend/cypress Minor
@commercetools-applications/merchant-center-template-starter-typescript Minor
@commercetools-applications/merchant-center-template-starter Minor
@commercetools-applications/merchant-center-custom-view-template-starter-typescript Minor
@commercetools-applications/merchant-center-custom-view-template-starter Minor
@commercetools-local/playground Minor
@commercetools-local/visual-testing-app Minor
@commercetools-backend/eslint-config-node Minor
@commercetools-backend/express Minor
@commercetools-backend/loggers Minor
@commercetools-frontend/actions-global Minor
@commercetools-frontend/application-components Minor
@commercetools-frontend/application-config Minor
@commercetools-frontend/application-shell-connectors Minor
@commercetools-frontend/assets Minor
@commercetools-frontend/babel-preset-mc-app Minor
@commercetools-frontend/browser-history Minor
@commercetools-frontend/codemod Minor
@commercetools-frontend/constants Minor
@commercetools-frontend/create-mc-app Minor
@commercetools-frontend/eslint-config-mc-app Minor
@commercetools-frontend/i18n Minor
@commercetools-frontend/jest-preset-mc-app Minor
@commercetools-frontend/jest-stylelint-runner Minor
@commercetools-frontend/l10n Minor
@commercetools-frontend/mc-dev-authentication Minor
@commercetools-frontend/mc-html-template Minor
@commercetools-frontend/mc-scripts Minor
@commercetools-frontend/notifications Minor
@commercetools-frontend/permissions Minor
@commercetools-frontend/react-notifications Minor
@commercetools-frontend/sdk Minor
@commercetools-frontend/sentry Minor
@commercetools-frontend/url-utils Minor
@commercetools-website/components-playground Minor
@commercetools-website/custom-views Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Apr 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
merchant-center-application-kit-components-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 23, 2024 6:16am

@ddouglasz
Copy link
Contributor Author

The key features, as concluded on a call with @FilPob and @CarlosCortizasCT should be:

  • - Tab key to focus on a menuItem
  • - If menu Item has subMenu, then Enter Key to navigate to the subMenuItems
  • - If no SubMenu, Tab key to Focus on next Available Item
  • - Enter Key to click on a SubMenu Item
  • - Escape Key to exit subMenu

@ddouglasz ddouglasz marked this pull request as ready for review April 10, 2024 14:40
@ddouglasz ddouglasz requested a review from a team as a code owner April 10, 2024 14:40
Copy link
Contributor

@kark kark left a comment

Choose a reason for hiding this comment

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

Big thanks for the improvements, it's a big step forward 🚀
Here's my feedback:

  1. I'm uncertain about one aspect—when a menu item is focused, the submenu appears, but then closes upon the next Tab key press. I would expect that once the submenu is open we are able to navigate it with the next Tab key press. Currently, it feels a bit ambiguous whether pressing Enter allows entry (although if this was a deliberate decision, I'm ok with that)

  2. Additionally, I've encountered what seems to be a bug. Pressing Enter on any menu item results in submenu items always being focusable.

@ddouglasz
Copy link
Contributor Author

ddouglasz commented Apr 17, 2024

  1. I'm uncertain about one aspect—when a menu item is focused, the submenu appears, but then closes upon the next Tab key press. I would expect that once the submenu is open we are able to navigate it with the next Tab key press. Currently, it feels a bit ambiguous whether pressing Enter allows entry (although if this was a deliberate decision, I'm ok with that)

Thank you Kacper,
The agreed flows are:

Scenario 1: When a menu item is focused, Enter key press should focus on the first item of it's submenu items, if there are any, and tab key to navigate within the submenu.

Scenario 2: If the Tab key is pressed when a menu item is focused then the focus should move to the next menu item and not into the submenu item.

  1. Additionally, I've encountered what seems to be a bug. Pressing Enter on any menu item results in submenu items always being focusable.

I will look into this, thank you! 👍🏾

@kark
Copy link
Contributor

kark commented Apr 18, 2024

Thanks Douglas 🙏,
I'm not sure if this is the definite answer to my concern

Scenario 1: When a menu item is focused, Enter key press should focus on the first item of it's submenu items, if there are any, and tab key to navigate within the submenu.

Scenario 2: If the Tab key is pressed when a menu item is focused then the focus should move to the next menu item and not into the submenu item.

Those scenarios are clear to me 👍 My question was: if a menu item gets focus, should the submenu open automatically or only after Enter press?

Note

Not that anything needs to change. In other implementations I've found, the submenu typically appears only upon pressing Enter. We don't have such a submenu indicator, obviously.
image

2024-04-18 06 42 47

@ddouglasz
Copy link
Contributor Author

ddouglasz commented Apr 18, 2024

My question was: if a menu item gets focus, should the submenu open automatically or only after Enter press?

Thanks again @kark , so that was the initial implementation I had but later updated to show a preview of the submenu when a menu item gains focus. it was one of the requirements we initially discussed with @FilPob (although in our last call, he was okay with it implemented either ways)

@CarlosCortizasCT
Copy link
Contributor

To be honest, I don't like the behaviour of displaying the submenu on keyboard navigation focus, but that's a design request so we must evaluate if it's not too complicated to implement.

@CarlosCortizasCT
Copy link
Contributor

Thanks a lot for working on this, @ddouglasz 🙇

From the functionality point of view, it is working as I think it was expected but I'd like for us to align on what we actually want to have as a feature.

Currently we have implemented the navigation using the tab and enter keys.
So when I get into the first root item of the navbar, I can get into their submenu items by using the enter key. If I then move around the submenu items and want to go back to the root items, I need to iterate over the previous submenu items. I don't have a quick access (eg: Escape key) to go back to the navbar root level.

Also, I think we said the tabular navigation was a first step but that we could build the arrows keys navigation also as an extra feature.

To me it's ok to merge the feature as it currently is, but I want us to be aligned in terms of the expectations of this PR and if we want to do more follow-up work.

cc/ @FilPob

@FilPob
Copy link

FilPob commented Apr 19, 2024

totally agree. I think for the scope of this PR the behaviour is OK. For follow up work we should align first on the expectations

@CarlosCortizasCT
Copy link
Contributor

@ddouglasz could you please updated the title and description of the PR accordingly? 🙏

@ddouglasz ddouglasz changed the title feat(navbar keyboard navigation): add arrow keys navigation to navbar feat(navbar keyboard navigation): optionally navigate to submenu items using the Tab and Enter keys. Apr 19, 2024
@ddouglasz
Copy link
Contributor Author

@ddouglasz could you please updated the title and description of the PR accordingly? 🙏

Updated. Thank you

Copy link
Contributor

@CarlosCortizasCT CarlosCortizasCT left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Thanks 👍

Copy link
Contributor

@Rhotimee Rhotimee left a comment

Choose a reason for hiding this comment

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

🙌🏽

Copy link
Contributor

@kark kark left a comment

Choose a reason for hiding this comment

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

Thanks Douglas. Just a final comment from my end

Copy link
Contributor

@kark kark left a comment

Choose a reason for hiding this comment

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

Thanks again, works as expected 🙌

@ddouglasz ddouglasz merged commit 3836786 into main Apr 23, 2024
18 checks passed
@ddouglasz ddouglasz deleted the SHIELD-994-keyboard-accessibility-enhancement-for-navbar branch April 23, 2024 10:36
@ct-changesets ct-changesets bot mentioned this pull request Apr 23, 2024
ddouglasz added a commit that referenced this pull request May 22, 2024
…s using the `Tab` and `Enter` keys. (#3482)

* feat(navbar keyboard navigation): add arrow keys navigation to navbar

* feat(navbar keyboard navigation): add arrow keys navigation to navbar

* feat(navbar keyboard navigation): add event types

* feat(navbar keyboard navigation): cleanup unused code

* feat(navbar keyboard navigation): make Tab and Enter key primarily for keyboard navigation

* feat(navbar keyboard navigation): make Tab and Enter key primarily for keyboard navigation

* feat(navbar keyboard navigation): tabIndex to focus submenu container

* feat(navbar keyboard navigation): update changeset

* feat(navbar keyboard navigation): rename state variable

* feat(navbar keyboard navigation): prevent default enter behaviour

* feat(navbar keyboard navigation): handle submenu state after onBlur

* feat(keyboard navigation): add submenu preview on navigation

* feat(keyboard navigation): remove unused variables

* feat(accessibility): remove unused hook

* feat(main nav accessibility): update focus when navigating to a new submenu

---------

Co-authored-by: Ddouglasz <douglas.egiemeh@gmail.com>
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.

None yet

5 participants