fix: prevent menu links from being focusable when menu is closed #986
Conversation
There was a problem hiding this comment.
Pull Request Overview
Fixes keyboard navigation accessibility issue where hidden menu links remained focusable when the menu was closed, causing unwanted menu expansion on mobile viewports.
- Adds dynamic
tabIndexattribute to all navigation links based on menu state - Prevents focus from moving to hidden menu links when tabbing away from menu button
- Ensures proper tab order flow for keyboard users
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@coseeian Please take a look |
There was a problem hiding this comment.
Thanks for your work on this.
For a more robust accessibility solution, I'd suggest using the hidden attribute on the container rather than managing tabIndex on each link.
<ul id="main-links-list" hidden={!isOpen}>
{links.map((link) => (
<li key={link.label}>
<a href={link.url}>{link.label}</a>
</li>
))}
</ul>(The snippet above is a conceptual example; you'll need to adapt it to fit your specific implementation.)
The key benefit is that hidden removes the elements from both the tab order and the accessibility tree. This is crucial for preventing a confusing experience where a screen reader might accidentally announce links that are supposed to be visually collapsed or not interactive.
Since .mainlinks doesn't use CSS transitions, using hidden won't negatively impact animations.
Additionally, to complete the semantics, we could add aria-controls to the toggle button. This explicitly links the controller to the content it manages, providing better context for assistive technology.
References:
What are your thoughts on this approach?
| <li> | ||
| <a className={styles.buttonlink} href="/donate/"> | ||
| <a | ||
| class={styles.buttonlink} |
There was a problem hiding this comment.
nit: the className to class changes aren't necessary for this PR's core functionality. To keep the diff focused, we could leave these as className for now.
| <li> | ||
| <a className={styles.buttonlink} href="https://editor.p5js.org"> | ||
| <a | ||
| class={styles.buttonlink} |
There was a problem hiding this comment.
nit: the className to class changes aren't necessary for this PR's core functionality. To keep the diff focused, we could leave these as className for now.
| {links.map((link) => ( | ||
| <li key={link.label}> | ||
| <a href={link.url}>{link.label}</a> | ||
| <a href={link.url} tabIndex={isOpen ? 0 : -1}> |
There was a problem hiding this comment.
minor: since <a> elements are naturally focusable, we only need tabIndex when hiding them. Consider using tabIndex={isOpen ? undefined : -1} instead of the ternary with 0, which avoids redundant attributes in DOM.
This would apply to all three link groups (nav links on L78, editor button on L89, and donate button on L101).
2 participants
Description
When keyboard users navigated to the menu button and pressed Tab to move focus away, the menu list would automatically expand but was not fully visible on mobile viewports. This occurred because the menu links were still in the tab order even when the menu was closed, causing focus to move inside the hidden menu panel.
Fixes #915
Solution
Added dynamic
tabIndexto navigation links:tabIndex={0}(links are focusable)tabIndex={-1}(links are removed from tab order)Menu list no longer automatically expands when keyboard users tab away from the menu button.
This makke sures that tabbing away from the menu button moves focus to the next element outside the menu, not to hidden links inside it.