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): ignore descendant elements in shadow dom #4410

Merged
merged 3 commits into from
Apr 23, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
28 changes: 26 additions & 2 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,37 @@ module.exports = {
selector: 'MemberExpression[property.name=tagName]',
message: "Don't use node.tagName, use node.nodeName instead."
},
// node.attributes can be clobbered so is unsafe to use
// @see https://github.com/dequelabs/axe-core/pull/1432
{
// node.attributes can be clobbered so is unsafe to use
// @see https://github.com/dequelabs/axe-core/pull/1432
// node.attributes
selector:
'MemberExpression[object.name=node][property.name=attributes]',
message:
"Don't use node.attributes, use node.hasAttributes() or axe.utils.getNodeAttributes(node) instead."
},
{
// vNode.actualNode.attributes
selector:
'MemberExpression[object.property.name=actualNode][property.name=attributes]',
message:
"Don't use node.attributes, use node.hasAttributes() or axe.utils.getNodeAttributes(node) instead."
},
// node.contains doesn't work with shadow dom
// @see https://github.com/dequelabs/axe-core/issues/4194
{
// node.contains()
selector:
'CallExpression[callee.object.name=node][callee.property.name=contains]',
message:
"Don't use node.contains(node2), use axe.utils.contains(node, node2) instead."
},
{
// vNode.actualNode.contains()
selector:
'CallExpression[callee.object.property.name=actualNode][callee.property.name=contains]',
message:
"Don't use node.contains(node2), use axe.utils.contains(node, node2) instead."
}
]
},
Expand Down
5 changes: 2 additions & 3 deletions lib/checks/mobile/target-size-evaluate.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
rectHasMinimumSize,
hasVisualOverlap
} from '../../commons/math';
import { contains } from '../../core/utils';

