rintoj / ngx-virtual-scroller

Virtual Scroll displays a virtual, "infinite" list.
https://rintoj.github.io/ngx-virtual-scroller
MIT License
977 stars 294 forks source link

DOM elements are not reused #400

Open furlongce opened 4 years ago

furlongce commented 4 years ago

I noticed in the readme, that there a few contradictory statements:

"This module displays a small subset of records just enough to fill the viewport and uses the same DOM elements as the user scrolls."

"virtual-scroller essentially uses *ngIf to remove items that are scrolled out of view. This is what gives the performance benefits compared to keeping all the off-screen items in the DOM."

Based on the first statement, I would have expected the Angular components to be recycled, and not be destroyed (similar to od-virtual-scroll). With that library, ngOnChanges fires and you can reuse the DOM element and simply update the information. For my use case, I'm displaying many HighCharts graphs in a grid, so creating the elements is very expensive when compared to updating the trend line in an existing chart.

This is negligible in modern browsers, but I've noticed that the performance is dramatically worse in Internet Explorer.

JoMas971 commented 4 years ago

I have the same issue. Even if i optimise all my components to use changeDetection: ChangeDetectionStrategy.OnPush, i'm creating many cell element that have their own listeners/calculations, it pretty basic stuff and i've did my best to optimise everything, but if i scroll really fast i can see some flickering effect and my cells are blinking. I would love to see the previous elements to be reused instead of deleted/re-added...

furlongce commented 4 years ago

Yeah, I did the same thing - even detached change detection entirely and managed it myself. I cannot match the performance of od-virtualscroll, which is a shame since this library provides a lot of benefits (in regard to resizing mostly) that the other library does not. Plus od-virtualscroll is hardly maintained anymore.

speige commented 4 years ago

Have you tried using trackBy as mentioned in the readme? I think that's the easiest way to allow re-use of DOM elements with Angular. If that doesn't work, feel free to submit a PR with any improvements you have.

lincolnthree commented 4 years ago

@speige trackBy works for changing the back-end data, but I believe what these folks are talking about (something I noticed as well) is that while you are scrolling the list, new DOM elements are created as elements scroll onto the screen. DOM elements must also be garbage collected (generally automatically of course) as they scroll out of the virtual-scroller viewpor / buffer.

This causes overhead where the nodes could otherwise simply be moved to the bottom of the list again (using translate), and new data could be pushed into them. Essentially, you would only ever create Math.min(BUFFER_SIZE, NUM_ELEMENTS) DOM elements, once, when the scroller is initialized, instead of creating new ones as scrolling occurs (which causes higher than necessary CPU usage).

The library is still better than displaying an entire list, don't get me wrong, it's just that this performance improvement would make a huge difference on top of what's already provided.

lincolnthree commented 4 years ago

This diagram explains the desired functionality:

image

From this article: https://www.joshmorony.com/boosting-scroll-performance-in-ionic-2/ which provides another explanation of the efficiency of DOM element recycling in this use-case.

speige commented 4 years ago

I understand what you're saying. Unfortunately, the change you're requesting would probably require a big change in the library, because we'd have to manipulate the DOM directly. The current creation/deletion of DOM elements is done via *ngFor, which is a built-in Angular directive.

<virtual-scroller #scroll [items]="items">
    <my-custom-component *ngFor="let item of scroll.viewPortItems">
    </my-custom-component>
</virtual-scroller>

TrackBy does allow Angular to re-use DOM elements instead of create/destroy. The library suggests using a uniqueIdentifier to ensure elements that have changed a re-generated, however it's theoretically possible to trick Angular by not using a unique identifier & instead using the array index, in this case it'd re-use all elements. I believe it would still re-evaluate data-bindings, so it should be safe, but it'd cause issues with complex components that have internal state set during ngOnInit/etc, as that would not re-execute.

If this "theory" proves to work, we could integrate it into the library, with an option to turn it off for "complex" components with internal state.

I'm a bit busy at the moment, would you mind experimenting to see if this concept could work? The new syntax would be something like this:

<virtual-scroller #scroll [items]="items">
    <my-custom-component *ngFor="let item of scroll.viewPortItems; index as i; trackBy: function() { return i; }">
    </my-custom-component>
</virtual-scroller>

I'm sure that syntax is wrong, but hopefully it's enough pseudo-code to get the point across.

lincolnthree commented 4 years ago

Understood. That's an interesting approach, but wouldn't that only really work for a few dom elements, then once the buffer is exhausted, they'd be collected like everything else?

However, it's possible a directive could be used to add functionality to this library:

<virtual-scroller #scroll [items]="items">
    <my-custom-component *scrollFor="let item; index as i; trackBy: function() { return i; }">
    </my-custom-component>
</virtual-scroller>

It would indeed require manual dom manipulation, but there's an example using this approach that could be used for inspiration: https://github.com/ionic-team/ionic/tree/master/core/src/components/virtual-scroll

Right now I use a combination of this library plus Ionic's virtual-scroll to get the best of both worlds, performance when multi-columns are not required, and this one when they are :)

speige commented 4 years ago

I believe the trackBy hack would work indefinitely, no worry of buffer exhaustion. However, I haven't tested it. My bigger concern is whether the data bindings would re-evaluate. I think for simple controls it would re-bind, for for complex ones with internal state, they wouldn't. Maybe not a great long-term approach, but worth researching as a temporary workaround.

lincolnthree commented 4 years ago

Hmmm... I see what you're saying.

I agree about the issues with ngOnInit, ngAfterViewInit, etc. I think that would be problematic (would probably already be enough to break my scenario already as our components use those hooks, and many others do.)

It could work for very simple components/inline templates/lists (at which point DOM creation is probably not a big deal anyway so I'm guessing it's not worth the time, but would be interesting to know anyway).

amin79 commented 4 years ago

@lincolnthree Hi. You mentioned that you use a combination of this library plus Ionic's virtual-scroll. Could please proved an example? I used ion-virtual-scroll, but it has overlapping problem when I use it in my Android device. I tried cdk-virtual-scroll-viewport, but also sometimes it doesn't show all the objects when I scroll and it flickers. With this library I have again problem on Android. First of all it takes about 2 sec when I scroll to render the new objects. And also in the edge of time for rendering, it escapes some objects. Thank you in advance.

lincolnthree commented 4 years ago

@amin79 I wish I could give you an example. I don't use them together, I just make tradeoffs and use ionic's virtual scroll sometimes and use this one sometimes depending on the performance needs. ionic handles same-height, single column virtual scrolling very well (and fast.) This library handles multi-column much better, but is slower from what I've found in my testing, when DOM elements are "heavy" due to this issue.

amin79 commented 4 years ago

@lincolnthree Thank you for your answer. Thats correct that ionic handle it very good. But there is a bug which is not solved after a long time. When I use ionic virtual scroll in a real device or emulator (for example for Android) and navigate to another page and open the keyboard and come back, the cards are overlapped. It's a week I'm looking for something that solved my problem. I found nxg-ui-scroll which was really good. But unfortunately it's not fit to my requirement. I use real time Firestore and in when I use it, I have no real time data.

roxteddy commented 2 years ago

We tried to implement the scroller in our ionic app and on iOS, the app/viewview reloads after a few up and down scolls. It seems to be caused by a node overload.