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

Updates to keyed lists break FLIP animations when they occur mid-animation #2612

Open
dead-claudia opened this issue Jul 19, 2020 · 3 comments
Assignees
Labels
Area: Core For anything dealing with Mithril core itself Type: Bug For bugs and any other unexpected breakage

Comments

@dead-claudia
Copy link
Member

dead-claudia commented Jul 19, 2020

Mithril version: 2.0.4

Browser and OS: Chrome 83.0.4103.116 on Windows 64-bit, Safari 13.1 on iOS, Firefox 78.0.2 on Windows 64-bit

Project:

Code

Live Flems

<button id="toggle">Toggle</button>
<div style="display:flex">
  <ul id="prev"><li>0</li><li>1</li><li>2</li><li>3</li><li>4</li><li>5</li><li>6</li><li>7</li></ul>
  <ul id="animate"></ul>
  <ul id="next"><li>0</li><li>1</li><li>2</li><li>3</li><li>4</li><li>5</li><li>6</li><li>7</li></ul>
</div>
const animateRoot = document.getElementById("animate")
let values = Array.from({length: 8}, (_, i) => i)

function render() {
  m.render(animateRoot, values.map(i => m("li", {key: i}, i)))
}

let interval = setTimeout(updateBody, 500)

function updateBody() {
  interval = setTimeout(updateBody, 500)
  const prev = values
  // https://en.wikipedia.org/wiki/Fisher%E2%80%93Yates_shuffle
  const next = values = prev.slice()
  for (let i = next.length - 1; i > 0; i--) {
    const j = Math.floor(Math.random() * (i + 1))
    const temp = next[j]
    next[j] = next[i]
    next[i] = temp
  }

  requestAnimationFrame(() => {
    const prevList = document.getElementById("prev").childNodes
    const nextList = document.getElementById("next").childNodes

    // Reorder elements in DOM
    for (let i = 0; i < prev.length; i++) {
      prevList[i].textContent = prev[i]
    }
    for (let i = 0; i < next.length; i++) {
      nextList[i].textContent = next[i]
    }

    const prevElems = Array.from(animateRoot.childNodes)
    
    // First
    const firstParent = animateRoot.getBoundingClientRect()
    const first = prevElems.map(e => {
      const child = e.getBoundingClientRect()
      return {
        top: child.top - firstParent.top,
        left: child.left - firstParent.left,
      }
    })

    render()

    // Last + Invert
    const lastParent = animateRoot.getBoundingClientRect()
    const movedElems = prevElems.filter((e, i) => {
      const last = e.getBoundingClientRect()
      const dx = first[i].left - (last.left - lastParent.left)
      const dy = first[i].top - (last.top - lastParent.top)
      if (dx === 0 && dy === 0) return false
      e.style.transform = `translate(${dx}px,${dy}px)`
      e.style.transitionDuration = "0s"
      return true
    })

    // Force re-layout
    document.body.offsetHeight

    // Play
    movedElems.forEach((e, i) => {
      const callback = () => {
        e.removeEventListener("transitionend", callback, false)
        e.classList.remove("transition")
      }
      e.classList.add("transition")
      e.addEventListener("transitionend", callback, false)
      e.style.transform = e.style.transitionDuration = ''
    })
  })
}

document.getElementById('toggle').onclick = () => {
  if (interval) {
    clearTimeout(interval)
    interval = null
  } else {
    updateBody()
  }
  p('toggle', interval != null)
}

render()

Steps to Reproduce

  1. Open web page

You can enable and disable the animations at will using the "Toggle" button, but that's not part of the repro.

Expected Behavior

The animation to be smooth.

Current Behavior

The animation appears choppy whenever the subtree updates, and list items often unexpectedly jump.

Context

The following vanilla code works correctly. It's a near-exact clone of the above, but it simply uses appendChild instead of a full keyed diff.

Live Flems

<button id="toggle">Toggle</button>
<div style="display:flex">
  <ul id="prev"><li>0</li><li>1</li><li>2</li><li>3</li><li>4</li><li>5</li><li>6</li><li>7</li></ul>
  <ul id="animate"><li>0</li><li>1</li><li>2</li><li>3</li><li>4</li><li>5</li><li>6</li><li>7</li></ul>
  <ul id="next"><li>0</li><li>1</li><li>2</li><li>3</li><li>4</li><li>5</li><li>6</li><li>7</li></ul>
