fix: avoid infinite loop on scrollToBottom with unknown sized elements

[ktsn]

fix #221

I've changed the termination condition to when the scrollTop value is not changed since the previous update.

Also added requestAnimationFrame during the loop because we need to wait delayed processing in RecycleView after triggering scroll event and DOM update.

To check the behavior the scroll to bottom example has been updated not to specify fixed size for each item.

[tjerman]

@ktsn This opens an issue of when you start scrolling up over the "skipped" items -- scrolling becomes jumpy.

In your example change 50 to 500 and start scrolling up (if you go slower it will be more noticeable).

I didn't go too much in depth code-wise, but I am guessing that when an item that has undefined height is found & calculated, items below it are pushed down. A potential fix would be that in case of scroll up, you push up items above it (it included).

[ktsn]

Ah I see. That's because the current RecycleView implementation doesn't expect size changes for elements which is upper position than active viewport. I'll look into the solution for it.

[ktsn]

Added a logic to avoid jumping contents by modifying scroll position when the upper items' size are changed.
I've tested it on Safari, Chrome and Firefox on macOS. Safari and Chrome work well but Firefox has some performance issue - when the scroll position modification occurs, the scrolling speed slow down. We may need to find more performant approach for it.

[hereiscasio]

@ktsn
Video for recording the Issue : https://www.dropbox.com/s/yusvnhbxobo2m59/demo.mov?dl=0

after i use the script below

mounted() {
    this.$refs.scroller.scrollToBottom();

Seems like scroller "freeze" the scrolling, so no matter how i scroll inside the scroller
my screen just show the same thing (i.e. freezed)

but i found the scroll event hander which was register on DynamicScroller
will be trigger as i scroll

i also try to use u solution, but it just not works

how can we solve this issue ?
( seems like we need to use dirty hack right now ? )

[ktsn]

@hereiscasio Are you sure you are using fixed code on my branch? In that case can you provide a reproduction I can try?

[hereiscasio]

@ktsn
i found u have 2 commits, and mainly u just modify the file: DynamicScroller.vue
so i just copy the whole file u presented here to my local DynamicScroller.vue

and indeed, it not works
so u mean it should works, right ?
because we face the same issues

let me try to prepare the reproduction link

[ktsn]

@hereiscasio If you mean you modified DynamicScroller.vue in your node_modules/vue-virtual-scroller, that's not update your imported vue-virtual-scroller as built files under dist/ is actually used. You need build the source code by yourself.

[hereiscasio]

https://codesandbox.io/s/vue-with-vuetify-eagles-wfvdl

[hereiscasio]

@ktsn
after i try, yes, it works !!!

thx !!!

BUT . . .

a wired error throw out after i use u branch
i haven't see this error before,
not sure what goes wrong,
because i just use the build files to my local repo

error throw from u code at this line:

if (typeof size === 'undefined' && !this.$_undefinedMap[id]) {
...

Can u fix that ?

One more thing, when i scrolling up,
it will jump for certain scrolling situation, no matter the item type ( i.e. pure text, image ... )

[hereiscasio]

Hi @ktsn

after i dig-in the source code,
i found u fixed ( see below, the watcher will not called ) will not works if i
i load data ( says, 500, but actually the number of items is not a matter here )
initially in data () { . . . }

And if i put this.$refs.scroller.scrollToBottom(); in mounted,
when i scroll up, u will find the content still jump ❗️

any idea ❓

watch: {
    itemsWithSize (next, prev) {
      const scrollTop = this.$el.scrollTop

      // Calculate total diff between prev and next sizes
      // over current scroll top. Then add it to scrollTop to
      // avoid jumping the contents that the user is seeing.
      let prevActiveTop = 0; let activeTop = 0
      const length = Math.min(next.length, prev.length)
      for (let i = 0; i < length; i++) {
        if (prevActiveTop >= scrollTop) {
          break
        }
        prevActiveTop += prev[i].size || this.minItemSize
        activeTop += next[i].size || this.minItemSize
      }
      const offset = activeTop - prevActiveTop

      if (offset === 0) {
        return
      }

      this.$el.scrollTop = this.$el.scrollTop + offset
    },
    . . . . . . .
}

[hereiscasio]

It will be great, if u can tell me why, although i can fix it now by using dirty hack

[ktsn]

error throw from u code at this line:
if (typeof size === 'undefined' && !this.$_undefinedMap[id]) {

What do you mean by that? This code is not in my diff.

will not works

Browser and reproduction?

[hereiscasio]

here it's : https://codesandbox.io/s/vue-with-vuetify-eagles-wfvdl

plz try to scroll up, u will find the content "jump"

but if u disable line 95, and enable line 96 & 103
( maybe u need to save it & refresh the page )

then u can do the same thing again,
u will find everything is ok ( i.e. no jump )

i'm just curious why

---

BTW

this codesandbox already use u branch code

[ktsn]

Your reproduction url seems not correct. It does not use scroll to bottom and your references (line 94, 95, 102) look not pointing correct position.

[hereiscasio]

@ktsn
sorry, i make some mistake, i already fix it

Basically, below works well

  data() {
    return {
      //messages: imageAndText
      messages: [] // #Issue C0711A1 solver-2
    };
  },
  mounted() {
    //this.messages = textOnly5K; // #Issue C0711A1 solver-2
    this.messages = imageAndText;
    this.$refs.scroller.scrollToBottom(); // #Issue C0711A1 producer-2
  },

But u will find jumping issue if u use below code
( try to scroll up right now )

  data() {
    return {
      messages: imageAndText
    };
  },
  mounted() {
    this.$refs.scroller.scrollToBottom(); // #Issue C0711A1 producer-2
  },

And actually, i have use scrollToBottom in mounted hook like below

  mounted() {
    this.$refs.scroller.scrollToBottom(); // #Issue C0711A1 producer-2
  }

[ktsn]

Thanks for finding it. I figured out the cause. It's because the itemsWithSize computed property is somehow called before calling created hook. Then it accesses this.$_undefinedMap[id] before initializing it.
Just changing created hook to beforeCreate solves the problem.

[hereiscasio]

@ktsn
Nice works !!!

i'm just lucky to find this issue because i'm dev chat app
and u know, chat app is really the hard software to dev
because there're lot of details requirements need to implement

but anyway, thx so much

cheers

[tjerman]

@ktsn good job on the fix, but there is another thing I'm worried about.

If i remember correctly, if you set scrollTop, the current scrolling will be canceled. For example on android - if I swipe down on a large list (so the scroll animation takes over), the scrolling will stop as soon as scrollTop is set.

Are you able to try this out? I might have a chance to later this weekend.

[Akryum]

The code handling scrollToBottom changed, do you still have the issue?

[ktsn]

@Akryum Looks like infinite loop problem is solved but the problem @tjerman mentioned (jumpy scroll) is still there.

[ktsn]

Ah but it's actually a separated problem. I'll make another pull request later.

[ktsn]

@Akryum I've opened a new PR #374