X Tutup
Skip to content

fix(dom_renderer): moveNodeAfterSiblings doesn't detach nodes. Fixes #5077#5520

Closed
mlynch wants to merge 2 commits intoangular:masterfrom
mlynch:fix-5077-move-nodes-after-sibling
Closed

fix(dom_renderer): moveNodeAfterSiblings doesn't detach nodes. Fixes #5077#5520
mlynch wants to merge 2 commits intoangular:masterfrom
mlynch:fix-5077-move-nodes-after-sibling

Conversation

@mlynch
Copy link
Copy Markdown

@mlynch mlynch commented Dec 1, 2015

This fixes the issue described in #5077, where moveNodesAfterSibling would 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.

@googlebot
Copy link
Copy Markdown

googlebot commented Dec 1, 2015

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. I signed it!) and we'll verify. Thanks.

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

tbosch
tbosch reviewed Dec 1, 2015
View reviewed changes
modules/angular2/src/platform/dom/dom_renderer.ts Outdated
Copy link
Copy Markdown
Contributor

@tbosch tbosch Dec 1, 2015

Choose a reason for hiding this comment

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

sib.nextSibling might not exist if sibling is the only child of an element.
In this case, you need to use appendChild...

Copy link
Copy Markdown
Contributor

@tbosch tbosch Dec 1, 2015

Choose a reason for hiding this comment

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

You could cache the parentNode outside of the loop

Copy link
Copy Markdown
Author

@mlynch mlynch Dec 4, 2015

Choose a reason for hiding this comment

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

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.

@tbosch
Copy link
Copy Markdown
Contributor

tbosch commented Dec 1, 2015

So something like this:

function moveNodesAfterSibling(sibling, nodes) {
  var parent = DOM.parentElement(sibling);
  if (nodes.length === 0 || isBlank(parent)) return;
  var nextSibling = sibling.nextSibling;
  if (isPresent(nextSibling)) {
    for (var i = 0; i < nodes.length; i++) {
      parent.insertBefore(nodes[i], nextSibling);
    }    
  } else {
    for (var i = 0; i < nodes.length; i++) {
      parent.appendChild(nodes[i]);
    }      
  }
}

@IgorMinar
Copy link
Copy Markdown
Contributor

IgorMinar commented Dec 2, 2015

@mlynch can you please sign cla?

@IgorMinar IgorMinar added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Dec 2, 2015
@IgorMinar IgorMinar added this to the beta-00 milestone Dec 2, 2015
@mlynch
Copy link
Copy Markdown
Author

mlynch commented Dec 3, 2015

I signed it, wasn't sure if we had a corp one already. I can look into those tweaks @tbosch

@googlebot
Copy link
Copy Markdown

googlebot commented Dec 3, 2015

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Dec 3, 2015
@IgorMinar
Copy link
Copy Markdown
Contributor

IgorMinar commented Dec 3, 2015

@mlynch thanks for CLA, can you make the changes @tbosch so that we can merge this in?

@IgorMinar
Copy link
Copy Markdown
Contributor

IgorMinar commented Dec 3, 2015

sorry for the typo: I meant to write if @mlynch could make the changes that @tbosch requested so that we can land this.

@mlynch if you want this to be for sure in beta then this needs to land this week. thanks

@mlynch
Copy link
Copy Markdown
Author

mlynch commented Dec 4, 2015

@tbosch see above, with caching. Didn't add the nextSibling check because, as I understand, semantics of insertBefore are the same as appendChild when the target node is null (which it would be if nextSibling does not exist).

tbosch
tbosch reviewed Dec 4, 2015
View reviewed changes
modules/angular2/src/platform/dom/dom_renderer.ts
Copy link
Copy Markdown
Contributor

@tbosch tbosch Dec 4, 2015

Choose a reason for hiding this comment

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

Why do we need to update sib and can't just always use sibling as the second argument?

Copy link
Copy Markdown
Author

@mlynch mlynch Dec 4, 2015

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@tbosch tbosch Dec 5, 2015

Choose a reason for hiding this comment

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

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);

@tbosch
Copy link
Copy Markdown
Contributor

tbosch commented Dec 4, 2015

@mlynch Just one more question (see above)...

@IgorMinar
Copy link
Copy Markdown
Contributor

IgorMinar commented Dec 8, 2015

and CI is still failing in a catastrophic way..

@mlynch
Copy link
Copy Markdown
Author

mlynch commented Dec 9, 2015

CI looks like dart type errors with ScriptElement. I don't have any of the dart tooling installed or know dart at all.

That solution looks fine to me if it works @tbosch. I'll defer to you on this.

@tbosch
Copy link
Copy Markdown
Contributor

tbosch commented Dec 9, 2015

@mlynch Ok, will take it over. See #5759

@tbosch
Copy link
Copy Markdown
Contributor

tbosch commented Dec 9, 2015

Closing this PR.

@tbosch tbosch closed this Dec 9, 2015
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot bot commented Sep 7, 2019

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

@tbosch tbosch

Labels

action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews cla: yes

Projects

None yet

Milestone

beta.0

Development

Successfully merging this pull request may close these issues.

4 participants

@mlynch @googlebot @tbosch @IgorMinar
X Tutup