-
Notifications
You must be signed in to change notification settings - Fork 779
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
Changes from all commits
8d35ed7
5d1c5af
26e9a51
41ec6fa
8e439d6
093026c
c4cd3cf
1a98329
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
import findNearbyElms from './find-nearby-elms'; | ||
import isInTabOrder from './is-in-tab-order'; | ||
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; | ||
const overlappingVNodes = findNearbyElms(vNode).filter(vNeighbor => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||
hasVisualOverlap(vNode, vNeighbor) && | ||
vNeighbor.getComputedStylePropertyValue('pointer-events') !== 'none' && | ||
!isDescendantNotInTabOrder(vNode, vNeighbor) | ||
); | ||
}); | ||
|
||
if (!overlappingVNodes.length) { | ||
return [nodeRect]; | ||
} | ||
|
||
const obscuringRects = overlappingVNodes.map( | ||
({ boundingClientRect: rect }) => rect | ||
); | ||
return splitRects(nodeRect, obscuringRects); | ||
} | ||
|
||
function isDescendantNotInTabOrder(vAncestor, vNode) { | ||
return ( | ||
vAncestor.actualNode.contains(vNode.actualNode) && !isInTabOrder(vNode) | ||
); | ||
} |
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 | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
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('ignores non-tabbable descendants of the target', () => { | ||
const vNode = queryFixture(` | ||
<button id="target" style="width: 30px; height: 40px; position: absolute; left: 10px; top: 5px"> | ||
<div style="position: absolute; left: 5px; top: 5px; width: 50px; height: 50px;"></div> | ||
</button> | ||
`); | ||
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); | ||
}); | ||
|
||
it('returns subset rect of the target with tabbable descendant', () => { | ||
const vNode = queryFixture(` | ||
<button id="target" style="width: 30px; height: 40px; position: absolute; left: 10px; top: 5px"> | ||
<div tabindex="0" style="position: absolute; left: 5px; top: 5px; width: 50px; height: 50px;"></div> | ||
</button> | ||
`); | ||
const rects = getTargetRects(vNode); | ||
console.log(JSON.stringify(rects)); | ||
assert.deepEqual(rects, [ | ||
new DOMRect(10, 5, 30, 7), | ||
new DOMRect(10, 5, 7, 40) | ||
]); | ||
}); | ||
}); |
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)); | ||
}); | ||
}); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 thetarget-offset
check is still vulnerable to the same issueThere was a problem hiding this comment.
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