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.

Sign up for GitHub

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

Jump to bottom

fix: colorio.js patch mocking CSS #4456

Open
wants to merge 42 commits into
base: develop
Choose a base branch
from
Open

fix: colorio.js patch mocking CSS #4456

wants to merge 42 commits into from

Conversation

gaiety-deque
Copy link
Contributor

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

@dependabot
chore: bump the npm-low-risk group across 1 directory with 9 updates
6a1f4ec 
Bumps the npm-low-risk group with 9 updates in the / directory:

| Package | From | To |
| --- | --- | --- |
| [@babel/core](https://github.com/babel/babel/tree/HEAD/packages/babel-core) | `7.24.4` | `7.24.5` |
| [@babel/preset-env](https://github.com/babel/babel/tree/HEAD/packages/babel-preset-env) | `7.24.4` | `7.24.5` |
| [@babel/runtime-corejs3](https://github.com/babel/babel/tree/HEAD/packages/babel-runtime-corejs3) | `7.24.4` | `7.24.5` |
| [clean-jsdoc-theme](https://github.com/ankitskvmdam/clean-jsdoc-theme) | `4.2.18` | `4.3.0` |
| [colorjs.io](https://github.com/LeaVerou/color.js) | `0.4.3` | `0.5.0` |
| [core-js](https://github.com/zloirock/core-js/tree/HEAD/packages/core-js) | `3.36.1` | `3.37.0` |
| [jsdoc](https://github.com/jsdoc/jsdoc) | `4.0.2` | `4.0.3` |
| [selenium-webdriver](https://github.com/SeleniumHQ/selenium) | `4.19.0` | `4.20.0` |
| [typescript](https://github.com/Microsoft/TypeScript) | `5.4.4` | `5.4.5` |



Updates `@babel/core` from 7.24.4 to 7.24.5
- [Release notes](https://github.com/babel/babel/releases)
- [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md)
- [Commits](https://github.com/babel/babel/commits/v7.24.5/packages/babel-core)

Updates `@babel/preset-env` from 7.24.4 to 7.24.5
- [Release notes](https://github.com/babel/babel/releases)
- [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md)
- [Commits](https://github.com/babel/babel/commits/v7.24.5/packages/babel-preset-env)

Updates `@babel/runtime-corejs3` from 7.24.4 to 7.24.5
- [Release notes](https://github.com/babel/babel/releases)
- [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md)
- [Commits](https://github.com/babel/babel/commits/v7.24.5/packages/babel-runtime-corejs3)

Updates `clean-jsdoc-theme` from 4.2.18 to 4.3.0
- [Release notes](https://github.com/ankitskvmdam/clean-jsdoc-theme/releases)
- [Changelog](https://github.com/ankitskvmdam/clean-jsdoc-theme/blob/master/CHANGELOG.md)
- [Commits](https://github.com/ankitskvmdam/clean-jsdoc-theme/commits)

Updates `colorjs.io` from 0.4.3 to 0.5.0
- [Release notes](https://github.com/LeaVerou/color.js/releases)
- [Commits](color-js/color.js@v0.4.3...v0.5.0)

Updates `core-js` from 3.36.1 to 3.37.0
- [Release notes](https://github.com/zloirock/core-js/releases)
- [Changelog](https://github.com/zloirock/core-js/blob/master/CHANGELOG.md)
- [Commits](https://github.com/zloirock/core-js/commits/v3.37.0/packages/core-js)

Updates `jsdoc` from 4.0.2 to 4.0.3
- [Release notes](https://github.com/jsdoc/jsdoc/releases)
- [Changelog](https://github.com/jsdoc/jsdoc/blob/4.0.3/CHANGES.md)
- [Commits](jsdoc/jsdoc@4.0.2...4.0.3)

Updates `selenium-webdriver` from 4.19.0 to 4.20.0
- [Release notes](https://github.com/SeleniumHQ/selenium/releases)
- [Commits](SeleniumHQ/selenium@selenium-4.19.0...selenium-4.20.0)

Updates `typescript` from 5.4.4 to 5.4.5
- [Release notes](https://github.com/Microsoft/TypeScript/releases)
- [Changelog](https://github.com/microsoft/TypeScript/blob/main/azure-pipelines.release.yml)
- [Commits](microsoft/TypeScript@v5.4.4...v5.4.5)

---
updated-dependencies:
- dependency-name: "@babel/core"
  dependency-type: direct:development
  update-type: version-update:semver-patch
  dependency-group: npm-low-risk
- dependency-name: "@babel/preset-env"
  dependency-type: direct:development
  update-type: version-update:semver-patch
  dependency-group: npm-low-risk
- dependency-name: "@babel/runtime-corejs3"
  dependency-type: direct:development
  update-type: version-update:semver-patch
  dependency-group: npm-low-risk
- dependency-name: clean-jsdoc-theme
  dependency-type: direct:development
  update-type: version-update:semver-minor
  dependency-group: npm-low-risk
- dependency-name: colorjs.io
  dependency-type: direct:development
  update-type: version-update:semver-minor
  dependency-group: npm-low-risk
- dependency-name: core-js
  dependency-type: direct:development
  update-type: version-update:semver-minor
  dependency-group: npm-low-risk
- dependency-name: jsdoc
  dependency-type: direct:development
  update-type: version-update:semver-patch
  dependency-group: npm-low-risk
- dependency-name: selenium-webdriver
  dependency-type: direct:development
  update-type: version-update:semver-minor
  dependency-group: npm-low-risk
- dependency-name: typescript
  dependency-type: direct:development
  update-type: version-update:semver-patch
  dependency-group: npm-low-risk
...

Signed-off-by: dependabot[bot] <support@github.com>
dbjorge
dbjorge previously requested changes May 10, 2024
View reviewed changes
package.json Outdated Show resolved Hide resolved
patches/colorjs.io+0.5.0.patch Outdated Show resolved Hide resolved
patches/color.unpatched.js Outdated Show resolved Hide resolved
straker
straker reviewed May 13, 2024
View reviewed changes
patches/colorjs.io+0.4.5.patch Outdated Show resolved Hide resolved
patches/colorjs.io+0.4.5.patch Outdated Show resolved Hide resolved
@gaiety-deque gaiety-deque marked this pull request as ready for review May 14, 2024 21:54
@gaiety-deque gaiety-deque requested a review from a team as a code owner May 14, 2024 21:54
gaiety-deque and others added 21 commits May 14, 2024 16:59
@gaiety-deque
fix: colorio.js patch mocking CSS
c62613f 
patching colorio.js with optional chaining for `CSS?.supports`

Refs: #4400
@gaiety-deque
import local colorjs instead of CDN
920c990
@gaiety-deque
copy in an unpatched file for testing against
576dc99
@gaiety-deque
patch colorjs with optional chaining to support CSS null
5c39c29
@gaiety-deque
removed commented out html script
841fb7d
@gaiety-deque
🤖 Automated formatting fixes
69b80f5
@gaiety-deque
debugging issue in CI
edf803a
@gaiety-deque
still debugging in CI
3a7b7e6
@gaiety-deque
undoes changes to prototype
eff6ad3
@gaiety-deque
patch is a build step
4fada62
@gaiety-deque
smaller patch, for 0.4.5
71cc2b4
@gaiety-deque
move to grunt build step instead of prebuild postbuild
ebd2e96
@gaiety-deque
🤖 Automated formatting fixes
57de085
@gaiety-deque
patch only color.js
89b7386
@gaiety-deque
correct import paths for proxied color.js in karma
4e6682d
@gaiety-deque
testing a fix in CI, revert
0cb35e9
@gaiety-deque
Revert "testing a fix in CI, revert"
af79b8b 
This reverts commit 0dd9bb5.
@gaiety-deque
add unpatched to build cache
2a4d76c
@gaiety-deque
add colorjs patched dist to build cache as well
20a8a6f
@gaiety-deque
🤖 Automated formatting fixes
c5444fd
@gaiety-deque
patch 0.4.3 instead of 0.4.5
ac45ddf
romankulkovsf

This comment was marked as spam.

Sign in to view

@romankulkovsf

This comment was marked as spam.

Sign in to view
@gaiety-deque gaiety-deque changed the base branch from develop to fix-flakey-marque-firefox-test May 17, 2024 17:57
@gaiety-deque gaiety-deque mentioned this pull request May 17, 2024
Merged
WilcoFiers
WilcoFiers previously requested changes May 20, 2024
View reviewed changes
.eslintignore Outdated Show resolved Hide resolved
Gruntfile.js Outdated
@@ -6,12 +6,14 @@ camelcase: ["error", {"properties": "never"}]
module.exports = function (grunt) {
'use strict';

grunt.loadNpmTasks('grunt-exec');
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think I said it needed to be in grunt :P. We did a good while back agree not to add more grunt tasks if we could at all avoid it. It seems viable to put this work in prebuild / predevelop scripts instead. Did you try that @gaiety-deque? If that was more difficult I can live with a Grunt solution, but if we can avoid these extra dependencies + more grunt tasks I think that's the way to go on this.

Base automatically changed from fix-flakey-marque-firefox-test to develop May 20, 2024 13:01
gaiety-deque added a commit that referenced this pull request May 20, 2024
@gaiety-deque
fix(all-rules): fix flakey all-rules firefox test (#4467)
3f13aa1 
wilco and I believe marque is to blame because it is animated


This fixes the `all-rules` check, particularly on Firefox. We believe
the issue is because marque has a moving bounding client rect, leading
to inconsistent results. So instead we set the motion to none for a
consistent testing environment.

---

To see that it works, I've run the CI tests on this branch five times.
It only failed on one unrelated flakey test this does not address.

Also, #4456 finally builds
after many consistent test failures in the same place in a row after
rebasing on this change.

We can see this test failing on other branches, such as `develop` in
places like this:
https://app.circleci.com/pipelines/github/dequelabs/axe-core/6332/workflows/b3e2a293-c89b-4201-898d-1c6c64ee2764
which shows it has nothing to do with the other PR.
WilcoFiers
WilcoFiers previously requested changes May 22, 2024
View reviewed changes
package.json Outdated Show resolved Hide resolved
test/core/patch.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@gaiety-deque gaiety-deque dismissed WilcoFiers’s stale review May 28, 2024 20:53

addressed change requests

WilcoFiers
WilcoFiers reviewed Jun 7, 2024
View reviewed changes
Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

Got a question

package.json Outdated
@@ -110,7 +112,7 @@
"sri-update": "grunt build && node build/sri-update && git add sri-history.json",
"sri-validate": "node build/sri-update --validate",
"fmt": "prettier --write .",
"prepare": "husky",
"prepare": "husky && npm run unpatch && npm run patch",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually need to unpatch first? I thought you had done that so we could get an unpatched version to test against. You patch-package just skips if you run it on an already patched package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're correct 09a36d5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephenmathieson stephenmathieson stephenmathieson left review comments

@WilcoFiers WilcoFiers WilcoFiers approved these changes

@romankulkovsf romankulkovsf romankulkovsf approved these changes

@dbjorge dbjorge Awaiting requested review from dbjorge

@straker straker Awaiting requested review from straker

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Don't break on window.CSS === null
6 participants
@gaiety-deque @romankulkovsf @dbjorge @WilcoFiers @stephenmathieson @straker