Introduce useCheckboxGroup & useCheckboxGroupState hooks

[Andarist]

Partially addresses #798

The goal of this PR is to only implement @react-aria and @react-stately parts of this feature.

Even though that at first glance this should be very similar to existing useRadioGroup - it is not (not in some details). useRadio is always considered to be a part of a group so it accepts RadioGroupState. However useCheckbox already accepts ToggleState and it didn't make much sense to me to implement support for CheckboxGroupState there - so I've decided to implement useGroupedCheckbox which adapts CheckboxGroupState to the ToggleState for a given checkbox and just pass it down to useCheckbox. This IMHO makes for a cleaner API and leverages composition nicely.

I've researched the topic of checkbox groups a little bit. Unfortunately, there are no aria practices written down for it and there is no dedicated role for it (like in the case of radiogroup), even though checkbox group is not an esoteric use case. I've found out two resources:

• https://blog.tenon.io/accessible-validation-of-checkbox-and-radiobutton-groups
• https://www.w3.org/TR/2016/WD-wai-aria-practices-1.1-20160317/examples/checkbox/checkbox-2.html

In the first article, authors compare various possible implementations, but in the end, I don't feel like they have reached the perfect solution - the whole thing is also very SR-dependent. The second one (coming from older version of aria practices? I don't know) simplifies the whole thing (it doesn't try to hack around SR differences) and just mentions that some things are up to a particular SR, such as reading the label for a whole checkbox group. So I've gone with the latter, simpler, but also saner in my eyes, solution.

TODO:

• more tests, I don't know how granular your want to be though - most of the tests are implemented now at @react-spectrum level, is it OK for you to implement more unit tests for the isolated useCheckboxGroup?
• decide about the name of useGroupedCheckbox, I'm not sure if it's the best one - but I couldn't think of any better
• decide where useCheckboxGroupState should live, right now it is included in @react-stately/toggle but this doesn't sound right, should maybe @react-stately/checkbox be created?

✅ Pull Request Checklist:

• Included link to corresponding React Spectrum GitHub Issue.
• Added/updated unit tests and storybook for this change (for new code or code which already has tests).
• Filled out test instructions.
• Updated documentation (if it already exists for this component).
• Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

yarn test

[devongovett]

Unfortunately, I don't believe the above properties are valid on a group aside from aria-disabled. See https://www.w3.org/TR/wai-aria-1.2/#group.

[devongovett]

@jnurthen do you know if there are any checkbox group patterns that would support this, similar to a radio group? Why does ARIA have one but not the other?

[Andarist]

Good catch, I forgot to recheck those. What should happen then? Should we maybe not provide those aria-* props here but pass those group props to the individual checkboxes? So one could disable the whole group instead of all checkboxes individually?

[devongovett]

Yeah I think that's probably the best option.

[Andarist]

Just want to reassure myself that we are on the same page - you are saying that the group should accept those props like isReadOnly, isRequired, isDisabled and "forward" them to individual checkbox, right?

I believe in such a case isRequired would have to be handled somehow differently? isRequired seems to mean that any of the checkboxes in a group has to be checked, not that all of them are checked. OTOH there are also other scenarios - like a list of agreements in which all of the checkboxes have to be checked, or a list of checkboxes with "select at least X". This would mean that this, in fact, up to the consumer to decide.

But as aria-invalid is not a valid attribute for a role="group" then there is no place to accept validationState right now as there is nothing that we could do automatically with it 🤔 Taking a look at useRadioGroup - validationState is part of the AriaRadioGroupProps, not RadioGroupState. So providing its value is always a responsibility of a consumer and this should be handled the same here.

Looking further at useRadioGroup - it accepts the very same props (isReadOnly, isRequired, isDisabled) but it just sets aria attributes based on them, it doesn't forward them anyhow to useRadio - even though it accepts them as well. So in the case of the radio group - the forwarding of those props has to be done by the consumer manually. This wiring happens within spectrum's RadioGroup & Radio components through context.

[devongovett]

So, thinking about this more, I think we should remove the isRequired and validationState props from CheckboxGroup. isRequired is just passed through as the required DOM prop on each checkbox, which makes them all required as you said, not if some are required. AFAIK there's no way of doing that in HTML. We don't implement any validation logic ourselves, so this would be up to the app. Similar reasoning for validationState. Marking every checkbox as invalid doesn't really make sense either.

As for isReadOnly and isDisabled, I think propagating them to each of the children makes sense, along with aria-disabled on the group itself. RadioGroup does this through context, but one could do so in other ways too (e.g. cloneElement or something). I think we should leave this up to consumers of React Aria and not be too prescriptive about it.

[Andarist]

I've removed most of those props from this interface - the only one that I have left is isDisabled as aria-disabled is allowed on a role="group".

isRequired was also left (but stays unused) because I extend LabelableProps and this interface supports isRequired:
https://github.com/adobe/react-spectrum/blob/8f7fbb33e34d228b3e2cb370634e4c2cc7ae9b0f/packages/%40react-types/shared/src/labelable.d.ts
although I'm not sure if this is not a mistake (I don't rly see it being used anywhere by LabelableProps consumers) and if it maybe shouldn't be moved to SpectrumLabelableProps. I've created a PR for such change if you agree with it: #877

