[MWPW-184883] [Preflight] Added canvas based color contrast checks to preflight#5335
[MWPW-184883] [Preflight] Added canvas based color contrast checks to preflight#5335robert-bogos wants to merge 7 commits intostagefrom
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
zagi25
left a comment
zagi25
left a comment
There was a problem hiding this comment.
Just a few nits that I noticed right away, will definitely revisit for a more detailed review.
|
This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR. |
mokimo
left a comment
mokimo
left a comment
There was a problem hiding this comment.
I'll spare the nits, overall you presented the concept and I'd be happy for this to be more of a living-thing and we will collect feedback in the real world about the details.
@skholkhojaev would also include this in his preflight logging - so we could check the pages where we find issues and spot check pages with issues to confirm the whole algorithm and flow works as expected
| const normalized = channel / 255; | ||
| return normalized <= 0.03928 | ||
| ? normalized / 12.92 | ||
| : ((normalized + 0.055) / 1.055) ** 2.4; | ||
| }; | ||
|
|
||
| function extractBgImageUrl(bgImageCss) { | ||
| if (!bgImageCss || bgImageCss === 'none') return null; | ||
| const match = bgImageCss.match(/url\(["']?([^"')]+)["']?\)/); | ||
| return match ? match[1] : null; | ||
| } | ||
| const relativeLuminance = ([r, g, b]) => ( | ||
| 0.2126 * srgbToLinear(r) + 0.7152 * srgbToLinear(g) + 0.0722 * srgbToLinear(b) | ||
| ); |
There was a problem hiding this comment.
These magic numbers would benefit from getting moved into variables... I couldn't even fathom where to start to understand where 0.03928 or 12.92 come from, they seem very specific and probably based on some mathematical constants?
Either way, making those a bit more clear might help long term
There was a problem hiding this comment.
Those are mathematical constants, I am a bit reluctant to move them into variables as they are not and will not be used in more than just one place. I have a comment above though, sending to the link where the math is explained.
| // Step 1: collect visible text nodes, their bounding boxes, and color. | ||
| const textElements = collectTextElements(elements); | ||
|
|
||
| // Step 2: add visibility hidden to all the text elements, to have a clean DOM for the canvas | ||
| hideTextElements(textElements); | ||
|
|
||
| // Step 3: capture DOM on canvas | ||
| const { base } = getConfig(); | ||
| await loadScript(`${base}/deps/html2canvas.js`); | ||
| const backgroundCanvas = await captureDocumentCanvas(); | ||
|
|
||
| // Step 4: restore the visibility of the text elements | ||
| restoreHiddenText(); | ||
|
|
||
| // Step 5: Capture a full snapshot, with text visible, for thumbnails | ||
| const fullCanvas = await captureDocumentCanvas(); | ||
|
|
||
| // Step 6: per character from each text node, sample background from textless canvas, | ||
| // calculate color contrast, keep the worst ratio per node | ||
| applyContrastSampling(textElements, backgroundCanvas, fullCanvas); | ||
|
|
||
| // Step 7: create violation entries for low-contrast text nodes | ||
| return buildViolations(textElements, minContrast); |
There was a problem hiding this comment.
love this detailed description,, that's 100% helpful
There was a problem hiding this comment.
so happy to know that is useful, I tried to make this as readable as possible ❤️
…nto 184883-preflight
|
This PR has not been updated recently and will be closed in 7 days if no action is taken. Please ensure all checks are passing, https://github.com/orgs/adobecom/discussions/997 provides instructions. If the PR is ready to be merged, please mark it with the "Ready for Stage" label. |
|
Closing this PR due to inactivity. |
|
This PR has not been updated recently and will be closed in 7 days if no action is taken. Please ensure all checks are passing, https://github.com/orgs/adobecom/discussions/997 provides instructions. If the PR is ready to be merged, please mark it with the "Ready for Stage" label. |
|
This PR has not been updated recently and will be closed in 7 days if no action is taken. Please ensure all checks are passing, https://github.com/orgs/adobecom/discussions/997 provides instructions. If the PR is ready to be merged, please mark it with the "Ready for Stage" label. |
5 participants
Description
This PR is adding a new way to calculate color contrast ratios within the Preflight a11y checks.
Related Issue
Resolves: MWPW-184883
Testing instructions
Screenshots:
Test URLs
Milo:
CC:
BACOM: