fix(link-in-text-block): Update rule to match current guidance, revise tests #3575

DaveTryon · 2022-07-28T17:29:05Z

This PR updates the link-in-text-block rule as described in #2817. The new rule converts a single check into 2 checks (link-in-text-block-style and link-in-text-block), as suggested. We use any, so the rule passes if either rule passes.

link-in-text-block-style check:

return false if the link is not actually a link (unchanged from before)
return true if the link and the containing text block have visually distinct styling independent of color

link-in-text-block check:

return false if the link is not actually a link (unchanged from before)
return true if either the foreground contrast ratio or the background contrast ratio is at least 3.0
return undefined if we couldn't get enough data to make a good decision. Possible values of messageKey:
- default if we fail to obtain the foreground color of either the link or the containing text block
- bgContrast if we fail to obtain background colors of either the link or the containing text block and no value for bgColor was set via incompleteData
- bgImage, bgGradient, imgNode, or bgOverlap if a value for bgColor was set via incompleteData when trying to obtain a background color
return false with an appropriate value for messageKey:
- bgContrast if the foreground color is unchanged and the background color changed
- fgContrast if the foreground color has changed or if the background color is unchanged

Strings in the json file have been updated to include more information about the colors that were used to compute the ratios. Unit tests and integration tests have been updated (and slightly regrouped) to pin the updated behavior.

This covers the rule changes for #2817, but it leaves the rule categorized as experimental. Changing the rule category will need a separate change.

Co-authored-by: Madalyn <[email protected]>

straker · 2022-07-28T21:30:02Z

Thanks for the pr! We'll need @WilcoFiers to look over it, which may take awhile as he is out.

WilcoFiers

We'll need integration tests for this.

lib/checks/color/link-in-text-block-evaluate.js

lib/checks/color/link-in-text-block.json

WilcoFiers · 2022-08-09T10:23:56Z

lib/checks/color/link-in-text-block-evaluate.js

