Missing role attributes in Core 5.6.1

astorije commented 2 years ago

Describe the bug

ARIA roles and attributes are missing in Clarity Core v5.6.1, failing our React Testing Library tests that use await screen.findByRole('button').

How to reproduce

Steps to reproduce the behavior:

I could not make this work with Stackblitz or Code Sandbox, so had to go the repro repo route: https://github.com/astorije/clarity-missing-role
Clone, install dependencies (yarn install)
Run yarn test on the main branch and see the error in the console output
Switch to the core-5.6.0 branch, install dependencies again (yarn install), and run yarn test to see the same tests pass

Here is the error output on v5.6.1:

 FAIL  src/App.test.tsx
  App
    ✕ foo? (1042 ms)

  ● App › foo?

    Unable to find role="button"

    Ignored nodes: comments, <script />, <style />
    <body
      cds-supports="js no-flex-gap"
      cds-version="5.6.1"
    >
      <div>
        <div
          cds-layout="m:md"
        >
          <cds-button
            _nfg=""
            action="solid"
            size="md"
            status="primary"
            tabindex="0"
          >
            Foo
          </cds-button>
        </div>
      </div>
    </body>

      10 |
      11 |     expect(
    > 12 |       await screen.findByRole("button", { name: "Foo" })
         |                    ^
      13 |     ).toBeInTheDocument();
      14 |   });
      15 | });

As you can tell, the cds-button element is missing the role attribute (and the aria-disabled one). I suspect the issue originated at https://github.com/vmware/clarity/compare/v5.6.0...v5.6.1#diff-40c8e7c2cb3e2e864efc9529dbba5def804aa9131b0ab70c82fb87cfc86aba00 (warning: GitHub's renderer currently removes the # between v5.6.1 and diff in this URL 🤷‍♂️)

Expected behavior

The core-5.6.0 contains the expected behavior. When running tests, we get:

 PASS  src/App.test.tsx
  App
    ✓ foo? (144 ms)

  console.log
    <body
      cds-supports="js no-flex-gap"
      cds-version="5.6.0"
    >
      <div>
        <div
          cds-layout="m:md"
        >
          <cds-button
            _nfg=""
            action="solid"
            aria-disabled="false"
            role="button"
            size="md"
            status="primary"
            tabindex="0"
          >
            Foo
          </cds-button>
        </div>
      </div>
    </body>

    /.../src/App.test.tsx:15:12
      13 |     ).toBeInTheDocument();
      14 |
    > 15 |     screen.debug();
         |            ^

Versions

Clarity project:

[x] Clarity Core
[ ] Clarity Angular/UI

Clarity version:

[ ] v3.x
[ ] v4.x
[x] v5.6.1

Framework:

[ ] Angular
[x] React
[ ] Vue
[ ] Other:

Additional notes

To be noted that the role does appear fine in the browser but not in the test DOM, not sure what kind of magic is happening there:

Screen Shot 2021-12-20 at 12 53 09 PM

coryrylan commented 2 years ago

We use AOM aria properties within the components. Are you using Jest or JSDom? It could be they don't support AOM

astorije commented 2 years ago

We're using Jest and JSDom by way of React Testing Library. Was AOM being used before v5.6.1? Everything worked fine in v5.6.0 and tests started failing as of v5.6.1 only.

URL above has an GitHub rendering issue, but I suspected this change caused this:

coryrylan commented 2 years ago

We have been working on moving away from overriding properties on the native HTMLElement prototype since it can cause unexpected behaviors in some browsers as well as performance issues. Some web component tools actively try to prevent this https://github.com/ionic-team/stencil/blob/main/src/compiler/transformers/reserved-public-members.ts

astorije commented 2 years ago

Good to know, thank you! Any recommendations you could provide us on how to move forward here? We're kinda stuck between a rock and a hard place on that one as well. If all else fails, we can mock the components as a last resort, though we try to avoid this as much as we can.

ashleyryan commented 2 years ago

I filed an issue again jsdom to support the AOM properties: https://github.com/jsdom/jsdom/issues/3323

We have a polyfill that will fall back to setAttribute for browsers that don't support the property (currently just Firefox), but it doesn't run in node because there were issues with jsdom.

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.

vmware-archive / clarity