HTML_CodeSniffer icon indicating copy to clipboard operation
HTML_CodeSniffer copied to clipboard

add rgba contrast checking

Open saltybeagle opened this issue 9 years ago • 12 comments

Updating an older PR with one that will merge cleanly. Original PR: https://github.com/squizlabs/HTML_CodeSniffer/pull/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. :)

saltybeagle avatar Aug 20 '15 17:08 saltybeagle

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.

luketw avatar Nov 04 '15 23:11 luketw

@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 avatar Nov 05 '15 01:11 mfairchild365

@luketw This has been updated and is now mergable.

mfairchild365 avatar May 31 '16 22:05 mfairchild365

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

phillbaker avatar Jun 26 '18 16:06 phillbaker

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

ironikart avatar Jun 26 '18 22:06 ironikart

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: https://github.com/dequelabs/axe-core/issues/708

phillbaker avatar Jun 26 '18 23:06 phillbaker

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.

ironikart avatar Jun 27 '18 23:06 ironikart

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?

phillbaker avatar Jun 28 '18 03:06 phillbaker

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

ironikart avatar Jun 28 '18 05:06 ironikart

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

phillbaker avatar Jun 28 '18 14:06 phillbaker

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.

mfairchild365 avatar Jun 28 '18 14:06 mfairchild365

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

ironikart avatar Jun 28 '18 23:06 ironikart