Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
Mar 18, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 38 additions & 21 deletions lib/checks/mobile/target-size-evaluate.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We talked about adding an early pass for elements that have 10x the required size. I don't mind that in a different PR, but I don't think we should close the issues until we have that in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd prefer to move that to it's own pr so we can get this in quicker. Which ticket do you think it shouldn't close?

const minSize = options?.minSize || 24;
const nodeRect = vNode.boundingClientRect;
const hasMinimumSize = rectHasMinimumSize.bind(null, minSize);
Expand All @@ -20,9 +20,24 @@ export default function targetSize(node, options, vNode) {
vNode,
nearbyElms
);
const hasOverflowingContent = () => {
straker marked this conversation as resolved.
Show resolved Hide resolved
this.data({ minSize, messageKey: 'contentOverflow' });
this.relatedNodes(mapActualNodes(overflowingContent));
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 &&
(fullyObscuringElms.length || !hasMinimumSize(nodeRect))
) {
return hasOverflowingContent();
}

// 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;
Expand All @@ -32,31 +47,32 @@ 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);
const largestInnerRect = getLargestUnobscuredArea(vNode, obscuredWidgets);

// 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)
) {
this.data({ minSize, messageKey: 'contentOverflow' });
this.relatedNodes(mapActualNodes(overflowingContent));
return undefined;
}
// 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 is obscured, and insufficient space is left
if (obscuredWidgets.length !== 0 && !hasMinimumSize(largestInnerRect)) {
if (!hasMinimumSize(largestInnerRect)) {
if (overflowingContent.length) {
return hasOverflowingContent();
}

const allTabbable = obscuredWidgets.every(isInTabOrder);
const messageKey = `partiallyObscured${allTabbable ? '' : 'NonTabbable'}`;

Expand All @@ -65,7 +81,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;
Expand Down Expand Up @@ -106,13 +122,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) {
straker marked this conversation as resolved.
Show resolved Hide resolved
return null;
}

// Of the unobscured inner rects, work out the largest
return getLargestRect(unobscuredRects);
}
Expand Down
3 changes: 2 additions & 1 deletion lib/checks/mobile/target-size.json
Original file line number Diff line number Diff line change
Expand Up @@ -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 calculate calculate largest obscured rectangle as there are too many overlapping widgets"
straker marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand Down
7 changes: 7 additions & 0 deletions lib/commons/math/split-rects.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 [];
straker marked this conversation as resolved.
Show resolved Hide resolved
}
}
return uniqueRects;
}
Expand Down
3 changes: 2 additions & 1 deletion locales/_template.json
Original file line number Diff line number Diff line change
Expand Up @@ -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 calculate calculate largest obscured rectangle as there are too many overlapping widgets"
straker marked this conversation as resolved.
Show resolved Hide resolved
}
},
"header-present": {
Expand Down
30 changes: 30 additions & 0 deletions test/checks/mobile/target-offset.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
30 changes: 30 additions & 0 deletions test/checks/mobile/target-size.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
9 changes: 9 additions & 0 deletions test/commons/math/split-rects.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Loading