Skip to content
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

Strange behavior with insertBefore vs appendChild and transitions #880

Open
dead-claudia opened this issue Jul 19, 2020 · 11 comments
Open

Comments

@dead-claudia
Copy link

When using insertBefore with transitions and the FLIP technique, I get very strange results across browsers.

The only difference is the way the DOM nodes are rearranged, which is what confuses me about this.

Code for each

insertBefore:

<!doctype html>
<ul id="animate"><li>0</li><li>1</li><li>2</li><li>3</li></ul>
<script>
// Factored out so it's clearer what elements are added and removed

const animateRoot = document.getElementById("animate")
const elem0 = animateRoot.childNodes[0]
const elem1 = animateRoot.childNodes[1]
const elem2 = animateRoot.childNodes[2]
const elem3 = animateRoot.childNodes[3]

// ordered: [0, 1, 2, 3]
// mixed:   [2, 0, 3, 1]

function mix() { // ordered -> mixed
  setTimeout(order, 500)
  
  // First
  const first0 = elem0.getBoundingClientRect().top
  const first1 = elem1.getBoundingClientRect().top
  const first2 = elem2.getBoundingClientRect().top
  const first3 = elem3.getBoundingClientRect().top

  // Update [0, 1, 2, 3] -> [2, 0, 3, 1]
  animateRoot.insertBefore(elem2, elem0)
  animateRoot.appendChild(elem1)

  // Last + Invert
  // Note: earlier `transitionDuration` must be set before later
  // `transform`, or even more weirdness occurs (just try it and see
  // what happens).
  elem0.style.transitionDuration =
  elem1.style.transitionDuration =
  elem2.style.transitionDuration =
  elem3.style.transitionDuration = "0s"
  elem0.style.transform = `translateY(${first0 - elem0.getBoundingClientRect().top}px)`
  elem1.style.transform = `translateY(${first1 - elem1.getBoundingClientRect().top}px)`
  elem2.style.transform = `translateY(${first2 - elem2.getBoundingClientRect().top}px)`
  elem3.style.transform = `translateY(${first3 - elem3.getBoundingClientRect().top}px)`

  // Force re-layout
  document.body.offsetHeight

  // Play
  elem0.classList.add("transition")
  elem1.classList.add("transition")
  elem2.classList.add("transition")
  elem3.classList.add("transition")
  elem0.style.transform = elem0.style.transitionDuration =
  elem1.style.transform = elem1.style.transitionDuration =
  elem2.style.transform = elem2.style.transitionDuration =
  elem3.style.transform = elem3.style.transitionDuration = ""
}

function order() { // mixed -> ordered
  setTimeout(mix, 500)
  
  // First
  const first0 = elem0.getBoundingClientRect().top
  const first1 = elem1.getBoundingClientRect().top
  const first2 = elem2.getBoundingClientRect().top
  const first3 = elem3.getBoundingClientRect().top

  // Update [2, 0, 3, 1] -> [0, 1, 2, 3]
  animateRoot.insertBefore(elem1, elem3)
  animateRoot.insertBefore(elem2, elem3)

  // Last + Invert
  // Note: earlier `transitionDuration` must be set before later
  // `transform`, or even more weirdness occurs (just try it and see
  // what happens).
  elem0.style.transitionDuration =
  elem1.style.transitionDuration =
  elem2.style.transitionDuration =
  elem3.style.transitionDuration = "0s"
  elem0.style.transform = `translateY(${first0 - elem0.getBoundingClientRect().top}px)`
  elem1.style.transform = `translateY(${first1 - elem1.getBoundingClientRect().top}px)`
  elem2.style.transform = `translateY(${first2 - elem2.getBoundingClientRect().top}px)`
  elem3.style.transform = `translateY(${first3 - elem3.getBoundingClientRect().top}px)`

  // Force re-layout
  document.body.offsetHeight

  // Play
  elem0.classList.add("transition")
  elem1.classList.add("transition")
  elem2.classList.add("transition")
  elem3.classList.add("transition")
  elem0.style.transform = elem0.style.transitionDuration =
  elem1.style.transform = elem1.style.transitionDuration =
  elem2.style.transform = elem2.style.transitionDuration =
  elem3.style.transform = elem3.style.transitionDuration = ""
}

setTimeout(mix, 500)
</script>

appendChild:

<!doctype html>
<ul id="animate"><li>0</li><li>1</li><li>2</li><li>3</li></ul>
<script>
// Factored out so it's clearer what elements are added and removed

const animateRoot = document.getElementById("animate")
const elem0 = animateRoot.childNodes[0]
const elem1 = animateRoot.childNodes[1]
const elem2 = animateRoot.childNodes[2]
const elem3 = animateRoot.childNodes[3]

// ordered: [0, 1, 2, 3]
// mixed:   [2, 0, 3, 1]

function mix() { // ordered -> mixed
  setTimeout(order, 500)
  
  // First
  const first0 = elem0.getBoundingClientRect().top
  const first1 = elem1.getBoundingClientRect().top
  const first2 = elem2.getBoundingClientRect().top
  const first3 = elem3.getBoundingClientRect().top

  // Update [0, 1, 2, 3] -> [2, 0, 3, 1]
  animateRoot.appendChild(elem2)
  animateRoot.appendChild(elem0)
  animateRoot.appendChild(elem3)
  animateRoot.appendChild(elem1)

  // Last + Invert
  // Note: earlier `transitionDuration` must be set before later
  // `transform`, or even more weirdness occurs (just try it and see
  // what happens).
  elem0.style.transitionDuration =
  elem1.style.transitionDuration =
  elem2.style.transitionDuration =
  elem3.style.transitionDuration = "0s"
  elem0.style.transform = `translateY(${first0 - elem0.getBoundingClientRect().top}px)`
  elem1.style.transform = `translateY(${first1 - elem1.getBoundingClientRect().top}px)`
  elem2.style.transform = `translateY(${first2 - elem2.getBoundingClientRect().top}px)`
  elem3.style.transform = `translateY(${first3 - elem3.getBoundingClientRect().top}px)`

  // Force re-layout
  document.body.offsetHeight

  // Play
  elem0.classList.add("transition")
  elem1.classList.add("transition")
  elem2.classList.add("transition")
  elem3.classList.add("transition")
  elem0.style.transform = elem0.style.transitionDuration =
  elem1.style.transform = elem1.style.transitionDuration =
  elem2.style.transform = elem2.style.transitionDuration =
  elem3.style.transform = elem3.style.transitionDuration = ""
}

function order() { // mixed -> ordered
  setTimeout(mix, 500)
  
  // First
  const first0 = elem0.getBoundingClientRect().top
  const first1 = elem1.getBoundingClientRect().top
  const first2 = elem2.getBoundingClientRect().top
  const first3 = elem3.getBoundingClientRect().top

  // Update [2, 0, 3, 1] -> [0, 1, 2, 3]
  animateRoot.appendChild(elem0)
  animateRoot.appendChild(elem1)
  animateRoot.appendChild(elem2)
  animateRoot.appendChild(elem3)

  // Last + Invert
  // Note: earlier `transitionDuration` must be set before later
  // `transform`, or even more weirdness occurs (just try it and see
  // what happens).
  elem0.style.transitionDuration =
  elem1.style.transitionDuration =
  elem2.style.transitionDuration =
  elem3.style.transitionDuration = "0s"
  elem0.style.transform = `translateY(${first0 - elem0.getBoundingClientRect().top}px)`
  elem1.style.transform = `translateY(${first1 - elem1.getBoundingClientRect().top}px)`
  elem2.style.transform = `translateY(${first2 - elem2.getBoundingClientRect().top}px)`
  elem3.style.transform = `translateY(${first3 - elem3.getBoundingClientRect().top}px)`

  // Force re-layout
  document.body.offsetHeight

  // Play
  elem0.classList.add("transition")
  elem1.classList.add("transition")
  elem2.classList.add("transition")
  elem3.classList.add("transition")
  elem0.style.transform = elem0.style.transitionDuration =
  elem1.style.transform = elem1.style.transitionDuration =
  elem2.style.transform = elem2.style.transitionDuration =
  elem3.style.transform = elem3.style.transitionDuration = ""
}

setTimeout(mix, 500)
</script>

I've reproduced this behavior explained above on each of the following platforms:

  • Chrome 83.0.4103.116 on Windows 10 64-bit
  • Safari 13.1 on iOS 13
  • Firefox 78.0.2 on Windows 10 64-bit

Relevant framework bugs I've filed, before I narrowed it down further to this:


I suspect it's a bug in all three of those listed browsers, but I'm filing an issue here in case it's actually a spec issue or if a spec note is necessary for this.

@annevk
Copy link
Member

annevk commented Jul 19, 2020

Maybe @emilio or @tabatkins knows why this happens. This should probably be filed over at https://github.com/w3c/csswg-drafts/issues/new though.

@dead-claudia
Copy link
Author

@annevk Filed: w3c/csswg-drafts#5334

@annevk
Copy link
Member

annevk commented Jul 20, 2020

Thanks to the explanation there I recalled whatwg/html#5484. @josepharhar here's a concrete example as to what a move primitive might improve.

@annevk annevk transferred this issue from whatwg/html Jul 20, 2020
@dead-claudia
Copy link
Author

If it helps, this is in the context of FLIP animations for list move transitions.

@dead-claudia
Copy link
Author

dead-claudia commented Aug 9, 2020

Question: would it be web-compatible to do this, or would this be too breaking?

Add the following operation to trees:

To move a child within a tree to a given index is to move the child within the tree's children from its current position to before the item in the tree's children with the given index prior to moving, without adding or removing. If the index is the size of the tree's children, this moves it to the end of the tree instead.

To move a child within a tree to the end is to move the child within the tree's children from its current position to after its last child, without adding or removing.

Note: moves can only be performed whenever the child is a child of that tree, and by extension, moves cannot be performed on trees with empty children.

This is just spec jargon for "don't do things that happen when you add or remove elements". In implementations, this would still necessitate removing and then re-adding, but only as an implementation detail.

Amend the adopt algorithm to this:

To adopt a node into a document with a given parent, run these steps:

  1. Let oldDocument be node's node document.
  2. If node's parent is not parent, then remove node.
  3. If document is not oldDocument, then:
    [...]

Amend step 4 of document.adoptNode(node) likewise to pass a parent of null.

Replace steps 7.2-7.3 of insert with the following:

  1. If node's parent is parent, then:
    1. If child is non-null, then move node within parent to child's index.
    2. Otherwise, move node within parent to the end.
  2. Otherwise, if child is null, then append node to parent's children.
  3. Otherwise, insert node into parent's children before child's index.

Amend step 11 of replace to be this:

  1. If child's parent is not parent, then:
    [...]

Amend replace all to move children rather than removing them if their parents are parent (as defined in the algorithm), and update its callees like .replaceChildren(...nodes) and .textContent appropriately.

The precise update needed is omitted as it would be rather involved and would span multiple otherwise unrelated algorithms, but that's because the removal would need moved from .replaceChildren(...) to the core algorithm, and in order to do that, the algorithm itself would need to change from operating on a single node at a time to operating on a list of nodes (what it should've been using the whole time IMHO).


To assess risk, you would want to check for elements that 1. would be moved to any position other than the end according to this change, 2. have active transitions, animations, or mutation observers, and 3. for transitions, didn't have their styles synchronously calculated (for .getBoundingClientRect() mainly) within the current rendering frame. AFAICT, there's no other way to observe this change from JS, and custom element callbacks aren't fired on children changes (only mutation observers are).

@josepharhar
Copy link
Contributor

Thanks for writing this up! It sounds like moving elements around within the same parent without clobbering state could be a huge win! I'm interested to hear what the react and preact maintainers say about it in the whatwg/html thread.

@yosunkayag
Copy link

yosunkayag commented Aug 11, 2020 via email

@dead-claudia
Copy link
Author

@josepharhar Thanks! Of course, I just maintain Mithril - React and Preact of course have different concerns as they're implemented slightly differently, but I suspect it should work for them if I were to take an educated guess. (Preact is a lot like us but with fewer internal optimizations based on a cursory look, and React uses linked lists for everything IIUC.)

@josepharhar
Copy link
Contributor

@sebmarkbage @marvinhagemeister @developit
I sort of already asked in this comment, but would @isiahmeadows's suggestion here of not clobbering state when moving around nodes within one parent solve react's and preact's issues with state being lost when moving nodes?

@marvinhagemeister
Copy link

@josepharhar Sorry for being late to answer! The improvements outlined here would help us tremendously for Preact 👍 Same goes for the other issue.

@annevk
Copy link
Member

annevk commented Sep 10, 2020

@isiahmeadows I don't think that would be web-compatible as the side effects of insertion and removal would not be triggered. We'd also want different mutation records for moving vs insertion/removal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants