feat(ListBox): fix labels of trigger buttons #4820

asudoh · 2019-12-05T10:05:30Z

This change fixes an DAP error in <ListBox.Field> that happens because its aria-label not matching its text content. DAP takes aria-labelledby as a preferred label content (over text content or aria-label), and thus this change add aria-labelledby to <ListBox.Field>.

The original aria-label content, which is for letting user know that the UI can open/close menu, is replaced with a browser's native approach to tell assistive technology that the button is a pop-up button and let screen reader announce as such.

This change also changes <label> to <span> for dropdown and non-filterable multi select given there is no form control (<input>) associated with it.

Fixes #4531.

Changelog

New

A mechanism to map <ListBox.Field> to list box label.

Changed

Replaced <label> with <span> for the labels of dropdown and (non-filterable) multi select.

Removed

aria-label from the <div role="button"> in <ListBox.Field>.

Testing / Reviewing

Testing should make sure combo box, drop down, multi select, filterable multi select are not broken.

For reviewers - It's easier to review with whitespace change ignored, it can be done by clicking on the gear icon in Files changed tab and check off "Hide whitespace changes".

This change fixes an DAP error in `<ListBox.Field>` that happens because its `aria-label` not matching its text content. DAP takes `aria-labelledby` as a preferred label content (over text content or `aria-label`), and thus this change add `aria-labelledby` to `<ListBox.Field>`. The original `aria-label` content, which is for lettting user know that the UI can open/close menu, is replaced with a browser's native approach to tell assistive technology that the button is a pop-up button and let screen reader announce as such. This change also changes `<label>` to `<span>` for dropdown and non-filterable multi select given there is no form control (`<input>`) associated with it. Fixes carbon-design-system#4531.

netlify · 2019-12-05T10:14:21Z

Deploy preview for the-carbon-components ready!

Built with commit 4894b65

https://deploy-preview-4820--the-carbon-components.netlify.com

netlify · 2019-12-05T10:17:49Z

Deploy preview for carbon-components-react ready!

Built with commit 4894b65

https://deploy-preview-4820--carbon-components-react.netlify.com

netlify · 2019-12-05T10:17:53Z

Deploy preview for carbon-elements ready!

Built with commit 4894b65

https://deploy-preview-4820--carbon-elements.netlify.com

joshblack · 2019-12-05T16:01:04Z

One thing, do we even need the aria-label to call out Open menu? Screen reader software would be able to say whether the listbox is open/closed through aria-haspopup and aria-expanded if I understand correctly.

asudoh · 2019-12-05T21:29:17Z

This PR removes aria-label to call out Open menu.

joshblack · 2019-12-05T23:02:33Z

For sure! Sorry if I wasn't clear. I was just thinking through the DAP violation and it seemed like only removing the aria-label would be required as the violation seemed to be due to the mismatch of the aria-label being Open menu but the content being different.

What is the expected behavior for VO if the button/field is associated with the label? Specifically, how are we thinking the value would be announced?

asudoh · 2019-12-06T09:39:43Z

If we just remove aria-label, the text content inside the trigger button is announced, which will introduce a regression losing the announcement of the trigger button's "toggling" nature.

joshblack · 2019-12-06T14:25:17Z

introduce a regression losing the announcement of the trigger button's "toggling" nature.

This is why I said this above:

Screen reader software would be able to say whether the listbox is open/closed through aria-haspopup and aria-expanded if I understand correctly.

As a result, the idea was that this would not need to be communicated through aria-label.

asudoh · 2019-12-06T23:36:35Z

Seems that you have a specific change you have in your mind, or specific lines in this PR that you think should be removed. Do you want to put the code snippet if it's the former? Do you want to put some links to the specific lines of code if it's the latter? Thanks!

joshblack · 2019-12-08T18:03:59Z

If I understand correctly, the diff would be:

Remove aria-label from bx--list-box__field (already done)
Associate bx--list-box__field with bx--label (already done) and bx--list-box__label through aria-labelledby so that the value is communicated
Associate bx--form__helper-text with bx--list-box__field through aria-describedby

Could easily be wrong though, I believe the structure we have in general is inverted (e.g. trigger is a descendant of listbox, two nodes with role="listbox", etc.) and we need to correct those as well for ARIA to work as intended 👍 This also changes between different types (listbox vs combobox, etc)

Reference:

cc @dakahn just to confirm, don't want to steer anyone in the wrong direction here 👍

asudoh · 2019-12-13T07:47:01Z

Associate bx--list-box__field with ... and bx--list-box__label through aria-labelledby so that the value is communicated

Did you mean a change to <div class="bx--list-box__field"> adding aria-labelledby pointing to <span class="bx--list-box__label"> or something else?

joshblack · 2019-12-13T19:38:42Z

Yup! <div class="bx--list-box__field" aria-labelledby="bx-label-id list-box__label-id">

…elref

asudoh · 2019-12-17T08:26:46Z

Above suggested changes are fine with me, updated.

abbeyhrt · 2019-12-17T21:15:00Z

Hey @asudoh! For the MultiSelect component, it seems like this PR gets rid of the "Accessible name does not match visible text" DAP violation but then creates a "An interactive/widget must have an accessible name" violation. Not sure if we want/need to address this is in this PR but figured i'd point it out!

asudoh · 2019-12-18T02:27:27Z

