[react-aria-components]: Checkbox applies data attributes to both the input and the label (should only be on the input)

chambleton-pcty commented 1 month ago

Provide a general summary of the issue here

[react-aria-components]: Checkbox applies data attributes to both the input and the label (should only be on the input).

UseCase: When i apply the following attribute to Checkbox, it's applied to both the label and the input element, which is causing problems with automated tests. The attribute should only be on the input element.

<Checkbox data-autotag-id="test">Example check</Checkbox>

renders to:

<label data-autotag-id="test">
  <span>
    <input type='checkbox' data-autotag-id="test">
  </span>
</label>

In looking at the react-aria-component Checkbox's code, I see this comment: // TODO: should data attrs go on the label or on the ? useCheckbox passes them to the input... https://github.com/adobe/react-spectrum/blob/main/packages/react-aria-components/src/Checkbox.tsx#L206

Could you please resolve this so the data attributes are only applied to the input element? Switch and Radio are working correctly.

Thank you! Chris Hambleton

🤔 Expected Behavior?

The attribute should only be on the input element.

<Checkbox data-autotag-id="test">Example check</Checkbox>

Should render to:

<label>
  <span>
    <input type='checkbox' data-autotag-id="test">
  </span>
</label>

😯 Current Behavior

UseCase: When i apply the following attribute to Checkbox, it's applied to both the label and the input element, which is causing problems with automated tests. The attribute should only be on the input element.

<Checkbox data-autotag-id="test">Example check</Checkbox>

renders to:

<label data-autotag-id="test">
  <span>
    <input type='checkbox' data-autotag-id="test">
  </span>
</label>

💁 Possible Solution

In looking at the react-aria-component Checkbox's code, I see this comment: // TODO: should data attrs go on the label or on the ? useCheckbox passes them to the input... https://github.com/adobe/react-spectrum/blob/main/packages/react-aria-components/src/Checkbox.tsx#L206

In that method, the additional data props should not be applied to the label in the renderProps, only the input element.

🔦 Context

We have data-autotag-id's that we use for automation testing. When these are applied and our tests look for that tagged element, it should only return the input, but is returning both the label and the input element, breaking the tests. Radio and Switch work correctly.

🖥️ Steps to Reproduce

On a Checkbox element, add a data-test-id tag then check the rendered output. You'll see that attribute is rendered on both the label and the input (it should only be on the input).

Version

react-aria-components v1.3.0

What browsers are you seeing the problem on?

Firefox, Chrome, Safari, Microsoft Edge

If other, please specify.

No response

What operating system are you using?

MacBook

🧢 Your Company/Team

Paylocity/Citrus

🕷 Tracking Issue

No response

snowystinger commented 1 month ago

Thanks for the Issue, that data attribute should only go to one place. However, I think it should go to the label as that's how Radio and Switch both work, it's the outer-most dom node https://codesandbox.io/p/sandbox/57y323

I can see we account for it in our tests as well. https://github.com/adobe/react-spectrum/blob/5a0b4fabcaa17508e46db901e25f745fdefa5de4/packages/react-aria-components/test/RadioGroup.test.js#L65 https://github.com/adobe/react-spectrum/blob/5a0b4fabcaa17508e46db901e25f745fdefa5de4/packages/react-aria-components/test/Switch.test.js#L35

Looks like the Checkbox test isn't quite checking enough https://github.com/adobe/react-spectrum/blob/5a0b4fabcaa17508e46db901e25f745fdefa5de4/packages/react-aria-components/test/Checkbox.test.js#L36

We do accept PRs if you'd like to create one for this? Thanks!