Skip to content

Conversation

@crisbeto
Copy link
Member

Along the same lines as #15499. Fixes users being able to focus a disabled button toggle by clicking on it. The issue comes from us preserving the -1 tabindex, even if the button is disabled.

@crisbeto crisbeto added the target: patch This PR is targeted for the next patch release label Mar 18, 2019
@crisbeto crisbeto requested a review from jelbourn as a code owner March 18, 2019 21:44
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Mar 18, 2019
@jelbourn
Copy link
Member

So, funny story, turns out we need to remove the tabindex="-1" from these components where we're redirecting focus to an underlying element. The problem is that some screen-readers (notably ChromeVox) will stop on anything with a tabindex like this when navigating through items (not with tab, but with the reader's own navigation). This leads to situations where the user ends up landing on both <mat-toggle-button> and then the internal <button>, making it seem like they hit the same thing twice.

But button-toggle in particular, I'd like to resolve this by eventually changing to <button mat-toggle-button>, but I'm not sure there's anything we can do for checkbox and radio button.

@crisbeto
Copy link
Member Author

The reason why we started setting a tabindex was to support cases like <mat-checkbox cdkFocusInitial>. If we were to remove the tabindex again, it will break those cases.

@jelbourn
Copy link
Member

jelbourn commented Mar 21, 2019

I think we'll have to use a different approach for that. Probably something like having MatCheckbox provide some symbol like IndirectFocusTarget with the custom action and having the cdkFocusInitial use that

@crisbeto
Copy link
Member Author

I don't think we can do that, because in a lot of cases we attach the focus trap directly to a DOM node which means that can't use a ContentChild to find the focus target.

@jelbourn
Copy link
Member

What if we just straight up monkey-patch the focus method on the host element?

@crisbeto
Copy link
Member Author

That technically would work, but it feels a little hacky. Also we have a check on the focus trap that logs a warning if the cdkFocusInitial element isn't focusable which we'd have to remove.

@jelbourn
Copy link
Member

Let's discuss this in the team weekly and decide on a final course of action

Along the same lines as angular#15499. Fixes users being able to focus a disabled button toggle by clicking on it. The issue comes from us preserving the -1 tabindex, even if the button is disabled.
@crisbeto crisbeto force-pushed the button-toggle-host-tabindex branch from 4fda4de to ff08948 Compare May 2, 2019 06:25
@crisbeto
Copy link
Member Author

Closing since this is no longer relevant after 39e4e24.

@crisbeto crisbeto closed this Dec 20, 2020
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants