Convert menus to disclosure pattern #1814

whabanks · 2024-04-22T20:35:35Z

Summary | Résumé

This PR refactors our main and account menus on mobile to leverage the disclosure menu design pattern. Previously we made use of an overlay pattern to handle display of the sub menu options.

Test instructions | Instructions pour tester la modification

On any page in the app, shrink the viewport until the mobile styling takes effect.
Tab to the Menu button in the header on the left and open the menu
Note that the focus was placed on the first item in the sub menu

Testing the keyboard navigation:

Use the arrow keys to move between the menu items:
- Up Arrow = Previous Item
- Down Arrow = Next Item
- Left Arrow = Previous Item
- Right Arrow = Next Item
- Tab = Next Item
- Shift + Tab = Previous Item
- Home Key / Cmd + ArrowLeft = Go to the first Item in the menu
- End Key / Cmd + ArrowRight = Go to the last Item in the menu
- Enter / Spacebar = Click menu item link
- Escape Key = Close the currently open menu
Pressing the Down Arrow / Right Arrow key when on the last menu item should return you to the first menu item.
Pressing the Up Arrow / Left Arrow key when on the first menu item should take you to the last menu item.
Pressing the Escape should close the menu and return the focus to the menu button. Tab navigation should remain intact. (e.g. Tabbing should continue from the menu button and not reset)

Try to break it

Try to find a way to have both the main menu and account menu open at the same time. It shouldn't be possible but if we can get into a state like this then keyboard navigation may start to behave incorrectly.
While the menu is open, keyboard navigation should remain scoped to the menu items. By focusing on an element outside of the menu via tabbing, the menu should automatically close. (blur event)
Look for situations / states where regular browser hotkeys are prevented from firing e.g. F12 for dev tools.

github-actions · 2024-04-22T20:41:18Z

🧪 Review environment

https://pku275z4rygxlbqjduenzipxhi0hsvmk.lambda-url.ca-central-1.on.aws/

- Fix alignment issues with main menu sub menu items - Correct alignment issues between the account menu and main menu buttoms in their flex container - Standarize the nav_menu_item_mobile link styling

- Adds support for navigating through menus with arrow keys left and right map to up and down the menu - Add support for going to the beginning / end of the menu via Home / End keys - Added focus control to prevent a situation where mutliple disclosure menus could be open at a time breaking the focus control

app/assets/javascripts/menu.js

- Fixed an issue where home/end keys were not working properly with mac keys - Fixed an issue where using Tab / Shift+Tab could break focus out of the disclosure menu without closing the menu - Fixed an issue where pressing the spacebar while on a menu link would scroll the page down instead of following the focused menu link

- Formatting

- Since the keydown events are bound to the window object if we preventDefault as soon as it fires it will prevent common hotkeys from working on the site

amazingphilippe

I followed your testing instructions and I found a few things:

I can open both menus by clicking on either one, then clicking on the other.
With VoiceOver in Chrome, I can navigate outside of the menus using the arrows. I can also open the other menu, though whenever I open the other menu, Focus gets moved back to the on that was opened first.
- Maybe the blur event doesn't get registered properly?
- You could do a check that only the last active menu can be open instead?
I wonder if you could listen to arrow only when no modifier keys are pressed? This might solve some issues with screen readers.

An a minor detail on design, we could add a width to the <ul> container so that around 20ch can fit. You can use the CSS ch unit for that! Perhaps a max-width of 100% too so that it never goes wider than the entire screen.

whabanks · 2024-05-06T18:06:42Z

With VoiceOver in Chrome, I can navigate outside of the menus using the arrows. I can also open the other menu, though whenever I open the other menu, Focus gets moved back to the on that was opened first.

This seems to be the difference between focus and browse modes when using screen readers. I can add some JS so that before we open a menu, we check for any other menus that are currently open and close them before opening the new one, that way we can appropriately set the current focus.

Maybe the blur event doesn't get registered properly?

The problem I ran into with using a blur event is we need to apply it to each item in the menu and then check if the newly focused item is part of the current menu or not before closing the menu. It can work this way but we run into the same problem between the browse and focus modes of screen readers.

An a minor detail on design, we could add a width to the <ul> container so that around 20ch can fit. You can use the CSS ch unit for that! Perhaps a max-width of 100% too so that it never goes wider than the entire screen.

This is a great idea, I will add this in!

- Menu widths are now limited to 20ch - Use nav_menu_item_mobile component for the mobile account menu instead of manually specifying list items and anchors - Corrected item alignment issues with the account menu - Regen tailwind

- Opening another menu will now close any currently open menus and reset focus before opening the next menu requested - Regen CSS - Stubbed a blur event to handle closing the currently open menu when clicking anything other than the menu toggle itself, or any of the menu items

- Implemented blur event that closes the currently open menu when a user clicks outside of the menu - Removed menuOverlay.js that is now no longer used and was interfering with the current implementation - Updated gulpfile - Added menu.css and styles to unify the disclosure menu max widths - Regened tailwind css

app/assets/javascripts/menu.js

andrewleith

Overall this works really well. Great job Will!

A couple of small issues I found while testing:

Focus is "remembered" when a menu is closed
Focus is not returned to the menu button when a menu is closed and there is more than one menu on the page

Reproduction

Focus is "remembered" when a menu is closed

click main menu
navigate to the second item (tab/left/down, doesnt matter which)
close the menu
open the menu again
keyboard navigate to the second item again:
❌ it actually goes to the third as it seems to start from the second one (even though it is not focused)

Focus is not returned to the menu button when a menu is closed and there is more than one menu on the page

Login (this makes it so there are two menus on the page, main + profile)
Open the main menu
Press esc to close
❌ Focus goes to the profile menu button, instead of the main menu button

app/assets/javascripts/menu.js

- Closing a menu now resets the property, ensuring that when the menu is re-opened navigation starts from the first item in the menu instead of the last item that was selected before the menu was closed - Opening a menu -> opening another menu -> then closing that menu will no longer return the focus to the button associated with the first menu that was opened.

tests_cypress/cypress/Notify/Admin/Pages/LoginPage.js

…test navigates to `/` at the start

- Removed redundant role=button - Added unique aria-label names for mobile menus - Fixed invalid descendant tags of <ul> elements - Added FR translation

andrewleith

Looks great, all tests are passing!

I just merged in main so the sitemap test should now pass and we should be all green here!

The requested changes have been addressed and Phil is on vacation. Dismissing so we can merge.

Begin converting menues to disclosure pattern

d3b58b0

whabanks changed the title ~~Begin converting menues to disclosure pattern~~ Convert menus to disclosure pattern Apr 22, 2024

Convert main menu botton to disclosure menu

af076f5

- Fix alignment issues with main menu sub menu items - Correct alignment issues between the account menu and main menu buttoms in their flex container - Standarize the nav_menu_item_mobile link styling

amazingphilippe added the a11y label Apr 26, 2024

whabanks and others added 2 commits April 30, 2024 12:28

Merge branch 'main' into a11y/disclosure-menu-pattern

211c569

github-advanced-security bot found potential problems Apr 30, 2024

View reviewed changes

app/assets/javascripts/menu.js Fixed Show fixed Hide fixed

whabanks added 2 commits May 1, 2024 15:51

rebuild JS, fix formatting

451f753

Fix tests

004f703

whabanks marked this pull request as ready for review May 2, 2024 13:04

whabanks added 2 commits May 2, 2024 10:51

Remove redundant event.preventDefault() calls

b40d39e

- Formatting

whabanks requested review from amazingphilippe and andrewleith May 2, 2024 18:24

whabanks and others added 4 commits May 2, 2024 14:50

Add comment, test all.min.js regen

6aadd43

Merge branch 'main' into a11y/disclosure-menu-pattern

3e8adc3

Restore e.preventDefault() calls

e092dd0

