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): correctly calculate bounding box #4125

Merged
merged 8 commits into from
Aug 21, 2023
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
14 changes: 5 additions & 9 deletions lib/checks/mobile/target-size-evaluate.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import { findNearbyElms, isFocusable, isInTabOrder } from '../../commons/dom';
import { getRoleType } from '../../commons/aria';
import { splitRects, hasVisualOverlap } from '../../commons/math';

const roundingMargin = 0.05;
import {
splitRects,
rectHasMinimumSize,
hasVisualOverlap
} from '../../commons/math';

/**
* Determine if an element has a minimum size, taking into account
Expand Down Expand Up @@ -165,12 +167,6 @@ function isDescendantNotInTabOrder(vAncestor, vNode) {
);
}

function rectHasMinimumSize(minSize, { width, height }) {
return (
width + roundingMargin >= minSize && height + roundingMargin >= minSize
);
}

function mapActualNodes(vNodes) {
return vNodes.map(({ actualNode }) => actualNode);
}
30 changes: 30 additions & 0 deletions lib/commons/dom/get-target-rects.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import findNearbyElms from './find-nearby-elms';
import { splitRects, hasVisualOverlap } from '../math';
import memoize from '../../core/utils/memoize';

export default memoize(getTargetRects);

/**
* Return all unobscured rects of a target.
* @see https://www.w3.org/TR/WCAG22/#dfn-bounding-boxes
* @param {VitualNode} vNode
* @return {DOMRect[]}
*/
function getTargetRects(vNode) {
const nodeRect = vNode.boundingClientRect;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will eventually need to be more complicated in order to address the issue @WilcoFiers pointed out the other day where boundingClientRect doesn't take into account cases where a target has a descendant element that's part of the target for the purposes of the SC, but which overflow the bounding box of the target (eg, a link with an inline image that exceeds the link's line height).

I think it's okay for this PR to not address that, but it'd be good to file a new tech debt item to track it and add a breadcrumb to the issue here.

Copy link
Contributor

Choose a reason for hiding this comment

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

#3673 is closely related, but the fix that closed it only covered the target-size check, I think the target-offset check is still vulnerable to the same issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this as part of #4120

const overlappingVNodes = findNearbyElms(vNode).filter(vNeighbor => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is just moved and not written in this PR, but I'm noticing as I'm looking at it that it's probably incorrectly considering non-separate-target-descendants as obscuring the target rather than being part of the target. I think this needs to be more complex, along the same lines as filterByElmsOverlap in target-size-evaluate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this as part of #4120

return (
straker marked this conversation as resolved.
Show resolved Hide resolved
vNeighbor.getComputedStylePropertyValue('pointer-events') !== 'none' &&
hasVisualOverlap(vNode, vNeighbor)
straker marked this conversation as resolved.
Show resolved Hide resolved
);
});

if (!overlappingVNodes.length) {
return [nodeRect];
}

const obscuringRects = overlappingVNodes.map(
({ boundingClientRect: rect }) => rect
);
return splitRects(nodeRect, obscuringRects);
}
45 changes: 4 additions & 41 deletions lib/commons/dom/get-target-size.js
Original file line number Diff line number Diff line change
@@ -1,47 +1,16 @@
import findNearbyElms from './find-nearby-elms';
import { splitRects, hasVisualOverlap } from '../math';
import getTargetRects from './get-target-rects';
import { rectHasMinimumSize } from '../math';
import memoize from '../../core/utils/memoize';

const roundingMargin = 0.05;

export default memoize(getTargetSize);

/**
* Compute the target size of an element.
* @see https://www.w3.org/TR/WCAG22/#dfn-targets
*/
function getTargetSize(vNode, minSize) {
const nodeRect = vNode.boundingClientRect;
const overlappingVNodes = findNearbyElms(vNode).filter(vNeighbor => {
return (
vNeighbor.getComputedStylePropertyValue('pointer-events') !== 'none' &&
hasVisualOverlap(vNode, vNeighbor)
);
});

if (!overlappingVNodes.length) {
return nodeRect;
}

return getLargestUnobscuredArea(vNode, overlappingVNodes, minSize);
}

// Find areas of the target that are not obscured
function getLargestUnobscuredArea(vNode, obscuredNodes, minSize) {
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) {
return null;
}

// Of the unobscured inner rects, work out the largest
return getLargestRect(unobscuredRects, minSize);
const rects = getTargetRects(vNode);
return getLargestRect(rects, minSize);
}

// Find the largest rectangle in the array, prioritize ones that meet a minimum size
Expand All @@ -58,9 +27,3 @@ function getLargestRect(rects, minSize) {
return areaA > areaB ? rectA : rectB;
});
}

function rectHasMinimumSize(minSize, { width, height }) {
return (
width + roundingMargin >= minSize && height + roundingMargin >= minSize
);
}
1 change: 1 addition & 0 deletions lib/commons/dom/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export { default as getOverflowHiddenAncestors } from './get-overflow-hidden-anc
export { default as getRootNode } from './get-root-node';
export { default as getScrollOffset } from './get-scroll-offset';
export { default as getTabbableElements } from './get-tabbable-elements';
export { default as getTargetRects } from './get-target-rects';
export { default as getTargetSize } from './get-target-size';
export { default as getTextElementStack } from './get-text-element-stack';
export { default as getViewportSize } from './get-viewport-size';
Expand Down
60 changes: 37 additions & 23 deletions lib/commons/math/get-offset.js
Original file line number Diff line number Diff line change
@@ -1,38 +1,52 @@
import { getTargetSize } from '../dom';
import { getTargetRects } from '../dom';
import { getBoundingRect } from './get-bounding-rect';
import { isPointInRect } from './is-point-in-rect';
import { getRectCenter } from './get-rect-center';
import rectHasMinimumSize from './rect-has-minimum-size';

/**
* Get the offset between node A and node B
* Get the offset between target A and neighbor B. This assumes that the target is undersized and needs to check the spacing exception.
* @method getOffset
* @memberof axe.commons.math
* @param {VirtualNode} vNodeA
* @param {VirtualNode} vNodeB
* @param {VirtualNode} vTarget
* @param {VirtualNode} vNeighbor
* @param {Number} radius
* @returns {number}
*/
export default function getOffset(vNodeA, vNodeB, minRadiusNeighbour = 12) {
const rectA = getTargetSize(vNodeA);
const rectB = getTargetSize(vNodeB);
export default function getOffset(vTarget, vNeighbor, minRadiusNeighbour = 12) {
const targetRects = getTargetRects(vTarget);
const neighborRects = getTargetRects(vNeighbor);

// one of the rects is fully obscured
if (rectA === null || rectB === null) {
if (!targetRects.length || !neighborRects.length) {
return 0;
}

const centerA = {
x: rectA.x + rectA.width / 2,
y: rectA.y + rectA.height / 2
};
const centerB = {
x: rectB.x + rectB.width / 2,
y: rectB.y + rectB.height / 2
};
const sideB = getClosestPoint(centerA, rectB);
const targetBoundingBox = targetRects.reduce(getBoundingRect);
const targetCenter = getRectCenter(targetBoundingBox);

return Math.min(
// subtract the radius of the circle from the distance
pointDistance(centerA, centerB) - minRadiusNeighbour,
pointDistance(centerA, sideB)
);
// calculate distance to the closest edge of each neighbor rect
let minDistance = Infinity;
for (const rect of neighborRects) {
if (isPointInRect(targetCenter, rect)) {
return 0;
}

const closestPoint = getClosestPoint(targetCenter, rect);
const distance = pointDistance(targetCenter, closestPoint);
minDistance = Math.min(minDistance, distance);
}

if (rectHasMinimumSize(minRadiusNeighbour * 2, targetBoundingBox)) {
return minDistance;
straker marked this conversation as resolved.
Show resolved Hide resolved
}

const neighborBoundingBox = neighborRects.reduce(getBoundingRect);
const neighborCenter = getRectCenter(neighborBoundingBox);
// subtract the radius of the circle from the distance to center to get distance to edge of the neighbor circle
const centerDistance =
pointDistance(targetCenter, neighborCenter) - minRadiusNeighbour;

return Math.max(0, Math.min(minDistance, centerDistance));
}

function getClosestPoint(point, rect) {
Expand Down
1 change: 1 addition & 0 deletions lib/commons/math/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@ export { default as getOffset } from './get-offset';
export { getRectCenter } from './get-rect-center';
export { default as hasVisualOverlap } from './has-visual-overlap';
export { isPointInRect } from './is-point-in-rect';
export { default as rectHasMinimumSize } from './rect-has-minimum-size';
export { default as rectsOverlap } from './rects-overlap';
export { default as splitRects } from './split-rects';
7 changes: 7 additions & 0 deletions lib/commons/math/rect-has-minimum-size.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
const roundingMargin = 0.05;

export default function rectHasMinimumSize(minSize, { width, height }) {
return (
width + roundingMargin >= minSize && height + roundingMargin >= minSize
);
}
59 changes: 59 additions & 0 deletions test/commons/dom/get-target-rects.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
describe('get-target-rects', () => {
const getTargetRects = axe.commons.dom.getTargetRects;
const { queryFixture } = axe.testUtils;

it('returns the bounding rect when unobscured', () => {
const vNode = queryFixture('<button id="target">x</button>');
const rects = getTargetRects(vNode);
assert.deepEqual(rects, [vNode.actualNode.getBoundingClientRect()]);
});

it('returns subset rect when obscured', () => {
const vNode = queryFixture(`
<button id="target" style="width: 30px; height: 40px; position: absolute; left: 10px; top: 5px">x</button>
<div style="position: absolute; left: 30px; top: 0; width: 50px; height: 50px;"></div>
`);
const rects = getTargetRects(vNode);
assert.deepEqual(rects, [new DOMRect(10, 5, 20, 40)]);
});

it('ignores elements with "pointer-events: none"', () => {
const vNode = queryFixture(`
<button id="target" style="width: 30px; height: 40px; position: absolute; left: 10px; top: 5px">x</button>
<div style="position: absolute; left: 30px; top: 0; width: 50px; height: 50px; pointer-events: none"></div>
`);
const rects = getTargetRects(vNode);
assert.deepEqual(rects, [vNode.actualNode.getBoundingClientRect()]);
});

it("ignores elements that don't overlap the target", () => {
const vNode = queryFixture(`
<button id="target" style="width: 30px; height: 40px; position: absolute; left: 10px; top: 5px">x</button>
<div style="position: absolute; left: 60px; top: 0; width: 50px; height: 50px;"></div>
`);
const rects = getTargetRects(vNode);
assert.deepEqual(rects, [vNode.actualNode.getBoundingClientRect()]);
});

it('returns each unobscured area', () => {
const vNode = queryFixture(`
<button id="target" style="width: 30px; height: 40px; position: absolute; left: 10px; top: 5px">x</button>
<div style="position: absolute; left: 30px; top: 0; width: 50px; height: 50px;"></div>
<div style="position: absolute; left: 0px; top: 20px; width: 50px; height: 10px"></div>
`);
const rects = getTargetRects(vNode);
assert.deepEqual(rects, [
new DOMRect(10, 5, 20, 15),
new DOMRect(10, 30, 20, 15)
]);
});

it('returns empty if target is fully obscured', () => {
const vNode = queryFixture(`
<button id="target" style="width: 30px; height: 40px; position: absolute; left: 10px; top: 5px">x</button>
<div style="position: absolute; left: 0; top: 0; width: 50px; height: 50px;"></div>
`);
const rects = getTargetRects(vNode);
assert.lengthOf(rects, 0);
});
});
4 changes: 2 additions & 2 deletions test/commons/dom/get-target-size.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ describe('get-target-size', () => {
<div style="position: absolute; left: 30px; top: 0; width: 50px; height: 50px; pointer-events: none"></div>
`);
const rect = getTargetSize(vNode);
assert.deepEqual(rect, new DOMRect(10, 5, 30, 40));
assert.deepEqual(rect, vNode.actualNode.getBoundingClientRect());
});

it("ignores elements that don't overlap the target", () => {
Expand All @@ -32,7 +32,7 @@ describe('get-target-size', () => {
<div style="position: absolute; left: 60px; top: 0; width: 50px; height: 50px;"></div>
`);
const rect = getTargetSize(vNode);
assert.deepEqual(rect, new DOMRect(10, 5, 30, 40));
assert.deepEqual(rect, vNode.actualNode.getBoundingClientRect());
});

it('returns the largest unobscured area', () => {
Expand Down
10 changes: 10 additions & 0 deletions test/commons/math/get-offset.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,14 @@ describe('getOffset', () => {
const nodeB = fixture.children[3];
assert.closeTo(getOffset(nodeA, nodeB, 30), 20, round);
});

it('returns 0 if center of nodeA is enclosed by nodeB', () => {
const fixture = fixtureSetup(`
<button style="width: 50px; height: 10px; margin: 0; padding: 0; position: absolute; top: 0; left: 0">&nbsp;</button>
<button style="width: 10px; height: 10px; margin: 0; padding: 0; position: absolute; top: 0; left: 20px;">&nbsp;</button>
`);
const nodeA = fixture.children[1];
const nodeB = fixture.children[3];
assert.equal(getOffset(nodeA, nodeB, 30), 0);
});
});
28 changes: 28 additions & 0 deletions test/commons/math/rect-has-minimum-size.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
describe('rectHasMinimumSize', () => {
const rectHasMinimumSize = axe.commons.math.rectHasMinimumSize;

it('returns true if rect is large enough', () => {
const rect = new DOMRect(10, 20, 10, 20);
assert.isTrue(rectHasMinimumSize(10, rect));
});

it('returns true for rounding margin', () => {
const rect = new DOMRect(10, 20, 9.95, 20);
assert.isTrue(rectHasMinimumSize(10, rect));
});

it('returns false if width is too small', () => {
const rect = new DOMRect(10, 20, 5, 20);
assert.isFalse(rectHasMinimumSize(10, rect));
});

it('returns false if height is too small', () => {
const rect = new DOMRect(10, 20, 10, 5);
assert.isFalse(rectHasMinimumSize(10, rect));
});

it('returns false when below rounding margin', () => {
const rect = new DOMRect(10, 20, 9.94, 20);
assert.isFalse(rectHasMinimumSize(10, rect));
});
});
Loading
Loading