Skip to content
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions lib/checks/label/help-same-as-label-evaluate.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
import { labelVirtual, accessibleText, sanitize } from '../../commons/text';
import { idrefs } from '../../commons/dom';
import {
labelVirtual,
accessibleTextVirtual,
sanitize
} from '../../commons/text';
import { getResolvedRefs } from '../../commons/dom';
import { hasAriaValue } from '../../commons/aria';

function helpSameAsLabelEvaluate(node, options, virtualNode) {
const labelText = labelVirtual(virtualNode);
Expand All @@ -12,11 +17,11 @@ function helpSameAsLabelEvaluate(node, options, virtualNode) {
if (!check) {
check = '';

if (node.getAttribute('aria-describedby')) {
const ref = idrefs(node, 'aria-describedby');
if (hasAriaValue(virtualNode, 'aria-describedby')) {
const ref = getResolvedRefs(virtualNode, 'aria-describedby');
check = ref
.map(thing => {
return thing ? accessibleText(thing) : '';
.map(vNode => {
return vNode ? accessibleTextVirtual(vNode) : '';
})
.join('');
}
Expand Down
8 changes: 5 additions & 3 deletions lib/checks/label/multiple-label-evaluate.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import {
getRootNode,
isHiddenForEveryone,
isVisibleToScreenReaders,
idrefs
getResolvedRefs
} from '../../commons/dom';
import { escapeSelector } from '../../core/utils';

Expand Down Expand Up @@ -40,8 +40,10 @@ function multipleLabelEvaluate(node) {
return undefined;
}
// make sure the ONE AT visible label is in the list of idRefs of aria-labelledby
const labelledby = idrefs(node, 'aria-labelledby');
return !labelledby.includes(ATVisibleLabels[0]) ? undefined : false;
const labelledby = getResolvedRefs(node, 'aria-labelledby');
return !labelledby.some(ref => ref?.actualNode === ATVisibleLabels[0])
? undefined
: false;
}

// only 1 CSS visible label
Expand Down
5 changes: 3 additions & 2 deletions lib/checks/label/title-only-evaluate.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import { labelVirtual, titleText } from '../../commons/text';
import { getAriaValue } from '../../commons/aria';

function titleOnlyEvaluate(node, options, virtualNode) {
const labelText = labelVirtual(virtualNode);
const title = titleText(virtualNode);
const ariaDescribedBy = virtualNode.attr('aria-describedby');
const ariaDescribedBy = getAriaValue(virtualNode, 'aria-describedby');

return !labelText && !!(title || ariaDescribedBy);
return !labelText && !!(title || ariaDescribedBy?.value);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is buggy (not new). We should consider building a proper accessibleDescription method and use that. Would you mind opening an issue for this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Opened #5212 to track building a proper accessibleDescription method and switching this check to use it.

}

export default titleOnlyEvaluate;
16 changes: 16 additions & 0 deletions test/checks/label/help-same-as-label.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
describe('help-same-as-label', () => {
const { checkSetup, html } = axe.testUtils;
const checkEvaluate = axe.testUtils.getCheckEvaluate('help-same-as-label');
const fixture = document.getElementById('fixture');

afterEach(() => {
Expand Down Expand Up @@ -82,4 +84,18 @@ describe('help-same-as-label', () => {
)
);
});

describe('ElementInternals', () => {
it('should return true if an element has a label and elementInternals aria-describedby with the same text', () => {
const params = checkSetup(html`
<div id="dby">Duplicate</div>
<testutils-element
id="target"
aria-label="Duplicate"
with-aria-describedby="dby"
></testutils-element>
`);
assert.isTrue(checkEvaluate.apply(null, params));
});
});
});
47 changes: 47 additions & 0 deletions test/checks/label/multiple-label.js
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,38 @@ describe('multiple-label', () => {
);
});

// ariaLabelledByElements (reflected AOM property) coverage adapted from
// @jcfranco's work in #5187 (issue #4943)
Comment on lines +246 to +247

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
// ariaLabelledByElements (reflected AOM property) coverage adapted from
// @jcfranco's work in #5187 (issue #4943)

We can do attribution in other ways. Doesn't belong in the code.

it('should return false when ariaLabelledByElements overrides aria-labelledby', () => {
fixtureSetup(html`
<input type="checkbox" id="F" aria-labelledby="G" />
<label for="F" id="G" aria-hidden="true">Please</label>
<label for="F" id="H">Excuse</label>
`);
const target = fixture.querySelector('#F');
target.ariaLabelledByElements = [fixture.querySelector('#H')];
assert.isFalse(
axe.testUtils
.getCheckEvaluate('multiple-label')
.call(checkContext, target)
);
});

it('should return false when ariaLabelledByElements overrides aria-labelledby with aria-label present', () => {
fixtureSetup(html`
<input type="checkbox" id="F" aria-labelledby="G" aria-label="Nope" />
<label for="F" id="G" aria-hidden="true">Please</label>
<label for="F" id="H">Excuse</label>
`);
const target = fixture.querySelector('#F');
target.ariaLabelledByElements = [fixture.querySelector('#H')];
assert.isFalse(
axe.testUtils
.getCheckEvaluate('multiple-label')
.call(checkContext, target)
);
});

it('should return false given multiple labels, one label visible, and no aria-labelledby', () => {
fixtureSetup(html`
<input type="checkbox" id="I" />
Expand Down Expand Up @@ -337,4 +369,19 @@ describe('multiple-label', () => {
.call(checkContext, shadowTarget.firstElementChild)
);
});

describe('ElementInternals', () => {
it('should return false given multiple labels, one label AT visible, and elementInternals aria-labelledby for AT visible', () => {
const checkEvaluate = axe.testUtils.getCheckEvaluate('multiple-label');
const params = axe.testUtils.checkSetup(html`
<label for="target" aria-hidden="true" id="l1">Please</label>
<label for="target" id="l2">Excuse</label>
<testutils-element
id="target"
with-aria-labelledby="l2"
></testutils-element>
`);
assert.isFalse(checkEvaluate.call(checkContext, ...params));
});
});
});
30 changes: 30 additions & 0 deletions test/checks/label/title-only.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
describe('title-only', () => {
const checkSetup = axe.testUtils.checkSetup;
const html = axe.testUtils.html;
const checkEvaluate = axe.testUtils.getCheckEvaluate('title-only');
const fixture = document.getElementById('fixture');

afterEach(() => {
Expand Down Expand Up @@ -61,4 +64,31 @@ describe('title-only', () => {
)
);
});

it('should return false if aria-describedby is empty and there is no title or label', () => {
const node = document.createElement('input');
node.type = 'text';
node.setAttribute('aria-describedby', '');

fixture.appendChild(node);

axe.testUtils.flatTreeSetup(fixture);

assert.isFalse(
checkEvaluate(node, undefined, axe.utils.getNodeFromTree(node))
);
});

describe('ElementInternals', () => {
it('should return true if an element only has elementInternals aria-describedby', () => {
const params = checkSetup(html`
<div id="dby">description</div>
<testutils-element
id="target"
with-aria-describedby="dby"
></testutils-element>
`);
assert.isTrue(checkEvaluate.apply(null, params));
});
});
});
Loading