-
Notifications
You must be signed in to change notification settings - Fork 794
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(valid-lang): ignore lang on elements with no text #3523
Changes from 6 commits
867681b
819e0ff
497621c
f9c82be
2437511
8007fb2
9f5df5f
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,24 @@ | ||
import { hasChildTextNodes } from './has-content-virtual'; | ||
import isVisualContent from './is-visual-content'; | ||
import isVisible from './is-visible'; | ||
|
||
/** | ||
* Check that a node has text, or an accessible name which language is defined by the | ||
* nearest ancestor's lang attribute. | ||
* @param {VirtualNode} virtualNode | ||
* @return boolean | ||
*/ | ||
export default function hasLangText(virtualNode) { | ||
if (typeof virtualNode.children === 'undefined' || hasChildTextNodes(virtualNode)) { | ||
return true; | ||
} | ||
if (virtualNode.props.nodeType === 1 && isVisualContent(virtualNode)) { | ||
// See: https://github.com/dequelabs/axe-core/issues/3281 | ||
return !!axe.commons.text.accessibleTextVirtual(virtualNode); | ||
} | ||
return virtualNode.children.some(child => ( | ||
!child.attr('lang') && // non-empty lang | ||
hasLangText(child) && // has text | ||
isVisible(child, true) // Not hidden for AT | ||
)); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,13 @@ | ||
import { getNodeFromTree } from '../../core/utils'; | ||
import AbstractVirtualNode from '../../core/base/virtual-node/abstract-virtual-node'; | ||
|
||
const visualRoles = [ | ||
'checkbox', | ||
'img', | ||
'meter', | ||
'progressbar', | ||
'scrollbar', | ||
'radio', | ||
'range', | ||
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. Range is an abstract role. This should've never been here. I replaced it with roles that inherit from range. |
||
'slider', | ||
'spinbutton', | ||
'textbox' | ||
|
@@ -13,34 +18,34 @@ const visualRoles = [ | |
* @method isVisualContent | ||
* @memberof axe.commons.dom | ||
* @instance | ||
* @param {Element} element The element to check | ||
* @param {Element|VirtualNode} element The element to check | ||
* @return {Boolean} | ||
*/ | ||
function isVisualContent(element) { | ||
/*eslint indent: 0*/ | ||
const role = element.getAttribute('role'); | ||
function isVisualContent(el) { | ||
const vNode = el instanceof AbstractVirtualNode ? el : getNodeFromTree(el); | ||
const role = axe.commons.aria.getExplicitRole(vNode); | ||
if (role) { | ||
return visualRoles.indexOf(role) !== -1; | ||
} | ||
|
||
switch (element.nodeName.toUpperCase()) { | ||
case 'IMG': | ||
case 'IFRAME': | ||
case 'OBJECT': | ||
case 'VIDEO': | ||
case 'AUDIO': | ||
case 'CANVAS': | ||
case 'SVG': | ||
case 'MATH': | ||
case 'BUTTON': | ||
case 'SELECT': | ||
case 'TEXTAREA': | ||
case 'KEYGEN': | ||
case 'PROGRESS': | ||
case 'METER': | ||
switch (vNode.props.nodeName) { | ||
case 'img': | ||
case 'iframe': | ||
case 'object': | ||
case 'video': | ||
case 'audio': | ||
case 'canvas': | ||
case 'svg': | ||
case 'math': | ||
case 'button': | ||
case 'select': | ||
case 'textarea': | ||
case 'keygen': | ||
case 'progress': | ||
case 'meter': | ||
return true; | ||
case 'INPUT': | ||
return element.type !== 'hidden'; | ||
case 'input': | ||
return vNode.props.type !== 'hidden'; | ||
default: | ||
return false; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
import { hasLangText } from '../commons/dom' | ||
|
||
function hasLangTextMatches(node, virtualNode) { | ||
return hasLangText(virtualNode); | ||
} | ||
|
||
export default hasLangTextMatches; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
{ | ||
"id": "valid-lang", | ||
"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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we switch these around? I.e. only look for the text if the lang value is invalid? |
||
"tags": ["cat.language", "wcag2aa", "wcag312", "ACT"], | ||
"actIds": ["de46e4"], | ||
"metadata": { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
describe('dom.hasLangText', function() { | ||
'use strict'; | ||
var hasLangText = axe.commons.dom.hasLangText; | ||
var fixture = document.getElementById('fixture'); | ||
var tree; | ||
|
||
it('returns true when the element has a non-empty text node as its content', function () { | ||
fixture.innerHTML = '<div id="target"> text </div>'; | ||
tree = axe.utils.getFlattenedTree(fixture); | ||
var target = axe.utils.querySelectorAll(tree, '#target')[0]; | ||
assert.isTrue(hasLangText(target)); | ||
}); | ||
|
||
it('returns true when the element has nested text node as its content', function () { | ||
fixture.innerHTML = '<div id="target"> <span> text </span> </div>'; | ||
tree = axe.utils.getFlattenedTree(fixture); | ||
var target = axe.utils.querySelectorAll(tree, '#target')[0]; | ||
assert.isTrue(hasLangText(target)); | ||
}); | ||
|
||
it('returns false when the element has nested text is hidden', function () { | ||
fixture.innerHTML = '<div id="target"> <span hidden> text </span> </div>'; | ||
tree = axe.utils.getFlattenedTree(fixture); | ||
var target = axe.utils.querySelectorAll(tree, '#target')[0]; | ||
assert.isFalse(hasLangText(target)); | ||
}); | ||
|
||
it('returns false when the element has an empty text node as its content', function () { | ||
fixture.innerHTML = '<div id="target"> <!-- comment --> </div>'; | ||
tree = axe.utils.getFlattenedTree(fixture); | ||
var target = axe.utils.querySelectorAll(tree, '#target')[0]; | ||
assert.isFalse(hasLangText(target)); | ||
}); | ||
|
||
it('returns false if all text is in a child with a lang attribute', function () { | ||
fixture.innerHTML = '<div id="target"><span lang="en"> text </span></div>'; | ||
tree = axe.utils.getFlattenedTree(fixture); | ||
var target = axe.utils.querySelectorAll(tree, '#target')[0]; | ||
assert.isFalse(hasLangText(target)); | ||
}); | ||
|
||
it('does not skip if lang is on the starting node', function () { | ||
fixture.innerHTML = '<div id="target" lang="en"><span> text </span></div>'; | ||
tree = axe.utils.getFlattenedTree(fixture); | ||
var target = axe.utils.querySelectorAll(tree, '#target')[0]; | ||
assert.isTrue(hasLangText(target)); | ||
}); | ||
|
||
it('ignores empty lang attributes', function () { | ||
fixture.innerHTML = '<div id="target"><span lang=""> text </span></div>'; | ||
tree = axe.utils.getFlattenedTree(fixture); | ||
var target = axe.utils.querySelectorAll(tree, '#target')[0]; | ||
assert.isTrue(hasLangText(target)); | ||
}); | ||
|
||
it('ignores null lang attributes', function () { | ||
fixture.innerHTML = '<div id="target"><span lang> text </span></div>'; | ||
tree = axe.utils.getFlattenedTree(fixture); | ||
var target = axe.utils.querySelectorAll(tree, '#target')[0]; | ||
assert.isTrue(hasLangText(target)); | ||
}); | ||
|
||
it('true for non-text content with an accessible name', function () { | ||
fixture.innerHTML = '<div id="target"><img alt="foo"></div>'; | ||
tree = axe.utils.getFlattenedTree(fixture); | ||
var target = axe.utils.querySelectorAll(tree, '#target')[0]; | ||
assert.isTrue(hasLangText(target)); | ||
}); | ||
|
||
it('false for non-text content without accessible name', function () { | ||
fixture.innerHTML = '<div id="target"><img alt=""></div>'; | ||
tree = axe.utils.getFlattenedTree(fixture); | ||
var target = axe.utils.querySelectorAll(tree, '#target')[0]; | ||
assert.isFalse(hasLangText(target)); | ||
}); | ||
|
||
it('returns false for non-text content with a lang attr', function () { | ||
fixture.innerHTML = '<div id="target"><img alt="foo" lang="en"></div>'; | ||
tree = axe.utils.getFlattenedTree(fixture); | ||
var target = axe.utils.querySelectorAll(tree, '#target')[0]; | ||
assert.isFalse(hasLangText(target)); | ||
}); | ||
}); |
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.
this call here is going to be slow
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.
Possibly, although it'll be rare. You generally don't see people put lang attributes on images with aria-labelledby or whatever.