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 false positives for hex with alpha-channel and false negatives for modern syntax in color-named #5718

Merged
merged 10 commits into from Nov 16, 2021

Conversation

ota-meshi
Copy link
Member

@ota-meshi ota-meshi commented Nov 14, 2021

Which issue, if any, is this issue related to?

Closes #5716

Is there anything in the PR that needs further explanation?

This PR fixes false positives for alpha-channel posted in #5716, and false negatives for modern syntax (e.g. rgba(0 0 0 / 1)).

@@ -109,6 +109,7 @@
},
"dependencies": {
"balanced-match": "^2.0.0",
"colord": "^2.9.1",
Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to use colord package instead of reference file.

Copy link
Member

Choose a reason for hiding this comment

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

The colord package looks nice to me. Just in case, could you tell us why you choose this package, please?

Copy link
Member Author

Choose a reason for hiding this comment

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

colord seems to be used in cssnano and I think we can trust it. Also, another well-known package, color, doesn't seem to be able to parse modern syntax (e.g. rgb(0 0 0)), so I think colord is better.

Please let me know if you know any other better packages.

Comment on lines 15 to 24
/* Syntaxes that is removed in Color Module Level 4 specification. */

// hwb() with comma
(_colordClass, parsers) => {
parsers.string.push([parseHwbWithCommaString, /** @type {any} */ ('hwb-with-comma')]);
},
// gray()
(_colordClass, parsers) => {
parsers.string.push([parseGrayString, /** @type {any} */ ('gray')]);
},
Copy link
Member Author

Choose a reason for hiding this comment

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

colord doesn't understand the old syntax, so I extended it.

@ota-meshi ota-meshi changed the title Fix false positives for #RRGGBBAA in color-named Fix false positives for alpha-channel and false negatives for modern syntax in color-named Nov 14, 2021
@ota-meshi ota-meshi changed the title Fix false positives for alpha-channel and false negatives for modern syntax in color-named Fix false positives for hex with alpha-channel and false negatives for modern syntax in color-named Nov 14, 2021
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

@ota-meshi Thank you for the great fix PR! 👍🏼

I've left some questions and refactoring suggestions, but this PR looks mostly fine to me! 👏🏼

lib/rules/color-named/__tests__/colord-utils.test.js Outdated Show resolved Hide resolved
lib/rules/color-named/__tests__/colord-utils.test.js Outdated Show resolved Hide resolved
lib/rules/color-named/colord-utils.js Outdated Show resolved Hide resolved
lib/rules/color-named/colord-utils.js Outdated Show resolved Hide resolved
lib/rules/color-named/colord-utils.js Outdated Show resolved Hide resolved
lib/rules/color-named/colord-utils.js Outdated Show resolved Hide resolved
lib/rules/color-named/index.js Show resolved Hide resolved
ota-meshi and others added 6 commits November 15, 2021 16:14
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
@ota-meshi
Copy link
Member Author

@ybiquitous I have added your requested changes to this PR. So could you check it again?

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

@ota-meshi Great work! LGTM 👏🏼

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

This rule was problematically slow in the past.

@ota-meshi It'd be good to get a benchmark of this branch against main before merging.

@ota-meshi
Copy link
Member Author

I didn't do that intentionally, but it also seems to be of value in terms of performance.

After:

npm run benchmark-rule -- color-named never 

Warnings: 0
Mean: 37.03908788571429 ms
Deviation: 20.985396496488296 ms

npm run benchmark-rule -- color-named always-where-possible

Warnings: 131
Mean: 41.65396371538462 ms
Deviation: 25.849404139328065 ms

Before:

npm run benchmark-rule -- color-named never 

Warnings: 0
Mean: 57.58543509615383 ms
Deviation: 40.58585746807126 ms

npm run benchmark-rule -- color-named always-where-possible

Warnings: 129
Mean: 69.16574440277776 ms
Deviation: 25.362776611378344 ms

@ota-meshi
Copy link
Member Author

The result of npm run benchmark-rule --color-named always-where-possible is different, so I checked the cause and noticed that the new logic was converted to transparent. I will fix it.

@ota-meshi
Copy link
Member Author

I fixed this PR. So please check it again.
45371c0

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

Excellent, thank you!

@jeddy3 jeddy3 merged commit c91774c into main Nov 16, 2021
@jeddy3 jeddy3 deleted the fix-color-named branch November 16, 2021 10:12
@jeddy3
Copy link
Member

jeddy3 commented Nov 16, 2021

  • Fixed: color-named false positives for hex with alpha-channel and false negatives for modern syntax (#5718).

@omgovich
Copy link

Hey guys! Thanks for choosing colord! Let me know if you will need any help!

@Mouvedia
Copy link
Contributor

After reading https://github.com/postcss/postcss-color-gray, I am wondering if gray() was actually implemented by one browser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Fix false positives for #RRGGBBAA in color-named
5 participants