fix(valid-lang): ignore lang on elements with no text #3523
Conversation
WilcoFiers
force-pushed
the
valid-lang-act-fix
branch
from
2e1e962
to
497621c
Compare
There was a problem hiding this comment.
While we're here, do we need to handle aria-owns applying the lang attribute to the owned children?
<div role="list" lang="fr" aria-owns="contents"></div>
<div id="contents" role="none">
<div role="listitem">Ceci est un paragraphe</div>
</div>In JAWS / Chrome and JAWS / Edge, JAWS applies the French accent to the owned listitems. However our code would treat the lang node as having no children text nodes.
For completeness:
- NVDA / Firefox - only applies French accent when the
#contentselement has thelang - JAWS / IE11 - only applies French accent when the
#contentselement has thelang - VO / Safari - I couldn't get this one to work as VoiceOver seems to ignore the
langattribute and autodetect the language from the sentence? Tried various combinations of words and using or not usinglangand VoiceOver would just apply whatever language it thought the sentence was
lib/commons/dom/is-visual-content.js
Outdated
| function isVisualContent(el) { | ||
| const vNode = el instanceof AbstractVirtualNode ? el : getNodeFromTree(el); | ||
| const role = axe.commons.aria.getExplicitRole(vNode); | ||
| console.log({ role }) |
There was a problem hiding this comment.
| console.log({ role }) |
| 'radio', | ||
| 'range', |
There was a problem hiding this comment.
Range is an abstract role. This should've never been here. I replaced it with roles that inherit from range.
| @@ -79,7 +79,7 @@ <h2>Ok</h2> | |||
| <title>I am a circle</title> | |||
| <circle cx="50" cy="50" r="40" fill="yellow"></circle> | |||
| </svg> | |||
| <div lang="en"></div> | |||
| <div lang="en">English</div> | |||
There was a problem hiding this comment.
Since this is now inapplicable, rather than a pass.
| var rule; | ||
|
|
||
| beforeEach(function() { | ||
| rule = axe.utils.getRule('valid-lang'); |
There was a problem hiding this comment.
There's no way to run these tests, since there's no way to get at the matcher. I'm deprecating the matcher in #3524
WilcoFiers
force-pushed
the
valid-lang-act-fix
branch
from
31b282e
to
8007fb2
Compare
| } | ||
| if (virtualNode.props.nodeType === 1 && isVisualContent(virtualNode)) { | ||
| // See: https://github.com/dequelabs/axe-core/issues/3281 | ||
| return !!axe.commons.text.accessibleTextVirtual(virtualNode); |
There was a problem hiding this comment.
this call here is going to be slow
There was a problem hiding this comment.
Possibly, although it'll be rare. You generally don't see people put lang attributes on images with aria-labelledby or whatever.
lib/rules/valid-lang.json
Outdated
| "selector": "[lang], [xml\\:lang]", | ||
| "matches": "not-html-matches", | ||
| "selector": "[lang]:not(html), [xml\\:lang]:not(html)", | ||
| "matches": "has-lang-text-matches", |
There was a problem hiding this comment.
Could we switch these around? I.e. only look for the text if the lang value is invalid?
|
|
||
| return false; | ||
| if ( // Except for `html`, ignore elements with no text | ||
| virtualNode.props.nodeName !== 'html' && |
There was a problem hiding this comment.
This can't be tested with native DOM, so I added a virtual-rule test for this: https://github.com/dequelabs/axe-core/pull/3523/files#diff-fa33ce173bbfcbc789400648240a5ff95ee85b8b174dddee84ae4f30d6370754R81
|
As responded on slack, I don't think AAA rules should be enabled by default. It creates duplicate issues, like 99% of the times this rule fails, so does the AA version of it. And we probably don't want color-contrast-enhanced running by default. |
* fix(valid-lang): ignore lang on elements with no text * Delete broken / unnecessary test * Fix failing tests * fix broken test * Address feedback * Disable flakey IE test * Make hasLangText part of valid-lang check
Have the valid-lang rule ignore lang attributes on elements without text, of where its text is in a child node with yet another lang attribute.
Closes issue: #3465
Following this PR, i'll make not-html-matches deprecated in #3524, to get the tests on this one passing though I'm already taking out the tests for it.