feat(new-rule): Add WCAG 2.2 target-size rule #3616

WilcoFiers · 2022-08-19T12:32:40Z

Better handle rounding errors, so that the minimum is not 23.95, but actually 24px
Tests for target-offset
Report the offset to the closest neighbour, and indicate the expected offset
Update the selector to ensure all focusable widgets are included -- may need a matcher too
Have a matcher filter out inline controls
Add more integration tests that deal with the selector / matcher
Document the new check options

Closes issue: #2652

WilcoFiers · 2022-09-06T09:10:20Z

lib/rules/widget-not-inline-matches.js

+  if ('option' === role) {
+    return false;
+  } else if (['combobox', 'listbox'].includes(role)) {


For consideration; I could potentially change the role type of these three so that everything we want is a widget, and everything we don't isn't. I didn't explore what the wider implications of that could be.

straker

I'm not done reviewing this, but figured I get this out there now so you could work on the things I did find.

lib/commons/dom/find-nearby-elms.js

lib/commons/math/get-offset.js

straker · 2022-09-06T21:19:09Z

lib/rules/widget-not-inline-matches.js

+  (node, vNode) => isWidgetType(vNode),
+  (node, vNode) => isNotAreaElement(vNode),
+  (node, vNode) => !svgNamespaceMatches(node, vNode),
+  (node, vNode) => isFocusable(vNode),


Should this exclude hidden elements? Currently an element with opacity: 0 will be included and then error in findNearbyElms as it's not in the grid and doesn't have a grid property so const gridCells = vNode._grid.cells; fails.

This is related to my comment about createGrid excluding hidden elements.

Tricky. An element with opacity: 0 is still clickable. All those other on screen visibility methods do prevent focus, but this one doesn't... Also, maybe I need to think about CSS pointer-events, which makes a target non-clickable....

straker · 2022-09-06T21:45:03Z

lib/commons/dom/find-nearby-elms.js

+
+function findNearbyElms(vNode, margin = 0) {
+  /*eslint no-bitwise: 0*/
+  const gridSize = createGrid();


So createGrid was written with color-contrast in mind and so doesn't include visually hidden elements in the grid. However, now that we want to use it for clickable widget elements it creates a bug. Take for example the following code (modified B4 example):

<script>function clicked() { console.log('click') }</script> <span class="failed h2 w3"></span> <span style="opacity: 0" class="failed h2 w3" onclick="clicked()"></span> <span class="failed h2 w3"></span>

Both of the visible spans pass the target-size rule, but the opacity 0 span is still clickable, meaning that it still would cause a problem of clicking the wrong element when too close together. I would imagine this should fail the rule still.

Another question: How should we handle things on-top of these nodes? For example, taking the following code (modified B4 example again):

<style> .foo { width: 400px; height: 400px; position: absolute !important; background: red; top: -5px; z-index: 2; } </style> <div> <div class="foo"></div> <span class="failed h2 w3"></span> <span class="failed h2 w3"></span> <span class="failed h2 w3"></span> </div>

The nodes still fail even though it's not possible to click on them. Same if a modal is currently open. Should we fail these nodes still?

I ended up doing the overlap thing in the target-size check, rather than in the matches, since target-size is already looking at overlapping elements. Much quicker to sort out there.

straker · 2022-09-06T22:11:42Z

lib/commons/dom/is-in-text-block.js


-  return parentText.length > linkText.length;
+function isWidget(domNode) {


This is the second instance of this function (the other being in widget-not-inline-matches.js, but they are slightly different. The widget-not-inline-matches.js only accounts for listbox and combobox composite roles, but this one includes all composite roles. Should this function be pulled out and made a shared function that both use?

Noticed that did you?... You're right. Need to fix that. I'll go find a better solution. I don't quite like how wonky this is with the listbox / combobox exception. I might just change what type we give to them and avoid needing a function at all.

straker · 2022-09-06T22:18:55Z

test/commons/dom/find-nearby-elms.js

+
+  beforeEach(function () {
+    fixture = fixtureSetup(
+      '<div id="n0" style="height:30px; margin-bottom:30px;">0</div>' +


This test fails when debugging in a browser due to a Mocha element.

Should probably set the width of these nodes to something small (like 30px) so it doesn't try to include extraneous elements outside of fixture.

How do I get to this? I couldn't find it. And should we care?

straker

More things (still working my way through)

lib/checks/mobile/target-offset.json

straker · 2022-09-08T22:48:22Z

lib/checks/mobile/target-offset-evaluate.js

+  let closestOffset = minOffset;
+  for (const vNeighbor of findNearbyElms(vNode, minOffset)) {
+    const role = getRole(vNeighbor);
+    if (getRoleType(role) !== 'widget') {


Should this match nodes based on how the matcher does it? So something like:

(node, vNode) => isWidgetType(vNode), (node, vNode) => isNotAreaElement(vNode), (node, vNode) => !svgNamespaceMatches(node, vNode), (node, vNode) => isFocusable(vNode)

This would fix a potential problem where a neighbor button is disabled (modified B4 example again). In this case the first and last button report as violations even though you can't click on the middle button.

<div> <button class="failed h2 w3"></button> <button disabled class="failed h2 w3"></button> <button class="failed h2 w3"></button> </div>

Good point, although not exactly. Disabled buttons should probably be excluded... that's a fair enough point, but I don't think we should exclude inline links for example, or area / SVG widgets. If you put a button too close to your SVG, the button's still the problem, even if axe passes on the SVG.

lib/checks/mobile/target-size.json

straker · 2022-09-08T23:05:27Z

lib/checks/mobile/target-size-evaluate.js

+  const nearbyElms = findNearbyElms(vNode);
+  const obscuringElms = nearbyElms.filter(vNeighbor => {
+    const role = getRole(vNeighbor);
+    return getRoleType(role) === 'widget' && hasVisualOverlap(vNode, vNeighbor);


Same question here about filtering same as the matcher function

straker

Finished

straker · 2022-09-12T15:47:00Z

lib/commons/math/get-offset.js

+    const centerB = rectB[start] + rectB[diameter] / 2;
+    const startDistance = Math.abs(centerB - rectA[start]);
+    const endDistance = Math.abs(centerB - rectA[end]);
+    if (startDistance >= endDistance) {


Should equal distances use the middle of B?

If the distance is the same it doesn't matter.

lib/commons/math/get-offset.js

lib/commons/math/has-visual-overlap.js

straker · 2022-09-12T21:40:51Z

lib/commons/math/has-visual-overlap.js

@@ -0,0 +1,44 @@
+import { visuallySort } from '../dom';


Is this function different than dom.visuallyOverlaps? Would hate to have multiple functions that are named the same do different things

Nope... but this is a good bit confusing. dom.visuallyOverlaps looks that a rect intersects with a particular element. It doesn't check whether A is on top of B, or B is on top of A. Probably dom.visuallyOverlaps should have been called rectIntersectsElement or something like that? Don't think it impacts this PR.

straker · 2022-09-12T21:55:28Z

test/rule-matches/widget-not-inline-matches.js

+    it('returns false for a non-tabbable button (widgets)', function () {
+      var vNode = queryFixture(
+        '<div role="option" tabindex="0" id="target"></div>'
+      );
+      var node = vNode.actualNode;
+      assert.isFalse(rule.matches(node, vNode));
+    });


Is this title correct? Why would this be non-tabbable?

straker · 2022-09-12T21:59:14Z

test/checks/mobile/target-size.js

+  });
+
+  // IE cannot count
+  (isIE11 ? xit : it)(


Can remove this now

lib/checks/mobile/target-size.json

straker · 2022-09-12T22:03:46Z

test/checks/mobile/target-size.js

+    });
+  });
+
+  it('returns true for obscured targets with insufficient space', function () {


Should this be sufficient space? The test below this one has the same title but a different return.

lib/commons/math/has-visual-overlap.js

WilcoFiers · 2022-09-16T11:34:00Z

lib/commons/dom/create-grid.js

+    if (rect.right <= 0 || rect.bottom <= 0) {
+      return;
+    }
+    // save a reference to where this element is in the grid so we
+    // can find it even if it's in a subgrid
+    vNode._grid ??= grid;


IMPORTANT: These lines have been modified to fix a bug where elements outside the viewport could still be added to the grid.

WilcoFiers · 2022-09-20T14:49:21Z

lib/checks/keyboard/no-focusable-content-evaluate.js

-    const role = getRole(child);
-    if (getRoleType(role) === 'widget' && isFocusable(child)) {
+    if (getRoleType(child) === 'widget' && isFocusable(child)) {


If you want me to pull these into their own PR let me know. It's not unrelated to the PR... but it's not very related either.

Done, I think

straker · 2022-09-20T22:26:21Z

lib/commons/aria/get-role-type.js

+  if (
+    role instanceof AbstractVirtualNode ||
+    (window?.Node && role instanceof window.Node)
+  ) {
+    role = axe.commons.aria.getRole(role);


Question: I've noticed that you've lately been wanting more strict type checking and guarding in our APIs. Is this something you want to do throughout the code base for our public API methods?

The simpler check here would be to see if role is not a string and assume it's a vNode / Node. A more strict type check would throw if it's not a type we expect (instead of assuming string as default).

I didn't noticed that. Have I really? I used instanceof here because that's what we usually do when we want to check nodes. There are lots of instanceof AbstractVirtualNode calls.. and a couple instanceof window.Node calls too.

Looking at where/how this function has been used, it's going to get null passed into it a good bit too, since getRole() can return null. I'll update it to reflect that.

straker · 2022-09-20T22:36:56Z

lib/commons/dom/find-nearby-elms.js

+    for (let col = leftCol; col <= rightCol; col++) {
+      // Don't loop on elements outside the grid
+      const length = gridCells[row]?.[col]?.length ?? -1;
+      for (let i = 0; i <= length; i++) {


Suggested change

for (let i = 0; i <= length; i++) {

for (let i = 0; i < length; i++) {

straker · 2022-09-20T22:44:09Z

lib/commons/dom/is-in-text-block.js

 * @return {Boolean}      [description]
 */
-function isInTextBlock(node) {
+function isInTextBlock(node, options) {
+  const { getRoleType } = axe.commons.aria;


Is there a reason you didn't import the function at the top of the file? We're trying to move away from using axe in the codebase.

Ran into this error again: #3281

Looks like I can work around it by importing directly from get-role-type, rather than from aria/index. Doing that.

straker · 2022-09-20T22:51:11Z

lib/checks/mobile/target-offset.json

+  "metadata": {
+    "impact": "serious",
+    "messages": {
+      "pass": "Target has sufficiently offset from its closest neighbor (${data.closestOffset}px should be at least ${data.minOffset}px)",


Suggested change

"pass": "Target has sufficiently offset from its closest neighbor (${data.closestOffset}px should be at least ${data.minOffset}px)",

"pass": "Target has sufficient offset from its closest neighbor (${data.closestOffset}px should be at least ${data.minOffset}px)",

straker · 2022-09-20T23:06:48Z

lib/rules/widget-not-inline-matches.js

-  }
-  const roleType = getRoleType(role);
-  return roleType === 'widget';
+  return getRoleType(vNode) === 'widget';


So this will return true for the option role, is that OK?

They do, but they're pulled out for not being focusable. There's an example that uses options (#pass2)

straker · 2022-09-20T23:10:18Z

lib/checks/mobile/target-size-evaluate.js

+    }
+    if (isEnclosedRect(vNode, vNeighbor)) {
+      this.data({ messageKey: 'obscured' });
+      return true;


Should add the node as a related node

straker · 2022-09-20T23:19:32Z

test/checks/mobile/target-size.js

+          'display: inline-block; width:40px; height:30px; margin-left: -100px;' +
+          '">x</span>'
+      );
+      assert.isTrue(check.evaluate.apply(checkContext, checkArgs));


I feel like this should be false. the span with tabindex=0 captures the click on the left side of the target, making the clickable width effectively 20px.

This is no different than if that was just a narrower button. If someone accidentally hits the span instead of the button, nothing happens. Or at least, we can't assume something is going to happen. If that span is a widget that is missing a role, a different accessibility violation exists in that page, and after they've fixed that (by adding a role to the span) this rule will work correctly.

We could maybe report this as incomplete? I'd prefer we do this later though. This PR is much larger than I want it to be already. Please open an issue for this.

straker · 2022-09-20T23:21:19Z

test/checks/mobile/target-size.js

+          'display: inline-block; width:40px; height:30px; margin-left: -100px;' +
+          '">x</button>'
+      );
+      assert.isTrue(check.evaluate.apply(checkContext, checkArgs));


I feel this should be false as well since the disabled button captures the click on the left side of the target, making the clickable width effectively 20px.

This can be debatable though as the disabled button doesn't cause a misclick to happen.

Same answer. I don't think we can fail this, maybe incomplete, but I'd prefer we leave that for another time.

All set, I think

feat(new-rule): Add WCAG 2.2 target-size rule

7d9b70c

paulirish mentioned this pull request Aug 23, 2022

Replace tap-targets with target-size rule from Axe and put under Accessibility, not SEO GoogleChrome/lighthouse#12345

Closed

WilcoFiers added 4 commits August 31, 2022 10:58

chore: handle rounding errors

54817d7

Fix has-visual-overlap test

80299b2

Fix failing tests?

302db49

Rename target-size.json file

0172fbe

WilcoFiers force-pushed the target-size-v2 branch from c1116b5 to 0172fbe Compare August 31, 2022 10:10

WilcoFiers added 6 commits August 31, 2022 12:31

Disable target-spacing rule on APG

5b1c1a3

Improve target-size messaging

a0eaaf8

Update is-in-text-block

c4029a5

Add target-size matches method

55b037c

Add integration tests for target-size

81feac6

Document target-size check options

eb27049

WilcoFiers marked this pull request as ready for review September 2, 2022 13:08

WilcoFiers requested a review from a team as a code owner September 2, 2022 13:08

WilcoFiers added 5 commits September 2, 2022 15:24

put create-grid into its own file

217ac64

Make all links focusable

10b8972

Simplify target-size full rest

297524f

Try to fix IE issue

a25d7a0

More cleanup and testing

f6b97df

WilcoFiers force-pushed the target-size-v2 branch from b6ad64b to d3902bb Compare September 2, 2022 14:43

Solve oddities with IE

8ab55c7

WilcoFiers force-pushed the target-size-v2 branch from d3902bb to 8ab55c7 Compare September 2, 2022 15:00

WilcoFiers commented Sep 6, 2022

View reviewed changes

straker requested changes Sep 6, 2022

View reviewed changes

straker requested changes Sep 9, 2022

View reviewed changes

straker previously requested changes Sep 12, 2022

View reviewed changes

straker reviewed Sep 12, 2022

View reviewed changes

lib/commons/math/has-visual-overlap.js Outdated Show resolved Hide resolved

WilcoFiers added 2 commits September 15, 2022 12:48

Rename minSpacing option to minSize

496f227

Merge branch 'develop' into target-size-v2

8ea60ec

WilcoFiers added 2 commits September 15, 2022 12:55

Remove IE11 xit

a5a0512

Fix off screen grid issues

368d612

WilcoFiers commented Sep 16, 2022

View reviewed changes

WilcoFiers added 4 commits September 16, 2022 14:28

Simplify using role types

b4462e5

cleanup

9e7e9b6

Handle fully obscuring elements

4fefa21

fix failing test

7885fe8

WilcoFiers commented Sep 20, 2022

View reviewed changes

Resolve last comments

840cf4e

WilcoFiers added 2 commits September 20, 2022 18:13

fix getRoleType throwing in NodeJS context

1e204c1

Editorial test fix

b2f12e8

straker previously requested changes Sep 20, 2022

View reviewed changes

Address review

17ce8ea

straker previously approved these changes Sep 21, 2022

View reviewed changes

Merge branch 'develop' into target-size-v2

d72b683

WilcoFiers dismissed straker’s stale review via d72b683 September 21, 2022 15:57

straker approved these changes Sep 21, 2022

View reviewed changes

WilcoFiers merged commit 691f1b6 into develop Sep 21, 2022

WilcoFiers deleted the target-size-v2 branch September 21, 2022 16:08

adamraine mentioned this pull request Jan 18, 2023

deps: upgrade axe-core to 4.6.3 GoogleChrome/lighthouse#14690

Merged

Shubhabrata08 mentioned this pull request Jan 13, 2024

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

Closed


		return parentText.length > linkText.length;
		function isWidget(domNode) {

	for (let i = 0; i <= length; i++) {
	for (let i = 0; i < length; i++) {

	"pass": "Target has sufficiently offset from its closest neighbor (${data.closestOffset}px should be at least ${data.minOffset}px)",
	"pass": "Target has sufficient offset from its closest neighbor (${data.closestOffset}px should be at least ${data.minOffset}px)",

feat(new-rule): Add WCAG 2.2 target-size rule #3616

feat(new-rule): Add WCAG 2.2 target-size rule #3616

Conversation

WilcoFiers commented Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

straker 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

straker Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilcoFiers Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

straker 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

straker 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

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

WilcoFiers Sep 21, 2022 • edited Loading

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

WilcoFiers commented Aug 19, 2022 •

edited

Loading

straker Sep 6, 2022 •

edited

Loading

WilcoFiers Sep 20, 2022 •

edited

Loading

WilcoFiers Sep 21, 2022 •

edited

Loading