search

angular / components

Component infrastructure and Material Design components for Angular
https://material.angular.io
MIT License
24.35k stars 6.74k forks source link

bug(COMPONENT): CDK Virtual Scroller jump back/flickers to items on top #27104

Open Donovantxy opened 1 year ago

Donovantxy commented 1 year ago

Is this a regression?

The previous version in which this bug was not present was

No response

Description

I'm getting an annoying behaviour while scrolling, where items snap/jump back and I'm never able to scroll all the way down the end of the list. I've notice though, that this bug happens only when I use the virtual scrolling within a material dialog

Template

<cdk-virtual-scroll-viewport class="scroll-viewport" itemSize="20">
   <li class="scan-list__item" cdkVirtualFor="let scan of pointClouds| keyvalue:classesByNameAsc;templateCacheSize:0">
        <div class="scan-list__scan">
            <div class="scan-list__scan__left">
              <div>{{scan.value.name}}</div>
            </div>
        </div>
   </li>
</cdk-virtual-scroll-viewport\>

CSS

.scroll-viewport { height: 200px; }

Reproduction

Expected Behavior

https://github.com/angular/components/assets/6199235/63111f97-c665-4cc0-8f8b-eead8d75a025

Actual Behavior

While scrolling down, the scroll jump up to the first times

Environment

aeslinger0 commented 1 year ago

I think when I got this behavior last it was because the itemSize was a tiny bit off of the actual row height. Maybe double check that your itemSize includes border or margins?

NikGurev commented 9 months ago

@Donovantxy did you manage to fix the issue?

RCout1nho commented 8 months ago

Does anyone have success with this?

Faulkner368 commented 8 months ago

I am currently experiencing this also, but only when applying templateCacheSize:0 which I need as I need the components to be destroyed and constructed when scrolling.

If I remove this templateCacheSize:0 it scrolls OK.

Anyone figured out the issue here ?

RCout1nho commented 8 months ago

@Faulkner368 the same happened to me, I think this happens because the cached elements increase the size of the container div and it must interfere with the scroll, I don't know if it's a bug in the lib or if it's possible to solve it by changing the container

Faulkner368 commented 8 months ago

@RCout1nho was wondering the same, if so I dont think there will be a work around as the component is getting destroyed it cant have any height

EmilOlovsson commented 5 months ago

@RCout1nho @Faulkner368 @NikGurev I have spent a few hours with this problem and I found a solution that worked for me.

Browser: Chrome (Version 124.0.6367.118 (Official Build) (64-bit))

Original layout:

<cdk-virtual-scroll-viewport>
    <div class="cdk-virtual-scroll-content-wrapper"></div>
    <div class="cdk-virtual-scroll-spacer"></div>
</cdk-virtual-scroll-viewport>

New layout:

<cdk-virtual-scroll-viewport>
    <div class="cdk-virtual-scroll-spacer"></div>
    <div class="cdk-virtual-scroll-content-wrapper"></div>
</cdk-virtual-scroll-viewport>

I moved .cdk-virtual-scroll-spacer before .cdk-virtual-scroll-content-wrapper

shiv19 commented 5 months ago

~I ended up using the appendOnly mode~

<cdk-virtual-scroll-viewport
            appendOnly
            itemSize="68"
            minBufferPx="1020"
            maxBufferPx="1360">
</cdk-virtual-scroll-viewport>

https://github.com/angular/components/issues/27104#issuecomment-2141015215

BdDragos commented 5 months ago

@RCout1nho @Faulkner368 @NikGurev I have spent a few hours with this problem and I found a solution that worked for me.

Browser: Chrome (Version 124.0.6367.118 (Official Build) (64-bit))

Original layout:

<cdk-virtual-scroll-viewport>
    <div class="cdk-virtual-scroll-content-wrapper"></div>
    <div class="cdk-virtual-scroll-spacer"></div>
</cdk-virtual-scroll-viewport>

New layout:

<cdk-virtual-scroll-viewport>
    <div class="cdk-virtual-scroll-spacer"></div>
    <div class="cdk-virtual-scroll-content-wrapper"></div>
</cdk-virtual-scroll-viewport>

I moved .cdk-virtual-scroll-spacer before .cdk-virtual-scroll-content-wrapper

Can you explain how you achieved this please? Since these are auto generated for me when I use cdkVirtualFor, and I also encounter this scrolling issue.

Faulkner368 commented 5 months ago

@BdDragos I found there were two causes, not having correctly calculated the itemSize but this is a well known cause and was not the issue for me, which was caused by using templateCacheSize:0.

Setting the cache to zero was not ideal since I want as much of a performance boost as possible so to understand my solution you need to understand why I was setting cache to zero.

I am working with existing, large and complex components that ideally would have been refactored but I did not have time for that. These components were processing and changing property values during construction and / or during OnInit life cycle which resulted in an issue where I was seeing the wrong text on screen for a component after scrolling and the bound data changing which occured because instead of constructing new components when scrolling it reuses previously constructed components by changing the bound data and during change detection this would get picked up; however the component is an existing one so the constructor and the OnInit method is not executed again so the processing that was needed in my case did not occur and was thus seeing the wrong values.

Setting cache to zero means it would not reuse existing components and would construct new ones, thus solving my issue. However it introduced the issue discussed here and I suspect its due to the calculations on itemSize when components are being destroyed and new ones created there is a brief moment where total height is not adding up... that is my suspicion anyway.

So I did a hacky fix as had insufficient time for a refactor on these large components, so I removed the templateCacheSize:0 and used OnChanges to do the processing that was taking place originally in OnInit and in the constructor so when the bindings are updated so when change detection runs everything was displayed as it should be.

