dequelabs
Graceful handling of color contrast check fails for oklch and oklab with `none`
Product
axe-core
Product Version
4.10.3
Latest Version
- [x] I have tested the issue with the latest version of the product
Issue Description
Expectation
Color contrast checks return results for issues on the page, or partial results in event of an error with a specific color.
Actual
Color contrast checks return an error with no results.
How to Reproduce
A page with CSS like this produces the issue: oklch(0.961073 0.000047911 none / 0.2)
Additional context
It seems the source of this is ColorJS treats none as NaN in the version used in axe-core. They have a beta that resolves this in the v6 line that handles none and NaN properly so those colors don't fail.
Related issue: #4269
Thanks for the issue. I agree that completely failing color contrast is not a good experience.
@straker
I looked at the https://github.com/dequelabs/axe-core/blob/develop/lib/commons/color/color.js file and couldn't find none being passed. In that case, would the fix be to upgrade to newer version of colorjs.io, given that seems to fix the issue per this comment by @pattonwebz ?
If yes, will work on the upgrade.
For this specifically we'll just want to catch any errors from parsing color strings and then incomplete the node with a message about unsupported color string, that way color contrast can continue running.
What that means is we'll probably want to pull in incompleteData and call .set on it when we encounter the error. The function takes a key and a message id. The key would probably be bgColor and the message id could be any meaningful identifier. The message id will then need to be added to the color contrast metadata file's incomplete section with a message of something like "Color string not supported".
We'll also want to add a test for handling unsupported color strings in the color.js test file and making sure no error is thrown and the incompleteData is set with what we expect (using the .get method).
@namansancheti looks like we're going to be taking this on internally so you won't have to work on the changes.
@straker I see, thanks for the detailed walkthrough on the required changes, in any case 👍
I'm more than happy to test this out as it gets worked through, feel free to ping me here or in any resulting PR :)
Should have posted in this sooner but I picked this up this morning and have begun looking into it.