[devongovett]

Thanks for working on this! Does useGroupedCheckbox mean that we'd need to have two components as well, e.g. add a <GroupedCheckbox> component instead of using <Checkbox>? If possible, I think we'd like to avoid this.

What if useCheckboxGroupState had a method like getCheckboxState(value: string): ToggleState? It would return state matching the same interface as useToggleState (isSelected and setSelected). Then Checkbox could consume a context provided by the group with the group state, and if available, call that method and use as the state for useCheckbox instead of its own. Unfortunately, hooks can't be conditional, so we'd still have to call useToggleState and not use the result but this doesn't seem too bad?

I think it would be harder for Checkbox to call either useCheckbox or useGroupedCheckbox depending on some context value because there'd be no group state to pass to useGroupedCheckbox when used by itself.

more tests, I don't know how granular your want to be though - most of the tests are implemented now at @react-spectrum level, is it OK for you to implement more unit tests for the isolated useCheckboxGroup?

Yeah we've done that so far, but I think testing with a dummy component like you started makes sense if the Spectrum part isn't implemented yet.

decide where useCheckboxGroupState should live

I think a new @react-stately/checkbox package makes sense.

[Andarist]

What if useCheckboxGroupState had a method like getCheckboxState(value: string): ToggleState?

Sure - this would be possible. Is there any precedence for such an API within the codebase right now?

Unfortunately, hooks can't be conditional,

That's not entirely true the only requirement is to have hooks called in a consistent order. A checkbox can't change being in a group between renders - that's just not possible to React. So, while this, of course, looks weird, suspicious and asks for punishment from React gods, it's technically perfectly fine:

const { inputProps } = checkboxGroupContext
  ? useGroupedCheckbox(props)
  : useCheckbox(props)

so we'd still have to call useToggleState and not use the result but this doesn't seem too bad?

I don't want to be purist - I certainly have done worse than this, but something like this always makes me consider alternatives first as I feel like this is a sign of a broken composition chain. Not the end of the world, but ideally this wouldn't happen if puzzles would fit perfectly.

Yeah we've done that so far, but I think testing with a dummy component like you started makes sense if the Spectrum part isn't implemented yet.

K, gonna add more like this 👍

I think a new @react-stately/checkbox package makes sense.

Gonna add this soon 👍

[devongovett]

Is there any precedence for such an API within the codebase right now?

Not really.

That's not entirely true the only requirement is to have hooks called in a consistent order

😲 that makes sense. Let me talk with the team and get back to you.

[Andarist]

😲 that makes sense. Let me talk with the team and get back to you.

