-
Notifications
You must be signed in to change notification settings - Fork 776
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
Shadow DOM support for color contrast matches #413
Conversation
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.
@marcysutton There are a bunch of changes that need to be made in lib/rules/color-contrast-matches.js
to get it to support shadow DOM correctly.
- When finding the appropriate
document
object to use for idref lookups, you need to useaxe.commons.dom.getRootNode(node)
. So for example before line 43, you need to callgetRootNode
to get the document for therelevantNode
so the idref dereferences correctly. - On line 50, where the code is looking for a wrapped input element, we need to use the
axe.utils.querySelectorAll()
function on the virtual node of the element. - On line 58, need to use
getRootNode
for the correct lookup - On line 69, we need to use the
virtualNode.children
array instead ofnode.childNodes
and then in thefor
loop, make sure we are looking at theactualNode
for each virtual child node. - Finally, we need some tests that test all of these conditions
66b1df7
to
6785537
Compare
@dylanb I've completed the changes and added some more tests. Ready for another review. |
lib/rules/color-contrast-matches.js
Outdated
@@ -2,7 +2,7 @@ | |||
|
|||
var nodeName = node.nodeName.toUpperCase(), | |||
nodeType = node.type, | |||
doc = document; | |||
doc = axe.commons.dom.getRootNode(node); |
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.
on line 40, the relevantNode
can change and therefore the doc
might change too for the lookup on line 43. I think this code should be moved to where the doc
is actually being used
var shadowRoot = document.getElementById('parent').attachShadow({ mode: 'open' }); | ||
shadowRoot.innerHTML = '<div id="shadowParent">' + | ||
'<label for="input" id="shadowLabel">Label' + |
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 is another explicit label
}); | ||
|
||
it('should look at the children of a virtual node for overlap', function () { |
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.
Shouldn't this test be one that overlaps and therefore returns false?
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.
Doesn't the default CSS overlap since the text node is a child?
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.
Also that conditional returns true–I thought it was checking to see if there was overlapping content worth evaluating.
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.
👍
assert.isTrue(rule.matches(firstChild, axe.utils.getNodeFromTree(tree[0], firstChild))); | ||
}); | ||
} |
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.
I think we should add a test where the implicit label is in the light DOM and the input in the shadow DOM
fixture.innerHTML = '<label>Text<div id="firstChild"></div></label>'
...
shadowRoot.innerHTML = '<input id="shadowTarget" type="text" />'
...
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.
Test added.
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.
actually, I was thinking of the disabled case where the input that is inside the shadow DOM is disabled and it gets excluded from the color contrast check - apologies for not being clear enough.
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.
The approach I took with other tests was to put a second, enabled input in the light DOM with the same ID and an explicit pairing with the label–that way it would return true
if the shadow boundary wasn't being handled properly. Otherwise it's difficult to test the isFalse
case by itself since that would mean it didn't get matched (and could have occurred for any number of reasons). The problem with this one is the <label>
is already in the light DOM, so it would get paired with the first input. for
/id
pairings also don't span shadow boundaries, because of scoping. So I'll find a different way to write this test.
lib/rules/color-contrast-matches.js
Outdated
@@ -40,41 +39,43 @@ if (nodeName === 'LABEL' || nodeParentLabel) { | |||
relevantNode = nodeParentLabel; | |||
} | |||
// explicit label of disabled input | |||
var doc = axe.commons.dom.getRootNode(node); |
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.
should be var doc = axe.commons.dom.getRootNode(relevantNode);
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.
Also, probably use let
to block scope it
a4294c7
to
f1d6a51
Compare
f1d6a51
to
3a5be92
Compare
@dylanb I've incorporated those two test changes, it's ready for another review. |
This PR builds on #404 to pull in some of the commons already in use.
Closes #393.