- Since the keydown events are bound to the window object if we preventDefault as soon as it fires it will prevent common hotkeys from working on the site

Check if menu is open before executing keypress logic

3f2f2d9

amazingphilippe previously requested changes May 3, 2024

View reviewed changes

whabanks and others added 4 commits May 6, 2024 17:01

Add default width for disclosure menus

02353f6

- Menu widths are now limited to 20ch - Use nav_menu_item_mobile component for the mobile account menu instead of manually specifying list items and anchors - Corrected item alignment issues with the account menu - Regen tailwind

Merge branch 'main' into a11y/disclosure-menu-pattern

edf27b6

github-advanced-security bot found potential problems May 13, 2024

View reviewed changes

app/assets/javascripts/menu.js Fixed Show fixed Hide fixed

app/assets/javascripts/menu.js Fixed Show fixed Hide fixed

whabanks added 2 commits May 14, 2024 09:40

Merge branch 'main' into a11y/disclosure-menu-pattern

cb79561

Merge branch 'main' into a11y/disclosure-menu-pattern

631ed69

andrewleith requested changes May 14, 2024

View reviewed changes

app/assets/javascripts/menu.js Outdated Show resolved Hide resolved

app/assets/javascripts/menu.js Outdated Show resolved Hide resolved

andrewleith added 3 commits May 14, 2024 15:07

task: add disclosure menu test suite

42552d9

task: add disclosure menu tests to CI

d92c8da

chore: formatting

ca97fc2

github-advanced-security bot found potential problems May 14, 2024

View reviewed changes

tests_cypress/cypress/Notify/Admin/Pages/LoginPage.js Fixed Show fixed Hide fixed

andrewleith and others added 14 commits May 14, 2024 15:30

fix: turn on test isolation in cypress

8d5ac3e

fix(app_pages tests): simplify code and rely on cy.session for login

3194e15

fix(disclosure menu tests): use cy.login command for login; ensure …

cfd7338

…test navigates to `/` at the start

test(disclosure_menu): add a11y test to suite

f0f9453

Merge branch 'main' into a11y/disclosure-menu-pattern

27a7302

Fix cypress a11y violations

60966c2

- Removed redundant role=button - Added unique aria-label names for mobile menus - Fixed invalid descendant tags of <ul> elements - Added FR translation

Fix typo

6e21768

Various cleanups

c152851

Merge branch 'main' into a11y/disclosure-menu-pattern

f387f03

Formatting, regen css

21ad3d6

Merge branch 'main' into a11y/disclosure-menu-pattern

34744ab

Merge branch 'main' into a11y/disclosure-menu-pattern

21d695b

Merge branch 'main' into a11y/disclosure-menu-pattern

a110357

Merge branch 'main' into a11y/disclosure-menu-pattern

ec9dc2e

andrewleith approved these changes May 22, 2024

View reviewed changes

whabanks requested a review from amazingphilippe May 22, 2024 18:17

whabanks merged commit be7bc71 into main May 22, 2024
9 checks passed

whabanks deleted the a11y/disclosure-menu-pattern branch May 22, 2024 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert menus to disclosure pattern #1814

Convert menus to disclosure pattern #1814

whabanks commented Apr 22, 2024 •

edited

github-actions bot commented Apr 22, 2024

amazingphilippe left a comment

whabanks commented May 6, 2024

andrewleith left a comment

andrewleith left a comment

Convert menus to disclosure pattern #1814

Convert menus to disclosure pattern #1814

Conversation

whabanks commented Apr 22, 2024 • edited

Summary | Résumé

Test instructions | Instructions pour tester la modification

Testing the keyboard navigation:

Try to break it

github-actions bot commented Apr 22, 2024

🧪 Review environment

amazingphilippe left a comment

Choose a reason for hiding this comment

whabanks commented May 6, 2024

andrewleith left a comment

Choose a reason for hiding this comment

Reproduction

andrewleith left a comment

Choose a reason for hiding this comment

whabanks commented Apr 22, 2024 •

edited