fix(target-size): do not crash for nodes with many overlapping widgets (

#4373) Decided that instead of returning `null`, `splitRects` should return an empty array to signify the bail out state. `splitRects` returns the original node if there is no overlap, so an empty array allows us to still signify the desired state as well as still allow all the code that used it to treat it as an array. Otherwise I would need to propagate a `null` check through 4 different functions in `target-offset-evaluate` (`getOffset`, `getTargetSize`, `getTargetRects`, etc.). In trying to catch the new state in `target-size-evaluate` it became difficult to figure out the logic of all the different things that needed to be checked to know what to return. I decided to refactor the entire function to make the flow easier by eliminating possibilities higher up so the if statements only needed to check a single condition rather than multiple conditions. The two key parts of the refactor was to move the overflow content check to the top. For overflowing content the return would always be undefined unless the target had sufficient size, in which case it would return true. This meant we can check those states first and not have to check for it again, which greatly simplifies all the logic of the if statements. In terms of the magic number for when to exit early, it was based on running performance tests against the code in #4359 and logging how long the inner loop took as `uniqueRects` size grew. Once the time got to ~2 seconds to complete I took that number then reduced it by a factor of 10x for slower machines. ``` uniqueRects: 4 time: 0.10000014305114746 uniqueRects: 7 time: 0 uniqueRects: 10 time: 0 uniqueRects: 13 time: 0 uniqueRects: 16 time: 0 uniqueRects: 19 time: 0.09999990463256836 uniqueRects: 22 time: 0 uniqueRects: 25 time: 0 uniqueRects: 34 time: 0.10000014305114746 uniqueRects: 43 time: 0.09999990463256836 uniqueRects: 55 time: 0.09999990463256836 uniqueRects: 75 time: 0 uniqueRects: 90 time: 0.09999990463256836 uniqueRects: 122 time: 0.09999990463256836 uniqueRects: 140 time: 0.10000014305114746 uniqueRects: 187 time: 0.20000004768371582 uniqueRects: 208 time: 0.09999990463256836 uniqueRects: 273 time: 0.10000014305114746 uniqueRects: 297 time: 0.8999998569488525 uniqueRects: 383 time: 0.2999999523162842 uniqueRects: 410 time: 0.20000004768371582 uniqueRects: 520 time: 0.2999999523162842 uniqueRects: 548 time: 0.2999999523162842 uniqueRects: 683 time: 0.2999999523162842 uniqueRects: 692 time: 0.5 uniqueRects: 720 time: 0.5999999046325684 uniqueRects: 775 time: 0.7000000476837158 uniqueRects: 872 time: 1.1999998092651367 uniqueRects: 1029 time: 0.5999999046325684 uniqueRects: 1267 time: 1 uniqueRects: 1610 time: 1.2000000476837158 uniqueRects: 2083 time: 1.3999998569488525 uniqueRects: 2089 time: 1.8999998569488525 uniqueRects: 2095 time: 1.9000000953674316 uniqueRects: 2101 time: 1.7000000476837158 uniqueRects: 2107 time: 1.6000001430511475 uniqueRects: 2113 time: 1.7999999523162842 uniqueRects: 2119 time: 1.6000001430511475 uniqueRects: 2125 time: 1.7000000476837158 uniqueRects: 2131 time: 1.6000001430511475 uniqueRects: 2164 time: 1.6999998092651367 uniqueRects: 2329 time: 1.7000000476837158 uniqueRects: 2365 time: 1.7000000476837158 uniqueRects: 2565 time: 2 uniqueRects: 2604 time: 2.0999999046325684 uniqueRects: 2840 time: 2.5 uniqueRects: 2882 time: 2.6000001430511475 uniqueRects: 3157 time: 2.5 uniqueRects: 3202 time: 2.5999999046325684 uniqueRects: 3519 time: 2.700000047683716 uniqueRects: 3567 time: 3.0999999046325684 uniqueRects: 3929 time: 3.4000000953674316 uniqueRects: 3980 time: 15.5 uniqueRects: 4390 time: 6.599999904632568 uniqueRects: 4442 time: 4.200000047683716 uniqueRects: 4901 time: 4.299999952316284 uniqueRects: 5534 time: 5.299999952316284 uniqueRects: 6366 time: 6.3999998569488525 uniqueRects: 7429 time: 7.8999998569488525 uniqueRects: 8762 time: 10.600000143051147 uniqueRects: 10407 time: 12.5 uniqueRects: 12409 time: 16.100000143051147 uniqueRects: 14816 time: 21.5 uniqueRects: 17677 time: 30.199999809265137 uniqueRects: 17686 time: 33.200000047683716 uniqueRects: 17695 time: 33.200000047683716 uniqueRects: 17704 time: 34.5 uniqueRects: 17713 time: 34.09999990463257 uniqueRects: 17722 time: 34.299999952316284 uniqueRects: 17731 time: 34.59999990463257 uniqueRects: 17740 time: 34.09999990463257 uniqueRects: 17749 time: 35.5 uniqueRects: 17806 time: 34.89999985694885 uniqueRects: 18319 time: 35.09999990463257 uniqueRects: 18379 time: 36.10000014305115 uniqueRects: 18951 time: 37.200000047683716 uniqueRects: 19014 time: 38.59999990463257 uniqueRects: 19646 time: 39.700000047683716 uniqueRects: 19712 time: 41.40000009536743 uniqueRects: 20407 time: 43.09999990463257 uniqueRects: 20476 time: 43.69999980926514 uniqueRects: 21237 time: 44.5 uniqueRects: 21309 time: 47.200000047683716 uniqueRects: 22139 time: 48.5 uniqueRects: 22214 time: 50.799999952316284 uniqueRects: 23116 time: 51.799999952316284 uniqueRects: 23192 time: 55.299999952316284 uniqueRects: 24167 time: 57 uniqueRects: 27536 time: 66.39999985694885 uniqueRects: 31476 time: 85.89999985694885 uniqueRects: 36043 time: 131.5 uniqueRects: 41300 time: 230.20000004768372 ``` Closes: #4359 Closes: #4360 --------- Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
dequelabs · Mar 18, 2024 · 1dbea83 · 1dbea83
1 parent 0f8a9af
commit 1dbea83
Show file tree

Hide file tree

Showing 9 changed files with 203 additions and 20 deletions.
diff --git a/lib/checks/mobile/target-size-evaluate.js b/lib/checks/mobile/target-size-evaluate.js
@@ -10,7 +10,7 @@ import {
  * Determine if an element has a minimum size, taking into account
  * any elements that may obscure it.
  */
-export default function targetSize(node, options, vNode) {
+export default function targetSizeEvaluate(node, options, vNode) {
   const minSize = options?.minSize || 24;
   const nodeRect = vNode.boundingClientRect;
   const hasMinimumSize = rectHasMinimumSize.bind(null, minSize);
@@ -21,8 +21,20 @@ export default function targetSize(node, options, vNode) {
     nearbyElms
   );
 
+  // Target has overflowing content;
+  //   and is either not fully obscured (so may not pass),
+  //   or has insufficient space (and so may not fail)
+  if (
+    overflowingContent.length &&
+    (fullyObscuringElms.length || !hasMinimumSize(nodeRect))
+  ) {
+    this.data({ minSize, messageKey: 'contentOverflow' });
+    this.relatedNodes(mapActualNodes(overflowingContent));
+    return undefined;
+  }
+
   // Target is fully obscured and no overflowing content (which may not be obscured)
-  if (fullyObscuringElms.length && !overflowingContent.length) {
+  if (fullyObscuringElms.length) {
     this.relatedNodes(mapActualNodes(fullyObscuringElms));
     this.data({ messageKey: 'obscured' });
     return true;
@@ -32,31 +44,34 @@ export default function targetSize(node, options, vNode) {
   const negativeOutcome = isInTabOrder(vNode) ? false : undefined;
 
   // Target is too small, and has no overflowing content that increases the size
-  if (!hasMinimumSize(nodeRect) && !overflowingContent.length) {
+  if (!hasMinimumSize(nodeRect)) {
     this.data({ minSize, ...toDecimalSize(nodeRect) });
     return negativeOutcome;
   }
 
   // Figure out the largest space on the target, not obscured by other widgets
   const obscuredWidgets = filterFocusableWidgets(partialObscuringElms);
+
+  // Target not obscured and has sufficient space
+  if (!obscuredWidgets.length) {
+    this.data({ minSize, ...toDecimalSize(nodeRect) });
+    return true;
+  }
+
   const largestInnerRect = getLargestUnobscuredArea(vNode, obscuredWidgets);
+  if (!largestInnerRect) {
+    this.data({ minSize, messageKey: 'tooManyRects' });
+    return undefined;
+  }
 
-  // Target has overflowing content;
-  //   and is either not fully obscured (so may not pass),
-  //   or has insufficient space (and so may not fail)
-  if (overflowingContent.length) {
-    if (
-      fullyObscuringElms.length ||
-      !hasMinimumSize(largestInnerRect || nodeRect)
-    ) {
+  // Target is obscured, and insufficient space is left
+  if (!hasMinimumSize(largestInnerRect)) {
+    if (overflowingContent.length) {
       this.data({ minSize, messageKey: 'contentOverflow' });
       this.relatedNodes(mapActualNodes(overflowingContent));
       return undefined;
     }
-  }
 
-  // Target is obscured, and insufficient space is left
-  if (obscuredWidgets.length !== 0 && !hasMinimumSize(largestInnerRect)) {
     const allTabbable = obscuredWidgets.every(isInTabOrder);
     const messageKey = `partiallyObscured${allTabbable ? '' : 'NonTabbable'}`;
 
@@ -65,7 +80,7 @@ export default function targetSize(node, options, vNode) {
     return allTabbable ? negativeOutcome : undefined;
   }
 
-  // Target not obscured, or has sufficient space
+  // Target has sufficient space
   this.data({ minSize, ...toDecimalSize(largestInnerRect || nodeRect) });
   this.relatedNodes(mapActualNodes(obscuredWidgets));
   return true;
@@ -106,13 +121,14 @@ function filterByElmsOverlap(vNode, nearbyElms) {
 // Find areas of the target that are not obscured
 function getLargestUnobscuredArea(vNode, obscuredNodes) {
   const nodeRect = vNode.boundingClientRect;
-  if (obscuredNodes.length === 0) {
-    return null;
-  }
   const obscuringRects = obscuredNodes.map(
     ({ boundingClientRect: rect }) => rect
   );
   const unobscuredRects = splitRects(nodeRect, obscuringRects);
+  if (unobscuredRects.length === 0) {
+    return null;
+  }
+
   // Of the unobscured inner rects, work out the largest
   return getLargestRect(unobscuredRects);
 }

diff --git a/lib/checks/mobile/target-size.json b/lib/checks/mobile/target-size.json
@@ -19,7 +19,8 @@
         "default": "Element with negative tabindex has insufficient size (${data.width}px by ${data.height}px, should be at least ${data.minSize}px by ${data.minSize}px). Is this a target?",
         "contentOverflow": "Element size could not be accurately determined due to overflow content",
         "partiallyObscured": "Element with negative tabindex has insufficient size because it is partially obscured (smallest space is ${data.width}px by ${data.height}px, should be at least ${data.minSize}px by ${data.minSize}px). Is this a target?",
-        "partiallyObscuredNonTabbable": "Target has insufficient size because it is partially obscured by a neighbor with negative tabindex (smallest space is ${data.width}px by ${data.height}px, should be at least ${data.minSize}px by ${data.minSize}px). Is the neighbor a target?"
+        "partiallyObscuredNonTabbable": "Target has insufficient size because it is partially obscured by a neighbor with negative tabindex (smallest space is ${data.width}px by ${data.height}px, should be at least ${data.minSize}px by ${data.minSize}px). Is the neighbor a target?",
+        "tooManyRects": "Could not get the target size because there are too many overlapping elements"
       }
     }
   }

diff --git a/lib/commons/math/split-rects.js b/lib/commons/math/split-rects.js
@@ -13,6 +13,13 @@ export default function splitRects(outerRect, overlapRects) {
     uniqueRects = uniqueRects.reduce((rects, inputRect) => {
       return rects.concat(splitRect(inputRect, overlapRect));
     }, []);
+
+    // exit early if we get too many rects that it starts causing
+    // a performance bottleneck
+    // @see https://github.com/dequelabs/axe-core/issues/4359
+    if (uniqueRects.length > 4000) {
+      return [];
+    }
   }
   return uniqueRects;
 }

diff --git a/locales/_template.json b/locales/_template.json
@@ -885,7 +885,8 @@
         "default": "Element with negative tabindex has insufficient size (${data.width}px by ${data.height}px, should be at least ${data.minSize}px by ${data.minSize}px). Is this a target?",
         "contentOverflow": "Element size could not be accurately determined due to overflow content",
         "partiallyObscured": "Element with negative tabindex has insufficient size because it is partially obscured (smallest space is ${data.width}px by ${data.height}px, should be at least ${data.minSize}px by ${data.minSize}px). Is this a target?",
-        "partiallyObscuredNonTabbable": "Target has insufficient size because it is partially obscured by a neighbor with negative tabindex (smallest space is ${data.width}px by ${data.height}px, should be at least ${data.minSize}px by ${data.minSize}px). Is the neighbor a target?"
+        "partiallyObscuredNonTabbable": "Target has insufficient size because it is partially obscured by a neighbor with negative tabindex (smallest space is ${data.width}px by ${data.height}px, should be at least ${data.minSize}px by ${data.minSize}px). Is the neighbor a target?",
+        "tooManyRects": "Could not get the target size because there are too many overlapping elements"
       }
     },
     "header-present": {

diff --git a/test/checks/mobile/target-offset.js b/test/checks/mobile/target-offset.js
@@ -120,6 +120,36 @@ describe('target-offset tests', () => {
     assert.deepEqual(relatedIds, ['#left', '#right']);
   });
 
+  it('returns false if there are too many focusable widgets', () => {
+    let html = '';
+    for (let i = 0; i < 100; i++) {
+      html += `
+        <tr>
+          <td><a href="#">Hello</a></td>
+          <td>Hello</td>
+          <td>Hello</td>
+          <td>Hello</td>
+          <td>Hello</td>
+          <td>Hello</td>
+          <td>Hello</td>
+          <td><button>view</button></td>
+          <td><button>download</button></td>
+          <td><button>expand</button></td>
+        </tr>
+      `;
+    }
+    const checkArgs = checkSetup(`
+      <div id="target" role="tabpanel" tabindex="0">
+        <table id="tab-table">${html}</table>
+      </div>
+    `);
+    assert.isFalse(checkEvaluate.apply(checkContext, checkArgs));
+    assert.deepEqual(checkContext._data, {
+      closestOffset: 0,
+      minOffset: 24
+    });
+  });
+
   describe('when neighbors are focusable but not tabbable', () => {
     it('returns undefined if all neighbors are not tabbable', () => {
       const checkArgs = checkSetup(

diff --git a/test/checks/mobile/target-size.js b/test/checks/mobile/target-size.js
@@ -167,6 +167,36 @@ describe('target-size tests', function () {
         assert.deepEqual(elmIds(checkContext._relatedNodes), ['#obscurer']);
       });
 
+      it('returns undefined if there are too many focusable widgets', () => {
+        let html = '';
+        for (let i = 0; i < 100; i++) {
+          html += `
+            <tr>
+              <td><a href="#">Hello</a></td>
+              <td>Hello</td>
+              <td>Hello</td>
+              <td>Hello</td>
+              <td>Hello</td>
+              <td>Hello</td>
+              <td>Hello</td>
+              <td><button>view</button></td>
+              <td><button>download</button></td>
+              <td><button>expand</button></td>
+            </tr>
+          `;
+        }
+        const checkArgs = checkSetup(`
+          <div id="target" role="tabpanel" tabindex="0">
+            <table id="tab-table">${html}</table>
+          </div>
+        `);
+        assert.isUndefined(check.evaluate.apply(checkContext, checkArgs));
+        assert.deepEqual(checkContext._data, {
+          messageKey: 'tooManyRects',
+          minSize: 24
+        });
+      });
+
       describe('for obscured targets with insufficient space', () => {
         it('returns false if all elements are tabbable', function () {
           var checkArgs = checkSetup(

diff --git a/test/commons/math/split-rects.js b/test/commons/math/split-rects.js
@@ -16,6 +16,15 @@ describe('splitRects', () => {
     assert.deepEqual(rects[0], rectA);
   });
 
+  it('returns empty array 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);
+  });
+
   describe('with one overlapping rect', () => {
     it('returns one rect if overlaps covers two corners', () => {
       const rectA = new DOMRect(0, 0, 100, 50);

diff --git a/test/integration/full/target-size/too-many-rects.html b/test/integration/full/target-size/too-many-rects.html
@@ -0,0 +1,53 @@
+<!doctype html>
+<html lang="en">
+  <head>
+    <title>Target-size too many rects Test</title>
+    <link
+      rel="stylesheet"
+      type="text/css"
+      href="/node_modules/mocha/mocha.css"
+    />
+    <script src="/node_modules/mocha/mocha.js"></script>
+    <script src="/node_modules/chai/chai.js"></script>
+    <script src="/axe.js"></script>
+    <script>
+      mocha.setup({
+        timeout: 10000,
+        ui: 'bdd'
+      });
+      var assert = chai.assert;
+    </script>
+  </head>
+  <body>
+    <main id="mocha"></main>
+
+    <section id="incomplete">
+      <div id="incomplete-many-rects" role="tabpanel" tabindex="0">
+        <table id="tab-table"></table>
+      </div>
+    </section>
+
+    <script>
+      let html = '';
+      for (let i = 0; i < 100; i++) {
+        html += `
+        <tr>
+          <td><a href="#">Hello</a></td>
+          <td>Hello</td>
+          <td>Hello</td>
+          <td>Hello</td>
+          <td>Hello</td>
+          <td>Hello</td>
+          <td>Hello</td>
+          <td><button>view</button></td>
+          <td><button>download</button></td>
+          <td><button>expand</button></td>
+        </tr>`;
+      }
+      document.querySelector('#tab-table').innerHTML = html;
+    </script>
+    <script src="/test/testutils.js"></script>
+    <script src="too-many-rects.js"></script>
+    <script src="/test/integration/adapter.js"></script>
+  </body>
+</html>
diff --git a/test/integration/full/target-size/too-many-rects.js b/test/integration/full/target-size/too-many-rects.js
@@ -0,0 +1,36 @@
+describe('target-size too many rects test', () => {
+  'use strict';
+  let results;
+
+  before(done => {
+    axe.testUtils.awaitNestedLoad(() => {
+      const options = {
+        runOnly: ['target-size'],
+        elementRef: true
+      };
+      const context = {
+        // ignore the incomplete table results
+        exclude: 'table tr'
+      };
+      axe.run(context, options, (err, r) => {
+        if (err) {
+          done(err);
+        }
+        results = r;
+        console.log(results);
+        done();
+      });
+    });
+  });
+
+  it('incompletes with too many rects', () => {
+    const incompleteNodes = results.incomplete[0]
+      ? results.incomplete[0].nodes
+      : [];
+    assert.lengthOf(incompleteNodes, 1);
+    assert.include(
+      incompleteNodes[0].element,
+      document.querySelector('#incomplete-many-rects')
+    );
+  });
+});