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(aria-valid-attr-value): aria-controls & aria-haspopup incomplete #4418

Merged
merged 2 commits into from Apr 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 13 additions & 2 deletions lib/checks/aria/aria-valid-attr-value-evaluate.js
Expand Up @@ -36,12 +36,23 @@ export default function ariaValidAttrValueEvaluate(node, options, virtualNode) {

const preChecks = {
// aria-controls should only check if element exists if the element
// doesn't have aria-expanded=false or aria-selected=false (tabs)
// doesn't have aria-expanded=false, aria-selected=false (tabs),
// or aria-haspopup (may load later)
// @see https://github.com/dequelabs/axe-core/issues/1463
// @see https://github.com/dequelabs/axe-core/issues/4363
'aria-controls': () => {
const hasPopup =
['false', null].includes(virtualNode.attr('aria-haspopup')) === false;

if (hasPopup) {
needsReview = `aria-controls="${virtualNode.attr('aria-controls')}"`;
messageKey = 'controlsWithinPopup';
}

return (
virtualNode.attr('aria-expanded') !== 'false' &&
virtualNode.attr('aria-selected') !== 'false'
virtualNode.attr('aria-selected') !== 'false' &&
hasPopup === false
);
},
// aria-current should mark as needs review if any value is used that is
Expand Down
3 changes: 2 additions & 1 deletion lib/checks/aria/aria-valid-attr-value.json
Expand Up @@ -15,7 +15,8 @@
"noIdShadow": "ARIA attribute element ID does not exist on the page or is a descendant of a different shadow DOM tree: ${data.needsReview}",
"ariaCurrent": "ARIA attribute value is invalid and will be treated as \"aria-current=true\": ${data.needsReview}",
"idrefs": "Unable to determine if ARIA attribute element ID exists on the page: ${data.needsReview}",
"empty": "ARIA attribute value is ignored while empty: ${data.needsReview}"
"empty": "ARIA attribute value is ignored while empty: ${data.needsReview}",
"controlsWithinPopup": "Unable to determine if aria-controls referenced ID exists on the page while using aria-haspopup: ${data.needsReview}"
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion locales/_template.json
Expand Up @@ -538,7 +538,8 @@
"noIdShadow": "ARIA attribute element ID does not exist on the page or is a descendant of a different shadow DOM tree: ${data.needsReview}",
"ariaCurrent": "ARIA attribute value is invalid and will be treated as \"aria-current=true\": ${data.needsReview}",
"idrefs": "Unable to determine if ARIA attribute element ID exists on the page: ${data.needsReview}",
"empty": "ARIA attribute value is ignored while empty: ${data.needsReview}"
"empty": "ARIA attribute value is ignored while empty: ${data.needsReview}",
"controlsWithinPopup": "Unable to determine if aria-controls referenced ID exists on the page while using aria-haspopup: ${data.needsReview}"
}
},
"aria-valid-attr": {
Expand Down
13 changes: 13 additions & 0 deletions test/checks/aria/valid-attr-value.js
Expand Up @@ -110,6 +110,19 @@ describe('aria-valid-attr-value', function () {
assert.isFalse(validAttrValueCheck.call(checkContext, null, null, vNode));
});

it('should return undefined on aria-controls with aria-haspopup as we cannot determine if it is in the DOM later', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let test that messageKey is set here. I would also like to see a test showing this fails with aria-expanded=false. An integration test would also be useful, just to be safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah today I learned how testing the messageKey works thanks!

Added that, and an integration test here a823712

var vNode = queryFixture(
'<button id="target" aria-controls="test" aria-haspopup="true">Button</button>'
);
assert.isUndefined(
validAttrValueCheck.call(checkContext, null, null, vNode)
);
assert.deepEqual(checkContext._data, {
messageKey: 'controlsWithinPopup',
needsReview: 'aria-controls="test"'
});
});

it('should pass on aria-owns and aria-expanded=false when the element is not in the DOM', function () {
var vNode = queryFixture(
'<button id="target" aria-owns="test" aria-expanded="false">Button</button>'
Expand Down
Expand Up @@ -360,4 +360,7 @@ <h2>Possible False Positives</h2>
<div role="checkbox" id="incomplete6" aria-checked>I'm not checked</div>
<div role="textbox" id="incomplete7" aria-invalid="">I'm actually valid</div>
<div role="textbox" id="incomplete8" aria-hidden>I'm not really gone</div>
<div id="incomplete9" aria-controls="stuff" aria-haspopup="true">
May have injected html to control dynamically later, who knows!
</div>
</div>
Expand Up @@ -236,6 +236,7 @@
["#incomplete5"],
["#incomplete6"],
["#incomplete7"],
["#incomplete8"]
["#incomplete8"],
["#incomplete9"]
]
}