USWDS - Navigation: Close actions. #4275
Conversation
- Added test case in navigation.spec.js - Updated navigation.js to accept `Esc` key to close nav and dropdowns
| @@ -79,6 +88,10 @@ const resize = () => { | |||
|
|
|||
| const onMenuClose = () => navigation.toggleNav.call(navigation, false); | |||
| const hideActiveNavDropdown = () => { | |||
| if (!navActive) { | |||
There was a problem hiding this comment.
This check used to be in line 91, 96, and 107. Moved it to hideActiveNavDropdown() instead, so it checks when the function is called.
- Eslint fix for config/gulp/test.js - Close active navigation on focusout - Add events to navigation tests for `focusout` and `escape` - Add test for collapsing nav on focusout
| const nav = event.target.closest(NAV_PRIMARY); | ||
|
|
||
| if (!nav.contains(event.relatedTarget)) { | ||
| hideActiveNavDropdown(); |
There was a problem hiding this comment.
Created a new selector in NAV_PRIMARY because NAV can contain other elements that aren't part of the primary navigation menu.
|
Functionality working as expected. However seeing a few errors in our test run, |
Now it opens a dropdown and checks if it gets closed on Escape. Instead of checking for the main menu closing.
src/js/components/navigation.js
Outdated
| @@ -129,6 +137,18 @@ navigation = behavior( | |||
| } | |||
| }, | |||
| }, | |||
| keydown: { | |||
| [NAV_PRIMARY]: keymap({ Escape: hideActiveNavDropdown }), | |||
There was a problem hiding this comment.
Switched this from BODY to avoid conflicts with other potential functionality.
| @@ -114,6 +133,19 @@ describe("navigation toggle", () => { | |||
| assert.strictEqual(accordionButton.getAttribute("aria-expanded"), "false"); | |||
| }); | |||
|
|
|||
| it("collapses dropdowns when the Escape key is hit", () => { | |||
There was a problem hiding this comment.
Unit test is now more specific. When you open a dropdown it should close on Esc.
|
I'm able to reproduce the unit test issue locally. Will need to research further because it's also pointing to modal js, not sure why. |
src/js/components/navigation.js
Outdated
| @@ -129,6 +137,18 @@ navigation = behavior( | |||
| } | |||
| }, | |||
| }, | |||
| keydown: { | |||
| [NAV_PRIMARY]: keymap({ Escape: hideActiveNavDropdown }), | |||
There was a problem hiding this comment.
Should focus be managed explicitly when Escape is pressed when focused on one of the submenu items? Similar to how the modal works, and based on related guidance here:
https://www.w3.org/TR/wai-aria-practices/examples/menubar/menubar-1/menubar-1.html#kbd2_label
Escape
- Closes submenu.
- Moves focus to parent menubar item.
| @@ -7,6 +7,25 @@ const accordion = require("../../../src/js/components/accordion"); | |||
|
|
|||
| const TEMPLATE = fs.readFileSync(path.join(__dirname, "template.html")); | |||
|
|
|||
| const EVENTS = { | |||
There was a problem hiding this comment.
Semi-related to my previous comment, I notice the tests have a lot of these event dispatching helpers. Have y'all considered using a library like Testing Library to simplify this? In my experience, it also helps account for a lot of non-obvious real-world details of events, like how clicking an element would also cause a focus event to be fired.
e.g.
import userEvent from "@testing-library/user-event";
// ...
it("collapses dropdowns when the Escape key is hit", () => {
userEvent.click(accordionButton);
userEvent.tab();
userEvent.keyboard("{escape}");
assert.strictEqual(document.activeElement, accordionButton);
assert.strictEqual(accordionButton.getAttribute("aria-expanded"), "false");
});There was a problem hiding this comment.
This would definitely be useful and help bring consistency to the codebase. Thanks for the suggestion.
We still have to take a good look at the unit testing library and see if it's still up to par with what's needed. We've talked about incorporating cypress in the past.
- Move escape functionality to it's own function
🚀 💯
Preview
Header preview →
Description
Close navigation with escape key. Mobile menu closes and resets on escape key. On larger screen sizes escape closes any active dropdowns. Closes #3396.
Dropdowns close on focusout. Tabbing away from primary navigation now closes any active dropdown. Closes #3528.
Additional information
Esckey to close nav and dropdownsBefore you hit Submit, make sure you’ve done whichever of these applies to you:
npm testand make sure the tests for the files you have changed have passed.