Skip to content
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: Use virtualNode in hidden-content rule #404

Merged
merged 7 commits into from
Jul 10, 2017
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
13 changes: 7 additions & 6 deletions lib/checks/visibility/hidden-content.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
let styles = window.getComputedStyle(node);
const whitelist = ['SCRIPT', 'HEAD', 'TITLE', 'NOSCRIPT', 'STYLE', 'TEMPLATE'];
if (!whitelist.includes(node.tagName.toUpperCase()) &&
axe.commons.dom.hasContent(virtualNode)) {

let whitelist = ['SCRIPT', 'HEAD', 'TITLE', 'NOSCRIPT', 'STYLE', 'TEMPLATE'];
if (!whitelist.includes(node.tagName.toUpperCase()) && axe.commons.dom.hasContent(node)) {
const styles = window.getComputedStyle(node);
if (styles.getPropertyValue('display') === 'none') {
return undefined;
} else if (styles.getPropertyValue('visibility') === 'hidden') {
if (node.parentNode) {
var parentStyle = window.getComputedStyle(node.parentNode);
}
// Check if visibility isn't inherited
const parent = axe.commons.dom.getComposedParent(node);
const parentStyle = parent && window.getComputedStyle(parent);
if (!parentStyle || parentStyle.getPropertyValue('visibility') !== 'hidden') {
return undefined;
}
Expand Down
5 changes: 1 addition & 4 deletions lib/commons/dom/find-up.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,8 @@ dom.findUp = function (element, target) {
return null;
}

parent = (element.assignedSlot) ? element.assignedSlot : element.parentNode;
if (parent.nodeType === 11) {
parent = parent.host;
}
// recursively walk up the DOM, checking each parent node
parent = dom.getComposedParent(element);
while (parent && matches.indexOf(parent) === -1) {
parent = (parent.assignedSlot) ? parent.assignedSlot : parent.parentNode;
if (parent && parent.nodeType === 11) {
Expand Down
19 changes: 19 additions & 0 deletions lib/commons/dom/get-composed-parent.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/*global dom */
/**
* Get an element's parent in the composed tree
* @param DOMNode Element
* @return DOMNode Parent element
*/
dom.getComposedParent = function getComposedParent (element) {
if (element.assignedSlot) {
return element.assignedSlot; // content of a shadow DOM slot
} else if (element.parentNode) {
var parentNode = element.parentNode;
if (parentNode.nodeType === 1) {
return parentNode; // Regular node
} else if (parentNode.host) {
return parentNode.host; // Shadow root
}
}
return null; // Root node
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for all the comments! They're helpful. 👍

};
46 changes: 27 additions & 19 deletions lib/commons/dom/has-content.js
Original file line number Diff line number Diff line change
@@ -1,27 +1,35 @@
/*global dom, aria, axe */
/*global dom, aria */
const hiddenTextElms = [
'HEAD', 'TITLE', 'TEMPLATE', 'SCRIPT','STYLE',
'IFRAME', 'OBJECT', 'VIDEO', 'AUDIO', 'NOSCRIPT'
];

function hasChildTextNodes (elm) {
if (!hiddenTextElms.includes(elm.actualNode.nodeName.toUpperCase())) {
return elm.children.some(({ actualNode }) => (
actualNode.nodeType === 3 && actualNode.nodeValue.trim()
));
}
}

/**
* Check that the element has visible content
* in the form of either text, an aria-label or visual content such as image
*
* @param {Object} virtual DOM node
* @return boolean
*/
dom.hasContent = function hasContent(elm) {
if (
elm.actualNode.textContent.trim() ||
aria.label(elm)
) {
return true;
}

const contentElms = axe.utils.querySelectorAll(elm, '*');
for (let i = 0; i < contentElms.length; i++) {
if (
aria.label(contentElms[i]) ||
dom.isVisualContent(contentElms[i].actualNode)
) {
return true;
}
}
return false;
dom.hasContent = function hasContent (elm) {
return (
// It has text
hasChildTextNodes(elm) ||
// It is a graphical element
dom.isVisualContent(elm.actualNode) ||
// It has an ARIA label
!!aria.label(elm) ||
// or one of it's descendants does
elm.children.some(child => (
child.actualNode.nodeType === 1 && dom.hasContent(child)
))
);
};
2 changes: 0 additions & 2 deletions lib/commons/dom/is-visible.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,9 @@ dom.isVisible = function (el, screenReader, recursed) {
}

parent = (el.assignedSlot) ? el.assignedSlot : el.parentNode;

if (parent) {
return dom.isVisible(parent, screenReader, true);
}

return false;

};
65 changes: 50 additions & 15 deletions test/checks/visibility/hidden-content.js
Original file line number Diff line number Diff line change
@@ -1,46 +1,81 @@
/* global xit */
describe('hidden content', function () {
'use strict';

var fixture = document.getElementById('fixture');

var fixture = document.getElementById('fixture');
var shadowSupport = document.body && typeof document.body.attachShadow === 'function';
var checkContext = {
_data: null,
data: function (d) {
this._data = d;
}
};

function checkSetup (html, options, target) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we abstract this into a reusable utility so it doesn't have to get repeated in every test file needing Shadow DOM support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. I like having some test utilities to work with, but I don't much like just dropping a bunch of extra functions on the global object. I suppose we could create a /test/utils dir, and have a global testUtils object that has everything on it. Something like that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Something we could import would be helpful–then if it needed additional code, we could change it in one place.

fixture.innerHTML = html;
axe._tree = axe.utils.getFlattenedTree(fixture);
var node = fixture.querySelector(target || '#target');
var virtualNode = axe.utils.getNodeFromTree(axe._tree[0], node);
return [node, options, virtualNode];
}

afterEach(function () {
fixture.innerHTML = '';
checkContext._data = null;
axe._tree = undefined;
});

it('should return undefined with display:none and children', function () {
fixture.innerHTML = '<div id="target" style="display: none;"><p>Some paragraph text.</p></div>';
var node = fixture.querySelector('#target');
assert.isUndefined(checks['hidden-content'].evaluate.call(checkContext, node));
var params = checkSetup('<div id="target" style="display: none;"><p>Some paragraph text.</p></div>');
assert.isUndefined(checks['hidden-content'].evaluate.apply(checkContext, params));
});

it('should return undefined with visibility:hidden and children', function () {
fixture.innerHTML = '<div id="target" style="visibility: hidden;"><p>Some paragraph text.</p></div>';
var node = fixture.querySelector('#target');
assert.isUndefined(checks['hidden-content'].evaluate.call(checkContext, node));
var params = checkSetup('<div id="target" style="visibility: hidden;"><p>Some paragraph text.</p></div>');
assert.isUndefined(checks['hidden-content'].evaluate.apply(checkContext, params));
});

it('should return true with visibility:hidden and parent with visibility:hidden', function () {
fixture.innerHTML = '<div style="visibility: hidden;"><p id="target" style="visibility: hidden;">Some paragraph text.</p></div>';
var node = fixture.querySelector('#target');
assert.isTrue(checks['hidden-content'].evaluate.call(checkContext, node));
var params = checkSetup('<div style="visibility: hidden;"><p id="target" style="visibility: hidden;">Some paragraph text.</p></div>');
assert.isTrue(checks['hidden-content'].evaluate.apply(checkContext, params));
});

it('should return true with aria-hidden and no content', function () {
fixture.innerHTML = '<span id="target" class="icon" aria-hidden="true"></span>';
var node = fixture.querySelector('#target');
assert.isTrue(checks['hidden-content'].evaluate.call(checkContext, node));
var params = checkSetup('<span id="target" class="icon" aria-hidden="true"></span>');
assert.isTrue(checks['hidden-content'].evaluate.apply(checkContext, params));
});

it('should skip whitelisted elements', function () {
var node = document.querySelector('head');
assert.isTrue(checks['hidden-content'].evaluate.call(checkContext, node));
axe._tree = axe.utils.getFlattenedTree(document.documentElement);
var virtualNode = axe.utils.getNodeFromTree(axe._tree[0], node);
assert.isTrue(checks['hidden-content'].evaluate(node, undefined, virtualNode));
});

(shadowSupport ? it : xit)('works on elements in a shadow DOM', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Crafty 👍

fixture.innerHTML = '<div id="shadow"> <div id="content">text</div> </div>';
var shadowRoot = document.getElementById('shadow').attachShadow({ mode: 'open' });
shadowRoot.innerHTML = '<div id="target" style="display:none">' +
'<slot></slot>' +
'</div>';
axe._tree = axe.utils.getFlattenedTree(fixture);

var shadow = document.querySelector('#shadow');
var virtualShadow = axe.utils.getNodeFromTree(axe._tree[0], shadow);
assert.isTrue(
checks['hidden-content'].evaluate(shadow, undefined, virtualShadow)
);

var target = shadowRoot.querySelector('#target');
var virtualTarget = axe.utils.getNodeFromTree(axe._tree[0], target);
assert.isUndefined(
checks['hidden-content'].evaluate(target, undefined, virtualTarget)
);

var content = document.querySelector('#content');
var virtualContent = axe.utils.getNodeFromTree(axe._tree[0], content);
assert.isTrue(
checks['hidden-content'].evaluate(content, undefined, virtualContent)
);
});
});
72 changes: 72 additions & 0 deletions test/commons/dom/get-composed-parent.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/* global xit */
describe('dom.getComposedParent', function () {
'use strict';
var getComposedParent = axe.commons.dom.getComposedParent;
var fixture = document.getElementById('fixture');
var shadowSupport = document.body && typeof document.body.attachShadow === 'function';
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it's short, but this line seems like something we could abstract into a common test utility.

Copy link
Contributor

Choose a reason for hiding this comment

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

This line is only shadow DOM v1, so it should only be used when we are sure that we don't need tests for shadow DOM v0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dylanb I'm not sure what you're suggesting. With Shadow DOM v0 I take it you mean the <content> element and createShadowRoot() method? Those are deprecated now, right? I hadn't considered it. I can see that we might need to write specific tests for our utilities to cover those cases, but I'm not sure our checks need them. Are you suggesting we do?

Copy link
Contributor

Choose a reason for hiding this comment

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

The fact that there are different APIs for v0 and v1 make SD pretty difficult to work with. I'd be curious to hear how much test coverage we need for both.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the general approach should be that if we are relying on utils and commons that have been tested with both versions, then the tests for checks and rules themselves do not need both


afterEach(function () {
fixture.innerHTML = '';
});

it('returns the parentNode normally', function () {
fixture.innerHTML = '<div id="parent"><div id="target"></div></div>';

var actual = getComposedParent(document.getElementById('target'));
assert.instanceOf(actual, Node);
assert.equal(actual, document.getElementById('parent'));
});

it('returns null from the documentElement', function () {
assert.isNull(
getComposedParent(document.documentElement)
);
});

(shadowSupport ? it : xit)('returns the slot node for slotted content', function () {
fixture.innerHTML = '<div id="shadow"><div id="target"></div></div>';
var shadowRoot = document.getElementById('shadow').attachShadow({ mode: 'open' });
shadowRoot.innerHTML = '<div id="grand-parent">' +
'<slot id="parent"></slot>' +
'</div>';

var actual = getComposedParent(fixture.querySelector('#target'));
assert.instanceOf(actual, Node);
assert.equal(actual, shadowRoot.querySelector('#parent'));
});

(shadowSupport ? it : xit)('returns explicitly slotted nodes', function () {
fixture.innerHTML = '<div id="shadow"><div id="target" slot="bar"></div></div>';
var shadowRoot = document.getElementById('shadow').attachShadow({ mode: 'open' });
shadowRoot.innerHTML = '<div id="grand-parent">' +
'<slot name="foo"></slot>' +
'<slot id="parent" name="bar"></slot>' +
'</div>';

var actual = getComposedParent(fixture.querySelector('#target'));
assert.instanceOf(actual, Node);
assert.equal(actual, shadowRoot.querySelector('#parent'));
});

(shadowSupport ? it : xit)('returns elements within a shadow tree', function () {
fixture.innerHTML = '<div id="shadow"> content </div>';
var shadowRoot = document.getElementById('shadow').attachShadow({ mode: 'open' });
shadowRoot.innerHTML = '<div id="parent">' +
'<slot id="target"></slot>' +
'</div>';

var actual = getComposedParent(shadowRoot.querySelector('#target'));
assert.instanceOf(actual, Node);
assert.equal(actual, shadowRoot.querySelector('#parent'));
});

(shadowSupport ? it : xit)('returns the host when it reaches the shadow root', function () {
fixture.innerHTML = '<div id="parent"> content </div>';
var shadowRoot = document.getElementById('parent').attachShadow({ mode: 'open' });
shadowRoot.innerHTML = '<div id="target"> <slot></slot> </div>';

var actual = getComposedParent(shadowRoot.querySelector('#target'));
assert.instanceOf(actual, Node);
assert.equal(actual, fixture.querySelector('#parent'));
});
});
34 changes: 34 additions & 0 deletions test/commons/dom/has-content.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
/* global xit */
describe('dom.hasContent', function () {
'use strict';
var hasContent = axe.commons.dom.hasContent;
var fixture = document.getElementById('fixture');
var shadowSupport = document.body && typeof document.body.attachShadow === 'function';
var tree;

it('returns false if there is no content', function () {
Expand Down Expand Up @@ -51,4 +53,36 @@ describe('dom.hasContent', function () {
hasContent(axe.utils.querySelectorAll(tree, '#target')[0])
);
});

it('is false if the element does not show text', function () {
fixture.innerHTML = '<style id="target"> #foo { color: green } </style>';
tree = axe.utils.getFlattenedTree(fixture);
assert.isFalse(
hasContent(axe.utils.querySelectorAll(tree, '#target')[0])
);
});

(shadowSupport ? it : xit)('looks at content of shadow dom elements', function () {
fixture.innerHTML = '<div id="target"></div>';
var shadow = fixture.querySelector('#target').attachShadow({ mode: 'open' });
shadow.innerHTML = 'Some text';
tree = axe.utils.getFlattenedTree(fixture);

assert.isTrue(
hasContent(axe.utils.querySelectorAll(tree, '#target')[0])
);
});

(shadowSupport ? it : xit)('looks at the slots in a shadow tree', function () {
fixture.innerHTML = '<div id="shadow">some text</div>';
var shadow = fixture.querySelector('#shadow').attachShadow({ mode: 'open' });
shadow.innerHTML = '<div class="target"><slot></slot></div>';
tree = axe.utils.getFlattenedTree(fixture);
var node = axe.utils.querySelectorAll(tree, '.target');

axe.log(tree, node);
assert.isTrue(
hasContent(axe.utils.querySelectorAll(tree, '.target')[0])
);
});
});