fix(color-contrast): get text stoke from offset shadows #4079
Conversation
| it('skips shadows offset with 0.5px or less', () => { | ||
| fixture.innerHTML = ` | ||
| <span style="text-shadow: | ||
| 0.5px 0.5px #000, |
There was a problem hiding this comment.
This test case assumes that 2 shadows on opposite corners would be recognized correctly as a normally-combined case; it'd be good to have a test case verifying that explicitly ahead of this one
| return shadowColors; | ||
| } | ||
|
|
||
| function getStrokeColor(thinShadows) { |
There was a problem hiding this comment.
| function getStrokeColor(thinShadows) { | |
| function getStrokeColors(thinShadows) { |
| assert.lengthOf(shadowColors, 4); | ||
| }); | ||
|
|
||
| it('skips raised-letter effects (shadows on 1 or 2 sides)', () => { |
There was a problem hiding this comment.
Would be good to have a test that verifies that exactly 3 sides is sufficient to be not-skipped
Co-authored-by: Dan Bjorge <dan.bjorge@deque.com>
| const edges = ['top', 'right', 'bottom', 'left']; | ||
|
|
||
| /** | ||
| * |
| // Remove any shadows that cover 1 or 2 sides only | ||
| const strokeShadows = Object.entries(colorMap).filter(([, sides]) => { | ||
| const sidesCovered = edges.filter(side => sides[side].length !== 0).length; | ||
| return sidesCovered >= 3; | ||
| }); |
There was a problem hiding this comment.
TODO: figure out calling incomplete when shadows are not on all sides.
| /** | ||
| * Work out which color(s) of an array of text shadows form a stroke around the text. | ||
| * @param {Array[]} testShadows Parsed test shadows (see color.parseTestShadow()) | ||
| * @returns {Array[]} Array of colors |
There was a problem hiding this comment.
| * @returns {Array[]} Array of colors | |
| * @returns {Array|null} Array of colors or null if text-shadow was too complex to measure |
| return { colorStr, sides, edgeCount }; | ||
| }); | ||
|
|
||
| // Skip shadows that cover too much of the text to be ignored, but not enough to be tested |
There was a problem hiding this comment.
| // Skip shadows that cover too much of the text to be ignored, but not enough to be tested | |
| // Bail immediately if any shadow group covers too much of the text to be ignored, but not enough to be tested |
| isSolid &&= sides[edge].every(([x, y]) => { | ||
| return ( | ||
| Math.abs(x) > OPAQUE_STROKE_OFFSET_MIN_PX || | ||
| Math.abs(y) > OPAQUE_STROKE_OFFSET_MIN_PX | ||
| ); | ||
| }); |
There was a problem hiding this comment.
I think this is a bit off; if one shadow impacts 2 edges, but is only > OPAQUE_STROKE_OFFSET_MIN_PX on one of those edges, this will currently treat both edges as solid instead of just one like it should. Here is a test case that demonstrates this (the same as the current applies an alpha value if not all shadows are offset by more than 1.5px test, except with the 4 shadows collapsed into two corner shadows):
it('applies an alpha value if not all sides are offset by more than 1.5px', () => {
const shadows = parseTextShadows(`
-1.5px -2px #000,
-2px 1.5px #000
`);
const shadowColors = getStrokeColorsFromShadows(shadows);
assert.lengthOf(shadowColors, 1);
assert.equal(shadowColors[0].red, 0);
assert.equal(shadowColors[0].green, 0);
assert.equal(shadowColors[0].blue, 0);
assert.closeTo(shadowColors[0].alpha, 0.46, 0.01);
});To fix this, I recommend updating getShadowColorsMap to produce a mapping from edge to list-of-thickness, instead of from edge to list-of-pixels-tuples (ie, replace all the borders.EDGE.push(pixels) statements with borders.EDGE.push(/* offsetX or offsetY as appropriate */)), and then update this check to be:
| isSolid &&= sides[edge].every(([x, y]) => { | |
| return ( | |
| Math.abs(x) > OPAQUE_STROKE_OFFSET_MIN_PX || | |
| Math.abs(y) > OPAQUE_STROKE_OFFSET_MIN_PX | |
| ); | |
| }); | |
| isSolid &&= sides[edge].every( | |
| offset => Math.abs(offset) > OPAQUE_STROKE_OFFSET_MIN_PX | |
| ); |
| const strokeColor = new Color(); | ||
| strokeColor.parseString(colorStr); | ||
|
|
||
| // Average the number of shadows around the text |
There was a problem hiding this comment.
| // Average the number of shadows around the text | |
| // Detect whether any sides' shadows are thin enough to be considered | |
| // translucent, and if so, calculate an alpha value to apply on top of | |
| // the parsed color. |
| * Using colorStr and thickness of sides, create a color object | ||
| */ | ||
| function shadowGroupToColor({ colorStr, sides, edgeCount }) { | ||
| if (edgeCount !== 4) { |
There was a problem hiding this comment.
It feels off that this check is still here when the caller is now also doing an overlapping edgeCount check. Maybe more clear to move this check out of shadowGroupToColor and update the .filter on L32 to filter out based on edgeCount before mapping this function, instead of mapping this function and then filtering based on its return value.
|
|
||
| it('double-counts shadows on corners', () => { | ||
| // Corner-shadows overlap on the sides of letters, increasing alpha | ||
| const shadows = parseTextShadows(` |
There was a problem hiding this comment.
This is the only test case that's covering shadow-on-corners behavior; it'd be good to have a case that verifies that "2 opposing corner shadows is enough to not return empty/null"
| @@ -54,7 +54,8 @@ function _getBackgroundColor(elm, bgElms, shadowOutlineEmMax) { | |||
| } | |||
|
|
|||
| const textRects = getVisibleChildTextRects(elm); | |||
| let bgColors = getTextShadowColors(elm, { minRatio: shadowOutlineEmMax }); | |||
| let bgColors = | |||
| getTextShadowColors(elm, { minRatio: shadowOutlineEmMax }) ?? []; | |||
There was a problem hiding this comment.
For the ?? [] to come up in color-contrast and not have already resulted in a complexTextShadows exit in color-contrast-evaluate, an element would need to have a "complex" (2-3 edge compound) shadow of blur radius between .001em and 0.2em. I think if that were to actually happen in practice and there was additionally a higher-blur-radius shadow(s), it probably doesn't make sense to silently throw away those higher-blur-radius shadow(s). I can be persuaded that either of "bail as incomplete" or "ignore those cases but still consider the thick shadow behind as a possible background color" would be reasonable.
link-in-text-block also uses getBackgroundColor and I think the ?? [] is even sketchier there, since it doesn't early-exit with incomplete for blur radius < .001 complex compound shadows like color-contrast does. Generally, it seems a bit strange to me that link-in-text-block-evaluate isn't reusing the same support for shadows providing contrast that color contrast is.
There was a problem hiding this comment.
I can't sneak anything past you can I? I realised that too when I wrote this. Wasn't sure whether I should care, but fair enough.
If several text shadows with some offset and with minimal blur are used together, they can combine into a text stroke effect. This PR attempts to identify such cases and combine these shadows into a single color.
Closes: #4064