add rgba contrast checking #134
Conversation
No need to, we are doing the maths!
The rest of the code expects bgColour to be a string
Everything else expects them as %. Also, rework to reduce rounding problems.
Take this structure of backgrounds for example, 1 being the closest element: 1. rgba(10,10,10, .5); 2. rgba(10,10,50, .1); 3. rgb(10,10,250); We need to transform background 2 into a solid color before we transform background 1 into a solid colour.
This follows the test procedure from http://www.w3.org/TR/2014/NOTE-WCAG20-TECHS-20140408/F68
We can't reliably compute the contrast of text over a background if the text is absolutely positioned over the background of another element that isn't a direct parent of the text element.
Make absolutely positioned contrast errors warnings
Conflicts: Standards/WCAG2AAA/Sniffs/Principle1/Guideline1_4/1_4_3_Contrast.js
was causing an error
Only push an error if `warning === false`
Merge with HTML_Codesniffer
Don't process the element if no body was could be found. This was causing a fatal js error.
Don't try to check SVG objects
Merge with upstream
Conflicts: Standards/WCAG2AAA/Sniffs/Principle1/Guideline1_4/1_4_3_Contrast.js Standards/WCAG2AAA/Sniffs/Principle4/Guideline4_1/4_1_2.js
Merge with upstream
|
Hi Brett (and Michael who submitted the original commit) Sorry for the slow response - as might be gleaned from other issues here, I've been flat-out with other development and haven't been able to get to this yet. I'll certainly take a look at it when I get the chance - might take a little while because of the complexity of it. |
|
@lwright-sq Thank you for looking at this. :) @saltybeagle I didn't notice that you opened a separate PR, thank you for doing that! |
This was throwing a false positive in some cases, where it should trigger a manual check instead.
…form Check for the will-change: 'transform' background image performance hack
Merge with upstream
|
@luketw This has been updated and is now mergable. |
form controls don't need labels if they are display:none
|
@ironikart would an updated PR help move this forward? I'd be willing to contribute an update. |
|
@phillbaker An updated PR would be great, I can't merge this one as it stands. There aren't a lot of details from @saltybeagle about the issue this PR was trying to solve, right now I'd just be guessing at it from the code. A specific use case that I could write a unit test for would help a lot. |
|
Here's the usecase we see: when calculating the contrast of text over a transparent background, currently the alpha components are ignored, so sometimes when text is actually in high contrast, we see false positives. Here's an example where the text of (mostly) a dark color ( The trick comes from the transparency - depending on the underlay color the calculation may or may not give the final answer. So what might be a more reasonable approach that doing the calculation using the included alpha levels, is to detect whether the background/foreground has alpha and log it as a warning to be manually checked instead of either triggering a false positive or false negative. What do you think? Here's a similar issue in aXe: dequelabs/axe-core#708 |
|
Thanks @phillbaker for the concise explanation. I'd agree giving a hard pass or fail wouldn't work well here due to the transparency (similar reason why we can't accurately detect font contrast of positioned elements) and would lean towards showing a warning for the element if the background contains an alpha value of less than 1. |
|
Hm, I might have spoken too soon. I didn't fully examine the PR and it looks like this section might take into account the layer background colors: @ironikart what do you think of that approach? |
|
@phillbaker That would work if we assumed that the transparency resulted in mixes of just colours, but what I suspect might happen with transparent backgrounds is revealing images or text where contrast can't be checked. The example I would think of is something like the homepage here: https://www.squiz.net/ - the news items have semi transparent backgrounds over an image. |
|
Ah, thanks @ironikart, that makes sense and explains your earlier example. What do you think about this approach:
|
|
I'm glad to see this move forward. If I remember correctly, much of this logic is based on DOM tree hierarchy rather than visual hierarchy, which will lead to false positives. For example, an element might not be a parent a given element in the DOM, but might be styled to be positioned behind the element via CSS. I'd suggest looking into document.elementsFromPoint(x, y); to account for this and get an accurate representation of the visual stack. |
|
@phillbaker That sounds like a solid way forward and for (1) the example for showing a warning is correct. FYI I think we don't account for css opacity which will probably throw another spanner in the works for testing contrast. @mfairchild365 I've used that in other projects (mostly for mouse positioning). In this case it is too difficult to determine contrast with positioned elements (e.g. text on text, semi transparency) so we just display a notice that we cannot accurately check them. I'm not sure how it could be done without sampling every pixel of the positioned element and being able to correlate that with the CSS rules. |
Updating an older PR with one that will merge cleanly. Original PR: #97
This converts both background and text from rgba to a solid rgb color during contrast checking.
Heads up: I'm not a js/node expert, nor have I worked with colors like this before, so I would like if this could be reviewed carefully. :)