-
Notifications
You must be signed in to change notification settings - Fork 12
Add readOnly prop to SingleSelect and MultiSelect #2873
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: a799388 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (9afaf81) and published all packages with changesets to npm. You can install the packages in ./dev/tools/deploy_wonder_blocks.js --tag="PR2873"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR2873 |
|
Size Change: +172 B (+0.16%) Total Size: 109 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-egylwtzumw.chromatic.com/ Chromatic results:
|
527557b to
2f845f6
Compare
| error: hasError, | ||
| testId: testId ? `${testId}-field` : undefined, | ||
| readOnly: readOnlyMessage || field.props.readOnly, | ||
| readOnly: !!readOnlyMessage || field.props.readOnly, |
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.
This makes it so the readOnly prop passed in is a boolean instead of a string
| cursor: "pointer", | ||
| // :focus-visible -> Provide focus styles for keyboard users only. | ||
| ...focusStyles.focus, | ||
| ":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.
Changed to :focus instead of :focus-visible so that it has the same behaviour as other form fields
Using :focus-visible (readonly state):
Screen.Recording.2025-11-26.at.11.19.02.AM.mov
Using :focus (readonly state):
Screen.Recording.2025-11-26.at.11.18.36.AM.mov
Using :focus-visible (default state - focus outline isn't shown on the selector):
Screen.Recording.2025-11-26.at.11.21.01.AM.mov
Using :focus(default state - focus outline is shown on the selector temporarily):
Screen.Recording.2025-11-26.at.11.21.13.AM.mov
| // are true, set to undefined so attribute isn't set | ||
| // Note: We set `aria-disabled` instead of `aria-readonly` due to | ||
| // low browser + screen reader support for `aria-readonly` on comboboxes. | ||
| "aria-disabled": readOnly || disabled || undefined, |
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.
Something new is that if readOnly and disabled props are falsy, the aria-disabled attribute won't be set to false explicitly anymore
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
marcysutton
left a comment
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.
This is great progress! I left some comments and questions, and found one bug with arrow key navigation.
I'm also wondering if ActionMenu is affected at all since it shares DropdownOpener. I didn't dig into that, though!
| pseudo: defaultPseudoStates, | ||
| pseudo: { | ||
| ...defaultPseudoStates, | ||
| // Using the focus selector instead so that the focus outline is shown |
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.
Thanks for adding a comment here! What an odd scenario where focus is preferable for once. Is it because it's technically programmatic 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.
It is so the focus behaviour looks more similar to other form fields! This PR comment shows the difference between using focus and focus-visible: #2873 (comment)
There is also a comment where we set the focus styles for context!
wonder-blocks/packages/wonder-blocks-dropdown/src/components/select-opener.tsx
Lines 262 to 267 in a799388
| ":focus": { | |
| // Using :focus instead of :focus-visible to ensure the focus ring is | |
| // visible when the button is focused (even when clicked). This makes | |
| // the focus behaviour more consistent with the other field components. | |
| ...focusStyles.focus[":focus-visible"], | |
| }, |
| browser + screen reader support currently with `combobox` roles. Using | ||
| `aria-disabled` and the `readOnlyMessage` provides contextual information to | ||
| users (`LabeledField`'s `readOnlyMessage` is included in the combobox element's | ||
| `aria-describedby` attribute) |
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.
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.
Good idea! I've included the LabeledField usage in the SingleSelect and MultiSelect Read Only stories. I also added examples with LabeledField for the readonly state in the a11y sections!
| <Canvas of={MultiSelectAccessibilityStories.WithVisibleAnnouncer} /> | ||
| <Canvas of={MultiSelectAccessibilityStories.WithVisibleAnnouncer} /> | ||
|
|
||
| ## Read only state |
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 thought of this by the time I read on to the SingleSelect docs, but the same applies here: can you add a working example with LabeledField to show the effect of aria-describedby and the "Read only" text?
| import userEvent from "@testing-library/user-event"; | ||
| import DropdownOpener from "../dropdown-opener"; | ||
|
|
||
| describe("DropdownOpener", () => { |
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'm surprised there weren't already tests for DropdownOpener! Thanks for adding them!
| ); | ||
|
|
||
| // Assert | ||
| expect(screen.getByRole("combobox")).not.toBeDisabled(); |
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.
That is such a nuanced difference with the toBeDisabled matcher... here's hoping they don't ever add aria-disabled to the heuristics!
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 ended up removing this test! I had originally added this when I first implemented aria-readonly before I manually tested for screen reader support (I was trying to confirm that it didn't have both aria-readonly and aria-disabled). Good to know though that we should probably explicitly check for disabled/aria-disabled attributes since this matcher is nuanced!
| <OptionItem label="item 2" value="2" /> | ||
| </MultiSelect>, | ||
| ); | ||
| const combobox = await screen.findByRole("combobox"); |
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 remembered today that Jeff suggested we avoid findBy queries unless absolutely necessary since they return Promises. Does this test pass with synchronous screen.getByRole?
| await userEvent.tab(); | ||
|
|
||
| // Assert | ||
| expect(await screen.findByRole("combobox")).toHaveFocus(); |
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.
Same as above: does this assertion work with screen.getByRole?
| ); | ||
|
|
||
| // Act | ||
| await userEvent.keyboard("{Tab}"); |
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.
Should we use await userEvent.tab(); for consistency with other tests? It is nice there's more than one way to tab programmatically!
| disabled={disabled} | ||
| // If readOnly is true, clickable behaviour should be disabled to | ||
| // prevent interactions. Note: DropdownOpener is responsible for | ||
| // adding attributes to the opener |
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.
Thanks for this comment about where the attributes are set! That will come in handy!
| onKeyDown={!disabled ? this.handleKeyDown : undefined} | ||
| onKeyUp={!disabled ? this.handleKeyUp : undefined} | ||
| onClick={allowInteraction ? this.handleClick : undefined} | ||
| onKeyDown={allowInteraction ? this.handleKeyDown : undefined} |
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.
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.
Good catch yes! I disabled interactions in DropdownCore now when the selects are in a readonly state. Also added some tests!
…ad only styles, adds the aria-readonly attribute, and disables event handlers when readOnly is true
… aria-readonly and disables the interactions
…is passed into opener components and that it can't be opened when the opened prop is true and readOnly is true
…se aria-readonly doesn't have great sr support right now (tested with Chrome + Firefox with NVDA, Safari + VoiceOver)
… focus selector for those tests
…pener for multiselect
…Select` and `MultiSelect`. It applies read only styling for the TB theme, adds aria-disabled=true to the combobox element and disables interactions on the field
…readOnly prop so it is a boolean
…row to open the single and multiselect
…so true. This disables key handlers for dropdowncore
…s before when we thought we could use aria-readonly
2f9c232 to
a799388
Compare
| jest.mock("react-popper", () => ({ | ||
| ...jest.requireActual("react-popper"), | ||
| Popper: jest.fn().mockImplementation(({children}) => { | ||
| // Mock `isReferenceHidden` to always return false (or true for testing visibility) | ||
| return children({ | ||
| ref: jest.fn(), | ||
| style: {}, | ||
| placement: "bottom", | ||
| isReferenceHidden: false, // Mocking isReferenceHidden | ||
| }); | ||
| }), | ||
| })); | ||
|
|
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 followed TDD to confirm a fix for the bug where the selects would open using the down arrow key. The SingleSelect and MultiSelect tests had different behaviours. Turns out, SingleSelect had a mock for react-popper. I've copied it over to MultiSelect so they have the same configuration!
| it.each([ | ||
| {key: "{Enter}", name: "Enter"}, | ||
| {key: " ", name: "Space"}, | ||
| {key: "{ArrowDown}", name: "Down arrow"}, | ||
| {key: "{ArrowUp}", name: "Up arrow"}, | ||
| ])( |
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.
Added more tests to make sure a variety of interactions don't open the dropdown
@marcysutton Good question. The DropdownOpener component supports a new wonder-blocks/packages/wonder-blocks-dropdown/src/components/action-menu.tsx Lines 264 to 275 in a799388
One difference is that |
marcysutton
left a comment
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.
Nice, great work @beaesguerra! I think you may need to update the branch from main for the Storybook failures, but otherwise looks good!



Summary:
As part of the Thunderblocks work, form fields have new visual styling for the readOnly state. This PR adds
readOnlysupport for SingleSelect and MultiSelect:We use
aria-disabledinstead ofaria-readonlyfor the read only state because of low browser + screen reader support.aria-readonlyon a combobox role is not currently read out (tested with NVDA + Chrome/Firefox and VoiceOver + Safari).aria-disabledwill read out "unavailable" for NVDA + Chrome/Firefox and "dimmed" for VoiceOver + Safari. When readOnly SingleSelect and MultiSelect components are used, they should be used with theLabeledFieldcomponent with thereadOnlyMessageprop set to provide context on why the field is in a read only state.Related links around aria-readonly:
Issue: WB-2046
Test plan:
For SingleSelect and MultiSelect:
/?path=/story/packages-labeledfield--fields