-
Notifications
You must be signed in to change notification settings - Fork 851
fix: color contrast fails for oklch and oklab with none #4959
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
base: develop
Are you sure you want to change the base?
Changes from 4 commits
d9ffda8
a6ae386
0a059a4
7f55cf7
1d8e936
cbeac46
08c13eb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,9 +75,17 @@ function _getBackgroundColor(elm, bgElms, shadowOutlineEmMax) { | |
| } | ||
|
|
||
| // Get the background color | ||
| const bgColor = getOwnBackgroundColor(bgElmStyle); | ||
| if (bgColor.alpha === 0) { | ||
| continue; | ||
| let bgColor; | ||
| try { | ||
| bgColor = getOwnBackgroundColor(bgElmStyle); | ||
| if (bgColor.alpha === 0) { | ||
| continue; | ||
| } | ||
| } catch (error) { | ||
| if (error.cause) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We shouldn't assume that any use of the
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was using the error.cause object to pass along the color string that caused the error. In color.js, it is given to the original error string but it becomes part of a full string. At the time, I did not see a way to get the color to have displayed in the colorParse data.colorParse message. I will see if there is another way I can capture the original color string. |
||
| incompleteData.set('colorParse', error.cause.value); | ||
| return null; | ||
chutchins25 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
|
|
||
| // abort if a node is partially obscured and obscuring element has a background | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,16 +32,23 @@ export default function getForegroundColor(node, _, bgColor, options = {}) { | |
| ]; | ||
| let fgColors = []; | ||
|
|
||
| for (const colorFn of colorStack) { | ||
| const color = colorFn(); | ||
| if (!color) { | ||
| continue; | ||
| } | ||
| try { | ||
| for (const colorFn of colorStack) { | ||
| const color = colorFn(); | ||
| if (!color) { | ||
| continue; | ||
| } | ||
|
|
||
| fgColors = fgColors.concat(color); | ||
| fgColors = fgColors.concat(color); | ||
|
|
||
| if (color.alpha === 1) { | ||
| break; | ||
| if (color.alpha === 1) { | ||
| break; | ||
| } | ||
| } | ||
| } catch (error) { | ||
| if (error.cause) { | ||
| incompleteData.set('colorParse', error.cause.value); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto for here (don't assume |
||
| return null; | ||
chutchins25 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,45 @@ describe('color-contrast', function () { | |
| axe._tree = undefined; | ||
| }); | ||
|
|
||
| it('should return undefined if cannot handle color', function () { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great tests! |
||
| var params = checkSetup( | ||
| '<div id="divundertest" style="color: oklch(0.961073 0.000047911 none / 0.2); background-color: black; font-size: 14pt; font-weight: 900;">' + | ||
| '<span id="target" style="font-weight:lighter;">My text</span></div>' | ||
| ); | ||
|
|
||
| var expectedRelatedNodes = fixture.querySelector('#divundertest'); | ||
| assert.isUndefined(contrastEvaluate.apply(checkContext, params)); | ||
| assert.deepEqual(checkContext._relatedNodes, [expectedRelatedNodes]); | ||
| assert.deepEqual(checkContext._data.messageKey, 'colorParse'); | ||
| assert.equal( | ||
| checkContext._data.colorParse, | ||
| 'oklch(0.961073 0.000047911 none / 0.2)' | ||
| ); | ||
| }); | ||
|
|
||
| it('should should return undefined if cannot handle backgroundcolor', function () { | ||
chutchins25 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| var params = checkSetup( | ||
| '<div style="color: gray; background-color: oklch(0.961073 0.000047911 none / 0.2); font-size: 14pt; font-weight: 900;">' + | ||
| '<span id="target" style="font-weight:lighter;">My text</span></div>' | ||
| ); | ||
| assert.isUndefined(contrastEvaluate.apply(checkContext, params)); | ||
| assert.deepEqual(checkContext._relatedNodes, []); | ||
| assert.deepEqual(checkContext._data.messageKey, 'colorParse'); | ||
| assert.equal( | ||
| checkContext._data.colorParse, | ||
| 'oklch(0.961073 0.000047911 none / 0.2)' | ||
| ); | ||
| }); | ||
|
|
||
| it('should should return undefined if cannot handle text-shadow', function () { | ||
chutchins25 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| var params = checkSetup( | ||
| '<div id="target" style="background-color: #fff; color:#000; text-shadow: 1px 1px oklch(0.961073 0.000047911 none / 0.2);">My text</div>' | ||
| ); | ||
|
|
||
| assert.isUndefined(contrastEvaluate.apply(checkContext, params)); | ||
| assert.deepEqual(checkContext._relatedNodes, []); | ||
| }); | ||
|
|
||
| it('should return true for hidden element', function () { | ||
| var params = checkSetup( | ||
| '<div style="color: gray; background-color: white; font-size: 14pt; font-weight: 100;">' + | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately axe-core has to still "support" IE11 (even though we deprecated it and stopped testing with it) and I don't believe the Error object supports
causein IE11. I tested it out and it doesn't seem to come through so we may not be able to do it this way.