react-spectrum icon indicating copy to clipboard operation
react-spectrum copied to clipboard

CheckboxGroup with more than one Text component that has unique id-props set causes an infinite React re-render loop

Open oscarcarlstrom opened this issue 1 year ago โ€ข 2 comments

Provide a general summary of the issue here

It seems like if you put more than one <Text>-component that has an id prop set inside a <CheckboxGroup>, React gets stuck in an infinite re-render loop.

This simple example causes infinite re-renders:

    <CheckboxGroup>
      <Label>Your options</Label>
      <Checkbox>Option 1</Checkbox>
      <Text id="first" slot="description">
        Choose this option to choose the first one
      </Text>
      <Checkbox>Option 2</Checkbox>
      <Text id="second" slot="description">
        Choose this option to choose the second one
      </Text>
    </CheckboxGroup>

But if you remove the id-prop it works fine:

    <CheckboxGroup>
      <Label>Your options</Label>
      <Checkbox>Option 1</Checkbox>
      <Text slot="description">
        Choose this option to choose the first one
      </Text>
      <Checkbox>Option 2</Checkbox>
      <Text slot="description">
        Choose this option to choose the second one
      </Text>
    </CheckboxGroup>

Using only one Text component also works fine:

    <CheckboxGroup>
      <Label>Your options</Label>
      <Checkbox>Option 1</Checkbox>
      <Text id="first" slot="description">
        Choose this option to choose the first one
      </Text>
      <Checkbox>Option 2</Checkbox>
    </CheckboxGroup>

๐Ÿค” Expected Behavior?

React should not re-render, since no prop seemingly changes (the id prop is constant, and never changes)

๐Ÿ˜ฏ Current Behavior

React re-renders until it reaches the max update depth.

๐Ÿ’ Possible Solution

No response

๐Ÿ”ฆ Context

Would like to use the Text-component inside the CheckboxGroup to provide descriptions related to the options. These could also contain links to something like terms and conditions, or similiar (which can not, and should not be part of the Checkbox itself): issue 1166. Setting the id props can help screen readers tie it to the correct Checkbox using aria-describedby .

๐Ÿ–ฅ๏ธ Steps to Reproduce

Simple example with no styles (open the console to see multiple Warning: Maximum update depth exceeded.) https://codesandbox.io/p/sandbox/divine-bush-hmfqgv

Version

1.1.1

What browsers are you seeing the problem on?

Firefox, Chrome

If other, please specify.

No response

What operating system are you using?

macOS

๐Ÿงข Your Company/Team

No response

๐Ÿ•ท Tracking Issue

No response

oscarcarlstrom avatar Apr 18 '24 17:04 oscarcarlstrom

So at the moment, the CheckboxGroup only supports a single Text element with slot="description" since said Text element serves as the descriptor for the CheckboxGroup as well. I imagine there is some logic in useCheckboxGroup that is getting confused by the existence of two distinct ids that it is supposed to link the CheckboxGroup to thus causing it to switch from one to the other over and over again.

For now you could maybe use non-slotted text elements and pass their ids as aria-describedbys to the Checkboxes, not something we officially support though. We'll be looking into adding support for HelpText (aka descriptions) to individual Checkboxes via https://github.com/adobe/react-spectrum/issues/6192, perhaps making this easier to setup

LFDanLu avatar Apr 19 '24 00:04 LFDanLu

Thanks @LFDanLu, yeah I noticed that the <Text/> component requires the slot prop, could bypass TypeScript of course. But I ended up just wrapping the content of the <Text/> component in a div, and put the id on that element instead. Works fine, but would be nice to get rid of that extra element. Looking forward to the <HelpText/> component!

oscarcarlstrom avatar Apr 19 '24 06:04 oscarcarlstrom