WB-1671: Dropdown: use combobox role in all openers
#2345
Conversation
🦋 Changeset detectedLatest commit: 65b00ff The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
|
NOTE: unchecking the |
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (1e9af86) and published all packages with changesets to npm. You can install the packages in webapp by running: ./services/static/dev/tools/deploy_wonder_blocks.js --tag="PR2345"Packages can also be installed manually by running: yarn add @khanacademy/wonder-blocks-<package-name>@PR2345 |
|
Size Change: +122 B (+0.12%) Total Size: 98.4 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-fixmheucmr.chromatic.com/ Chromatic results:
|
jandrade
changed the title
combobox role in all openerscombobox role in all openers
marcysutton
force-pushed
the
dropdown-combobox
branch
from
c4fb2b1 to
f90f127
Compare
marcysutton
changed the title
combobox role in all openerscombobox role in all openers
|
I rebased this one to bring it up to date and I'm proposing we merge it to deal with a number of screen reader issues for Dropdowns. Essentially, we need the |
🎉
packages/wonder-blocks-dropdown/src/components/dropdown-opener.tsx
Outdated
Show resolved
Hide resolved
| @@ -569,6 +569,7 @@ const MultiSelect = (props: Props) => { | |||
| disabled={isDisabled} | |||
| id={uniqueOpenerId} | |||
| aria-controls={dropdownId} | |||
| placeholder={noneSelected} | |||
There was a problem hiding this comment.
Should the placeholder be menuText so the aria-label changes when the selected values change? Otherwise, it'll always have an aria-label for no items selected. menuText isn't always a string though since it can be custom jsx! Or maybe the select opener wouldn't need an aria-label since the component should be used with a <label>? 🤔 What do you think!
Here's a screenshot that shows the aria-label with "0 items" while the component shows "3 items":
There was a problem hiding this comment.
Good catch @beaesguerra! I don't think that aria-label is necessarily the same as a placeholder for a combobox. This text about the number of items is more of a value than a label. I'm guessing the intent was to use a placeholder as a label in the absence of a separate label element (or associated label, however it's marked up). But we have to be careful with aria-label to not override an associated label, if it's paired with <label for="$idMatchingThisCombobox">. I'm looking into it!
There was a problem hiding this comment.
@jandrade or @beaesguerra do you know if the MultiSelect component is used with an explicit label (<label for="thisSelectComponentWithMatchingID">? I don't see a story for it like there is on SingleSelect.
I'll dig into this more -- but I'm wondering if we have any way to conditionally add an aria-label to the opener for placeholders and not clash with other labeling mechanisms. Any input you can add would be helpful!
There was a problem hiding this comment.
It should be used with a label, specifically LabeledField 😄 Here's an example in Storybook: https://5e1bf4b385e3fb0020b7073c-fnsnwmxvwb.chromatic.com/?path=/story/packages-labeledfield--fields
There was a problem hiding this comment.
Agreed, we should use LabeledField (or label on very specific cases), but there might be places where we are not using labels and aria-label could be useful. For example, the course selector in the teacher dashboard.
You'll need to have "Teacher" access (available with your @khanacademy.org account), then go to /teacher/dashboard, click on any class and use the dropdown on the top left.
NOTE: This could be a case where we could benefit from having an actual label and this could be validated with the TX team.
There was a problem hiding this comment.
Oh yes, LabeledField 🤦♀️! Thank you so much for this context. I think the challenge is at the Combobox level, where it won't know whether it's being used by labeled field. Fortunately I think we can leverage the Accessible Name Computation spec to prioritize the LabeledField label over an aria-label (in the event both are present). We can also instruct consumers when and when not to use aria-label in docs!
packages/wonder-blocks-dropdown/src/components/select-opener.tsx
Outdated
Show resolved
Hide resolved
.changeset/tasty-rockets-mix.md
Outdated
| "@khanacademy/wonder-blocks-dropdown": patch | ||
| --- | ||
|
|
||
| Update dropdown openers to use `role="combobox"` instead of `button`. |
There was a problem hiding this comment.
(very) nit: The ActionMenu also uses DropdownOpener and it still uses role="button"!
| Update dropdown openers to use `role="combobox"` instead of `button`. | |
| Update dropdown openers for SingleSelect and MultiSelect to use `role="combobox"` instead of `button`. |
There was a problem hiding this comment.
Seems very important to me! Good call!
There was a problem hiding this comment.
Added another note to the changelog about labeling!
| @@ -77,7 +77,7 @@ describe("MultiSelect", () => { | |||
| const {userEvent} = doRender(uncontrolledMultiSelect); | |||
|
|
|||
| // Act | |||
| await userEvent.click(await screen.findByRole("button")); | |||
| await userEvent.click(await screen.findByRole("combobox")); | |||
There was a problem hiding this comment.
When we make internal changes that break existing tests, do we consider it a major breaking change since it could break tests in consuming projects? In this case, I'm thinking it's not since the role isn't part of the external API of the component! I'm curious to hear what others think though!
(I might have asked this before but wanted to double check!)
There was a problem hiding this comment.
That's a really good question. I do think it should be a major breaking change! Initially I thought it would be a minor change (and I think this PR already had a changeset with patch indicated?). But I asked Chat GPT about it to counter any of my own biases and got a really good response:
Changing an ARIA role from button to combobox in an open source design system component would generally be considered a major change according to Semantic Versioning (SemVer).
Here's why:
- Breaking change: The ARIA role defines how assistive technologies and user agents interpret the element. Changing the role from
buttontocomboboxcould fundamentally alter how the component behaves for users relying on assistive technologies like screen readers. For example:
- A
buttonrole indicates a simple button for triggering an action, whereas acomboboxrole indicates an interactive control for selecting a value, often with a dropdown, which could change the expected behavior of the component significantly.- This change would affect accessibility, user experience, and potentially even the visual appearance or interaction model if other associated attributes (like
aria-expanded,aria-controls, etc.) also need to be modified.- Backward compatibility: If users of the component are relying on the previous behavior (button), changing the ARIA role to
comboboxcould break their existing implementations. This would be a backward-incompatible change.
There was a problem hiding this comment.
cc: @jandrade in case you have thoughts on how this is normally handled in WB!
If this is a major breaking change, we'll have to make sure to coordinate the perseus + webapp updates since WB is a peer dependency in perseus and we have to be more careful with the order of releases for breaking changes!
There was a problem hiding this comment.
The point about changing how Screen Readers could change based on these changes is very compelling. I've tended not to mark as major these kind of changes as the public API does not change, but as it affects the way this component is announced by SRs, then that makes this a great candidate for a major change. For the consumer unit tests changing, I don't consider it a breaking change as the bundled code does not change (or the code that is being shipped and consumed by the end user).
@beaesguerra makes another great point about coordinating with the Perseus team as I know they use the dropdown package in their widgets.
|
I've updated this PR with a few things:
The one outstanding thing I'm not sure how to handle is labeling of a custom opener. My initial attempt didn't work, as I'm not totally understanding that API just yet! Seems like it could be done as a follow-up and not block this work. |
Changing an ARIA role is considered a major breaking change in semver
This makes it more explicit that everything isn't a label in a Dropdown. You can also customize values using this object.
MultiSelect/SingleSelect no longer fall back to placeholder values for labels, so stories have been updated to reflect this.
VoiceOver was reading out "space" when it shouldn't have
"Text" in Dropdowns is confusing for content because it also potentially includes JSX nodes. Content seems more clear to me.
marcysutton
force-pushed
the
dropdown-combobox
branch
from
0cdb657 to
04fb60c
Compare
marcysutton
force-pushed
the
dropdown-combobox
branch
from
6d2fec3 to
6e0aa39
Compare
| /> | ||
| </MultiSelect> | ||
| <LabeledField | ||
| label={<strong>Associated label element</strong>} |
There was a problem hiding this comment.
Do we need the label wrapped in the <strong> tag? I'm wondering if we should use the default styling in our examples so we can avoid one-offs!
There was a problem hiding this comment.
We probably don't need it! I was trying to make it match how it looked before.
There was a problem hiding this comment.
Do these examples even get used somehow? I don't think they are actually rendered. Maybe something broke along the way? I'm considering removing them since the examples come from the *.accessibility.stories files.
There was a problem hiding this comment.
I'm also a bit unsure when we should create both .accessibility.mdx files and .accessibility.stories.ts files. I'm okay with combining them and keeping the docs in .accessibility.stories.ts files instead!
We could reference to it from the .mdx file though, here's an example:
cc: @jandrade in case you have thoughts on this!
| it("labels the custom opener with `aria-label` on MultiSelect", async () => { | ||
| // Arrange | ||
| doRender( | ||
| <MultiSelect | ||
| aria-label="Search" | ||
| onChange={jest.fn()} | ||
| opener={(eventState: any) => <button onClick={jest.fn()} />} | ||
| > | ||
| <OptionItem label="item 1" value="1" /> | ||
| <OptionItem label="item 2" value="2" /> | ||
| <OptionItem label="item 3" value="3" /> | ||
| </MultiSelect>, | ||
| ); | ||
|
|
||
| // Act | ||
| const opener = await screen.findByRole("combobox"); | ||
|
|
||
| // Assert | ||
| expect(opener).toHaveAccessibleName("Search"); | ||
| }); | ||
|
|
||
| it("prioritizes `aria-label` on the custom opener", async () => { | ||
| // Arrange | ||
| doRender( | ||
| <MultiSelect | ||
| aria-label="Not winning the label race" | ||
| onChange={jest.fn()} | ||
| opener={(eventState: any) => ( | ||
| <button | ||
| aria-label="Search button" | ||
| onClick={jest.fn()} | ||
| /> | ||
| )} | ||
| > | ||
| <OptionItem label="item 1" value="1" /> | ||
| <OptionItem label="item 2" value="2" /> | ||
| <OptionItem label="item 3" value="3" /> | ||
| </MultiSelect>, | ||
| ); | ||
|
|
||
| // Act | ||
| const opener = await screen.findByRole("combobox"); | ||
|
|
||
| // Assert | ||
| expect(opener).toHaveAccessibleName("Search button"); | ||
| }); |
There was a problem hiding this comment.
Thanks for adding these test cases!
| @@ -251,6 +253,7 @@ const MultiSelect = (props: Props) => { | |||
| shortcuts = false, | |||
| style, | |||
| className, | |||
| "aria-label": ariaLabel, | |||
There was a problem hiding this comment.
(non-blocking) docs: One thing I realized with the aria attributes is that they don't automatically show up in the Storybook component docs 😢 : https://5e1bf4b385e3fb0020b7073c-oodutnedcb.chromatic.com/?path=/docs/packages-dropdown-multiselect--docs
We could explicitly add it to the story argTypes so they show up in the table! This can also be part of other work, since this is common to all components that extend the AriaProps type
There was a problem hiding this comment.
That is a great idea! I can open an issue for it, unless there already is one!
Co-authored-by: Bea Esguerra <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/dropdown-combobox #2345 +/- ##
=================================================
=================================================
Continue to review full report in Codecov by Sentry.
|
…` to `main` (#2450) ## Summary: This PR includes the following commits: - WB-1671: Dropdown: use `combobox` role in all openers (#2345) - FEI-5533: Re-enable select keyboard tests for Dropdown and Clickable (#2420) Issue: https://khanacademy.atlassian.net/browse/WB-1824 ## Test plan: 1. Review tests to ensure they pass Original approved PRs: - #2345 - #2420 Author: marcysutton Reviewers: jandrade Required Reviewers: Approved By: jandrade Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Chromatic - Build and test on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ⏭️ Chromatic - Skip on Release PR (changesets), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ✅ gerald, ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Chromatic - Build and test on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ⏭️ Chromatic - Skip on Release PR (changesets), ✅ gerald, ⏭️ dependabot, ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Chromatic - Build and test on regular PRs / chromatic (ubuntu-latest, 20.x), ⌛ undefined, ⌛ undefined Pull Request URL: #2450
Summary:
Addreses an issue in WB dropdowns where we were using the
buttonrole in theopener, which was causing issues with screen readers. This change updates the
role to
comboboxthat is more appropriate for the dropdown opener.Issue: https://khanacademy.atlassian.net/browse/WB-1671
Test plan:
Navigate to the dropdowns in the Storybook and verify that the role of the
opener is
combobox.More details TBD