@abbeyhrt Thank you for testing - Not sure if it's a false positive or not, but fixed anyway.

joshblack · 2020-01-07T22:07:43Z

@abbeyhrt do you have a chance to re-review?? 👀

abbeyhrt · 2020-01-07T22:41:36Z

packages/react/src/components/Dropdown/Dropdown.js

    const title = titleText ? (
-      <label htmlFor={id} className={titleClasses}>
+      <span id={id} className={titleClasses}>


It seems like the id being used here and in ListBox is causing a DAP violation:

Great catch @abbeyhrt! Was an error in rebasing. Fixed.

abbeyhrt

Looks great! 🎉

asudoh added the status: ready for review 👀 label Dec 5, 2019

asudoh requested review from emyarod and abbeyhrt December 5, 2019 10:05

asudoh requested a review from a team as a code owner December 5, 2019 10:05

asudoh added 4 commits December 17, 2019 14:24

feat(ListBox): more field associations

07e0276

chore(test): update snapshot

e6081a0

Merge remote-tracking branch 'upstream/master' into listbox-field-lab…

c922d29

…elref

test(ListBox): update tests

c416aa4

fix(MultiSelect): fix new DAP errors

a44a9a4

asudoh and others added 2 commits December 18, 2019 14:35

chore(test): update snapshot

bd87704

Merge branch 'master' into listbox-field-labelref

954d35d

joshblack approved these changes Jan 6, 2020

View reviewed changes

joshblack added status: merge conflict 🚫 and removed status: ready for review 👀 labels Jan 6, 2020

Merge branch 'master' into listbox-field-labelref

6809161

asudoh removed the status: merge conflict 🚫 label Jan 6, 2020

Merge branch 'master' into listbox-field-labelref

8a5f6c8

abbeyhrt reviewed Jan 7, 2020

View reviewed changes

fix(Dropdown): fix duplicated ID

e4267b6

abbeyhrt approved these changes Jan 8, 2020

View reviewed changes

asudoh added the status: ready to merge 🎉 label Jan 8, 2020

asudoh added 4 commits January 9, 2020 11:19

Merge branch 'master' into listbox-field-labelref

395a2b1

chore(Dropdown): format fix

db0f1b5

Merge branch 'master' into listbox-field-labelref

dd0923a

fix(Dropdown): remove auto-generated ID

4894b65

asudoh merged commit f87a204 into carbon-design-system:master Jan 9, 2020

asudoh deleted the listbox-field-labelref branch January 9, 2020 08:58

joshblack mentioned this pull request Jan 31, 2020

chore(project): update carbon to v10.10.0-rc.0 #5236

Closed

This was referenced Feb 25, 2020

chore(release): v10.10.0 #5432

Closed

v10.10.0 Release #5398

Closed

asudoh mentioned this pull request Feb 26, 2020

[AVT] Accessible name must match or contain the visible label text #5402

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ListBox): fix labels of trigger buttons #4820

feat(ListBox): fix labels of trigger buttons #4820

asudoh commented Dec 5, 2019

netlify bot commented Dec 5, 2019 •

edited

Loading

netlify bot commented Dec 5, 2019 •

edited

Loading

netlify bot commented Dec 5, 2019 •

edited

Loading

joshblack commented Dec 5, 2019

asudoh commented Dec 5, 2019

joshblack commented Dec 5, 2019

asudoh commented Dec 6, 2019

joshblack commented Dec 6, 2019 •

edited

Loading

asudoh commented Dec 6, 2019

joshblack commented Dec 8, 2019

asudoh commented Dec 13, 2019

joshblack commented Dec 13, 2019

asudoh commented Dec 17, 2019

abbeyhrt commented Dec 17, 2019

asudoh commented Dec 18, 2019

joshblack commented Jan 7, 2020

abbeyhrt Jan 7, 2020

asudoh Jan 7, 2020

abbeyhrt left a comment

feat(ListBox): fix labels of trigger buttons #4820

feat(ListBox): fix labels of trigger buttons #4820

Conversation

asudoh commented Dec 5, 2019

Changelog

Testing / Reviewing

netlify bot commented Dec 5, 2019 • edited Loading

netlify bot commented Dec 5, 2019 • edited Loading

netlify bot commented Dec 5, 2019 • edited Loading

joshblack commented Dec 5, 2019

asudoh commented Dec 5, 2019

joshblack commented Dec 5, 2019

asudoh commented Dec 6, 2019

joshblack commented Dec 6, 2019 • edited Loading

asudoh commented Dec 6, 2019

joshblack commented Dec 8, 2019

asudoh commented Dec 13, 2019

joshblack commented Dec 13, 2019

asudoh commented Dec 17, 2019

abbeyhrt commented Dec 17, 2019

asudoh commented Dec 18, 2019

joshblack commented Jan 7, 2020

abbeyhrt Jan 7, 2020

Choose a reason for hiding this comment

asudoh Jan 7, 2020

Choose a reason for hiding this comment

abbeyhrt left a comment

Choose a reason for hiding this comment

netlify bot commented Dec 5, 2019 •

edited

Loading

netlify bot commented Dec 5, 2019 •

edited

Loading

netlify bot commented Dec 5, 2019 •

edited

Loading

joshblack commented Dec 6, 2019 •

edited

Loading