-
Notifications
You must be signed in to change notification settings - Fork 783
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
Update remaining checks to support Shadow DOM #733
Conversation
Until we have bi-directional node/vNode APIs it didn't make sense to use virtual methods everywhere
var mainIsFound = checks['has-at-least-one-main'].evaluate.apply(checkContext, params); | ||
assert.isFalse(mainIsFound); | ||
assert.deepEqual(checkContext._data, mainIsFound); | ||
}); | ||
|
||
it('should return false if div has empty role', function() { | ||
var params = checkSetup('<div id = "target" role = "">Empty role</div>'); | ||
var params = checkSetup('<div id="target" role = "">Empty role</div>'); |
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.
Why didn't you fix the role = ""
?
@@ -1,4 +1,5 @@ | |||
let siblings = Array.from(node.parentNode.children); | |||
let parent = node.parentNode; |
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 change does not seem necessary
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.
ah, a leftover thing from some experimentation I did. you're right–I thought I restored it, but I guess not.
this._data = d; | ||
}, | ||
relatedNodes: function (nodes) { | ||
this._relatedNodes = Array.isArray(nodes) ? nodes : [nodes]; |
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 we make MockCheckContext
do the same conditional assignment as this is doing?
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.
It does do it–that's how I got the tests to pass
}); | ||
|
||
it('should return false if no div has role property', function() { | ||
var params = checkSetup('<div id = "target">No role</div>'); | ||
var params = checkSetup('<div id="target">No role</div>'); |
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 a style
change and should not be mixed up with other changes
@@ -13,17 +13,14 @@ describe('tabindex', function () { | |||
fixture.appendChild(node); | |||
|
|||
assert.isFalse(checks.tabindex.evaluate(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.
style changes
* @return Array | ||
*/ | ||
testUtils.shadowCheckSetup = function (content, shadowContent, targetSelector, options) { | ||
testUtils.shadowCheckSetup = function (content, shadowContent, options, targetSelector) { |
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.
Why the switch?
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.
To make it more consistent with checkSetup
. There are no instances of targetSelector
, yet there are options passed in. I wanted to preserve the functionality but make the two methods more consistent
d2ce6e5
to
0e48413
Compare
I've updated the PR with a style-specific commit and addressed the other feedback. |
I went through each of the checks to see if there was Shadow DOM coverage, and there were a couple that needed updating:
There were a few others where I simply added unit tests:
And a few I updated to use accessibleTextVirtual:
I also did a bunch of test cleanup and fixed up the shadowCheckSetup utility to support a wider range of scenarios.
Closes #690