-
Notifications
You must be signed in to change notification settings - Fork 791
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
[WIP] feat(rule): label content name mismatch #1159
Changes from all commits
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,34 @@ | ||
function getAriaText(elm) { | ||
if (elm.getAttribute('aria-label')) { | ||
return elm | ||
.getAttribute('aria-label') | ||
.toLowerCase() | ||
.trim(); | ||
} | ||
if (elm.getAttribute('aria-labelledby')) { | ||
return elm | ||
.getAttribute('aria-labelledby') | ||
.toLowerCase() | ||
.trim(); | ||
} | ||
return null; | ||
} | ||
|
||
const accessibleText = getAriaText(node); | ||
// if no accessible text - fail | ||
if (!accessibleText || !accessibleText.length) { | ||
return false; | ||
} | ||
|
||
const contentText = node.textContent.toLowerCase().trim(); | ||
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. You should use |
||
// if no text content - fail | ||
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. Why wouldn't we ignore these types of elements? |
||
if (!contentText || !contentText.length) { | ||
return false; | ||
} | ||
|
||
// if text content is not part of accessible text - fail | ||
if (!accessibleText.includes(contentText)) { | ||
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 don't know that we have a string polyfill of 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. We seem to have a polyfill for |
||
return false; | ||
} | ||
|
||
return true; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
{ | ||
"id": "label-content-name-mismatch", | ||
"evaluate": "label-content-name-mismatch.js", | ||
"metadata": { | ||
"impact": "serious", | ||
"messages": { | ||
"pass": "Interactive element contains visible label as part of it's accessible name", | ||
"fail": "Interactive element does not contain visible label as part of it's accessible name" | ||
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 suggest: The visible text of the element is different from its accessible name |
||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
/** | ||
* Applicability: | ||
* This rule applies to any element that has: | ||
* - a semantic role that is a widget that supports name from content, and | ||
* - visible text content, and | ||
* - an aria-label or aria-labelledby attribute. | ||
*/ | ||
const { aria } = axe.commons; | ||
const role = aria.getRole(node, { noImplicit: false, fallback: true }); | ||
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. Because of our accessibility support baseline, axe-core shouldn't support fallback roles for the time being. Also |
||
|
||
// no role - exclude | ||
if (!role) { | ||
return false; | ||
} | ||
|
||
const rolesWithNameFromContents = aria.getRolesWithNameFromContents(); | ||
|
||
// if role is not one of roles with name from contents - exclude | ||
if (!rolesWithNameFromContents.includes(role)) { | ||
return false; | ||
} | ||
|
||
// if no accessible name attributes - exclude | ||
if (!node.getAttribute('aria-labelledby') && !node.getAttribute('aria-label')) { | ||
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. You should probably use |
||
return false; | ||
} | ||
|
||
// if no text content - exclude | ||
if (node.textContent.toLowerCase().trim().length <= 0) { | ||
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 don't think `.textContent is reliable enough for this. We don't want to check against hidden text. I think we have a visibleText method (not sure about the name) that gets you only text that's displayed. 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. Do you mean |
||
return false; | ||
} | ||
|
||
return true; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
{ | ||
"id": "label-content-name-mismatch", | ||
"matches": "label-content-name-mismatch-matches.js", | ||
"tags": [ | ||
"wcag21a", | ||
"wcag253" | ||
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. We should make this rule experimental for the time being. I don't trust that this isn't going to give false positives. |
||
], | ||
"metadata": { | ||
"description": "Ensures that interactive elements labelled through their content must have their visible label as part of their accessible name", | ||
"help": "Interactive elements must have their visible label as part of their accessible name" | ||
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. Name from content isn't just for interactive elements. I'm struggling to come up with a short but still descriptive text for this rule. Maybe talk to Jean or Carrie for suggestions? 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. @jeankaplansky - can you help with a better help text here? 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. Yep. I'm on it. 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. @WilcoFiers , @JKODU : Is there a specific reason we cannot say "Ensures that ELEMENTS labeled through their content must have their visible label as part of their accessible name"? Ditto with the value for help: "ELEMENTS must have their visible label as part of their accessible name." This approach accounts for all elements regardless of functional semantic intent (interactive or plain content). |
||
}, | ||
"all": [], | ||
"any": [ | ||
"label-content-name-mismatch" | ||
], | ||
"none": [] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,127 @@ | ||
describe('label-content-name-mismatch', function() { | ||
'use strict'; | ||
|
||
var fixture = document.getElementById('fixture'); | ||
var check = undefined; | ||
var options = undefined; | ||
// var checkSetup = axe.testUtils.checkSetup; | ||
// var shadowSupport = axe.testUtils.shadowSupport; | ||
|
||
beforeEach(function() { | ||
check = checks['label-content-name-mismatch']; | ||
}); | ||
|
||
afterEach(function() { | ||
fixture.innerHTML = ''; | ||
check = undefined; | ||
axe._tree = undefined; | ||
}); | ||
|
||
it('returns false when element has an empty aria-label', function() { | ||
fixture.innerHTML = | ||
'<button id="target" name="link" aria-label="">The full label</button>'; | ||
var node = fixture.querySelector('#target'); | ||
var tree = (axe._tree = axe.utils.getFlattenedTree(fixture)); | ||
var vNode = axe.utils.getNodeFromTree(tree[0], node); | ||
var actual = check.evaluate(node, options, vNode); | ||
assert.isFalse(actual); | ||
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. If aria-label is empty / whitespace only it ignored as the accessible name, and the content will be used as label instead. This case should pass. 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.
|
||
}); | ||
|
||
it('returns false when element has an empty aria-labelledby', function() { | ||
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. Same here, empty aria-labelledby will be ignored by the name computation. This shouldn't fail. 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. Same comment as above, repeating:
Perhaps the accessible name computation should take this into consideration. Correct me if I am wrong. |
||
fixture.innerHTML = | ||
'<button id="target" name="link" aria-labelledby="">The full label</button>'; | ||
var node = fixture.querySelector('#target'); | ||
var tree = (axe._tree = axe.utils.getFlattenedTree(fixture)); | ||
var actual = check.evaluate( | ||
node, | ||
options, | ||
axe.utils.getNodeFromTree(tree[0], node) | ||
); | ||
assert.isFalse(actual); | ||
}); | ||
|
||
it('returns false when element has no text content', function() { | ||
fixture.innerHTML = | ||
'<button id="target" name="link" aria-label="Ok"></button>'; | ||
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. This too, if there is no content, there is no mismatch and so this shouldn't fail. |
||
var node = fixture.querySelector('#target'); | ||
var tree = (axe._tree = axe.utils.getFlattenedTree(fixture)); | ||
var actual = check.evaluate( | ||
node, | ||
options, | ||
axe.utils.getNodeFromTree(tree[0], node) | ||
); | ||
assert.isFalse(actual); | ||
}); | ||
|
||
it('returns false when accessible text does not contain text content', function() { | ||
fixture.innerHTML = | ||
'<button id="target" name="link" aria-label="Ok">test page</button>'; | ||
var node = fixture.querySelector('#target'); | ||
var tree = (axe._tree = axe.utils.getFlattenedTree(fixture)); | ||
var actual = check.evaluate( | ||
node, | ||
options, | ||
axe.utils.getNodeFromTree(tree[0], node) | ||
); | ||
assert.isFalse(actual); | ||
}); | ||
|
||
it('returns true when visible label and accessible name matches when trailing white spaces are removed', function() { | ||
fixture.innerHTML = | ||
'<div id="target" role="link" aria-label="next page ">next page</div>'; | ||
var node = fixture.querySelector('#target'); | ||
var tree = (axe._tree = axe.utils.getFlattenedTree(fixture)); | ||
var actual = check.evaluate( | ||
node, | ||
options, | ||
axe.utils.getNodeFromTree(tree[0], node) | ||
); | ||
assert.isTrue(actual); | ||
}); | ||
|
||
it('returns true when visible label and accessible name matches after handling character insensitivity', function() { | ||
fixture.innerHTML = | ||
'<div id="target" role="link" aria-label="Next Page">next page</div>'; | ||
var node = fixture.querySelector('#target'); | ||
var tree = (axe._tree = axe.utils.getFlattenedTree(fixture)); | ||
var actual = check.evaluate( | ||
node, | ||
options, | ||
axe.utils.getNodeFromTree(tree[0], node) | ||
); | ||
assert.isTrue(actual); | ||
}); | ||
|
||
it('returns true when full visible label is contained in the accessible name', function() { | ||
fixture.innerHTML = | ||
'<button id="target" name="link" aria-label="Next Page in the list">Next Page</button>'; | ||
var node = fixture.querySelector('#target'); | ||
var tree = (axe._tree = axe.utils.getFlattenedTree(fixture)); | ||
var actual = check.evaluate( | ||
node, | ||
options, | ||
axe.utils.getNodeFromTree(tree[0], node) | ||
); | ||
assert.isTrue(actual); | ||
}); | ||
|
||
it('returns false when visible label doesn’t match accessible name', function() { | ||
fixture.innerHTML = | ||
'<div id="target" role="link" aria-label="OK">Next</div>'; | ||
var node = fixture.querySelector('#target'); | ||
var tree = (axe._tree = axe.utils.getFlattenedTree(fixture)); | ||
var vNode = axe.utils.getNodeFromTree(tree[0], node); | ||
var actual = check.evaluate(node, options, vNode); | ||
assert.isFalse(actual); | ||
}); | ||
|
||
it('returns false when not all of visible label is included in accessible name', function() { | ||
fixture.innerHTML = | ||
'<button id="target" name="link" aria-label="the full">The full label</button>'; | ||
var node = fixture.querySelector('#target'); | ||
var tree = (axe._tree = axe.utils.getFlattenedTree(fixture)); | ||
var vNode = axe.utils.getNodeFromTree(tree[0], node); | ||
var actual = check.evaluate(node, options, vNode); | ||
assert.isFalse(actual); | ||
}); | ||
}); | ||
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. You're missing quite a few tests here:
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
<!-- Pass --> | ||
<div id='pass1' role="link" aria-label="next page ">next page</div> | ||
<div id='pass2' role="link" aria-label="Next Page">next page</div> | ||
<button id='pass3' name="link" aria-label="Next Page in the list">Next Page</button> | ||
<!-- Fail --> | ||
<div id="fail1" role="link" aria-label="OK">Next</div> | ||
<button id="fail2" name="link" aria-label="the full">The full label</button> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
{ | ||
"description": "label-content-name-mismatch test", | ||
"rule": "label-content-name-mismatch", | ||
"violations": [ | ||
["#fail1"], | ||
["#fail2"] | ||
], | ||
"passes": [ | ||
["#pass1"], | ||
["#pass2"], | ||
["#pass3"] | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
describe('label-content-name-mismatch-matches', function() { | ||
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 don't think the following test case is addressed: |
||
'use strict'; | ||
|
||
var fixture = document.getElementById('fixture'); | ||
var rule; | ||
|
||
beforeEach(function() { | ||
rule = axe._audit.rules.find(function(rule) { | ||
return rule.id === 'label-content-name-mismatch'; | ||
}); | ||
}); | ||
|
||
it('returns false if given element has no role (including fallback)', function() { | ||
// div has no fallback role | ||
fixture.innerHTML = '<div></div>'; | ||
var target = fixture.querySelector('div'); | ||
var actual = rule.matches(target); | ||
assert.isFalse(actual); | ||
}); | ||
|
||
it('returns false if element role is not supported roles with name from contents', function() { | ||
// textbox is not a supported role with name from content | ||
fixture.innerHTML = '<textarea></textarea>'; | ||
var target = fixture.querySelector('textarea'); | ||
var actual = rule.matches(target); | ||
assert.isFalse(actual); | ||
}); | ||
|
||
it('returns false if element does not have accessible name attribute (aria-label or aria-labelledby)', function() { | ||
fixture.innerHTML = '<button name="link">The full label</button>'; | ||
var target = fixture.querySelector('button'); | ||
var actual = rule.matches(target); | ||
assert.isFalse(actual); | ||
}); | ||
|
||
it('returns true if element has accessible name and supported role with name from contents', function() { | ||
fixture.innerHTML = | ||
'<button name="link" aria-label="The full">The full label</button>'; | ||
var target = fixture.querySelector('button'); | ||
var actual = rule.matches(target); | ||
assert.isTrue(actual); | ||
}); | ||
|
||
it('returns false as for non-widget role', function() { | ||
fixture.innerHTML = '<a aria-label="OK">Next</a>'; | ||
var target = fixture.querySelector('a'); | ||
var actual = rule.matches(target); | ||
assert.isFalse(actual); | ||
}); | ||
|
||
it('returns false for widget role that does not support name from content', function() { | ||
fixture.innerHTML = | ||
'<input type="email" aria-label="E-mail" value="Contact">'; | ||
var target = fixture.querySelector('input'); | ||
var actual = rule.matches(target); | ||
assert.isFalse(actual); | ||
}); | ||
|
||
it('returns false for non-widget role that does support name from content', function() { | ||
fixture.innerHTML = '<div role="doc-chapter" aria-label="OK">Next</div>'; | ||
var target = fixture.querySelector('div'); | ||
var actual = rule.matches(target); | ||
assert.isFalse(actual); | ||
}); | ||
|
||
it('returns true for non-widget role that does support name from content', function() { | ||
fixture.innerHTML = '<div role="tooltip" aria-label="OK">Next</div>'; | ||
var target = fixture.querySelector('div'); | ||
var actual = rule.matches(target); | ||
assert.isTrue(actual); | ||
}); | ||
|
||
it('returns false for empty text content', function() { | ||
fixture.innerHTML = '<button aria-label="close"></button>'; | ||
var target = fixture.querySelector('button'); | ||
var actual = rule.matches(target); | ||
assert.isFalse(actual); | ||
}); | ||
|
||
it('returns false non text content', function() { | ||
fixture.innerHTML = | ||
'<button aria-label="close"><i class="fa fa-icon-close"></i></button>'; | ||
var target = fixture.querySelector('button'); | ||
var actual = rule.matches(target); | ||
assert.isFalse(actual); | ||
}); | ||
}); | ||
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. Need tests for aria-labelledby. |
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.
aria-labelledby is an element reference. You're just picking up the ID of the element, not the actual text. We have some code for this, but its not isolated properly. Let me complete my accessible name update before completing this. One of the things I'm doing there is to create a getAriaLabelledbyName function.