From 839e189f108e7f20aa925c54e5e4f230725beed6 Mon Sep 17 00:00:00 2001 From: Wilco Fiers Date: Mon, 18 Mar 2024 19:20:55 +0100 Subject: [PATCH 1/2] fix(target-size): always pass 10x targets (to avoid perf bottleneck) --- lib/checks/mobile/target-offset-evaluate.js | 9 ++++++++- lib/checks/mobile/target-offset.json | 5 ++++- lib/checks/mobile/target-size-evaluate.js | 7 +++++++ lib/checks/mobile/target-size.json | 3 ++- locales/_template.json | 8 ++++++-- test/checks/mobile/target-offset.js | 17 +++++++++++++++++ test/checks/mobile/target-size.js | 10 ++++++++++ 7 files changed, 54 insertions(+), 5 deletions(-) diff --git a/lib/checks/mobile/target-offset-evaluate.js b/lib/checks/mobile/target-offset-evaluate.js index e799cb68f1..8e45fc8164 100644 --- a/lib/checks/mobile/target-offset-evaluate.js +++ b/lib/checks/mobile/target-offset-evaluate.js @@ -1,11 +1,18 @@ import { findNearbyElms, isFocusable, isInTabOrder } from '../../commons/dom'; import { getRoleType } from '../../commons/aria'; -import { getOffset } from '../../commons/math'; +import { getOffset, rectHasMinimumSize } from '../../commons/math'; const roundingMargin = 0.05; export default function targetOffsetEvaluate(node, options, vNode) { const minOffset = options?.minOffset || 24; + // Bail early to avoid hitting very expensive calculations. + // Targets are so large they are unlikely to fail. + if (rectHasMinimumSize(minOffset * 10, vNode.boundingClientRect)) { + this.data({ messageKey: 'large', minOffset }); + return true; + } + const closeNeighbors = []; let closestOffset = minOffset; for (const vNeighbor of findNearbyElms(vNode, minOffset)) { diff --git a/lib/checks/mobile/target-offset.json b/lib/checks/mobile/target-offset.json index 2aebbdf9f4..c06ee11d7a 100644 --- a/lib/checks/mobile/target-offset.json +++ b/lib/checks/mobile/target-offset.json @@ -7,7 +7,10 @@ "metadata": { "impact": "serious", "messages": { - "pass": "Target has sufficient space from its closest neighbors. Safe clickable space has a diameter of ${data.closestOffset}px which is at least ${data.minOffset}px.", + "pass": { + "default": "Target has sufficient space from its closest neighbors. Safe clickable space has a diameter of ${data.closestOffset}px which is at least ${data.minOffset}px.", + "large": "Target far exceeds the minimum size of ${data.minOffset}px." + }, "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?", diff --git a/lib/checks/mobile/target-size-evaluate.js b/lib/checks/mobile/target-size-evaluate.js index a0224c8ad2..159d9cabb2 100644 --- a/lib/checks/mobile/target-size-evaluate.js +++ b/lib/checks/mobile/target-size-evaluate.js @@ -13,6 +13,13 @@ import { export default function targetSize(node, options, vNode) { const minSize = options?.minSize || 24; const nodeRect = vNode.boundingClientRect; + // Bail early to avoid hitting very expensive calculations. + // Targets are so large they are unlikely to fail. + if (rectHasMinimumSize(minSize * 10, nodeRect)) { + this.data({ messageKey: 'large', minSize }); + return true; + } + const hasMinimumSize = rectHasMinimumSize.bind(null, minSize); const nearbyElms = findNearbyElms(vNode); const overflowingContent = filterOverflowingContent(vNode, nearbyElms); diff --git a/lib/checks/mobile/target-size.json b/lib/checks/mobile/target-size.json index 0fff548a65..d4f40268a8 100644 --- a/lib/checks/mobile/target-size.json +++ b/lib/checks/mobile/target-size.json @@ -9,7 +9,8 @@ "messages": { "pass": { "default": "Control has sufficient size (${data.width}px by ${data.height}px, should be at least ${data.minSize}px by ${data.minSize}px)", - "obscured": "Control is ignored because it is fully obscured and thus not clickable" + "obscured": "Control is ignored because it is fully obscured and thus not clickable", + "large": "Target far exceeds the minimum size of ${data.minSize}px." }, "fail": { "default": "Target has insufficient size (${data.width}px by ${data.height}px, should be at least ${data.minSize}px by ${data.minSize}px)", diff --git a/locales/_template.json b/locales/_template.json index 371653ca9b..74a34c5680 100644 --- a/locales/_template.json +++ b/locales/_template.json @@ -865,7 +865,10 @@ "fail": "${data} on tag disables zooming on mobile devices" }, "target-offset": { - "pass": "Target has sufficient space from its closest neighbors. Safe clickable space has a diameter of ${data.closestOffset}px which is at least ${data.minOffset}px.", + "pass": { + "default": "Target has sufficient space from its closest neighbors. Safe clickable space has a diameter of ${data.closestOffset}px which is at least ${data.minOffset}px.", + "large": "Target far exceeds the minimum size of ${data.minOffset}px." + }, "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?", @@ -875,7 +878,8 @@ "target-size": { "pass": { "default": "Control has sufficient size (${data.width}px by ${data.height}px, should be at least ${data.minSize}px by ${data.minSize}px)", - "obscured": "Control is ignored because it is fully obscured and thus not clickable" + "obscured": "Control is ignored because it is fully obscured and thus not clickable", + "large": "Target far exceeds the minimum size of ${data.minSize}px." }, "fail": { "default": "Target has insufficient size (${data.width}px by ${data.height}px, should be at least ${data.minSize}px by ${data.minSize}px)", diff --git a/test/checks/mobile/target-offset.js b/test/checks/mobile/target-offset.js index f160baa10f..a216cac926 100644 --- a/test/checks/mobile/target-offset.js +++ b/test/checks/mobile/target-offset.js @@ -166,5 +166,22 @@ describe('target-offset tests', () => { }); assert.deepEqual(relatedIds, ['#left', '#right']); }); + + it('returns true if the target is 10x the minOffset', () => { + const checkArgs = checkSetup( + 'x' + + 'x' + + 'x' + ); + assert.isTrue(checkEvaluate.apply(checkContext, checkArgs)); + assert.equal(checkContext._data.minOffset, 24); + assert.equal(checkContext._data.messageKey, 'large'); + }); }); }); diff --git a/test/checks/mobile/target-size.js b/test/checks/mobile/target-size.js index f01a05841b..c0c38bc059 100644 --- a/test/checks/mobile/target-size.js +++ b/test/checks/mobile/target-size.js @@ -58,6 +58,16 @@ describe('target-size tests', function () { }); }); + it('returns true for very large targets', function () { + var checkArgs = checkSetup( + '' + ); + assert.isTrue(check.evaluate.apply(checkContext, checkArgs)); + assert.deepEqual(checkContext._data, { messageKey: 'large', minSize: 24 }); + }); + describe('when fully obscured', function () { it('returns true, regardless of size', function () { var checkArgs = checkSetup( From 081f112e747c978b858ae8f0328a8646aa28c435 Mon Sep 17 00:00:00 2001 From: Wilco Fiers Date: Mon, 18 Mar 2024 19:39:12 +0100 Subject: [PATCH 2/2] Update to latest --- test/checks/mobile/target-offset.js | 16 +++++--------- test/checks/mobile/target-size.js | 16 +++++--------- .../full/target-size/too-many-rects.html | 21 +++++++++---------- 3 files changed, 20 insertions(+), 33 deletions(-) diff --git a/test/checks/mobile/target-offset.js b/test/checks/mobile/target-offset.js index 1f9e80613d..367968f448 100644 --- a/test/checks/mobile/target-offset.js +++ b/test/checks/mobile/target-offset.js @@ -125,21 +125,15 @@ describe('target-offset tests', () => { for (let i = 0; i < 100; i++) { html += ` - Hello - Hello - Hello - Hello - Hello - Hello - Hello - - - + A + + + `; } const checkArgs = checkSetup(` -
+
${html}
`); diff --git a/test/checks/mobile/target-size.js b/test/checks/mobile/target-size.js index 6c308f4490..688f809753 100644 --- a/test/checks/mobile/target-size.js +++ b/test/checks/mobile/target-size.js @@ -182,21 +182,15 @@ describe('target-size tests', function () { for (let i = 0; i < 100; i++) { html += ` - Hello - Hello - Hello - Hello - Hello - Hello - Hello - - - + A + + + `; } const checkArgs = checkSetup(` -
+
${html}
`); diff --git a/test/integration/full/target-size/too-many-rects.html b/test/integration/full/target-size/too-many-rects.html index 4ce85fd174..0a90cd4782 100644 --- a/test/integration/full/target-size/too-many-rects.html +++ b/test/integration/full/target-size/too-many-rects.html @@ -22,7 +22,12 @@
-
+
@@ -32,16 +37,10 @@ for (let i = 0; i < 100; i++) { html += ` - Hello - Hello - Hello - Hello - Hello - Hello - Hello - - - + A + + + `; } document.querySelector('#tab-table').innerHTML = html;