Scrolling up triggers jumpy/seemingly erratic rendering

[maxruby]

Problem

We have an Angular application (both v8 and v9.1.0) in our company in which we want to achieve the following:

• virtual scroll over a large list of items (only render the items within the visible viewport)
• when reaching a percentage of the viewport (> 70%), retrieve more items from our API
• when scrolling up, do not trigger any events or re-render

What we experience in our implementation described in detail below is that scrolling down works as expected, but scrolling up results in erratic rendering and jumpy behavior. After some investigation, I found a solution for the problem we encountered. Please consider integrating our findings/fix to make the implementation of this library more stable.

Documentation

GIF showing how scrolling down loads more items smoothly, scrolling up results in large and erratic rendering

Reproduction

Unfortunately, I can not share the code or a minimal example, but provide here as much as information as possible to explain how to reproduce the problem.

Angular version: v9.1.0-next.1 (but also observed in v8.0.0)

package.json

    "@angular/common": "~9.1.0-next.1",
    "@angular/compiler": "~9.1.0-next.1",
    "@angular/core": "~9.1.0-next.1",

The component which implements the VirtualScroller class is a so called UnitListComponent which consists roughly of the following (the full code can not be provided):

component.html

<div class="ion-list-wrapper" scroll-y="false">
  <!-- Units list -->
    <ion-list>
        <virtual-scroller
            class="scroll-viewport"
            #scroll
            (vsEnd)="onScrolledToEnd($event)"
            [items]="unitItems">
            <app-unit-list-item *ngFor="let unitItem of scroll.viewPortItems; trackBy: trackByFn"
            class="unit-list-item"
                [id]="unitItem.id"
                [checked]="unitItem.checked"
                [title]="unitItem.title"
                [price]="unitItem.price"
                [priceUnit]="unitItem.priceUnit"
                [area]="unitItem.area"
                [areaUnit]="unitItem.areaUnit"
                [rooms]="unitItem.rooms"
                [floor]="unitItem.floor"
                [blocked]="unitItem.blocked"
                [state]="unitItem.state"
                (clicked)="selectUnitId($event)">
            </app-unit-list-item>
        </virtual-scroller>
    </ion-list>
</div>

component.ts

  @Input() unitItems: UnitListItem[] = [];

  onScrolledToEnd(event) {
  // skip scrolling in the following cases
  // 1. render item index => total item count
  // 2. first 2 scroll events due to https://github.com/rintoj/ngx-virtual-scroller/issues/263
  if ((!isNullOrUndefined(this.unitItems)
    && event.endIndex !== this.unitItems.length - 1)
    || event.startIndex < 2) {
    return;
  }
  this.scrolled.emit();
}

component.scss

@import 'global-utilities';

.ion-list-wrapper {
  ion-list {
    padding-top: 0;
    padding-bottom: 0;
  }

  .scroll-viewport {
    height: calc( 100vh - 373px );
  }
}

In summary, the implementation makes the following assumptions:

• a parent container component passes a unitItems array (using an async pipe) to the UniListComponent
• an @Input in turns passes the unitItems to the [items] @Input in the virtual-scroller element
• a scroll down event triggers the onScrolledToEnd method which emits an event to the parent component as @Output
• scroll up should trigger nothing

Findings and current "fix"

After looking more closely at the code in src/virtual-scroller.ts, I discovered that disabling or returning inside the check if (maxRunTimes > 0) in https://github.com/rintoj/ngx-virtual-scroller/blob/3e4f8435a9d21d5032812763b825324747ab00a9/src/virtual-scroller.ts#L845 prevents the observed erratic behavior. Whether this is the right solution or not to solve the problem is not yet clear, but the change definitely and reliably eliminates the erratic jumpy behavior.

Current "fix"

change refresh_internal from

    if (maxRunTimes > 0) {
      this.refresh_internal(false, refreshCompletedCallback, maxRunTimes - 1);
      return;
    }

to

    if (maxRunTimes > 0) {
       return;
    }

GIF demonstrating correct virtual scrolling in our component after the "fix"

In conclusion, in order to be able to continue using this library directly, I would really appreciate if a more permanent fix was added to avoid this jumpy behavior. Hopefully our findings can help pin down the source of this issue and lead to a proper fix as soon as possible. I am happy to support with finding a more permanent fix for this problem after getting your feedback.

[speige]

You'll notice the following comment at the beginning of the refresh_internal function

		//note: maxRunTimes is to force it to keep recalculating if the previous iteration caused a re-render (different sliced items in viewport or scrollPosition changed).
		//The default of 2x max will probably be accurate enough without causing too large a performance bottleneck
		//The code would typically quit out on the 2nd iteration anyways. The main time it'd think more than 2 runs would be necessary would be for vastly different sized child items or if this is the 1st time the items array was initialized.
		//Without maxRunTimes, If the user is actively scrolling this code would become an infinite loop until they stopped scrolling. This would be okay, except each scroll event would start an additional infinte loop. We want to short-circuit it to prevent this.

Essentially, this component is used for many scenarios by many people, so there are situations where only calling refresh_internal 1x will cause inaccurate rendering. The main situation is when child items aren't all the same size. I guess it's a tradeoff between preformance/flickering & accurate rendering.

I'd be happy to accept your change, if it was modified in a way to prevent it from breaking other code.

I think one option would be maxRunTimes = firstRun || this._enableUnequalChildrenSizes ? 2 : 1

I'll experiment with it & get back to you.

[maxruby]

@speige Thank you for the explanation. Indeed I read the doc comment above the refresh_internal method and that is partly why I decided to try to find a fix for my case.

Essentially, this component is used for many scenarios by many people, so there are situations where only calling refresh_internal 1x will cause inaccurate rendering.

I realize that you are aware of this, but the trouble is that the way this functionality is implemented in the code right now does not lend itself to optimizaton. I think that it would be best to refactor out refresh_internal using the Strategy Pattern and isolate each case with its own logic rather than having lots of conditionals inside one method that does it all.

Let me know if I can support.