Sure thing, I understand that this is not the usual solution 😅 I also don't want to insist on this - just thinking about the whole API of this project and don't want to introduce new patterns too hastily (like the proposed getCheckboxState)

[devongovett]

So basically it's between these two options:

1. Swap out the ARIA hook:

let {inputProps} = groupState
  ? useGroupedCheckbox(props, groupState)
  : useCheckbox(props, useToggleState(props));

2. Swap out the state:

let state = groupState
  ? groupState.getCheckboxState(props.value)
  : useToggleState(props);
let {inputProps} = useCheckbox(props, state);

Really not too different in the end. I think if there were behavioral or ARIA differences then (1) would make sense. But since we're really just swapping out the state, then maybe (2) makes more sense in this case.

[Andarist]

I've implemented more test and refactor useGroupedCheckbox to the proposed getCheckboxState. It "has" to be used like this:

react-spectrum/packages/@react-aria/checkbox/test/useCheckboxGroup.test.tsx

Line 23 in 500d35a

const {inputProps} = useCheckbox({...props, name: checkboxGroupState.name}, checkboxGroupState.getCheckboxState(props), ref);

There are 2 usability issues with this (IMHO:

• the caller needs to merge state.name with props manually - when useRadio handles this automatically
• the caller needs to pass props to both useCheckbox and state.getCheckboxState which looks odd. You have been proposing to only pass value to this getter but to match RadioGroup behavior it also needs onChange & isReadOnly, so passing the whole props made the most sense for me here (even though I still feel it's rather weird that one has to do the wiring like this). Of course, useGroupedCheckbox can be easily implemented using this API but I don't feel that providing 2 APIs for the kinda same thing is a good thing either.

I've also experimented with an alternative implementation for useCheckboxGroupState's internals, but I'm not sure how you would like this, so I haven't included it and I'm just proposing it here:

alternative useCheckboxGroupState implementation based on reducerLike

index c5d40246..5da6c702 100644
--- a/packages/@react-stately/toggle/src/useCheckboxGroupState.ts
+++ b/packages/@react-stately/toggle/src/useCheckboxGroupState.ts
@@ -49,42 +49,49 @@ export function useCheckboxGroupState(props: CheckboxGroupProps = {}): CheckboxG
   let name = useMemo(() => props.name || `checkbox-group-${instance}-${++i}`, [props.name]);
   let [selectedValue, setValue] = useControlledState(props.value, props.defaultValue || [], props.onChange);
 
+  const reducerLike = (action: { type: 'add' | 'remove' | 'toggle', value: string }) => {
+    if (props.isReadOnly) {
+      return;
+    }
+    setValue(values => {
+      let {type, value} = action;
+      switch (type) {
+        case 'add':
+          if (values.includes(value)) {
+            return values;
+          }
+          break;
+        case 'remove':
+          if (!values.includes(value)) {
+            return values;
+          }
+          break;
+        case 'toggle':
+          type = values.includes(value) ? 'remove' : 'add';
+          break;
+      }
+
+      switch (type) {
+        case 'add':
+          return values.concat(value);
+        case 'remove':
+          return values.filter(existingValue => existingValue !== value);
+      }
+    });
+  }
+
   const state: CheckboxGroupState = {
     name,
     value: selectedValue,
     setValue,
     addValue(value) {
-      if (props.isReadOnly) {
-        return;
-      }
-      setValue(values => {
-        if (!values.includes(value)) {
-          return values.concat(value);
-        }
-        return values;
-      });
+      reducerLike({ type: 'add', value });
     },
     removeValue(value) {
-      if (props.isReadOnly) {
-        return;
-      }
-      setValue(values => {
-        if (values.includes(value)) {
-          return values.filter(existingValue => existingValue !== value);
-        }
-        return values;
-      });
+      reducerLike({ type: 'remove', value });
     },
     toggleValue(value) {
-      if (props.isReadOnly) {
-        return;
-      }
-      setValue(values => {
-        if (values.includes(value)) {
-          return values.filter(existingValue => existingValue !== value);
-        }
-        return values.concat(value);
-      });
+      reducerLike({ type: 'toggle', value });
     },
     getCheckboxState(props) {
       const {value, onChange, isReadOnly} = props;
@@ -95,11 +102,7 @@ export function useCheckboxGroupState(props: CheckboxGroupProps = {}): CheckboxG
             return;
           }
 
-          if (isSelected) {
-            state.addValue(value);
-          } else {
-            state.removeValue(value);
-          }
+          reducerLike({ type: isSelected ? 'add' : 'remove', value });
 
           if (onChange) {
             onChange(isSelected);

Let me know what do you think about this one and if there is anything else I should do to get this merged in.

[devongovett]

Looks great! One question about docs and I think we can merge this.

[devongovett]

Was this supposed to be docs for useCheckboxGroupState? I believe docs for useToggleState is already in @react-stately/toggle.

[Andarist]

Yeah, when creating @react-stately/checkbox directory I've just copied the other one as a template and haven't noticed that docs directory got added. I've tweaked the content so it points to the correct package right now.

[devongovett]

In regards to your questions:

1. I don't think needing to pass name manually is too big of a problem, but I see the concern. Anyway, it's likely that we're going to need to pass other props in from the group context, e.g. visual styling that can be set on the group and apply to each of the checkboxes, so checkbox is going to need to be somewhat aware of whether its in a group.
2. I agree that passing the whole props object to getCheckboxState is a little strange. It does kinda feel like the separate stately hook pattern of providing props and getting back state. We don't really have any hooks that derive one state from another right now though (e.g. useCheckboxState(props, groupState)).

Given these concerns, do you still feel that useGroupedCheckbox is cleaner? I suppose it would be able to wrap up all of this into a single hook that might be slightly easier to use. I wouldn't be opposed if you wanted to go that way. Maybe we could name it useCheckboxGroupItem instead though - a little long but would match useActionGroupItem, useMenuItem, useBreadcrumbItem, etc.

Another option would be for useCheckboxGroupItem to return the props for the checkbox rather than calling useCheckbox internally. Then you could compose them like this:

function Checkbox(props) {
  let groupState = useContext(CheckboxGroupContext);
  if (groupState) {
    let {checkboxProps} = useCheckboxGroupItem(props, groupState);
    props = checkboxProps;
  }

  let state = useToggleState(props);
  let {inputProps} = useCheckbox(props, state);

  // ...
}

Or via a wrapper component to Checkbox. WDYT?

[Andarist]

Given these concerns, do you still feel that useGroupedCheckbox is cleaner?

I kinda do.

Another option would be for useCheckboxGroupItem to return the props for the checkbox rather than calling useCheckbox internally.

This would be OK for me (still prefer useGroupedCheckbox's API better though). I've looked into mentioned useActionGroupItem, useMenuItem & useBreadcrumbItem and they all return props that are meant to be, more or less, directly applied to elements. I'm worried that the proposed pattern not only deviates from the existing APIs but it's also rather hard to document and teach.

To sum it up - I would personally revert it to the previous variant, just with a renamed name: useCheckboxGroupItem. I don't have that much of a problem with @devongovett's latest proposal, so I could rewrite this to that. Definitely the currently implemented getCheckboxState is my least favorite one.

I don't think there is much sense in debating this any further unless somebody else would like to pitch in (any other opinions in the Spectrum team?). Pick your poison and I will adjust the code accordingly 😉

[devongovett]

To sum it up - I would personally revert it to the previous variant, just with a renamed name: useCheckboxGroupItem

I'm fine with that.

[Andarist]

@devongovett I've changed the implementation to the discussed useCheckboxGroupItem and CI is ✅ , please take a look if there is anything leftover to do right now :)

[devongovett]

Thanks!