Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
This is a followup to #3443571-36: Mobile version of Navigation should have focus trap, but it should be released along with that issue.
the close button should be reachable by keyboard same as there should be an option to close the mobile navigation by pressing the ESC key. but from my perspective if moved to a follow up those followup(s) and this issue should go into the same new version of drupal.
I've limited the scope to the close button because changing it looks quick and easy, and @rkoller told me that ESC already has another function.
Setting to Major priority because removing the "Expand sidebar" button from the tab sequence in #3443571: Mobile version of Navigation should have focus trap may remove the only mechanism for closing the sidebar by keyboard.
Personally, I consider this change to be within the scope of that issue for the same reason.
WCAG success criteria
Steps to reproduce
- Enable the Navigation module.
- Log in as admin.
- Reduce the viewport width until the Navigation "Expand sidebar" button appears at top-left.
- Activate the "Expand sidebar" button to open the sidebar.
- Press the
TABkey until the focus reaches the first menu item in the sidebar. Currently, the first menu item is "Shortcuts".
Expected behavior
The sidebar close button receives focus before the first menu item receives focus.
Actual behavior
Focus bypasses the close button and goes straight to the first menu item.
Note: The focus indicator for the Home link (Drupalicon) at the top of the sidebar is currently broken.
Proposed resolution
Remove 'tabindex': '-1', from the corresponding line in navigation.html.twig.
Remaining tasks
User interface changes
The close button will be reachable by keyboard with the TAB key.
Introduced terminology
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | issue-3572169-14-screenshot-after-adding-comment.png | 18.98 KB | kentr |
| #12 | close-button-tab.mp4 | 1.86 MB | mherchel |
Issue fork drupal-3572169
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3572169-close-button-in-tab-sequence
changes, plain diff MR !14681
Comments
Comment #2
kentr commentedComment #4
kentr commentedSeeking feedback before writing tests.
Maybe doesn't need screenshots.
Comment #5
kentr commentedComment #6
kentr commentedComment #7
rkollerIn discussion with @katannshaw, @mgifford, @mherchel, and myself, we considered this follow up to the focus trap issue as a blocker in #3391723: [PLAN] Accessibility review for new Navigation bar
p.s. not sure if we should keep the focus trap issue as the parent or better move it to related issues and make the a11y meta the parent instead?
Comment #8
smustgrave commentedThis change actually seems small enough that I think manual testing should be enough
Comment #9
rkolleri've just manually tested, everything works as expected now. the only thing i wonder about. at the moment the close button is placed second, in the tab order within the mobile menu, right after the logo, then the rest of the menu items is placed. would it make sense to place it in the tab order between the last menu item and the logo again? cuz a screenreader user expanded the menu intentionally so the odds are low that the person directly wants to close it again? and on the other hand there is also the option to close the menu by the esc button in case they change their mind (when the related issue gets in).
Comment #10
kentr commented@rkoller
Yes. I did that to tie this to the focus trap issue, but y'all made it an official higher-level blocker.
Do you consider that to be a blocker? What are your thoughts about doing it as a followup?
Well, color me surprised! 😃
It might be quick to add a check that the close button has the correct (or absent) value for
tabindex. At least, I'll add a comment to the file that the close button is intentionally included in the tab sequence to help prevent a regression.Comment #11
rkollerthanks for relinking the issues! and in regard to the position of the close button in the tabbing order. i dont wanted to imply it should be a blocker at all. i was just thinking out loud and was curious what others think. to me on second thought it just feels like there is no perfect solution in particular due to the fact that this is not a dialog modal. you could find for potential approach pro and cons. so i guess for now best might be to stick with the dom order like you did. eric bailey wrote something in that vein recently https://ericwbailey.website/published/you-probably-shouldnt-be-annotatin.... and a +1 for at least adding a comment to prevent any regression.
Comment #12
mherchelThis looks great to me. The close button is focused after the logo, which is expected.
I also
Tested to ensure the button does not receive focus when the drawer is closed
Tested to ensure the button doesn't receive focus at desktop widths.
I also agree that this does not need tests. This is not something that is likely to break.
Video attached.
Comment #13
kentr commentedI'm still going to add a comment to the file.
For the record, I disagree about tests. I consider the issue to be a fundamental part of proper dialog behavior. I'll leave the decision to others, though.
Comment #14
kentr commentedHere's a static screenshot of the focused close button after adding the comment, just to show the general visual.
Comment #19
catchCommitted/pushed to main, 11.x, and 11.3.x, thanks!