</div>
const animateRoot = document.getElementById("animate")
let cachedElems = Array.from(animateRoot.childNodes)
let interval = setTimeout(updateBody, 500)

function render() {
  for (const elem of cachedElems) animateRoot.appendChild(elem)
}

function updateBody() {
  interval = setTimeout(updateBody, 500)

  const prevElems = cachedElems
  const nextElems = cachedElems = prevElems.slice()
  // https://en.wikipedia.org/wiki/Fisher%E2%80%93Yates_shuffle
  for (let i = nextElems.length - 1; i > 0; i--) {
    const j = Math.floor(Math.random() * (i + 1))
    const temp = nextElems[j]
    nextElems[j] = nextElems[i]
    nextElems[i] = temp
  }

  requestAnimationFrame(() => {
    const prevList = document.getElementById("prev").childNodes
    const nextList = document.getElementById("next").childNodes

    // Reorder elements in DOM
    for (let i = 0; i < prevElems.length; i++) {
      prevList[i].textContent = prevElems[i].textContent
    }

    for (let i = 0; i < nextElems.length; i++) {
      nextList[i].textContent = nextElems[i].textContent
    }

    // First
    const firstParent = animateRoot.getBoundingClientRect()
    const first = prevElems.map(e => {
      const child = e.getBoundingClientRect()
      return {
        top: child.top - firstParent.top,
        left: child.left - firstParent.left,
      }
    })

    render()

    // Last + Invert
    const lastParent = animateRoot.getBoundingClientRect()
    const movedElems = prevElems.filter((e, i) => {
      const last = e.getBoundingClientRect()
      const dx = first[i].left - (last.left - lastParent.left)
      const dy = first[i].top - (last.top - lastParent.top)
      if (dx === 0 && dy === 0) return false
      e.style.transform = `translate(${dx}px,${dy}px)`
      e.style.transitionDuration = "0s"
      return true
    })

    // Force re-layout
    document.body.offsetHeight

    // Play
    movedElems.forEach((e, i) => {
      const callback = () => {
        e.removeEventListener("transitionend", callback, false)
        e.classList.remove("transition")
      }
      e.classList.add("transition")
      e.addEventListener("transitionend", callback, false)
      e.style.transform = e.style.transitionDuration = ''
    })
  })
}

// Utility bits
document.getElementById('toggle').onclick = () => {
  if (interval) {
    clearTimeout(interval)
    interval = null
  } else {
    updateBody()
  }
  p('toggle', interval != null)
}

render()

Edit:

Mithril's far from the only one with this issue: https://twitter.com/isiahmeadows1/status/1284726730574315522

Note: Vue is not affected by this bug and carries the same behavior as the vanilla version - go here and spam the "Shuffle" button to see.

Edit 2: Also relevant: https://github.com/whatwg/html/issues/5742

@dead-claudia dead-claudia added Type: Bug For bugs and any other unexpected breakage Area: Core For anything dealing with Mithril core itself labels Jul 19, 2020
@dead-claudia dead-claudia self-assigned this Jul 19, 2020
@dead-claudia
Copy link
Member Author

Note to whoever aims to fix this: it requires a lot of experimentation and will require significant knowledge of the internals. It'll be easiest if you set the bundler to watch mode and create a throwaway HTML file with the above body and keep the devtools open with caching disabled, so you can refresh the page frequently. And no, you won't be able to automatically test this until you can determine what causes the jumpiness.

@dead-claudia dead-claudia changed the title Keyed diffs break FLIP animations when they occur mid-animation Updates to keyed lists break FLIP animations when they occur mid-animation Jul 19, 2020
@yossicadaner
Copy link

@isiahmeadows
I am not sure if this helps, but I think it's related to something we discussed in the past
#2135

@dead-claudia
Copy link
Member Author

@yossicadaner That's a separate issue, and this one's really rooted in a spec oddity (bug?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Core For anything dealing with Mithril core itself Type: Bug For bugs and any other unexpected breakage
Projects
Status: High priority
Development

No branches or pull requests

2 participants