Problem/Motivation

On the "mobile" version, when the sidebar is closed, the elements within the sidebar are still focusable and in the tab ring.

Tabbing into the sidebar causes the focus to disappear.

The submenu toggle buttons can be activated, but the submenus are hidden.

The items are still available in the aural interface (originally reported by @rkoller: #3543541: [PP-1] on the mobile viewport with the navigation sidebar collapsed the menu items are still available in the aural interface).

screenshot showing voiceover buttons menu incorrectly listing items from the collapsed sidebar

WCAG success criteria

Steps to reproduce

  1. Enable the Navigation module.
  2. Log in as admin.
  3. Reduce the window width so that the "mobile" sidebar is active.
  4. Ensure that the sidebar is closed ("Expand sidebar" button is visible).
  5. Start tabbing from the top of the page before the "Expand sidebar" button, and continue past the "Expand sidebar" button.
Expected

After reaching the "Expand sidebar" button, focus moves to the next visible focusable item on the page.

Actual

Focus disappears as it moves into the closed sidebar.

Proposed resolution

  • When the mobile sidebar is closed, completely hide it with visibility: hidden.
  • Add visibility to the sidebar's CSS transition so that open & close animations are preserved.
  • Ensure this does not affect the desktop sidebar.

The attached videos demonstrate before / after with visibility: hidden.

Remaining tasks

User interface changes

  • The focus will stay on visible elements.
  • Elements in the collapsed sidebar will not be available in the aural interface.

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#31 63b511bb62385a9e25f4db3f03324933.gif1.57 MBfinnsky
#30 after.mp4418 KBrkoller
#30 before.mp4892.14 KBrkoller
#25 issue-3541910-25-using-animated-visibility-only.480p.mov1022.98 KBkentr
#14 Issue-3541910-after.gif979.6 KBsandip
#10 6bc82d5d5a9d79f540048d1d03c56327.gif744.3 KBfinnsky
#6 sidebar_accessibility_tree_before.png145.61 KBshqshqnk
#6 sidebar_accessibility_tree_after.png113.87 KBshqshqnk
hidden-sidebar-items-focusable.before.mov1.16 MBkentr
hidden-sidebar-items-focusable.after_.mov634.42 KBkentr

Issue fork drupal-3541910

Command icon 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:

About issue forks

Comments

kentr created an issue. See original summary.

kentr’s picture

kentr commented
Related issues: +#3543541: [PP-1] on the mobile viewport with the navigation sidebar collapsed the menu items are still available in the aural interface

There's overlap with #3543541: [PP-1] on the mobile viewport with the navigation sidebar collapsed the menu items are still available in the aural interface.

The proposed fix here does remove them from the VoiceOver rotor, so I think it hides them from the aural interface.

catch’s picture

catch
he/him
Primary language English
commented
Priority: Normal » Major

Postponing #3543541: [PP-1] on the mobile viewport with the navigation sidebar collapsed the menu items are still available in the aural interface on this issue and bumping to major.

I can't see why we'd want to allow tabbing into hidden elements when the sidebar is collapsed, that's what the expand button is for, so the propose resolution makes sense to me.

shqshqnk made their first commit to this issue’s fork.

shqshqnk opened merge request !13952

shqshqnk’s picture

shqshqnk commented
StatusFileSize
new sidebar_accessibility_tree_after.png113.87 KB
new sidebar_accessibility_tree_before.png145.61 KB

Fixed the issue with hidden navigation links still focusable on mobile by hiding them with visibility: hidden when the sidebar is collapsed.

shqshqnk’s picture

shqshqnk commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Think we could maybe expand an existing javascript test to test for focus.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
Issue tags: +Needs manual testing

Let's get some manual testing of this first before trying to write automated tests for it.

finnsky’s picture

finnsky commented
StatusFileSize
new 6bc82d5d5a9d79f540048d1d03c56327.gif744.3 KB

I think this needs to be fixed differently.
Neither
display:none
nor
visibility:hidden

will work here because we have a collapse animation. It's gone now. Perhaps we could add visibility:hidden with some delay.

regression

finnsky’s picture

finnsky commented
Status: Needs review » Needs work
finnsky’s picture

finnsky commented

I think it's better to do this through

https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Global_attri...

We already have examples of use in the module.
For example, we did something similar here
https://www.drupal.org/project/navigation/issues/3436130

sandip’s picture

sandip commented

I am working on it based on the suggestion at #12

sandip’s picture

sandip commented
Status: Needs work » Needs review
StatusFileSize
new Issue-3541910-after.gif979.6 KB

Please review the changes. Adding a gif to see after changes.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Reviewed & tested by the community

will work here because we have a collapse animation. It's gone now. Perhaps we could add visibility:hidden with some delay.

visibility supports CSS transitions. So it can work with visibility (we do so within Olivero).

But, the solution within the MR (toggling inert via JS) works just as well.

Tested this out, and works well!

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Reviewed & tested by the community » Needs work

Setting back to NW. Just discovered that at wide widths, and the navigation is in its "narrow" mode, it's set to inert, which makes it inoperable.

nod_’s picture

nod_
Primary language French
Location Lille
commented

And the tests caught it so we do have some test for this somehow :)

sandip’s picture

sandip commented

Yes i got the issue i am looking into it

sandip’s picture

sandip commented

I removed the inert toggle for desktop screens as it isn’t needed there, and everything is working as expected now. The only remaining issue is that when I change the screen width in the browser’s DevTools, the toggleTriggers function doesn’t run on screen width change, so I am thinking we may need to call toggleTriggers inside a screen change listener, but I am not sure if thats the best approach.

Does it make sense to call toggleTriggers on viewport changes or is there a better alternative? Looking for suggestions or best practices here.

kentr’s picture

kentr commented

With the issues in #19:

Using a listener to detect width changes sounds complex and flaky.

Animating visibility (#15) sounds feasible and much simpler than using JS.

Or if animating visibility isn't acceptable, what about using visibility on a child of the animated element (if there is a single child element that contains the problem)?

kentr’s picture

kentr commented

Playing around in devtools on 11.x (without this MR), the following CSS appears to fix the tabbing problem while preserving the animations.

It can be cleaned up a little.

/* We apply transitions after page load to avoid shifts. */
[data-admin-toolbar-transitions] {
  :is(#extra-specificity-hack, [data-drupal-admin-styles]) {
    &.admin-toolbar {
      /* START CHANGE */
      transition: transform var(--admin-toolbar-transition), visibility var(--admin-toolbar-transition);

      @media (width < 64rem) {
        visibility: hidden;
      }
      /* END CHANGE */
      
      @media (min-width: 64rem) {
        transition: width var(--admin-toolbar-transition);

        ~ .dialog-off-canvas-main-canvas {
          transition: margin-inline-start var(--admin-toolbar-transition);
        }
      }
    }
  }
}

:is(#extra-specificity-hack, [data-drupal-admin-styles]) {
  &.admin-toolbar {
    [data-admin-toolbar="expanded"] & {
      transform: none;
      /* START CHANGE */
      visibility: visible;
      /* END CHANGE */
    }
  }
}
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

@kentr Do you want to open up a new MR?

If you're not familiar with either the MR process or contributing code to core, I'm more than happy to help!

kentr’s picture

kentr commented

@mherchel: Sure. I didn't want to step on any toes.

kentr opened merge request !14200

kentr’s picture

kentr commented
StatusFileSize
new issue-3541910-25-using-animated-visibility-only.480p.mov1022.98 KB

MR !14200 has test failures, but the ones I saw look unrelated.

I've run some of them locally with no issues. I'll hazard a guess that they're related to package download problems in CI. Maybe there are still DDOS problems.

I reran some of them on gitlab, and am rerunning others now.

I'll look more into the failures when I have a chance, if someone else doesn't get to it first.

Here's a screen recording showing tabbing past the closed sidebar (rather than into it), tabbing through the expanded sidebar, the open & close actions, and tabbing into the "desktop" sidebar.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Test failures looked random, re-ran the two remaining failed jobs, this looks like it needs review.

kentr’s picture

kentr commented

I think the test failures are related to #3557585: Update to Composer 2.9.2.

kentr’s picture

kentr commented
Issue tags: +Needs rebase

Ah, yeah. The fork / branch is behind.

I'll rebase it.

kentr’s picture

kentr commented
Issue tags: -Needs rebase

Rebased.

rkoller’s picture

rkoller
he/him
Primary language German
Location Nürnberg, Germany
commented
StatusFileSize
new before.mp4892.14 KB
new after.mp4418 KB

I'Ve already tested this last night. made me already close the postponed #3543541: [PP-1] on the mobile viewport with the navigation sidebar collapsed the menu items are still available in the aural interface as a duplicate cuz this issue also fixes the problem in the aural interface. tested in the latest safari, firefox, and edge on macOS 26.2, all work. i'll also add the before and after videos i've created before i saw @kentr already uploaded one. i upload them anyway cuz they illustrate aside the tab behavior also the aural interface in voiceover as well as the available list of links in the voiceover rotor. tests are green. so from my manual testing perspective a +1 for RTBC. thank you for working on that @kentr! i leave it to needs review cuz i am not qualified to review the code.

finnsky’s picture

finnsky commented
StatusFileSize
new 63b511bb62385a9e25f4db3f03324933.gif1.57 MB

I started testing and found another bug with this animation.
The navigation block width is set incorrectly for different screen sizes. This becomes apparent when I increase the animation time.

width bug

I decided to fix it here to completely resolve this issue.

Otherwise, everything is ready for RTBC, I don’t mark it that way because I added the code myself

kentr’s picture

kentr commented
Issue summary: View changes
Issue tags: -Needs accessibility review +wcag2411, +wcag131, +wcag132

What about an automated test?

I updated the IS and tags to encompass #3543541: [PP-1] on the mobile viewport with the navigation sidebar collapsed the menu items are still available in the aural interface.

@rkoller, please change it as needed.

Also:

  • Updated the Proposed resolution to reflect the discussion.
  • Removed Needs accessibility review because my original concerns about changing the aural interface were resolved and @rkoller has tested the changes (#30).
finnsky’s picture

finnsky commented

Rebased

What about an automated test?

IDK. Probably we need to write them.

catch’s picture

catch
he/him
Primary language English
commented

If we have an existing automated test that can be adapted to cover this, that's great. If it needs entirely new test coverage, I'd be happy to add that in a follow-up given this has had extensive manual testing. It seems like one of the bigger remaining navigation accessibility issues (at least the aural interface part, not sure how important keyboard navigation on mobile is in practice) so would be good to get in if people are otherwise happy.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

quietone’s picture

quietone commented
Issue tags: +11.4.0 release priority

kentr changed the visibility of the branch 3541910-elements-in-closed to hidden.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs review » Reviewed & tested by the community

I personally think this is fine to commit without the automated tests. Confirmed this manually both with the steps from the issue summary and with screen reader.

  • catch committed cc67522d on 11.x 
    fix: #3541910 Elements in closed sidebar are focusable

By: kentr
By:...

  • catch committed 2ead80d6 on main 
    fix: #3541910 Elements in closed sidebar are focusable

By: kentr
By:...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Patch (to be ported)

Opened #3570453: Test coverage for closed sidebar focus in Navigation for test coverage, would be great to do that with playwright or a functional js + Axe test if we can.

While I'm not qualified to review the CSS changes, this all seems sensible and has had multiple rounds of review and testing, so I'm happy going head here. Committed/pushed to main and 11.x, thanks!

Do we think this can/should be backported to 11.3.x? If so happy to do that but a bit wary due to some other Navigation CSS regressions in 11.3.0 due to the reset, so would rather here from someone else that it'll be fine before doing so (and maybe shortly after next week's patch release instead of shortly before). Moving to 'to be ported' for that.

catch closed merge request !14200

smustgrave opened merge request !15373

smustgrave’s picture

smustgrave commented

Actually since it's been 2 months do we want to backport anymore?

smustgrave’s picture

smustgrave commented
Status: Patch (to be ported) » Fixed

Going to go ahead and mark this fixed. Since 11.4 is closer to coming out then before. If I'm wrong let me know.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.