fix: correct the display of checkbox read-only state #3328
Conversation
🦋 Changeset detectedLatest commit: 67057d1 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 |
|
🚀 Deployed on https://pr-3328--spectrum-css.netlify.app |
File metricsSummaryTotal size: 4.30 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
Detailscheckbox
fieldgroup
* Results are not gzipped or minified. * An ASCII character in UTF-8 is 8 bits or 1 byte. |
There was a problem hiding this comment.
Love this- the read-only checkboxes are focusable, but cannot be checked/unchecked. 👍 I love that you added a bunch of the documentation from the guidelines site- thank you!
I did have a question about the expectations for what would be announced by a screenreader in read-only cases. Maybe that's something that's part of the research on read-only that's coming up. When using voice over, a read-only checkbox is announced like any of the other non-disabled checkboxes, so how would a user know that they cannot change the selection? I would think this would probably be a bigger discussion involving all of our team and some accessibility folks, so I'm not going to let it block my approval right now. Just wondered if you had any knowledge I don't!
| * A group of read-only checkboxes that have been checked. In U.S. English, use commas to delineate items within read-only checkbox groups. In other languages, use the locale-specific formatting. | ||
| * Implementations should include the following behavior for read-only checkboxes: | ||
| * - Read-only checkboxes are immutable, i.e. their checked state cannot be changed. | ||
| * - Unlike disabled checkbox groups, the normally focusable elements of a checkbox group should remain focusable. | ||
| */ | ||
| export const ReadOnly = Template.bind({}); |
There was a problem hiding this comment.
Do you think we should eventually add a "read-only radio group" to the docs page? I was wondering why field group only has "read-only checkboxes" on this page, since read-only radios exist too.
There was a problem hiding this comment.
That'd be a good idea for CSS-1005 that looks at this issue for radios.
marissahuysentruyt
requested review from
marissahuysentruyt
and removed request for
marissahuysentruyt
marissahuysentruyt
dismissed
their stale review
I reviewed too early before the PR was ready.
jawinn
force-pushed
the
jawinn/css-1004-checkbox-readonly-refactor
branch
from
32a1113
to
c2cac21
Compare
Good call-out. VoiceOver reads it the same way that you had heard, for React Spectrum's version that uses |
jawinn
force-pushed
the
jawinn/css-1004-checkbox-readonly-refactor
branch
from
c2cac21
to
f1def57
Compare
This removes some unnecessary read-only styles. Read-only just needs to override disabled styles; otherwise it uses default styles (for both default and emphasized). Also adjusts template to include aria-disabled when in the read-only state, so screen readers such as VoiceOver do not announce the input as normal. Note that aria-readonly is not used as it is not announced property and MDN states that it is "not relevant" for input with type checkbox.
The previous display of the read-only state, without checkboxes and displaying commas did not match up with any guidelines. This update is to make the display of read-only match with what is implemented in React Spectrum. It also includes aria-labeledby in the template so the group is announced by screen readers.
jawinn
force-pushed
the
jawinn/css-1004-checkbox-readonly-refactor
branch
from
f1def57
to
67057d1
Compare
| @@ -46,6 +50,7 @@ export const Template = ( | |||
| label, | |||
| isRequired, | |||
| alignment: labelPosition === "side" ? "right" : "top", | |||
| id: groupLabelId, | |||
| }, | |||
| context, | |||
| ), | |||
There was a problem hiding this comment.
For the input loop, consider checking one of them to be able to test for sure the immutability. Here's how I did it for the radio update as one option.
There was a problem hiding this comment.
I think this is looking good. The read-only behavior works as you described, VoiceOver is announcing the read-only checkboxes in a way that is distinct from other editable checkboxes, and field group no longer has those commas! Just a few things from me!
- love all of the new tests you added
- thanks for going so deep into the
aria-labelled/aria-readonly/aria-disabledattributes. I might ping you for resources so I can keep those straight and know when to use them.
| @@ -50,7 +50,7 @@ export const Template = ({ | |||
| typeof size !== "undefined", | |||
| [`${rootClass}--emphasized`]: isEmphasized, | |||
| ["is-indeterminate"]: isIndeterminate, | |||
| ["is-disabled"]: isDisabled|| isReadOnly, | |||
| ["is-disabled"]: isDisabled, | |||
There was a problem hiding this comment.
I like that we were able to remove the isReadOnly arg from this. It immediately makes sense now to any newcomer to the codebase.
| @@ -62,13 +62,19 @@ export const Template = ({ | |||
| type="checkbox" | |||
| class="${rootClass}-input" | |||
| aria-labelledby=${ifDefined(ariaLabelledby)} | |||
| aria-disabled=${ifDefined(isReadOnly ? "true" : undefined)} | |||
There was a problem hiding this comment.
I can look it up on MDN, but I wonder why aria-disabled adds the "dimmed" part of the screen reader announcement, as opposed to just saying something like "read-only." 🤔
| e.preventDefault(); | ||
| e.target.checked = !e.target.checked; | ||
| } | ||
| if (isDisabled || isReadOnly) return; |
There was a problem hiding this comment.
Would we ever get to this "or" portion of the code, the || isReadOnly?
The code would run through the first if(isReadOnly), and if it's also disabled, it'll return anyways, but I guess my question is do we need the "or read only" in this if(isDisabled || isReadOnly) block. Could it run through both if blocks, if a read-only checkbox isn't disabled? Should the return just go in the if(isReadOnly) block?
| @@ -27,6 +27,10 @@ export const CheckboxGroup = Variants({ | |||
| testHeading: "Checked", | |||
| isChecked: true, | |||
| }, | |||
| { | |||
| testHeading: "Indeterminate", | |||
There was a problem hiding this comment.
Great catch on all of the missing indeterminate tests!
Description
As noted in CSS-833, the CSS, SWC, and RSP implementations of read-only checkboxes/checkbox groups, and the docs surrounding them, conflicted with each other. After discussing with the design team, it was decided that the first step would be for CSS to bring our visual styles in line with React's implementation.
Field group:
aria-labeledbyconnected to the group label, so this is announced as a group by the screen reader.Checkbox:
disabledattribute removed when read-only. Read-only still needs to be focusable / in the tab order.aria-disabled="true"when read-only, so screen readers do not announce as a normal interactive checkbox. For VoiceOver, this will add "dimmed" now and won't announce the shortcut to check or uncheck. Note that I've not usedaria-readonlylike as seen in React Spectrum as it is not announced by the screen reader (any differently than a normal checkbox) and MDN states that it is "not relevant" for input with type checkbox.This also updates all of the docs surrounding these things and adds a new storybook Docs example for Checkbox. The field group usage notes have been cleaned up as well.
CSS-1004
How and where has this been tested?
Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.
Validation steps
Regression testing
Expected changes:
Validate:
Screenshots
Before:
After:
To-do list