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

Don't break on window.CSS === null #4400

Closed
WilcoFiers opened this issue Apr 4, 2024 · 1 comment · Fixed by #4456 · May be fixed by Stanislav1975/sitespeed.io#11, Stanislav1975/sitespeed.io#12 or organich/lighthouse#4
Closed

Don't break on window.CSS === null #4400

WilcoFiers opened this issue Apr 4, 2024 · 1 comment · Fixed by #4456 · May be fixed by Stanislav1975/sitespeed.io#11, Stanislav1975/sitespeed.io#12 or organich/lighthouse#4
Assignees
Labels
dependencies Pull requests that update a dependency file fix Bug fixes support
Milestone

Comments

@WilcoFiers
Copy link
Contributor

WilcoFiers commented Apr 4, 2024

Angular + Jest documentation recommends a solution which ends up setting window.CSS to null. This creates an issue when axe-core imports colorjs, which has a falsey check for window.CSS, but when null errors out.

Since ColorJS didn't accept our patch, we'll try to use patch-package to fix the issue ourselves. We'll need some tests to go along with that to confirm our solution continues to work after updating ColorJS.

patch-package should probably run in the build step, not as a post-install.

@WilcoFiers WilcoFiers added fix Bug fixes dependencies Pull requests that update a dependency file labels Apr 4, 2024
@WilcoFiers WilcoFiers added this to the Axe-core 4.10 milestone Apr 4, 2024
@gaiety-deque
Copy link
Contributor

gaiety-deque commented May 7, 2024

Just for full context, window.CSS is no longer recommended in the Jest for Angular docs (thanks Michael) thymikee/jest-preset-angular@ac30648 which is live on their docs site https://thymikee.github.io/jest-preset-angular/docs/getting-started/installation/#global-mocks

gaiety-deque added a commit that referenced this issue May 9, 2024
patching colorio.js with optional chaining for `CSS?.supports`

Refs: #4400
gaiety-deque added a commit that referenced this issue May 14, 2024
patching colorio.js with optional chaining for `CSS?.supports`

Refs: #4400
gaiety-deque added a commit that referenced this issue Jun 13, 2024
Adds `patch-package` as suggested in the issue this PR fixes. The goal
is to allow `window.CSS` to be mocked `null` in situations such as using
JSdom.

Tests cover a version that is patched and one that is unpatched to
demonstrate the patch truly fixes the issue.

fixes: #4400

---

**Developer Notes/Questions** for review:

- Someone mentioned the patch step should _not_ be `postinstall`, what
should it be instead?
- For test coverage purposes it's useful to keep an unpatched version,
currently this is done manually in `patches/color.unpatched.js` but
perhaps there's a better way
- It's a bit gross modifying so many pre-compiled `dist` files
- Instead of a patch, we could pull in colorjs.io as a submodule and
build it ourselves, unsure how to propagate that to consumers of
axe-core however
- Perhaps this is fine and I could document how to update the patch in
the future

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Steven Lambert <2433219+straker@users.noreply.github.com>
Co-authored-by: gaiety-deque <gaiety-deque@users.noreply.github.com>
WilcoFiers added a commit that referenced this issue Jul 29, 2024
##
[4.10.0](v4.9.1...v4.10.0)
(2024-07-29)

### Features

- **new-rule:** summary elements must have an accessible name
([#4511](#4511))
([0d8a99e](0d8a99e)),
closes [#4510](#4510)

### Bug Fixes

- **all-rules:** fix flakey all-rules firefox test
([#4467](#4467))
([3f13aa1](3f13aa1))
- **aria-allowed-attr:** allow aria-multiline=false for element with
contenteditable
([#4537](#4537))
([f019068](f019068))
- **aria-allowed-attr:** allow aria-required=false when normally not
allowed ([#4532](#4532))
([2e242e1](2e242e1))
- **aria-prohibited-attr:** allow aria-label/ledby on decendants of
widget ([#4541](#4541))
([07c5d91](07c5d91))
- **aria-roledescription:** keep disabled with { runOnly: 'wcag2a' }
([#4526](#4526))
([5b4cb9d](5b4cb9d)),
closes [#4523](#4523)
- **autocomplete-valid:** incomplete for invalid but safe values
([#4500](#4500))
([e31a974](e31a974)),
closes [#4492](#4492)
- **build:** limit locales to valid files when using the --all-lang
option ([#4486](#4486))
([d3db593](d3db593)),
closes [#4485](#4485)
- colorio.js patch mocking CSS
([#4456](#4456))
([3ef9353](3ef9353)),
closes [#4400](#4400)
- correct typos in texts
([#4499](#4499))
([11fad59](11fad59))
- **landmark-unique:** follow spec, aside -> landmark
([#4469](#4469))
([e32f803](e32f803)),
closes [#4460](#4460)
- **required-attr:** allow aria-valuetext on slider instead of valuenow
([#4518](#4518))
([135898b](135898b)),
closes [#4515](#4515)

This PR was opened by a robot 🤖 🎉
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment