CheckboxControl: Support indeterminate state #39462
Conversation
Mamaduka
added
[Type] Enhancement
|
Size Change: +154 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thank you for working on this, @Mamaduka! Also appreciate the addition of a Storybook example!
The implementation is pretty simple. While I included a few safeguards for the impossibles states, the parent component is responsible for correctly setting both indeterminate and checked props.
I tested the behaviour of a native input type="checkbox" with respect to the indeterminate and checked properties:
- if
indeterminateistrue, the checkbox is computed as indeterminate (regardless of the value ofchecked) - if
indeterminateisfalse, then the value ofcheckedis used to determine the checkbox's status
checkbox-indeterminate-checked.mp4
In the light of the above, I wonder if we should just rely more on the native browser behavior:
- avoid "computing" the
checkedprop passed to theinputelement (i.e.! indeterminate && checked), and instead just passcheckeddirectly - use the css
:checkedand:indeterminatepseudo-classes in CSS to show/hide icons
Example
diff --git a/packages/components/src/checkbox-control/index.js b/packages/components/src/checkbox-control/index.js
index 332db9214a..5341b999e2 100644
--- a/packages/components/src/checkbox-control/index.js
+++ b/packages/components/src/checkbox-control/index.js
@@ -57,24 +57,22 @@ export default function CheckboxControl( {
type="checkbox"
value="1"
onChange={ onChangeValue }
- checked={ ! indeterminate && checked }
+ checked={ checked }
aria-describedby={ !! help ? id + '__help' : undefined }
{ ...props }
/>
- { indeterminate && ! checked ? (
- <Icon
- icon={ reset }
- className="components-checkbox-control__indeterminate"
- role="presentation"
- />
- ) : null }
- { checked ? (
- <Icon
- icon={ check }
- className="components-checkbox-control__checked"
- role="presentation"
- />
- ) : null }
+ { /* Shown/hidden via CSS rules */ }
+ <Icon
+ icon={ reset }
+ className="components-checkbox-control__indeterminate"
+ role="presentation"
+ />
+ { /* Shown/hidden via CSS rules */ }
+ <Icon
+ icon={ check }
+ className="components-checkbox-control__checked"
+ role="presentation"
+ />
</span>
<label
className="components-checkbox-control__label"
diff --git a/packages/components/src/checkbox-control/style.scss b/packages/components/src/checkbox-control/style.scss
index a9fac4381f..fbcc49e16a 100644
--- a/packages/components/src/checkbox-control/style.scss
+++ b/packages/components/src/checkbox-control/style.scss
@@ -65,6 +67,7 @@ $checkbox-input-size-sm: 24px; // Width & height for small viewports.
svg.components-checkbox-control__checked,
svg.components-checkbox-control__indeterminate {
+ display: none;
fill: $white;
cursor: pointer;
position: absolute;
@@ -80,3 +83,13 @@ svg.components-checkbox-control__indeterminate {
user-select: none;
pointer-events: none;
}
+
+.components-checkbox-control__input[type='checkbox']:checked
+ ~ svg.components-checkbox-control__checked {
+ display: block;
+}
+
+.components-checkbox-control__input[type='checkbox']:indeterminate
+ ~ svg.components-checkbox-control__indeterminate {
+ display: block;
+}Finally, it'd be great if you also could:
- (not blocking, can be done in a follow-up) add some unit tests (just noticing this component doesn't have any!), especially around the
indeterminatestatus - add a CHANGELOG entry
|
Thanks for the feedback, @ciampo. I will push updates shortly.
Any reason for preferring this method instead of conditionally rendering elements? |
It's mainly to make sure that the behaviour is consistent with the browser's logic, rather than reimplementing the logic ourselves in JS. This was my suggestion, of course I'm open to hear what everyone thinks too! |
|
I've no strong opinions on this one 😄 However, I remember that we try to avoid rendering extra DOM elements when possible. cc @jasmussen |
|
No strong opinion here either! Though I do appreciate us being able to use the icon from the icon library as-is, outside of any CSS background shenanigans we might need to get into otherwise. |
|
What about using the Something like { ref.matches(':checked') && ( <CheckedIcon /> ) }
{ ref.matches(':indeterminate') && ( <IndeterminateIcon /> ) } |
|
I just tried that and got some unexpected results 😄 CleanShot.2022-03-16.at.18.54.28.mp4 |
|
Hey @Mamaduka , I just pushed an update to the PR (hope you don't mind!) that should make the suggested approach work — basically the main issue, previously, was that the component wouldn't re-render when the Using |
|
Thank you, @ciampo. I've going to rebase and resolve the merge conflict. |
Mamaduka
force-pushed
the
try/checkbox-control-indeterminate
branch
from
365ea61 to
27b7940
Compare
|
@Mamaduka Should I be able to test this by checking the Options > Lock from a block or does this have to be done via Storybook? If via Storybook, how can I check locally? Never used Storybook with Gutenberg before. |
|
Hi, @alexstine Currently, it's only possible to test via Storybook. After this is merged, I'm going to update components that use an "indeterminate" state. You can run the following command, and it will open Storybook locally: |
|
@Mamaduka The screen reader is not announcing the state difference, very odd. I am going to ask for @MarcoZehe opinion on this because I am not sure how to make this work for screen readers. The aria-checked attribute did not help us and now this new state isn't either. I hope I haven't asked you to make this for no good reason if we've got a lack of support in browsers or screen readers. I know I've heard my screen reader announce a checkbox as mixed before, but can't remember where or how it was implemented. Let's see if we can get some guidance... |
|
The aria-checked property takes one of three possible values:
|
|
Thank you
Thank you both, @alexstine and @MarcoZehe ! @Mamaduka , would you mind adding |
|
@ciampo Sure. The current implementation in Block Lock modal uses gutenberg/packages/block-editor/src/components/block-lock/modal.js Lines 58 to 65 in d7dc3cd I thought inputs with type "checkbox" and "radio" don't require aria attributes because of built-in semantics. Happy to make necessary changes, but it would be great to add inline comments on why those are needed to avoid regressions. |
|
Thank you!
That's interesting — I personally would assume the same when using standard HTML element. I say, let's apply those changes and test again.
Comments are definitely needed here to explain better what's going on in the code! Although the better way to avoid regressions would be to add unit tests, and test specifically for roles, properties (e.g. |
|
Perhaps this StackOverflow question and answer explain some of this bettzer. In essence, there is no way from HTML to set the indeterminate checkbox state, only from JavaScript. I tested the JSFiddle that is linked to from the answer, and Chrome does indeed expose the indeterminate checkbox, and when I toggle it, NVDA also gets notified of state to unchecked to checked. But the indeterminate state can only be set via the JS property, not via markup alone. |
Our current implementation in this PR should already take care of that, as it sets Hopefully this approach, together with |
Mamaduka
force-pushed
the
try/checkbox-control-indeterminate
branch
from
27b7940 to
dec8b06
Compare
|
If indeterminate is set this way already, then aria-checked should indeed not be needed. I don't fully understand the issue at hand. Are there exact steps with something that contains this PR to see the problem @alexstine is having? |
|
Thanks for the confirmation, @MarcoZehe. @alexstine, can you give us more details on how we can reproduce the issue? |
|
I can definitely say that the JSFiddle that is linked from the Stackoverflow entry works fine in Chrome, but not Firefox. Firefox doesn't expose the indeterminate state to NVDA properly in the JSFiddle. That seems to be a browser bug. |
Makes sense why it isn't working then. I only tested in Storybook with Firefox. I will try Chrome this afternoon. If all checks out, I'll review the code and hopefully we can get this in. Sorry about the confusion all. |
|
Thanks, everyone for testing 🙇 @ciampo, I can do a test coverage follow-up next week + update two components to use new props. |
Sounds good to me — let's get this merged once tests pass. I just wanted to thank everyone involved in this PR, it's been an excellent example of open source collaboration. |
* CheckboxControl: Support indeterminate state * Add story * Feedback * Update changelog * Use `useRefEffect` and `element.matches` to show/hide icons * Fix changelog * Set aria-checked * Revert aria-checked for testing Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
What?
Closes #39434.
PR adds indeterminate state support to the CheckboxControl.
Why?
Previously, we could achieve this by manually setting
aria-checked="mixed"on the component. However, Spec provides a "native" way to handle this state, so I think this should be the preferred method.How?
The implementation is pretty simple. While I included a few safeguards for the impossibles states, the parent component is responsible for correctly setting both
indeterminateandcheckedprops.Testing Instructions
Tested locally using Storybook
Screenshots or screencast
CleanShot.2022-03-15.at.17.32.32.mp4