So in summary I fixed this by removing templateCacheSize:0 and achieved my goal by using OnChanges to do the processing that was previously happening in OnInit and constructor, hope that helps

BdDragos commented 5 months ago

For me the bug happens with and without the templaceCacheSize (but templateCacheSize not to 0 fixes it in most cases*). Unfortunately in our case ngOnChanges is not a solution since there are some very complex components that need to be rendered and I also depend on other browser limitations that would make the processing not possible without a lot of refactoring and extra code.

Faulkner368 commented 5 months ago

I hadn't seen this without templaceCacheSize:0 and most often seemed from stackoverflow and the likes to be caused by element sizes not matching what you have defined in itemSize, have you physically checked in the DOM he heights of all the visible elements, I think I initially had issue not having accounted for border

BdDragos commented 5 months ago

I hadn't seen this without templaceCacheSize:0 and most often seemed from stackoverflow and the likes to be caused by element sizes not matching what you have defined in itemSize, have you physically checked in the DOM he heights of all the visible elements, I think I initially had issue not having accounted for border

Yes, I set the itemSize to two decimals accuracy. I also tried to make it "perfect" and intentionally not perfect (small variations), still the same issue.

Faulkner368 commented 5 months ago

Hard to know then without seeing it first hand, maybe if you have time see if can reproduce in something like https://stackblitz.com/ and share here or Stackoverflow and someone maybe able to help but in the least can help narrow down the cause when in a clean / simple app

EmilOlovsson commented 5 months ago

@RCout1nho @Faulkner368 @NikGurev I have spent a few hours with this problem and I found a solution that worked for me. Browser: Chrome (Version 124.0.6367.118 (Official Build) (64-bit)) Original layout:

<cdk-virtual-scroll-viewport>
    <div class="cdk-virtual-scroll-content-wrapper"></div>
    <div class="cdk-virtual-scroll-spacer"></div>
</cdk-virtual-scroll-viewport>

New layout:

<cdk-virtual-scroll-viewport>
    <div class="cdk-virtual-scroll-spacer"></div>
    <div class="cdk-virtual-scroll-content-wrapper"></div>
</cdk-virtual-scroll-viewport>

I moved .cdk-virtual-scroll-spacer before .cdk-virtual-scroll-content-wrapper

Can you explain how you achieved this please? Since these are auto generated for me when I use cdkVirtualFor, and I also encounter this scrolling issue.

@BdDragos This should work for you.

// HTML


<cdk-virtual-scroll-viewport
    #virtualscroll
    autosize
    [minBufferPx]="A_NUMBER_HERE"
    [maxBufferPx]="A_NUMBER_HERE"
>
    // Your content here
</cdk-virtual-scroll-viewport>

// .ts file

import {CdkVirtualScrollViewport} from "@angular/cdk/scrolling";

@ViewChild('virtualscroll') private _virtualscrollRef?: CdkVirtualScrollViewport;

ngAfterViewInit() {
    if (this._virtualscrollRef) {
        // @ts-ignore
        const scrollable = this._virtualscrollRef.scrollable.elementRef.nativeElement;
        if (scrollable && scrollable.children) {
            const spacer = scrollable.children[1];
            if (spacer && spacer.classList.contains('cdk-virtual-scroll-spacer')) {
                // Move spacer as first child
                scrollable.insertBefore(spacer, scrollable.firstChild);
            } else {
                console.error("Could not find spacer");
            }
        } else {
            console.error("Could not find scrollable");
        }
    }
}
BdDragos commented 5 months ago

I succeeded in shifting the elements with a JavaScript type of solution after I thought a bit.

    effect(() => {
      const nativElem = this.cdkScrollElement()?.elementRef.nativeElement;
      if (nativElem) {
        const last = nativElem.lastElementChild;
        if (nativElem.firstElementChild && last) {
          nativElem.replaceChild(nativElem.firstElementChild, nativElem.lastElementChild);
          nativElem.insertBefore(last, nativElem.firstElementChild);
        }
      }
    });

the above will be put into the constructor(), and it will use a signal based viewChild to trigger as soon as the rendering of the cdkScrollElement is done. The viewChild should be binded directly to the <cdk-virtual-scroll-viewport tag

public cdkScrollElement = viewChild<CdkVirtualScrollViewport | undefined>('cdkScrollElement');

This fixed my issue it seems, now I can scroll without problems. Thanks for the suggestion. @shiv19

Hard to know then without seeing it first hand, maybe if you have time see if can reproduce in something like https://stackblitz.com/ and share here or Stackoverflow and someone maybe able to help but in the least can help narrow down the cause when in a clean / simple app

It would take too much time to set up everything, the behaviour is pretty complex, but the shifting did it.

shiv19 commented 5 months ago

@BdDragos In my case the actual problem was not having the correct item size, and also rending category headers just inside the for loop even though they were not a part of the list itself. I changed the category headers to be a part of the list and gave a fixed height for the category rows and the list item rows. And everything is working fine now. I even removed the appendOnly mode.

This is fine when all the list items can have a fixed height. When items can't have a fixed height, we'll need to use a custom virtual scroll strategy.

I found a few articles that talk about this:

https://dev.to/georgii/virtual-scrolling-of-content-with-variable-height-with-angular-3a52#final-code

https://angularindepth.com/posts/1091/writing-custom-virtual-scroll-strategy

And here's some code from the CDK experimental: https://github.com/angular/components/blob/main/src%2Fcdk-experimental%2Fscrolling%2Fauto-size-virtual-scroll.ts

canadakickass commented 4 months ago

I can't believe it! switching the spacer fixed the issue for me also. Should this change be brought to the CDK?

EdGuan commented 4 months ago

switching the spacer fixed my issue as well. Im currently using Angular 17, and my issue is not caused by the itemSize and templaceCacheSize:0.