+  // Message reflects the highest contrast ratio since it's the smallest fix
+  if (textContrast >= backgroundContrast) {


I see where you're coming from, but I feel this may not be the right direction. For a link with 1. no underline, and 2. bad contrast, this is going to end up telling it to improve the contrast. I think what might be better is that we pull the contrast testing into a separate check, and make those "any" checks, so we can report it like so:

Fix one of the following:

The link has has no styling (such as underline) to distinguish it from the surrounding text

The link text insufficient contrast of 2:1 with the surrounding text. (Minimum contrast 3:1, link text: #ABC123, surrounding text: #CDE234)

For the background, I think that can be combined with the text color check. People don't use backgrounds on inline links all too often, we don't have to explicitly call it out unless there is a background on the link IMO.

Alternatively, we could do the background and foreground thing in the same check, and report whichever has the highest number like you're doing here. Changing background colors on inline links is rare, so it may not be necessary to point that option out unless someone is already using this. Might help on performance a bit too.

Re performance; When splitting this check it'll be good to do some performance testing, make sure we don't significantly slow down this rule. Color contrast can be expensive after all. If it does slow down we'll probably have to look at doing some more caching / memoization.

Refactored to use any. I'll reach out to you offline for details on how you generally handle perf testing

WilcoFiers · 2022-08-10T15:19:47Z

@DaveTryon Is this ready again?

madalynrose · 2022-08-10T16:36:03Z

lib/checks/color/link-in-text-block.json

  "metadata": {
    "impact": "serious",
    "messages": {
      "pass": "Links can be distinguished from surrounding text in some way other than by color",
-      "fail": "Links need to be distinguished from surrounding text in some way other than by color",
+      "fail": {
+        "fgContrast": "The link has insufficient color contrast of ${data.contrastRatio}:1 with the surrounding text. (Minimum contrast is ${data.requiredContrastRatio}:1, link text: ${data.nodeColor}, surrounding text: ${data.parentColor})",


A thought that just occurred to me is that these messages mention color contrast when what we're actually analyzing is relative luminance contrast. Worth capturing in the error message to avoid confusion?

Color contrast is a better known term here. It'll e better understood. It means more or less the same thing anyway.

DaveTryon · 2022-08-10T22:04:38Z

test/integration/rules/link-in-text-block/link-in-text-block.html

+<p style="color: black; background-image: linear-gradient(red, yellow)">
+  paragraph of text (incomplete: link has insufficient text contrast and
+  paragraphs has a background gradient)
+  <a
+    style="text-decoration: none; color: black"
+    href="#"
+    id="incomplete-low-contrast-parent-has-gradient"
+  >
+    Link test</a
+  >
 </p>


@WilcoFiers, I added a local integration test for this where the linear-gradient style is on the link instead of the paragraph. The browser rendered it correctly, but the rule doesn't return undefined. Debugging showed that the call to elementHasImage is returning false. That seems strange to me, but I didn't try to fix it in this PR. Just for reference, here's the change that I made to this test:

<p style="color: black"> paragraph of text (incomplete: link has insufficient text contrast and paragraphs has a background gradient) <a style="text-decoration: none; color: black; background-image: linear-gradient(red, yellow)" href="#" id="incomplete-low-contrast-link-has-gradient" > Link test</a > </p>

DaveTryon · 2022-08-10T22:07:12Z

@DaveTryon Is this ready again?

@WilcoFiers: Yes, please take another pass. I left a comment of some behavior that I didn't expect in the integration tests

checking again

WilcoFiers

Looks great. Apologies for the delayed review. I've been kind of swamped by WCAG 2.2 / 3.0 work the last couple weeks. I think we're almost ready to merge this in.

lib/checks/color/link-in-text-block-style-evaluate.js

WilcoFiers · 2022-08-25T11:45:15Z

lib/checks/color/link-in-text-block.json

  "metadata": {
    "impact": "serious",
    "messages": {
      "pass": "Links can be distinguished from surrounding text in some way other than by color",
-      "fail": "Links need to be distinguished from surrounding text in some way other than by color",
+      "fail": {
+        "fgContrast": "The link has insufficient color contrast of ${data.contrastRatio}:1 with the surrounding text. (Minimum contrast is ${data.requiredContrastRatio}:1, link text: ${data.nodeColor}, surrounding text: ${data.parentColor})",


Color contrast is a better known term here. It'll e better understood. It means more or less the same thing anyway.

lib/checks/color/link-in-text-block-evaluate.js

Co-authored-by: Wilco Fiers <[email protected]>

WilcoFiers

Reviewed for security. Thanks @DaveTryon!

DaveTryon and others added 5 commits July 27, 2022 17:13

fix: Update link-in-text-block + unit tests

9d3b006

Remove unintended change

c993011

Reword message

1cb1ea0

Co-authored-by: Madalyn <[email protected]>

Reword message

58f17ef

Co-authored-by: Madalyn <[email protected]>

Update templates after updating messages

f06c140

DaveTryon requested a review from a team as a code owner July 28, 2022 17:29

straker assigned WilcoFiers Jul 28, 2022

WilcoFiers previously requested changes Aug 9, 2022

View reviewed changes

DaveTryon added 5 commits August 9, 2022 10:40

Merge branch 'develop' into address-2817

36de55d

Update templates (npm run build)

06b35f5

Split link-in-text-block into 2 checks

ea6090f

Use a check option for the required contrast ratio

ec5b15b

Prefer foreground changes over background changes

79b509f

DaveTryon dismissed a stale review via 79b509f August 9, 2022 23:48

WilcoFiers self-requested a review August 10, 2022 15:19

madalynrose reviewed Aug 10, 2022

View reviewed changes

DaveTryon added 2 commits August 10, 2022 10:00

Reduce noise in _templates.json

136e64b

Update integration tests

022fa5a

DaveTryon changed the title ~~fix: Update link-in-text-block and associated unit tests~~ fix(link-in-text-block): Update rule to match current guidance, revise tests Aug 10, 2022

Add an incomplete test

adc5d0c

DaveTryon commented Aug 10, 2022

View reviewed changes

fix typo

d382c2e

WilcoFiers requested changes Aug 25, 2022

View reviewed changes

DaveTryon and others added 3 commits September 9, 2022 09:09

Apply 2 suggestions from code review

9a7f2f3

Co-authored-by: Wilco Fiers <[email protected]>

Fix whitespace

0e977cb

Exit loop early if parentBlock comes back as undefined

747129a

DaveTryon requested a review from WilcoFiers September 9, 2022 16:19

WilcoFiers approved these changes Sep 13, 2022

View reviewed changes

WilcoFiers merged commit edb88ed into dequelabs:develop Sep 13, 2022

straker mentioned this pull request Oct 17, 2022

Restore link-in-text-block integration tests #2827

Closed

Shubhabrata08 mentioned this pull request Jan 13, 2024

Investigate the accessibility rules configured in the jest-axe library. microsoft/fluentui-charting-contrib#28

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(link-in-text-block): Update rule to match current guidance, revise tests #3575

fix(link-in-text-block): Update rule to match current guidance, revise tests #3575

DaveTryon commented Jul 28, 2022 •

edited

Loading

straker commented Jul 28, 2022

WilcoFiers left a comment

WilcoFiers Aug 9, 2022

DaveTryon Aug 10, 2022

WilcoFiers commented Aug 10, 2022

madalynrose Aug 10, 2022

WilcoFiers Aug 25, 2022

DaveTryon Aug 10, 2022 •

edited

Loading

DaveTryon commented Aug 10, 2022

WilcoFiers left a comment

WilcoFiers Aug 25, 2022

WilcoFiers left a comment

		// Message reflects the highest contrast ratio since it's the smallest fix
		if (textContrast >= backgroundContrast) {

fix(link-in-text-block): Update rule to match current guidance, revise tests #3575

fix(link-in-text-block): Update rule to match current guidance, revise tests #3575

Conversation

DaveTryon commented Jul 28, 2022 • edited Loading

straker commented Jul 28, 2022

WilcoFiers left a comment

Choose a reason for hiding this comment

WilcoFiers Aug 9, 2022

Choose a reason for hiding this comment

DaveTryon Aug 10, 2022

Choose a reason for hiding this comment

WilcoFiers commented Aug 10, 2022

madalynrose Aug 10, 2022

Choose a reason for hiding this comment

WilcoFiers Aug 25, 2022

Choose a reason for hiding this comment

DaveTryon Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

DaveTryon commented Aug 10, 2022

WilcoFiers left a comment

Choose a reason for hiding this comment

WilcoFiers Aug 25, 2022

Choose a reason for hiding this comment

WilcoFiers left a comment

Choose a reason for hiding this comment

DaveTryon commented Jul 28, 2022 •

edited

Loading

DaveTryon Aug 10, 2022 •

edited

Loading