/**
* Determine if an element has a minimum size, taking into account
Expand Down Expand Up @@ -185,9 +186,7 @@ function toDecimalSize(rect) {
}

function isDescendantNotInTabOrder(vAncestor, vNode) {
return (
vAncestor.actualNode.contains(vNode.actualNode) && !isInTabOrder(vNode)
);
return contains(vAncestor, vNode) && !isInTabOrder(vNode);
straker marked this conversation as resolved.
Show resolved Hide resolved
}

function mapActualNodes(vNodes) {
Expand Down
5 changes: 2 additions & 3 deletions lib/commons/dom/get-target-rects.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import findNearbyElms from './find-nearby-elms';
import isInTabOrder from './is-in-tab-order';
import { splitRects, hasVisualOverlap } from '../math';
import memoize from '../../core/utils/memoize';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a separate import on purpose as memoize when destructred from the utils import path causes problems for esbuild and it doesn't hoist the memoize function high enough in the build for functions to use it. We've gotten around that by importing it directly.

import { contains } from '../../core/utils';

export default memoize(getTargetRects);

Expand Down Expand Up @@ -32,7 +33,5 @@ function getTargetRects(vNode) {
}

function isDescendantNotInTabOrder(vAncestor, vNode) {
return (
vAncestor.actualNode.contains(vNode.actualNode) && !isInTabOrder(vNode)
);
return contains(vAncestor, vNode) && !isInTabOrder(vNode);
}
2 changes: 2 additions & 0 deletions lib/core/utils/contains.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@ export default function contains(vNode, otherVNode) {
!vNode.shadowId &&
!otherVNode.shadowId &&
vNode.actualNode &&
// eslint-disable-next-line no-restricted-syntax
typeof vNode.actualNode.contains === 'function'
) {
// eslint-disable-next-line no-restricted-syntax
return vNode.actualNode.contains(otherVNode.actualNode);
}

Expand Down
22 changes: 21 additions & 1 deletion test/commons/dom/get-target-rects.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
describe('get-target-rects', () => {
const getTargetRects = axe.commons.dom.getTargetRects;
const shadowSupport = axe.testUtils.shadowSupport.v1;
const { queryFixture } = axe.testUtils;
const fixture = document.getElementById('fixture');

it('returns the bounding rect when unobscured', () => {
const vNode = queryFixture('<button id="target">x</button>');
Expand Down Expand Up @@ -74,10 +76,28 @@ describe('get-target-rects', () => {
</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)
]);
});

(shadowSupport ? it : xit)(
straker marked this conversation as resolved.
Show resolved Hide resolved
'ignores non-tabbable descendants of the target that are in shadow dom',
() => {
fixture.innerHTML =
'<button id="target" style="width: 30px; height: 40px; position: absolute; left: 10px; top: 5px"><span id="shadow"></span></button>';
const target = fixture.querySelector('#target');
const shadow = fixture
.querySelector('#shadow')
.attachShadow({ mode: 'open' });
shadow.innerHTML =
'<div style="position: absolute; left: 5px; top: 5px; width: 50px; height: 50px;"></div>';

axe.setup(fixture);
const vNode = axe.utils.getNodeFromTree(target);
const rects = getTargetRects(vNode);
assert.deepEqual(rects, [vNode.actualNode.getBoundingClientRect()]);
}
);
});
66 changes: 66 additions & 0 deletions test/integration/full/target-size/shadow-dom.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
<!doctype html>
<html lang="en">
<head>
<title>Target-size too many rects Test</title>
straker marked this conversation as resolved.
Show resolved Hide resolved
<link
rel="stylesheet"
type="text/css"
href="/node_modules/mocha/mocha.css"
/>
<script src="/node_modules/mocha/mocha.js"></script>
<script src="/node_modules/chai/chai.js"></script>
<script src="/axe.js"></script>
<script>
mocha.setup({
timeout: 10000,
ui: 'bdd'
});
var assert = chai.assert;
</script>
</head>
<body>
<main id="mocha"></main>
<script>
const shadowTemplate = document.createElement('template');
shadowTemplate.innerHTML =
'<div id="shadow-container"><slot></slot></div>';
class ShadowOpenWebComponent extends HTMLElement {
connectedCallback() {
const shadowRoot = this.attachShadow({ mode: 'open' });
shadowRoot.appendChild(shadowTemplate.content.cloneNode(true));
}
}

customElements.define('shadow-open', ShadowOpenWebComponent);
</script>
<style>
#container {
font-size: 16px;
}
#title {
background-color: #cff;
}
#content {
background-color: #cfc;
margin-top: 10px;
}
</style>
<div id="container">
<div id="title">
<a id="title-link" href="#target">
<shadow-open>
<div>Title</div>
</shadow-open>
</a>
</div>
<div id="content">
<a id="content-link" href="#target">
<div>Content</div>
</a>
</div>
</div>
<script src="/test/testutils.js"></script>
<script src="shadow-dom.js"></script>
<script src="/test/integration/adapter.js"></script>
</body>
</html>
45 changes: 45 additions & 0 deletions test/integration/full/target-size/shadow-dom.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
describe('target-size shadow dom test', () => {
'use strict';
let results;

before(done => {
axe.testUtils.awaitNestedLoad(() => {
const options = {
runOnly: ['target-size'],
elementRef: true
};
const context = {
// ignore the mocha links
exclude: '#mocha'
};
axe.run(context, options, (err, r) => {
straker marked this conversation as resolved.
Show resolved Hide resolved
if (err) {
done(err);
}
results = r;
console.log(results);
done();
});
});
});

describe('violations', function () {
it('should find 0', function () {
assert.lengthOf(results.violations, 0);
});
});

describe('passes', function () {
it('should find 2', function () {
assert.lengthOf(results.passes[0].nodes, 2);
});
});

it('should find 0 inapplicable', function () {
assert.lengthOf(results.inapplicable, 0);
});

it('should find 0 incomplete', function () {
assert.lengthOf(results.incomplete, 0);
});
});
Loading