Repository navigation
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
There was a problem hiding this comment.
sib.nextSibling might not exist if sibling is the only child of an element.
In this case, you need to use appendChild...
There was a problem hiding this comment.
You could cache the parentNode outside of the loop
There was a problem hiding this comment.
If sib.nextSibling is null because there is no next sibling, it's fine, the node will get appended to parentNode's children list: https://developer.mozilla.org/en-US/docs/Web/API/Node/insertBefore
Would you agree?
I'll try the caching approach.
|
So something like this: |
|
@mlynch can you please sign cla? |
IgorMinar
added
the
action: cleanup
|
I signed it, wasn't sure if we had a corp one already. I can look into those tweaks @tbosch |
|
CLAs look good, thanks! |
|
@tbosch see above, with caching. Didn't add the |
There was a problem hiding this comment.
Why do we need to update sib and can't just always use sibling as the second argument?
There was a problem hiding this comment.
Well we need to make sure each node is appended after the one before it. I'd have to see what you're thinking exactly but my first thought is, keeping sibling.nextSibling fixed would mean the nodes get appended in reverse order.
There was a problem hiding this comment.
Max, keeping sibling.nextSibling the same works / does not skrew up the order. Try this snippet:
function moveNodesAfterSibling(sibling, nodes) {
var parent = sibling.parentElement;
var nextSibling = sibling.nextSibling;
for (var i = 0; i < nodes.length; i++) {
parent.insertBefore(nodes[i], nextSibling);
}
}
var parent = document.createElement('div');
parent.innerHTML = '<script></script>';
var nodes = [document.createElement('div'), document.createElement('a'), document.createElement('b')];
moveNodesAfterSibling(parent.firstChild, nodes);
console.log(parent.innerHTML);
|
@mlynch Just one more question (see above)... |
|
and CI is still failing in a catastrophic way.. |
|
CI looks like dart type errors with That solution looks fine to me if it works @tbosch. I'll defer to you on this. |
|
Closing this PR. |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This fixes the issue described in #5077, where
moveNodesAfterSiblingwould incorrectly cause the browser to detach and reattach existing nodes, which caused such undesirable effects as scrollTop being reset to zero.Now, nodes will be appended sequentially and never move the target sibling node.