Skip to content
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

add rgba contrast checking #134

Open
wants to merge 38 commits into
base: master
from

Conversation

@saltybeagle
Copy link

commented Aug 20, 2015

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. :)

mfairchild365 and others added some commits Apr 29, 2014

Don't warn about rgba anymore
No need to, we are doing the maths!
Convert to a colour string
The rest of the code expects bgColour to be a string
Return rgb channels as %
Everything else expects them as %.  Also, rework to reduce rounding problems.
Correctly calculate layered transparencies
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.
Make absolutely positioned contrast errors warnings
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.
Merge pull request #1 from mfairchild365/contrast-abs
Make absolutely positioned contrast errors warnings
Merge remote-tracking branch 'squizlabs/master'
Conflicts:
	Standards/WCAG2AAA/Sniffs/Principle1/Guideline1_4/1_4_3_Contrast.js
`msg` is already defined
was causing an error
Fix false positive
Only push an error if `warning === false`
Merge pull request #2 from mfairchild365/master
Merge with HTML_Codesniffer
Don't try to check SVG objects
Don't process the element if no body was could be found.
This was causing a fatal js error.
Merge pull request #3 from mfairchild365/svg-objects
Don't try to check SVG objects
Merge branch 'master' of https://github.com/squizlabs/HTML_CodeSniffer
Conflicts:
	Standards/WCAG2AAA/Sniffs/Principle1/Guideline1_4/1_4_3_Contrast.js
	Standards/WCAG2AAA/Sniffs/Principle4/Guideline4_1/4_1_2.js
@luketw

This comment has been minimized.

Copy link
Member

commented Nov 4, 2015

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.

@mfairchild365

This comment has been minimized.

Copy link
Contributor

commented Nov 5, 2015

@lwright-sq Thank you for looking at this. :)

@saltybeagle I didn't notice that you opened a separate PR, thank you for doing that!

@mfairchild365

This comment has been minimized.

Copy link
Contributor

commented May 31, 2016

@luketw This has been updated and is now mergable.

@ironikart ironikart self-assigned this Aug 25, 2016

@phillbaker

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

@ironikart would an updated PR help move this forward? I'd be willing to contribute an update.

@ironikart

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

@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.

@phillbaker

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

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 (rgb(0,0,0,.95)) is compared to a (mostly) transparent color (rgb(0,0,0,.05)). As the link shows, if the alpha is taken into account the ratio is closer to 8 rather than the 1 reported today.

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

@ironikart

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2018

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.

@phillbaker

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2018

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:

https://github.com/unl/HTML_CodeSniffer/blob/9bc316cb5ec52fdea1f3bfbee088f6e22a6bbb55/HTMLCS.Util.js#L381-L411

@ironikart what do you think of that approach?

@ironikart

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2018

@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.

@phillbaker

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2018

Ah, thanks @ironikart, that makes sense and explains your earlier example.

What do you think about this approach:

  1. An initial PR, based on this one, to detect apha in either font text color or background color and issue a warning. I'm just getting familiar with the code base, is this a good example of showing a warning?
  2. A follow up PR, also based on the code here, that: if none of the backgrounds are images, uses the algorithm to "flatten" the color of fonts/backgrounds, otherwise continues to issue the warning from (1).
@mfairchild365

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2018

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.

@ironikart

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2018

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.