Added focus trap to mobile menu #10 #12
Conversation
There was a problem hiding this comment.
Hello, thank you for the MR!
This is quite a good focus trap solution. It works as expected on Chrome, unfortunately on Safari pressing tab on the first item causes the user to skip outside the menu entirely. This is because Safari requires the user to press tab + option to tab to the next link item, and by default tab skips the user to the next button.
Other developers tend to use external libraries to manage focus traps for this reason - something like https://www.npmjs.com/package/focus-trap-react would be great for this project.
If you're not interested in using this, please can you make the minor fixes I've listed, and then this can be merged (a partial improvement in user functionality is better than none). We can keep the original issue open with an updated comment.
Best wishes and many thanks, James
src/components/Header/Header.tsx
Outdated
| const menuRef = useRef<HTMLDivElement | null>(null); // Reference for the menu | ||
| const closeButtonRef = useRef<HTMLButtonElement | null>(null); // Reference for the close button | ||
| const firstFocusableElementRef = useRef(null); // First focusable element in the menu |
There was a problem hiding this comment.
Nitpick: you've added good names for each of these useRef elements, so we can remove the comments beside their definitions.
It's good programming practice to use comments; often the aim is to try and add reasons for our implementation methods in those comments instead of describing things that we've already written. If you've written a comment that clarifies an obscure or confusing block of code, sometimes it's worth taking a step back and wondering if there's a better way you can write the code so it's understandable from just reading it.
| const menuRef = useRef<HTMLDivElement | null>(null); // Reference for the menu | |
| const closeButtonRef = useRef<HTMLButtonElement | null>(null); // Reference for the close button | |
| const firstFocusableElementRef = useRef(null); // First focusable element in the menu | |
| const menuRef = useRef<HTMLDivElement | null>(null); | |
| const closeButtonRef = useRef<HTMLButtonElement | null>(null); | |
| const firstFocusableElementRef = useRef(null); |
src/components/Header/Header.tsx
Outdated
| const focusableElements = menuRef.current?.querySelectorAll<HTMLElement>('a, button') || []; | ||
| const firstElement = focusableElements[0]; | ||
| const lastElement = focusableElements[focusableElements.length - 1]; |
There was a problem hiding this comment.
Issue: It looks like the lint task is having an issue with this line being too long - we can fix this automatically by running the npm run format command, which is generally good to do before making a commit. You can then test to see if the lint subtask will succeed locally by running npm run lint and making sure no warnings appear.
Another good approach is to install the prettier VSCode extension if you're using this development tool. You can then configure this to automatically apply formatting on file save / show red squiggles and tell you want the errors are in the code editor itself, which helps save time.
src/components/Header/Header.tsx
Outdated
| className={`${styles.nav} ${isMenuOpen ? styles.navOpen : '' | ||
| }`} | ||
| ref={menuRef} | ||
| aria-hidden={!isMenuOpen} |
There was a problem hiding this comment.
Issue: It looks like this aria-hidden attribute is being picked up as an accessibility issue from the Storybook automated test runner.
│ 'aria-hidden-focus' │ 'serious' │ 'Ensure aria-hidden elements are not focusable nor contain focusable elements' │ 1
I think we can safely remove this attribute, as if the menu isn't open, then none of the links will be displayed, so the user can't accidentally tab through them. I tend to use aria-hidden quite rarely, more often for child items within an interactive element rather than a container element.
| aria-hidden={!isMenuOpen} |
|
hi James, thank you so much for your feedback and suggestion! I will address the changes you've highlighted and create a new follow-up PR as soon as possible. |
No description provided.