vuejs / core

🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
https://vuejs.org/
MIT License
47.7k stars 8.33k forks source link

Should keyed v-for array refs be ordered? #4010

Open soc221b opened 3 years ago

soc221b commented 3 years ago

Version

3.1.2

Reproduction link

vue 3

Steps to reproduce

  1. open sandbox
  2. open console
  3. click reverse
  4. see first and second refs log

What is expected?

second log should be:

[
  <div>bar</div>,
  <div>foo</div>,
]

What is actually happening?

second log still be:

[
  <div>foo</div>,
  <div>bar</div>,
]

Ref: https://v3.vuejs.org/guide/migration/array-refs.html

posva commented 3 years ago

I would say having a function gives you the flexibility of ordering the nodes however you want but I'm not sure if this was intended

DV8FromTheWorld commented 3 years ago

I would expect that the ordering of refs received would be stable between renders (and between Vue2 and Vue3). Without it being stable it makes things like "focus the first ref in the list" quite difficult as the ordering will reverse after the initial render.

Here is a simplified repro example that shows that re-renders reverse the order of the refs compared to the actual rendered content. Repro example on SFC Playground

DV8FromTheWorld commented 3 years ago

The problem originates from the fact that patchKeyedChildren iterates in reverse-order https://github.com/vuejs/vue-next/blob/ba89ca9ecafe86292e3adf751671ed5e9ca6e928/packages/runtime-core/src/renderer.ts#L2049-L2051

This means that any setRef calls that occur in patch that rely on the v-for : key iteration done in this loop will end up being provided to :ref="..." functions in reverse order on patch-renders compared to initial-renders.

DV8FromTheWorld commented 3 years ago

This is actually pretty problematic for us in terms of migrating to Vue3. We have lots of accessibility functionalities that rely on being able to properly traverse our $refs for keyboard navigation, focus transferring, etc.

Without a stable, in-order sort for these refs.. we're kinda stuck.

DV8FromTheWorld commented 3 years ago

You may be able to work around this problem via passing the intended index of the ref to your handleItemRef and using it to set the ref into the array instead of relying on .push(ref).

The follow example highlights how this could be implemented

<template>
  <ul>
    <li 
      v-for="(item, idx) of items"
      :key="item.id"
      :ref="ref => handleItemRef(idx, ref)">
      {{item.name}}
    </li>
  </ul>
</template>

<script>
export default {
  data() {
    return {
      itemRefs: [],
      items: [
        { name: 'Item 1', id: 1 },
        { name: 'Item 2', id: 2 },
        { name: 'Item 3', id: 3 },
      ]
    }
  },
  methods: {
    handleItemRef(refIndex, ref) {
      if (ref) {
        this.itemRefs[refIndex] = ref
     }
    }
  },
  beforeUpdate() {
    this.itemRefs = []
  }
}
</script>

Note, this cannot be done when dealing with the Vue3 Migration Build's support of V_FOR_REF as the developer isn't dealing with the :ref="<function>" api yet. As such, this ordering of refs for migration users will continue to be invalid.

LinusBorg commented 3 years ago

You can actually sort the elements yourself, may be a viable workaround for the time being:

    onUpdated(() => {
      console.log(sortElementsByPosition(itemRefs));
    });

    function sortElementsByPosition(elements) {
      return elements.slice().sort((a, b) => {
        return a.compareDocumentPosition(b) & 2 ? 1 : -1
      })
    }

though I can't say much about how this would perform for really long lists (couple thousand items)

DV8FromTheWorld commented 3 years ago

Interesting. Didn't know about Node.compareDocumentPosition.

DV8FromTheWorld commented 3 years ago

As a note, I'd be happy to PR a solution to this issue, however, given the fact that the internals of the patch system are built intentionally with reverse-order patching for optimization I've not touched the code yet as it isn't clear how the project might want to address this.

Any thoughts or guidance here would be appreciated.

DV8FromTheWorld commented 3 years ago

Bump

cyfung1031 commented 3 years ago

You may be able to work around this problem via passing the intended index of the ref to your handleItemRef and using it to set the ref into the array instead of relying on .push(ref).

The follow example highlights how this could be implemented

<template>
  <ul>
    <li 
      v-for="(item, idx) of items"
      :key="item.id"
      :ref="ref => handleItemRef(idx, ref)">
      {{item.name}}
    </li>
  </ul>
</template>

<script>
export default {
  data() {
    return {
      itemRefs: [],
      items: [
        { name: 'Item 1', id: 1 },
        { name: 'Item 2', id: 2 },
        { name: 'Item 3', id: 3 },
      ]
    }
  },
  methods: {
    handleItemRef(refIndex, ref) {
      if (ref) {
        this.itemRefs[refIndex] = ref
     }
    }
  },
  beforeUpdate() {
    this.itemRefs = []
  }
}
</script>

Note, this cannot be done when dealing with the Vue3 Migration Build's support of V_FOR_REF as the developer isn't dealing with the :ref="<function>" api yet. As such, this ordering of refs for migration users will continue to be invalid.

@iendeavor @DV8FromTheWorld

Don't rely on the ordering of :ref="someFunc" It is just something uncontrollable. (see #4646)

DV8FromTheWorld commented 3 years ago

After reading through #4646 I don't agree to be honest. Your problem was caused by, frankly, not properly handling the reactivity system in Vue.

The same problem would have occurred for any computed / reactive situation, it just might not have been obvious in terms of the result. Given that you aren't setting up erroneous reactive dependencies it will be fine. And if you do need access to do comparisons like that, you can sidestep the reactivity tracking via variable._value instead of variable.value

LinusBorg commented 2 years ago

I think this should be resolved ever since we brought back the old Vue 2 v-for ref behaviour. Am I correct in assuming that, @DV8FromTheWorld ?

kubajmarek commented 7 months ago

Bumping the question above, although after refreshing the initial reproduction it doesn't seem to be resolved.

Additionally, changing the :key to include index gives some weird results. For 2 elements: after reverse itemRefs is always in the (reverse) order to actual state For 3 elements: after reverse itemRefs has middle element as first item, other items are in reverse order to actual state For 4 elements: same as 2