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

fix: minimal ARIA string reflection polyfill for JSDOM #207

Merged
merged 2 commits into from
Aug 17, 2023

Conversation

nolanlawson
Copy link
Contributor

It's clear to me from Jest tests running in core that a lot of folks took a dependency on LWC's ARIA polyfill, at least for basic string reflection such as role/ariaLabel/ariaExpanded/etc. Unfortunately it's not supported in JSDOM yet (jsdom/jsdom#3323), which makes it hard to drop the polyfill from LWC OSS (salesforce/lwc#3666).

I propose that we put a minimal ARIA polyfill into @lwc/jest-preset, implementing only the standard supported in Chrome/Safari. Reason being:

  • JSDOM might not merge the fix (Implement ARIAMixin string reflection jsdom/jsdom#3586) anytime soon
  • Even if they do, the latest version of jest-environment-jsdom is using jsdom v20, which is 2 major versions behind (latest is v22)
  • It would be a lot of effort to fix people's tests in core, and the only impact it would even have outside Jest is to fix Firefox in non-LEX environments

This allows us to at least get the global polyfill out of the core LWC OSS repo, without needlessly breaking a bunch of Jest tests.


div[prop] = null;
expect(div.getAttribute(attribute)).toBeNull();
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually testing the polyfill, since the polyfill loads before @lwc/engine-dom.

Copy link
Contributor

@abdulsattar abdulsattar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

test/src/modules/smoke/aria/__tests__/aria.spec.js Outdated Show resolved Hide resolved
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

Successfully merging this pull request may close these issues.

2 participants