fix(color-contrast): account for text-shadow #2334

WilcoFiers · 2020-06-30T11:45:30Z

Take text-shadow into account when computing the background color of a text node.

Related #246

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

Follows the commit message policy, appropriate for next version
Code is reviewed for security

stephenmathieson

It doesn't look like we're accounting for HSL colors here. Are we not concerned with them?

stephenmathieson · 2020-07-01T18:51:53Z

lib/commons/color/color.js

@@ -83,6 +107,31 @@ function Color(red, green, blue, alpha) {
 		}
 	};

+	this.parseHexString = function(colorString) {


NBD either way, but now that we can import things, I'm curious why you didn't grab something from npm rather than building this yourself.

Security / package size. I know this does exactly what we need, and nothing else.

Our bundler should be tree-shaking any unused dependency code out for us, and we would "bundle" it at build-time so it'd be easier to analyze the sources. I understand the security concern, but am just worried about adding more to the list of complex things we're responsible for maintaining. Not worth blocking this PR, but I thought once we'd migrated to modules, we'd stop "reinventing the wheel".

stephenmathieson · 2020-07-01T18:53:54Z

lib/standards/css-colors.js

@@ -0,0 +1,147 @@
+// Source: https://www.w3.org/TR/css-color-3/#svg-color
+const cssColors = {


There are a few packages available from npm that have these listed in them already. Do we want to be responsible for keeping this list up-to-date, or should we offload that responsibility to the maintainers of one of the existing packages?

Also, this is out-of-date. For example "rebeccapurple" has been added.

Yer right, we should use CSS 4 stuff here. As for why not NPM. Partially because if we do this ourselves we get exactly what we want, and it is more secure.

Are we going to remember to update this when changes are made to the spec?

I'm cued into W3C publications, I do keep an eye out for things like this.

I could be convinced either way. On the one had, theres https://github.com/bahamas10/css-color-names which auto-generates a single JSON list of colors (which is nicer than hand maintaining it). On the other hand, a list like this allows users to add their own so we aren't waiting on new names to work in all the browsers before we add it.

Personally, if we don't care about users being able to customize this list then we should probably just grab it from that npm package so we don't have to worry about it.

stephenmathieson

I'm not convinced that we're any more secure by not using npm packages for this. We (or someone else?) could vet any packages we rely on and we could use strict versioning to ensure the vetted version is always installed. We'd have "dependabot" (or alike) warn us if a security concern was found, and the dependency would be "bundled" into axe itself. The way our build works would prevent any additional trips to the npm registry for the end-user.

Can you please clarify the security concern?

If you'd rather not discuss this on your PR, I'm happy to open an issue to hold this conversation.

straker · 2020-07-06T20:39:21Z

lib/standards/css-colors.js

@@ -0,0 +1,147 @@
+// Source: https://www.w3.org/TR/css-color-3/#svg-color
+const cssColors = {


I could be convinced either way. On the one had, theres https://github.com/bahamas10/css-color-names which auto-generates a single JSON list of colors (which is nicer than hand maintaining it). On the other hand, a list like this allows users to add their own so we aren't waiting on new names to work in all the browsers before we add it.

Personally, if we don't care about users being able to customize this list then we should probably just grab it from that npm package so we don't have to worry about it.

straker · 2020-07-06T23:37:02Z

lib/commons/color/color.js

@@ -38,8 +42,30 @@ function Color(red, green, blue, alpha) {
 		);
 	};

-	var rgbRegex = /^rgb\((\d+), (\d+), (\d+)\)$/;
-	var rgbaRegex = /^rgba\((\d+), (\d+), (\d+), (\d*(\.\d+)?)\)/;
+	const rgbRegex = /^rgb\(([0-9.]+),\s*([0-9.]+),\s*([0-9.]+)\)$/i;


Can rgb values be decimals?

You mean floating point number? Yeah, they can. I didn't know that either until I looked into this.

straker · 2020-07-06T23:53:23Z

lib/commons/color/color.js

+	const rgbRegex = /^rgb\(([0-9.]+),\s*([0-9.]+),\s*([0-9.]+)\)$/i;
+	const rgbaRegex = /^rgba\(([0-9.]+),\s*([0-9.]+),\s*([0-9.]+),\s*([0-9.]*(\.[0-9.]+)?)\)$/i;


Not a big deal but we could combine the two different regexs into one. This will match rgb values as well as rgba values and capture all 4 groups.

Suggested change

const rgbRegex = /^rgb$([0-9.]+),\s*([0-9.]+),\s*([0-9.]+)$$/i;

const rgbaRegex = /^rgba$([0-9.]+),\s*([0-9.]+),\s*([0-9.]+),\s*([0-9.]*(\.[0-9.]+)?)$$/i;

const rgbRegex = /^rgba?$([\d.]+)\,\s*([\d.]+),\s*([\d.]+),?(?:$|\s*([\d.]+)\))/

'rgb(12, 14, 1)'.match(rgbRegex); // [rgb(12, 14, 1), 12, 14, 1, undefined] 'rgba(255, 1, 24, 0.8)'.match(rgbRegex); // [rgba(255, 1, 24, 0.8), 255, 1, 24, 0.8]

lib/commons/color/get-text-shadow-colors.js

straker · 2020-07-07T14:53:21Z

lib/commons/color/get-text-shadow-colors.js

+function blurRadiusToAlpha(blurRadius) {
+	// This formula is an estimate based on various tests.
+	// Different people test this differently, so opinions may vary.
+	return 3.7 / (blurRadius + 8);


Could you leave a comment in the code explaining the formula and magic numbers? If we needed to adjust this formula based on user feedback, what would we need to adjust to get the desired results?

Why was this resolved? I wouldn't be able to change this formula if we had to adjust it based on user feedback. Where does 3.7 and blurRadius + 8 come from? I know it was based on testing, if they are truly magic then maybe include the results of those tests so we can better gauge how to adjust this in the future.

Afraid I don't have the data anymore, sorry. These estimates are pretty rough.

test/commons/color/get-text-shadow-colors.js

stephenmathieson

Don't want this to get lost. Are we not worried about hsl() and hsla() colors?

Co-authored-by: Steven Lambert <[email protected]>

Ready for another review, me thinks

straker · 2020-07-17T16:03:50Z

lib/commons/color/get-text-shadow-colors.js

+function blurRadiusToAlpha(blurRadius) {
+	// This formula is an estimate based on various tests.
+	// Different people test this differently, so opinions may vary.
+	return 3.7 / (blurRadius + 8);


Why was this resolved? I wouldn't be able to change this formula if we had to adjust it based on user feedback. Where does 3.7 and blurRadius + 8 come from? I know it was based on testing, if they are truly magic then maybe include the results of those tests so we can better gauge how to adjust this in the future.

straker · 2020-07-17T16:07:56Z

test/integration/full/contrast/code-highlighting.js

@@ -1,5 +1,18 @@
 describe('color-contrast code highlighting test', function() {
 	'use strict';
+	var results;
+
+	before(function(done) {


This will undo the changes I made to fix the flaky test. The before that waits for the window load event needs to happen first before we run axe on the file.

Updated

fix(color-contrast): account for text-shadow

79bad36

WilcoFiers requested a review from a team as a code owner June 30, 2020 11:45

WilcoFiers added 3 commits July 1, 2020 17:51

chore: IE text-shadow issues

4fd6adb

chore: more IE fixes

eedd650

chore: more IE tests

dc13514

stephenmathieson reviewed Jul 1, 2020

View reviewed changes

chore: fix failing tests

98bd942

stephenmathieson reviewed Jul 4, 2020

View reviewed changes

WilcoFiers added 3 commits July 6, 2020 11:58

chore: Merge branch 'develop' into text-shadow

efca19e

chore: fix IE stuff

e895aaf

chore: ci fix

362dd78

straker previously requested changes Jul 7, 2020

View reviewed changes

stephenmathieson reviewed Jul 7, 2020

View reviewed changes

WilcoFiers and others added 2 commits July 8, 2020 12:35

Update lib/commons/color/get-text-shadow-colors.js

0d5de61

Co-authored-by: Steven Lambert <[email protected]>

chore: support hsl colors

7107406

WilcoFiers changed the title ~~fix(color-contrast): account for text-shadow~~ [WIP] fix(color-contrast): account for text-shadow Jul 15, 2020

WilcoFiers mentioned this pull request Jul 15, 2020

feat(color): allow all valid CSS colors #2381

Merged

2 tasks

chore: Merge branch 'develop' into text-shadow

14e8569

WilcoFiers changed the title ~~[WIP] fix(color-contrast): account for text-shadow~~ fix(color-contrast): account for text-shadow Jul 17, 2020

Apply suggestions from code review

306493a

Co-authored-by: Steven Lambert <[email protected]>

WilcoFiers requested a review from straker July 17, 2020 14:37

straker previously requested changes Jul 17, 2020

View reviewed changes

revert code-highlighting.js

6e02807

straker approved these changes Jul 20, 2020

View reviewed changes

WilcoFiers merged commit 3eb6d2c into develop Jul 20, 2020

WilcoFiers deleted the text-shadow branch July 20, 2020 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(color-contrast): account for text-shadow #2334

fix(color-contrast): account for text-shadow #2334

WilcoFiers commented Jun 30, 2020 •

edited by straker

Loading

stephenmathieson left a comment

stephenmathieson Jul 1, 2020

WilcoFiers Jul 3, 2020

stephenmathieson Jul 4, 2020

stephenmathieson Jul 1, 2020

stephenmathieson Jul 1, 2020

WilcoFiers Jul 3, 2020

stephenmathieson Jul 4, 2020

WilcoFiers Jul 6, 2020

straker Jul 6, 2020

stephenmathieson left a comment

straker Jul 6, 2020

straker Jul 6, 2020

WilcoFiers Jul 14, 2020

straker Jul 6, 2020

straker Jul 7, 2020

straker Jul 17, 2020

WilcoFiers Jul 20, 2020

stephenmathieson left a comment

straker Jul 17, 2020

straker Jul 17, 2020

		@@ -0,0 +1,147 @@
		// Source: https://www.w3.org/TR/css-color-3/#svg-color
		const cssColors = {

		const rgbRegex = /^rgb\(([0-9.]+),\s([0-9.]+),\s([0-9.]+)\)$/i;
		const rgbaRegex = /^rgba\(([0-9.]+),\s([0-9.]+),\s([0-9.]+),\s([0-9.](\.[0-9.]+)?)\)$/i;

	const rgbRegex = /^rgb\(([0-9.]+),\s([0-9.]+),\s([0-9.]+)\)$/i;
	const rgbaRegex = /^rgba\(([0-9.]+),\s([0-9.]+),\s([0-9.]+),\s([0-9.](\.[0-9.]+)?)\)$/i;
	const rgbRegex = /^rgba?\(([\d.]+)\,\s([\d.]+),\s([\d.]+),?(?:\)\|\s*([\d.]+)\))/

fix(color-contrast): account for text-shadow #2334

fix(color-contrast): account for text-shadow #2334

Conversation

WilcoFiers commented Jun 30, 2020 • edited by straker Loading

Reviewer checks

stephenmathieson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenmathieson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenmathieson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilcoFiers commented Jun 30, 2020 •

edited by straker

Loading