Skip to content

fix(material/sidenav): fix tabindex issue on side mode #30979

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(material/sidenav): fix tabindex issue on side mode #30979

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a1c6329
fix(material/sidenav): fix tabindex issue on side mode
DBowen33 Apr 29, 2025
51c1075
fix(material/sidenav): redo logic
DBowen33 May 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/material/sidenav/drawer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ export class MatDrawerContent extends CdkScrollable implements AfterContentInit
// this was also done by the animations module which some internal tests seem to depend on.
// Simulate it by toggling the `hidden` attribute instead.
'[style.visibility]': '(!_container && !opened) ? "hidden" : null',
'tabIndex': '-1',
'[attr.tabindex]': 'mode === "side" ? null : "-1"',
Copy link
Member

@crisbeto crisbeto May 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it needs to remain focusable, because this logic falls back to it if it can't find any focusable elements inside the content. Maybe it's more appropriate to clear the tabindex when the sidenav is closed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clearing the tabindex when the sidenav is closed will still cause TB focus to focus on the container when it's open due to the tabindex="-1" still being present.

I was thinking that since mode="side", the autofocus value will always be dialog according to this logic. Since the value is dialog, it won't reach the first-tabbable case when it's side mode.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that you can do '[attr.tabindex]': 'isOpen ? -1 : null' which would prevent it from receiving focus. Another option can be to mark it as inert.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to this: '[attr.tabindex]': '(!opened || mode === "side") ? null : -1',
Let me know if that makes sense, anytime the sidenav is closed or mode is side, the tab-index is cleared.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't understand why we need to special-case side mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like instead of basing the special case off side mode, it needs to be based off whether contents are present in the drawer.

Having '[attr.tabindex]': 'isOpen ? -1 : null' makes sense because you don't need the tabindex attribute when the drawer is closed. However, tabindex="-1" needs to be present when opened if there are no tabbable elements so that the container can get focus and not break keyboard navigation.

So it looks like I need to set some logic to where if there are no tabbable elements in the drawer when opened, then the tabindex="-1" needs to be present, if not it can be cleared. In this case, it will solve the original issue of mobile screen readers focusing on the drawer container when it has tabbable elements inside.

Are there any existing properties or functions that see whether the drawer has tabbable contents?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that we need to cater to the cases where there's no focusable content in the sidenav. It's up to the user to provide some. We should just make sure that the user can't access the content while it's off-screen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on making sure the user can't access the content while it's off-screen. However, the root of the issue lies when the side drawer is open and there are focusable elements in the side drawer, there is still a tabindex="-1" on the side drawer container.

This causes Talkback to focus on the container anytime the user wants to get to the focusable objects in the container due to the tabindex="-1". If it's up to the user to provide content in the side drawer, is the tabindex="-1" still needed even when the side drawer is open?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we fix it by forwarding focus to the first focusable descendant whenever focus lands on the drawer container?

},
changeDetection: ChangeDetectionStrategy.OnPush,
encapsulation: ViewEncapsulation.None,
Expand Down
Loading