Skip to content

fix(button-toggle): able to focus disabled button via click #15521

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.

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

Closed

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