-
Notifications
You must be signed in to change notification settings - Fork 792
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: Let findUp work on shadow root children #447
Conversation
# Conflicts: # test/testutils.js
b423673
to
02daad1
Compare
@@ -9,33 +9,24 @@ | |||
* @return {HTMLElement|null} Either the matching HTMLElement or `null` if there was no match | |||
*/ | |||
dom.findUp = function (element, target) { | |||
'use strict'; |
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.
These changes don't seem to actually change anything in terms of functionality - is this purely a performance fix?
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.
ok, so the difference here is that if the starting node is the highest node in the shadow DOM, then the old code did not find a matching host node - whereas this code does?
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 problem is that if the child is a directly on the shadow root, it would start looking from the host (outside the shadow), even though the matches array still only contained elements inside the shadow tree.
Followup thought, should this use element.matches() instead of doing querySelectAll() and see if the element is included? I imagine that'd be faster.
test/commons/dom/find-up.js
Outdated
}); | ||
|
||
(shadowSupport.v1 ? it : xit)('should work on shadow root children', function () { | ||
fixture.innerHTML = '<div role="list"><div id="something"></div></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.
shouldn't this attach to the div with role="list"?
Should help resolve #422.