Skip to content
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(material/core): remove tabindex from mat-option #26917

Merged
merged 1 commit into from
Apr 14, 2023

Conversation

zarend
Copy link
Contributor

@zarend zarend commented Apr 12, 2023

Remove the tabindex attribute added to MatOption components. MatOption does not need tabindex because the parent component manages focus by setting aria-activedescendant attribute. Previously, MatOption set tabindex but was also a referenced by aria-activedescendant. This was not the correct ARIA semantics. Align closer to ARIA spec by using only aria-activedescendant rather than both.

Tabindex="-1" seems to be causing a problem in #26861 where VoiceOver with Firefox moves DOM focus from the combobox to the option when opening the listbox popup.

Unblocks #26861.

@zarend zarend added Accessibility This issue is related to accessibility (a11y) target: patch This PR is targeted for the next patch release area: material/autocomplete area: material/select dev-app preview When applied, previews of the dev-app are deployed to Firebase labels Apr 12, 2023
@zarend zarend added the action: merge The PR is ready for merge by the caretaker label Apr 13, 2023
@zarend zarend force-pushed the mat-option-remove-tabindex-2 branch from 0ecd962 to a398e06 Compare April 13, 2023 18:44
Remove the tabindex attribute added to MatOption components. MatOption
does not need tabindex because the parent component manages focus by
setting `aria-activedescendant` attribute. Previously, MatOption set
tabindex but was also a referenced by aria-activedescendant. This was
not the correct ARIA semantics. Align closer to ARIA spec by using only
aria-activedescendant rather than both.

Tabindex="-1" seems to be causing a problem in angular#26861 where VoiceOver
with Firefox moves DOM focus from the combobox to the option when
opening the listbox popup.

Unblocks angular#26861.
@zarend zarend force-pushed the mat-option-remove-tabindex-2 branch from a398e06 to cdcec28 Compare April 13, 2023 18:45
@github-actions
Copy link

Deployed dev-app for a398e06 to: https://ng-dev-previews-comp--pr-angular-components-26917-te8fy1tr.web.app

Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt.

@zarend
Copy link
Contributor Author

zarend commented Apr 14, 2023

Since this was last reviewed, I moved _getTabIndex back to MatOptionBase. That's where the method originally was. Moving that method caused the typescript types to break for some situations. That's because some members expect a MatOption and adding a _getTabIndex method to LegacyMatOption makes it no longer type compatible with MatOption.

@angular-robot angular-robot bot merged commit 97410fa into angular:main Apr 14, 2023
angular-robot bot pushed a commit that referenced this pull request Apr 14, 2023
Remove the tabindex attribute added to MatOption components. MatOption
does not need tabindex because the parent component manages focus by
setting `aria-activedescendant` attribute. Previously, MatOption set
tabindex but was also a referenced by aria-activedescendant. This was
not the correct ARIA semantics. Align closer to ARIA spec by using only
aria-activedescendant rather than both.

Tabindex="-1" seems to be causing a problem in #26861 where VoiceOver
with Firefox moves DOM focus from the combobox to the option when
opening the listbox popup.

Unblocks #26861.

(cherry picked from commit 97410fa)
angular-robot bot pushed a commit that referenced this pull request Apr 14, 2023
Remove the tabindex attribute added to MatOption components. MatOption
does not need tabindex because the parent component manages focus by
setting `aria-activedescendant` attribute. Previously, MatOption set
tabindex but was also a referenced by aria-activedescendant. This was
not the correct ARIA semantics. Align closer to ARIA spec by using only
aria-activedescendant rather than both.

Tabindex="-1" seems to be causing a problem in #26861 where VoiceOver
with Firefox moves DOM focus from the combobox to the option when
opening the listbox popup.

Unblocks #26861.

(cherry picked from commit 97410fa)
@nspire909
Copy link

How was this not considered a breaking change? By removing the tabindex from the MatOption component, you make it not focusable. So $event.relatedTarget in the focusout event will always be null, no mater where on the screen you click. With the tabindex attribute on the MatOption, $event.relatedTarget will be set to one of the MatOption elements if you click inside the dropdown and null if you click outside. I am not sure how you are supposed to tell if you clicked inside or outside of the dropdown without this. Either way I can fix it by manually adding the tabindex, but I feel this should have been considered a breaking change and not pushed out in a patch release.

@zarend
Copy link
Contributor Author

zarend commented Apr 27, 2023

Hello @nspire909 ,

MatOption is intended to be used with the aria-activedescendant pattern. The parent element always has browser focus and it references the active option with aria-activedescendant.

It's a bit fuzzy sometimes, but generally the attributes that that components render are not considered part of the public API, so changing them is generally not considered a breaking change. I hope this change was not a significant inconvenience.

I am not sure how you are supposed to tell if you clicked inside or outside of the dropdown without this

I was wondering if you could help us understand what you needs are. What are you trying to accomplish by detecting if clicks are inside or outside the dropdown?

-Zach

crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Apr 28, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@angular/cdk](https://github.com/angular/components) | dependencies | patch | [`15.2.6` -> `15.2.8`](https://renovatebot.com/diffs/npm/@angular%2fcdk/15.2.6/15.2.8) |
| [@angular/material](https://github.com/angular/components) | dependencies | patch | [`15.2.6` -> `15.2.8`](https://renovatebot.com/diffs/npm/@angular%2fmaterial/15.2.6/15.2.8) |

---

### Release Notes

<details>
<summary>angular/components</summary>

### [`v15.2.8`](https://github.com/angular/components/blob/HEAD/CHANGELOG.md#&#8203;1528-nickel-nickel-2023-04-19)

[Compare Source](angular/components@15.2.7...15.2.8)

##### material

| Commit | Type | Description |
| -- | -- | -- |
| [af293a124](angular/components@af293a1) | fix | **core:** remove tabindex from mat-option ([#&#8203;26917](angular/components#26917)) |
| [5623a8c8b](angular/components@5623a8c) | fix | **form-field:** inconsistent height for non-text inputs ([#&#8203;26941](angular/components#26941)) |

<!-- CHANGELOG SPLIT MARKER -->

### [`v15.2.7`](https://github.com/angular/components/blob/HEAD/CHANGELOG.md#&#8203;1527-paper-pelican-2023-04-13)

[Compare Source](angular/components@15.2.6...15.2.7)

##### material

| Commit | Type | Description |
| -- | -- | -- |
| [de22cdc72](angular/components@de22cdc) | fix | **table:** correct filterPredicate typo ([#&#8203;26902](angular/components#26902)) |

##### material-luxon-adapter

| Commit | Type | Description |
| -- | -- | -- |
| [6c00403a8](angular/components@6c00403) | fix | zone on DateTime ignored ([#&#8203;26887](angular/components#26887)) |

<!-- CHANGELOG SPLIT MARKER -->

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [x] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS40Ni4wIiwidXBkYXRlZEluVmVyIjoiMzUuNjEuMCJ9-->

Co-authored-by: cabr2-bot <[email protected]>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1865
Reviewed-by: Epsilon_02 <[email protected]>
Co-authored-by: Calciumdibromid Bot <[email protected]>
Co-committed-by: Calciumdibromid Bot <[email protected]>
@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 May 28, 2023
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Accessibility This issue is related to accessibility (a11y) action: merge The PR is ready for merge by the caretaker area: material/autocomplete area: material/select dev-app preview When applied, previews of the dev-app are deployed to Firebase target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants