Skip to content
This repository has been archived by the owner on Dec 7, 2022. It is now read-only.

handle nodes hosted in a <template> #338

Merged
merged 5 commits into from
Mar 3, 2017

Conversation

valdrinkoshi
Copy link
Contributor

Fixes #337.
@alice FYI

Copy link
Contributor

@alice alice left a comment

Choose a reason for hiding this comment

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

One suggestion but looks good.

@@ -175,7 +175,8 @@ axs.properties.hasDirectTextDescendant = function(element) {
while (treeWalker.nextNode()) {
var resultElement = treeWalker.currentNode;
var parent = resultElement.parentNode;
var tagName = parent.tagName.toLowerCase();
// parent might not be an Element (e.g. DocumentFragment).
var tagName = (parent.tagName || '').toLowerCase();
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to just switch to parentElement on the previous line?

@valdrinkoshi valdrinkoshi changed the title handle parents that are not of type Element handle nodes hosted in a <template> Mar 3, 2017
@valdrinkoshi
Copy link
Contributor Author

valdrinkoshi commented Mar 3, 2017

@alice I've realized the issue happens only if a node is hosted in a <template>, because if we use a DocumentFragment to host nodes and then add the fragment to the main document, the parent of each node will be the new parent, not the DocumentFragment. This is not the case for <template>:

var frag = document.createDocumentFragment();
frag.appendChild(document.createElement('div'));
document.body.appendChild(frag); // the <div>.parentNode is <body>

var templ = document.createElement('template');
templ.content.appendChild(document.createElement('div'));
document.body.appendChild(templ); // the <div>.parentNode is <template>.content!

I've updated the test and the implementation to handle specifically <template>, PTAL 🍰

Copy link
Contributor

@alice alice left a comment

Choose a reason for hiding this comment

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

Nice fix, LGTM 👍

@addyosmani
Copy link
Member

LGTM2

@addyosmani addyosmani merged commit 05ca43e into GoogleChrome:master Mar 3, 2017
@valdrinkoshi valdrinkoshi deleted the fix-337 branch March 3, 2017 19:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants