Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

toBeDisabled should match against web components / custom elements #367

Closed
astorije opened this issue May 21, 2021 · 5 comments
Closed

toBeDisabled should match against web components / custom elements #367

astorije opened this issue May 21, 2021 · 5 comments

Comments

@astorije
Copy link
Contributor

  • @testing-library/jest-dom version: 5.12.0
  • @testing-library/react version: 11.2.7
  • node version: 14.16.1
  • yarn version: 1.22.5

Relevant code or config:

I used @cds/core (Clarity) which is a library of web components + @cds/react which is a bunch of React wrappers for said web components. Pretty much everything else is default config.

What you did:

This is the failing test:

  render(<CdsButton disabled>Foo</CdsButton>);
  expect(await screen.findByText("Foo")).toBeDisabled();

CdsButton under the hood renders a cds-button web component. It uses an async test because Clarity fills a bunch of props asynchronously, including disabled, aria-disabled, role, etc.

What happened:

Screen Shot 2021-05-21 at 4 14 38 PM

Text version
FAIL  ./index.test.tsx
  ✕ should make sure the button is disabled (43 ms)

  ● should make sure the button is disabled

    expect(element).toBeDisabled()

    Received element is not disabled:
      <cds-button _nfg="" action="solid" aria-disabled="true" disabled="" loading-state="default" role="button" size="md" status="primary" tabindex="-1" />

      5 | test("should make sure the button is disabled", async () => {
      6 |   render(<CdsButton disabled>Foo</CdsButton>);
    > 7 |   expect(await screen.findByText("Foo")).toBeDisabled();
        |                                          ^
      8 | });
      9 |

      at index.test.tsx:7:42
      at step (index.test.tsx:33:23)
      at Object.next (index.test.tsx:14:53)
      at fulfilled (index.test.tsx:5:58)

Reproduction:

A minimal reproduction repo can be found at https://github.com/astorije/repro-web-component-tobedisabled.

Steps to reproduce:

  • Clone the repo
  • Run yarn install
  • Run yarn test
  • The error above will be thrown

Problem description:

I scratched my head for a bit as I initially thought Clarity was doing something unwanted, but as soon as I opened the toBeDisabled source, I realized that it filters again a hardcoded list).
Since no web components would ever be found in this list, toBeDisabled would not support them.

Suggested solution:

The doc says:

According to the specification, the following elements can be actually disabled: button, input, select, textarea, optgroup, option, fieldset.

However, that WHATWG spec lists an extra bullet point that is missing from that list:

a form-associated custom element that is disabled

So the hardcoded list needs to support web components aka custom elements.

According to MDN:

A DOMString representing the name you are giving to the element. Note that custom element names require a dash to be used in them (kebab-case); they can't be single words.

So my suggested solution is to add elements whose tags contain a hyphen to the list of elements that can actually be disabled.

@nickserv
Copy link
Member

Are these still form elements though? As far as I remember, the disabled role is specifically for form elements, or at least our usage of it.

@astorije
Copy link
Contributor Author

They can be! In the example described above, CdsButton is essentially indistinguishable from a styled button. I think the .toBeDisabled() assertion should honor the HTML spec in that regard.

We resorted to use .toHaveAttribute('disabled') and .not.toHaveAttribute('disabled') for now, but it's rather worrisome because if any of our developers misses this detail and accidentally uses .not.toBeDisabled() (which we initially enforced with https://github.com/testing-library/eslint-plugin-jest-dom/blob/master/docs/rules/prefer-enabled-disabled.md but we had to disable this rule from our ESLint config) instead of .not.toHaveAttribute('disabled'), that could give a false negative (aka .not.toBeDisabled() would not fail with, e.g., <CdsButton disabled />).

@tolgahanacar
Copy link

i had the same problem. I will create a new issue with details.

@tolgahanacar
Copy link

I solved the problem.
./app.js/
button data-testid="r_button" type="submit" disabled>r_sub /button
./app.test.js/
const rbutton = screen.getByTestId("r_button"); //get id
expect(rbutton).toBeDisabled();

@astorije
Copy link
Contributor Author

astorije commented Feb 3, 2022

This has been fixed by #368, thanks all!

@astorije astorije closed this as completed Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants