fix(target-size): pass for element that has nearby elements that are obscured (#4422)

Had to update how we handled the too many rects break early since it
would return an empty array, which when looking at the lengths of the
arrays in `getOffset` made it difficult to know which case needed to be
handled (returned empty due to too many rects or returned empty because
there wasn't any visible rect). Talked to Wilco and we agreed that when
we encountered too many rects we could throw and handle the error case
in both checks.

Closes: #4387

Author: straker

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

@@ -20,8 +20,27 @@ export default function targetOffsetEvaluate(node, options, vNode) {
       continue;
     }
     // the offset code works off radius but we want our messaging to reflect diameter
-    const offset =
-      roundToSingleDecimal(getOffset(vNode, vNeighbor, minOffset / 2)) * 2;
+    let offset = null;
+    try {
+      offset = getOffset(vNode, vNeighbor, minOffset / 2);
+    } catch (err) {
+      if (err.message.startsWith('splitRects')) {
+        this.data({
+          messageKey: 'tooManyRects',
+          closestOffset: 0,
+          minOffset
+        });
+        return undefined;
+      }
+
+      throw err;
+    }
+
+    if (offset === null) {
+      continue;
+    }
+
+    offset = roundToSingleDecimal(offset) * 2;
     if (offset + roundingMargin >= minOffset) {
       continue;
     }

lib/checks/mobile/target-offset.json

@@ -14,7 +14,8 @@
       "fail": "Target has insufficient space to its closest neighbors. Safe clickable space has a diameter of ${data.closestOffset}px instead of at least ${data.minOffset}px.",
       "incomplete": {
         "default": "Element with negative tabindex has insufficient space to its closest neighbors. Safe clickable space has a diameter of ${data.closestOffset}px instead of at least ${data.minOffset}px. Is this a target?",
-        "nonTabbableNeighbor": "Target has insufficient space to its closest neighbors. Safe clickable space has a diameter of ${data.closestOffset}px instead of at least ${data.minOffset}px. Is the neighbor a target?"
+        "nonTabbableNeighbor": "Target has insufficient space to its closest neighbors. Safe clickable space has a diameter of ${data.closestOffset}px instead of at least ${data.minOffset}px. Is the neighbor a target?",
+        "tooManyRects": "Could not get the target size because there are too many overlapping elements"
       }
     }
   }

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

@@ -131,8 +131,10 @@ function getLargestUnobscuredArea(vNode, obscuredNodes) {
   const obscuringRects = obscuredNodes.map(
     ({ boundingClientRect: rect }) => rect
   );
-  const unobscuredRects = splitRects(nodeRect, obscuringRects);
-  if (unobscuredRects.length === 0) {
+  let unobscuredRects;
+  try {
+    unobscuredRects = splitRects(nodeRect, obscuringRects);
+  } catch (err) {
     return null;
   }
 

lib/commons/math/get-offset.js

@@ -18,7 +18,7 @@ export default function getOffset(vTarget, vNeighbor, minRadiusNeighbour = 12) {
   const neighborRects = getTargetRects(vNeighbor);
 
   if (!targetRects.length || !neighborRects.length) {
-    return 0;
+    return null;
   }
 
   const targetBoundingBox = targetRects.reduce(getBoundingRect);

lib/commons/math/split-rects.js

@@ -18,7 +18,7 @@ export default function splitRects(outerRect, overlapRects) {
     // a performance bottleneck
     // @see https://github.com/dequelabs/axe-core/issues/4359
     if (uniqueRects.length > 4000) {
-      return [];
+      throw new Error('splitRects: Too many rects');
     }
   }
   return uniqueRects;

locales/_template.json

@@ -872,7 +872,8 @@
       "fail": "Target has insufficient space to its closest neighbors. Safe clickable space has a diameter of ${data.closestOffset}px instead of at least ${data.minOffset}px.",
       "incomplete": {
         "default": "Element with negative tabindex has insufficient space to its closest neighbors. Safe clickable space has a diameter of ${data.closestOffset}px instead of at least ${data.minOffset}px. Is this a target?",
-        "nonTabbableNeighbor": "Target has insufficient space to its closest neighbors. Safe clickable space has a diameter of ${data.closestOffset}px instead of at least ${data.minOffset}px. Is the neighbor a target?"
+        "nonTabbableNeighbor": "Target has insufficient space to its closest neighbors. Safe clickable space has a diameter of ${data.closestOffset}px instead of at least ${data.minOffset}px. Is the neighbor a target?",
+        "tooManyRects": "Could not get the target size because there are too many overlapping elements"
       }
     },
     "target-size": {

test/checks/mobile/target-offset.js

@@ -98,6 +98,21 @@ describe('target-offset tests', () => {
     assert.closeTo(checkContext._data.closestOffset, 24, 0.2);
   });
 
+  it('ignores obscured widget elements as neighbors', () => {
+    const checkArgs = checkSetup(`
+      <div style="position: fixed; bottom: 0">
+        <a href="#">Go to top</a>
+      </div>
+      <div id="target" style="position: fixed; bottom: 0; left: 0; right: 0; background: #eee">
+        Cookies: <a href="#">Accept all cookies</a>
+      </div>
+    `);
+
+    assert.isTrue(checkEvaluate.apply(checkContext, checkArgs));
+    assert.equal(checkContext._data.minOffset, 24);
+    assert.closeTo(checkContext._data.closestOffset, 24, 0.2);
+  });
+
   it('sets all elements that are too close as related nodes', () => {
     const checkArgs = checkSetup(
       '<a href="#" id="left" style="' +
@@ -120,7 +135,7 @@ describe('target-offset tests', () => {
     assert.deepEqual(relatedIds, ['#left', '#right']);
   });
 
-  it('returns false if there are too many focusable widgets', () => {
+  it('returns undefined if there are too many focusable widgets', () => {
     let html = '';
     for (let i = 0; i < 100; i++) {
       html += `
@@ -137,8 +152,9 @@ describe('target-offset tests', () => {
         <table id="tab-table">${html}</table>
       </div>
     `);
-    assert.isFalse(checkEvaluate.apply(checkContext, checkArgs));
+    assert.isUndefined(checkEvaluate.apply(checkContext, checkArgs));
     assert.deepEqual(checkContext._data, {
+      messageKey: 'tooManyRects',
       closestOffset: 0,
       minOffset: 24
     });

test/commons/math/get-offset.js

@@ -33,24 +33,46 @@ describe('getOffset', () => {
     assert.closeTo(getOffset(nodeA, nodeB), 63.6, round);
   });
 
-  it('returns 0 if nodeA is overlapped by nodeB', () => {
+  it('returns null if nodeA is overlapped by nodeB', () => {
     const fixture = fixtureSetup(`
       <button style="width: 10px; height: 10px; margin: 0; padding: 0; position: absolute; top: 0; left: 0">&nbsp;</button>
       <button style="width: 30px; height: 30px; margin: 0; padding: 0; position: absolute; top: -10px; left: -10px">&nbsp;</button>
     `);
     const nodeA = fixture.children[1];
     const nodeB = fixture.children[3];
-    assert.equal(getOffset(nodeA, nodeB), 0);
+    assert.isNull(getOffset(nodeA, nodeB));
   });
 
-  it('returns 0 if nodeB is overlapped by nodeA', () => {
+  it('returns null if nodeB is overlapped by nodeA', () => {
     const fixture = fixtureSetup(`
       <button style="width: 10px; height: 10px; margin: 0; padding: 0; position: absolute; top: 0; left: 0">&nbsp;</button>
       <button style="width: 30px; height: 30px; margin: 0; padding: 0; position: absolute; top: -10px; left: -10px">&nbsp;</button>
     `);
     const nodeA = fixture.children[3];
     const nodeB = fixture.children[1];
-    assert.equal(getOffset(nodeA, nodeB), 0);
+    assert.isNull(getOffset(nodeA, nodeB));
+  });
+
+  it('returns null if nodeA is overlapped by another node', () => {
+    const fixture = fixtureSetup(`
+      <button style="width: 10px; height: 10px; margin: 0; padding: 0; position: absolute; top: 0; left: 0">&nbsp;</button>
+      <button style="width: 30px; height: 30px; margin: 0; padding: 0; position: absolute; top: 0; left: 20px">&nbsp;</button>
+      <div style="background: white; width: 30px; height: 30px; margin: 0; padding: 0; position: absolute; top: -10px; left: -10px">&nbsp;</div>
+    `);
+    const nodeB = fixture.children[1];
+    const nodeA = fixture.children[3];
+    assert.isNull(getOffset(nodeA, nodeB));
+  });
+
+  it('returns null if nodeB is overlapped by another node', () => {
+    const fixture = fixtureSetup(`
+      <button style="width: 10px; height: 10px; margin: 0; padding: 0; position: absolute; top: 0; left: 0">&nbsp;</button>
+      <button style="width: 30px; height: 30px; margin: 0; padding: 0; position: absolute; top: 0; left: 20px">&nbsp;</button>
+      <div style="background: white; width: 30px; height: 30px; margin: 0; padding: 0; position: absolute; top: -10px; left: -10px">&nbsp;</div>
+    `);
+    const nodeA = fixture.children[3];
+    const nodeB = fixture.children[1];
+    assert.isNull(getOffset(nodeA, nodeB));
   });
 
   it('subtracts minNeighbourRadius from center-to-center calculations', () => {

test/commons/math/split-rects.js

@@ -16,13 +16,16 @@ describe('splitRects', () => {
     assert.deepEqual(rects[0], rectA);
   });
 
-  it('returns empty array if there are too many overlapping rects', () => {
+  it('throws if there are too many overlapping rects', () => {
     const rects = [];
     for (let i = 0; i < 100; i++) {
       rects.push(new DOMRect(i, i, 50, 50));
     }
     const rectA = new DOMRect(0, 0, 1000, 1000);
-    assert.lengthOf(splitRects(rectA, rects), 0);
+
+    assert.throws(() => {
+      splitRects(rectA, rects);
+    }, 'splitRects: Too many rects');
   });
 
   describe('with one overlapping rect', () => {

test/integration/full/target-size/too-many-rects.js

@@ -9,6 +9,7 @@ describe('target-size too many rects test', () => {
         elementRef: true
       };
       const context = {
+        include: '#incomplete',
         // ignore the incomplete table results
         exclude: 'table tr'
       };