Skip to content

docs(cdk-experimental/listbox): add docs & examples #25317

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

Merged
merged 3 commits into from
Aug 1, 2022

Conversation

mmalerba
Copy link
Contributor

No description provided.

@mmalerba mmalerba added target: minor This PR is targeted for the next minor release dev-app preview When applied, previews of the dev-app are deployed to Firebase labels Jul 20, 2022
@mmalerba mmalerba requested a review from zarend July 20, 2022 23:55
@github-actions
Copy link

github-actions bot commented Jul 21, 2022

@mmalerba
Copy link
Contributor Author

I embedded all the examples I added in the dev-app, you can check them out here: https://ng-comp-dev--pr-25317-2998a6aea0f4c5d81c00616767c9e8e1-rzze2wbv.web.app/cdk-experimental-listbox

Copy link
Contributor

@zarend zarend left a comment

Choose a reason for hiding this comment

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

I only skimmed looking over the code, but I did test this with voiceover on chrome. Here's what I found.

The checkmark unicode character should aria-hidden="true" because it is decorative. Otherwise screen readers will actually try to read the checkmark. Probably fine to not put aria-hidden="true" on the emojis.

image

^ Syrah is selected, but it should only be focused and not selected. This also happens in the examples: "Listbox with Value Binding", "Listbox with compareWith Function", "Listbox with Reactive Forms", "Listbox with Forms Validation", "Listbox with Disabled Options", "Horizontal Listbox", "Listbox with Custom Typeahead", and "Listbox with Custom Keyboard Navigation Options"

The listbox spec has an optional requirement where pressing Home and End moves focus to the start or end of the listbox. Not required, but it's an improvement if you're looking for something. https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/listbox_role

@mmalerba
Copy link
Contributor Author

@zarend

checkmark

ok, its on a ::before element, so aria-hidden isn't possible, but I switched it to a background image, which hopefully works

focus/selected

are you saying that the sceenreader is reading "Syrah" as selected, or that it looks selected? The background color was supposed to indicate focused-ness but I was applying it based on the active class which remains if the entire listbox is defocused. I've switched it to target :focus so hopefully it makes more sense now

home/end

are you saying those keys don't work for you? they seem to be working for me

@zarend
Copy link
Contributor

zarend commented Jul 28, 2022

@zarend

checkmark

ok, its on a ::before element, so aria-hidden isn't possible, but I switched it to a background image, which hopefully works

focus/selected

are you saying that the sceenreader is reading "Syrah" as selected, or that it looks selected? The background color was supposed to indicate focused-ness but I was applying it based on the active class which remains if the entire listbox is defocused. I've switched it to target :focus so hopefully it makes more sense now

home/end

are you saying those keys don't work for you? they seem to be working for me

Home/End are not working correctly when I have VoiceOver turned on. This only aplys to the examples "Listbox with Value Binding", "Listbox with compareWith Function", "Listbox with Reactive Forms", "Listbox with Forms Validation", "Listbox with Disabled Options", "Horizontal Listbox", "Listbox with Custom Typeahead", and "Listbox with Custom Keyboard Navigation Options". However, it works fine when voiceover is turned off.

Yes voiceover is reading Syrah as selected when I navigate to it via the down arrow. When I navigate via screen reader navigation (cmd + option + down), Voice Over reads Syrah, but not as selected.

@zarend
Copy link
Contributor

zarend commented Jul 28, 2022

cdk-listbox-voiceover.mov
  1. Tab to listbox
  2. press End
  3. (last item in listbox is activated but screen reader focus seems to move to a button at the top of the page).

Looks like the listbox focuses the correct item when I press End with VoiceOver enabled, but the screen reader's navigation state gets in a bad state.

@mmalerba
Copy link
Contributor Author

mmalerba commented Aug 1, 2022

@zarend I definitely want to look into the strange a11y behavior you're seeing, but I'll do it in a followup PR

@mmalerba mmalerba added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release and removed target: minor This PR is targeted for the next minor release labels Aug 1, 2022
@mmalerba mmalerba merged commit 083f437 into angular:main Aug 1, 2022
mmalerba added a commit to mmalerba/components that referenced this pull request Aug 1, 2022
* docs(cdk-experimental/listbox): add docs & examples

* fixup! docs(cdk-experimental/listbox): add docs & examples

* fixup! docs(cdk-experimental/listbox): add docs & examples
mmalerba added a commit that referenced this pull request Aug 1, 2022
* docs(cdk-experimental/listbox): add docs & examples

* fixup! docs(cdk-experimental/listbox): add docs & examples

* fixup! docs(cdk-experimental/listbox): add docs & examples
@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 Sep 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker dev-app preview When applied, previews of the dev-app are deployed to Firebase target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants