From 616a1a2cdcab56be9ae336d3a2c0141e7576509d Mon Sep 17 00:00:00 2001 From: Steven Lambert <2433219+straker@users.noreply.github.com> Date: Mon, 22 Apr 2024 14:39:17 -0600 Subject: [PATCH 1/4] fix(target-size): pass for element that is nearby elements that are obscured --- lib/checks/mobile/target-offset-evaluate.js | 28 ++++++++++++++----- lib/checks/mobile/target-offset.json | 3 ++- lib/checks/mobile/target-size-evaluate.js | 15 ++++++----- lib/commons/math/get-offset.js | 2 +- lib/commons/math/split-rects.js | 2 +- locales/_template.json | 3 ++- test/checks/mobile/target-offset.js | 5 ++-- test/commons/math/get-offset.js | 30 ++++++++++++++++++--- test/commons/math/split-rects.js | 7 +++-- 9 files changed, 71 insertions(+), 24 deletions(-) diff --git a/lib/checks/mobile/target-offset-evaluate.js b/lib/checks/mobile/target-offset-evaluate.js index 8e45fc8164..ddc0e832f7 100644 --- a/lib/checks/mobile/target-offset-evaluate.js +++ b/lib/checks/mobile/target-offset-evaluate.js @@ -20,13 +20,29 @@ 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; - if (offset + roundingMargin >= minOffset) { - continue; + try { + let offset = getOffset(vNode, vNeighbor, minOffset / 2); + if (offset === null) { + continue; + } + + offset = roundToSingleDecimal(offset) * 2; + if (offset + roundingMargin >= minOffset) { + continue; + } + closestOffset = Math.min(closestOffset, offset); + closeNeighbors.push(vNeighbor); + } catch (err) { + if (err.message.startsWith('splitRects')) { + this.data({ + messageKey: 'tooManyRects', + closestOffset: 0, + minOffset + }); + return undefined; + } + throw err; } - closestOffset = Math.min(closestOffset, offset); - closeNeighbors.push(vNeighbor); } if (closeNeighbors.length === 0) { diff --git a/lib/checks/mobile/target-offset.json b/lib/checks/mobile/target-offset.json index c06ee11d7a..498ea64dc8 100644 --- a/lib/checks/mobile/target-offset.json +++ b/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" } } } diff --git a/lib/checks/mobile/target-size-evaluate.js b/lib/checks/mobile/target-size-evaluate.js index 6ee45bc9c5..42b300e73f 100644 --- a/lib/checks/mobile/target-size-evaluate.js +++ b/lib/checks/mobile/target-size-evaluate.js @@ -131,13 +131,16 @@ function getLargestUnobscuredArea(vNode, obscuredNodes) { const obscuringRects = obscuredNodes.map( ({ boundingClientRect: rect }) => rect ); - const unobscuredRects = splitRects(nodeRect, obscuringRects); - if (unobscuredRects.length === 0) { - return null; + try { + const unobscuredRects = splitRects(nodeRect, obscuringRects); + // Of the unobscured inner rects, work out the largest + return getLargestRect(unobscuredRects); + } catch (err) { + if (err.message.startsWith('splitRects')) { + return null; + } + throw err; } - - // Of the unobscured inner rects, work out the largest - return getLargestRect(unobscuredRects); } // Find the largest rectangle in the array, prioritize ones that meet a minimum size diff --git a/lib/commons/math/get-offset.js b/lib/commons/math/get-offset.js index 664871fda0..957d4e43c4 100644 --- a/lib/commons/math/get-offset.js +++ b/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); diff --git a/lib/commons/math/split-rects.js b/lib/commons/math/split-rects.js index ca4ef232a6..ea6ba53971 100644 --- a/lib/commons/math/split-rects.js +++ b/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; diff --git a/locales/_template.json b/locales/_template.json index a911c60144..6ea35122f6 100644 --- a/locales/_template.json +++ b/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": { diff --git a/test/checks/mobile/target-offset.js b/test/checks/mobile/target-offset.js index 367968f448..ef5ff78c9b 100644 --- a/test/checks/mobile/target-offset.js +++ b/test/checks/mobile/target-offset.js @@ -120,7 +120,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 +137,9 @@ describe('target-offset tests', () => { ${html}
`); - assert.isFalse(checkEvaluate.apply(checkContext, checkArgs)); + assert.isUndefined(checkEvaluate.apply(checkContext, checkArgs)); assert.deepEqual(checkContext._data, { + messageKey: 'tooManyRects', closestOffset: 0, minOffset: 24 }); diff --git a/test/commons/math/get-offset.js b/test/commons/math/get-offset.js index 073c8d0649..c4a5e89649 100644 --- a/test/commons/math/get-offset.js +++ b/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(` `); 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(` `); 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(` + + +
 
+ `); + 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(` + + +
 
+ `); + const nodeA = fixture.children[3]; + const nodeB = fixture.children[1]; + assert.isNull(getOffset(nodeA, nodeB)); }); it('subtracts minNeighbourRadius from center-to-center calculations', () => { diff --git a/test/commons/math/split-rects.js b/test/commons/math/split-rects.js index 9463a7fbde..968b8c7b79 100644 --- a/test/commons/math/split-rects.js +++ b/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', () => { From 703e67a42214290202a0375de4ee49ae8bc0ec4d Mon Sep 17 00:00:00 2001 From: Steven Lambert <2433219+straker@users.noreply.github.com> Date: Mon, 22 Apr 2024 14:41:20 -0600 Subject: [PATCH 2/4] test --- test/checks/mobile/target-offset.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/test/checks/mobile/target-offset.js b/test/checks/mobile/target-offset.js index ef5ff78c9b..747d9a4faa 100644 --- a/test/checks/mobile/target-offset.js +++ b/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(` +
+ Go to top +
+
+ Cookies: Accept all cookies +
+ `); + + 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( '