vmware-clarity / core

Clarity is a scalable, accessible, customizable, open-source design system built with web components. Works with any JavaScript framework, created for enterprises, and designed to be inclusive.
https://clarity.design
MIT License
163 stars 42 forks source link

Custom elements supporting `disabled` need to be `formAssociated` #153

Closed dturcotte closed 2 years ago

dturcotte commented 2 years ago

Describe the bug

According to this spec on disabled: https://html.spec.whatwg.org/multipage/semantics-other.html#disabled-elements

An element is said to be actually disabled if it is one of the following:

a form-associated custom element that is disabled

Clarity core custom elements don't appear to set formAssociated. We discovered this when testing-library/user-event changed their isDisabled check for user interactions to enforce this: https://github.com/testing-library/user-event/blame/main/src/utils/misc/isDisabled.ts#L26-L33

If this is part of the spec for disabled custom elements then this may affect more than just testing-library/user-event. However this is the first time we've seen any lib enforce it. It seems to be fine in an actual browser.

How to reproduce

I'll update this with a repro, but in the meantime:

Steps to reproduce the behavior:

  1. Render a disabled CdsButton such as:
    <CdsButton disabled onClick={() => alert('should not happen')}>
    Disabled button
    </CdsButton>
  2. Using testing-library/user-event@14.0.0-beta.13 or later,
    user.click(screen.getByRole('button', { name: 'Disabled button' }))
  3. Click should not happen. Instead, it does.

Expected behavior

user.click on a disabled cds-button (and other relevant form-like elements) shouldn't trigger onClick

Versions

Clarity project:

Clarity version:

Framework:

Framework version: React 18.2.0

Device:

Additional notes

For those with access to CloudHealth's github, example is here: https://github.com/CloudHealth/cht-ui-component-lib/blob/cf527be82f915accf3e3306b290bccc48c25f64f/test/src/ButtonMenu.test.tsx#L122

ashleyryan commented 2 years ago

I brought this up to the testing-library team. I don't want to just add formAssociated to our button element because it's not entirely correct without attachInternals which doesn't have full browser support yet (and would require reworking our component a lot if we were to use a polyfill, which I'm not equipped to do)

ashleyryan commented 2 years ago

A workaround would be to put the following in your jest setup file, right alongside all of the mocks:

(CdsButton as typeof CdsButton & {formAssociated?: boolean}).formAssociated = true;

That made the tests pass in my reproduction repository.

ashleyryan commented 2 years ago

I'm closing this because it has a workaround and I'm not comfortable adding this property just for react unit tests. If this becomes an issue for more form components, let me know.

github-actions[bot] commented 2 years ago

Hi there 👋, this is an automated message. To help Clarity keep track of discussions, we automatically lock closed issues after 14 days. Please look for another open issue or open a new issue with updated details and reference this one as necessary.