[WIP] feat(rule): label content name mismatch #1159
Conversation
jeeyyy
changed the title
| .toLowerCase() | ||
| .trim(); | ||
| } | ||
| if (elm.getAttribute('aria-labelledby')) { |
There was a problem hiding this comment.
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.
| return false; | ||
| } | ||
|
|
||
| const contentText = node.textContent.toLowerCase().trim(); |
There was a problem hiding this comment.
You should use text.sanitize. It takes things like double spaces into account as well. Related question, does textContent solve for HTML entities like ? We should probably have a test that makes sure HTML Entities aren't seen as different characters.
| } | ||
|
|
||
| const contentText = node.textContent.toLowerCase().trim(); | ||
| // if no text content - fail |
There was a problem hiding this comment.
Why wouldn't we ignore these types of elements?
| "matches": "label-content-name-mismatch-matches.js", | ||
| "tags": [ | ||
| "wcag21a", | ||
| "wcag253" |
There was a problem hiding this comment.
We should make this rule experimental for the time being. I don't trust that this isn't going to give false positives.
| "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.
I suggest: The visible text of the element is different from its accessible name
| assert.isFalse(actual); | ||
| }); | ||
|
|
||
| it('returns false when element has an empty aria-labelledby', function() { |
There was a problem hiding this comment.
Same here, empty aria-labelledby will be ignored by the name computation. This shouldn't fail.
There was a problem hiding this comment.
Same comment as above, repeating:
getAriaLabelledbyText authored for accessible name computation, does not take that into account, and does not return the content as text :/.
Perhaps the accessible name computation should take this into consideration. Correct me if I am wrong.
|
|
||
| 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.
This too, if there is no content, there is no mismatch and so this shouldn't fail.
| var actual = check.evaluate(node, options, vNode); | ||
| assert.isFalse(actual); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
You're missing quite a few tests here:
- aria-labelledby with reference to an element that matches / that doesn't match
- aria-labelledby with references to multiple elements
- aria-labelledby with references to non-existing elements
- Elements with hidden texts like: `? help
- Elements with ASCII as non-text content
<button aria-label="close">X</button> - ...
| @@ -0,0 +1,87 @@ | |||
| describe('label-content-name-mismatch-matches', function() { | |||
There was a problem hiding this comment.
I don't think the following test case is addressed:
https://auto-wcag.github.io/auto-wcag/_testcases-embeds/SC2-5-3-label-content-name-mismatch_inapplicable_example_5.html
| var actual = rule.matches(target); | ||
| assert.isFalse(actual); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Need tests for aria-labelledby.
jeeyyy
changed the title
|
Dependency on - #1163 |
|
Dependency on - #1163 |
This PR implements a new WCAG2.1 rule - label content name mismatch
Closes issue:
Reviewer checks
Required fields, to be filled out by PR reviewer(s)