-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
fix(cdk/tree): assorted bug fixes #28305
fix(cdk/tree): assorted bug fixes #28305
Conversation
it('initializes with the first item activated', () => { | ||
expect(keyManager.getActiveItemIndex()).withContext('active item index').toBe(0); | ||
it('does not initialize with the first item activated', () => { | ||
expect(keyManager.getActiveItemIndex()).withContext('active item index').toBe(-1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, I thought we do want disabled options to be keyboard focusable? That's to align with WAI ARIA recommendation.
https://www.w3.org/WAI/ARIA/apg/practices/keyboard-interface/#focusabilityofdisabledcontrols
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm... true. do we still want to ignore clicks as well? or rather, should clicking a disabled item still focus it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that https://www.w3.org/WAI/ARIA/apg/practices/keyboard-interface/#focusabilityofdisabledcontrols recommends that disabled treeitems be focusable, but I don't think it says anything about keyboard vs mouse focus. I'll see if I can look more to see if there's a recommendation for mouse focus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ultimately we can try it as-is (we ignore a click if it's disabled), and see if anyone reports a bug?
* since the click could trigger some other component that wants to grab its own focus | ||
* (e.g. menu, dialog). | ||
*/ | ||
private _shouldFocus = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I understand why we wouldn't want to focus on click. Native elements usual focus themselves when clicked.
Do we have an example of other component that grabs its own focus?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
native elements have focus by virtue of the browser automatically leaving focus on the last interacted element; they do not force focus to be on them. the problem here is essentially:
<mat-tree-node
[matMenuTriggerFor]="menu">
</mat-tree-node>
in this scenario, without this change:
- mat-menu tries to focus the first menu item when it opens
- the tree then steals focus back to the tree item
similar behaviour occurs with dialogs where we take focus back from the dialog.
with this change, if there is no additional behaviour where focus is taken from the tree item, the browser will still leave the tree item as the currently active element since it's marked as focusable via tabindex="0"
* fix(cdk/tree): fix errors from testing * fix(cdk/tree): tests * fix(cdk/tree): update api docs * fix(cdk/a11y): allows disabled items to receive initial focus * fix(cdk/tree): don't focus on click, corrected updating aria-sets * fix(cdk/tree): update api goldens
* fix(cdk/tree): fix errors from testing * fix(cdk/tree): tests * fix(cdk/tree): update api docs * fix(cdk/a11y): allows disabled items to receive initial focus * fix(cdk/tree): don't focus on click, corrected updating aria-sets * fix(cdk/tree): update api goldens
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
No description provided.