fix(virtualScroll) flickering issue fixes. No need for re-rendering the whole DOM #12917

masimplo · 2017-09-18T13:53:14Z

Short description of what this resolves:

This is a very extensive refactoring of the virtual-utils file that changes the ways DOM nodes are handled (aka the bees knees PR). Instead of deleting the nodes when a dataset change has occured and starting from scratch, there is a more "surgical" approach of reusing existing nodes and creating/disposing nodes very sparingly. This leads to great performance gains and takes care of the dreaded flickering effect when data changes.
In summary this resolves issues with flickering when data is updated, clears DOM when data is cleared, adds a few optimisations all around.

Changes proposed in this pull request:

Slow path and fast path have similar performance
Nodes are always reused and removed only if they become orphaned.
New nodes are created very sparingly
When scrolling to the two extremes (top/bottom) do not dispose of cells that are out of view and recreate them when scrolling backwards.
Initialise virtualTrackBy and virtualScroll properties like ngForOf does, when both are ready to be ready inside ngOnChanges
Replace deprecated trackByFn with new trackByFunction
After vigorous scrolling on slow devices nodes failed to render at all (blank screen or only some of the nodes rendered. Fixed it by calling populateNodeData once more in scrollEnd event.

Ionic Version: 3.6.1

Fixes:
#12035
#12034
#11439
#12036 (not fix completely, but make it less likely to occur since most nodes are reused)
#12047 (probably... couldn't reproduce with the refactored code. Might still be there)
#9655

Includes:
#12813

Known issues:
~~(Pre-existed) When scrolling very fast in a device the list might become blank until you scroll it a bit again or new data arrives.~~
(Current) during scrolling one node gets destroyed and another created. This happens rarely and seems to have no real impact to performance or UI. Will look into it.

Latest build for anyone wanting to try:
ionic-angular.zip built on 19/09/2017@14:22GMT includes commit 564ef41
ionic-angular.zip build on 20/09/2017@08:36GMT includes commit 3497d40
ionic-angular.zip build on 28/09/2017@22:46GMT for ionic version 3.7.0

Patch file for v3.6.1:
virtual-scroll.patch.zip
Patch file for v3.7.0:
virtual-scroll.patch.zip

Special thanks to:
@zafeirism for helping with the whole refactoring
@danielsogl for pointing out that TrackByFn is deprecated
@mnewmedia for their PR and ngForOf reference initialising differ

HugoHeneault · 2017-09-18T14:15:07Z

Have to test this on my app! I'll come back to you after :)

masimplo · 2017-09-18T14:56:07Z

@HugoHeneault please do... and let me know if you notice anything out of place. I have thoroughly tested this in our code, but would love to see how it behaves in other codebases as well.

We are using immutables and custom ItemSliding components with buttons on both sides and avatars in our VS. Also using headers. Probably going to work for other configurations as well, but better make sure. Also please test it in an actual device, as browser testing is not that reliable in performance tests.

There are still a few optimisations I have in mind, but don't want to overdo it and not get this merged due to "too many changes"

faraazc · 2017-09-18T18:02:07Z

@masimplo thanks a lot ..Issues related to vs was really becoming hindrance to using ionic..

I will definitely test and get back.
One question regarding below item do you still see it?. For me it appears in older code base when the scroll reaches end fastly..

(Pre-existed) When scrolling very fast in a device the list might become blank until you scroll it a bit again or new data arrives.

masimplo · 2017-09-18T18:30:20Z

@faraazc the list of know issues, is what I have identified as existing problems. I managed to reproduce it with my latest code after doing some vigorous scrolling on a device.
I will be focusing to find out what is causing this now, since I believe this is the single most annoying issue remaining.
If anyone has any ideas on why this might be caused I would be open to suggestions, if not I feel comfortable navigating the existing code now, so I will probably be able to pin it down soon. It is just that it is a bit hard to reproduce this that is making it hard for me to solve.

What I do know about this issue, is that the nodes are there but the translate3d numbers are wrong probably because they don't get the chance to update.

masimplo · 2017-09-19T13:17:30Z

@faraazc can you please try out my latest commit, as I believe it fixes the issue you are talking about.
During scrollEnd, we were running updateDimensions and adjustRendered but where not applying the calculated values to the nodes before doing a DOM write. I am now calling populateNodeData in between and seems to eliminate the issue.
Would be preferable if scrollUpdate managed to finish its job, but I guess it is fired one time less than necessary in such situations.

masimplo · 2017-09-19T22:47:59Z

@danbucholtz you have requested a review for #12194, when you get around to it, please review this one too as an alternative. I believe this provides a more lasting solution to VS issues as it improves performance at the algorithm level, rather than hogging the system to do faster DOM redraws.

faraazc · 2017-09-20T06:46:51Z

@masimplo I tested with your latest commit. I see VS is broken and there is large gaps in between an item and items are in correct order. Works fine with your previous commit (with your ionic-angular.zip attached by you).

I tried very basic one

constructor(public navCtrl: NavController) {
for (let i = 0; i < 30; i++) {
this.items.push( this.items.length );
}
}
doInfinite(infiniteScroll) {
console.log('Begin async operation');

setTimeout(() => {
  for (let i = 0; i < 30; i++) {
    this.items.push( this.items.length );
  }

  console.log('Async operation has ended');
  infiniteScroll.complete();
}, 500);

}
trackStand(index, stand) {
console.log(index, stand);
return stand;
}

<ion-content padding> <ion-list [virtualScroll]="items" [virtualTrackBy]="trackStand"> <ion-item *virtualItem="let item"> {{ item }} </ion-item> </ion-list> <ion-infinite-scroll (ionInfinite)="doInfinite($event)"> <ion-infinite-scroll-content></ion-infinite-scroll-content> </ion-infinite-scroll> </ion-content>

Attached zip has screen recorded
vs_basic.zip
preview, also tested on device same thing appears.

masimplo · 2017-09-20T07:44:19Z

hmmm this is weird. I tried all the e2e tests in VS and everything seems to work fine with latest code base.
Here is a recording from the infinite-scroll one:

Only difference I see between the two is that you are defining a virtualTrackBy function. ~~Will try this too~~(tried it, everything works as expected), but try running without it and let me know if it makes any difference.
Can you post some more info about your setup? Which angular-ionic version are you currently using? Do you know which commit worked for you as I don't remember which one was the previous zip I uploaded?
Finally try defining approximateHeight as it helps rendering and let me know if it makes a difference.

And here is latest build:
ionic-angular.zip

masimplo · 2017-09-20T11:43:29Z

@faraazc I managed to reproduce your issue by modifying the infinite-scroll e2e test to render bigger list items (99px) with images. Unfortunately this exists both in the new code and the old code.
There is a function that calculated the new height, but only returns it if on of two conditions are met:

The difference between old and new height to be greater than the new height times 0.25
percent to bottom is greater than .995
When using infinite scroll with larger items then if infinite scroll brings say 30 items then this criteria is not passed, but if you bring 80 new records it does. As you scroll down this gets harder and harder to pass as the difference between heights is constant but the 0.25 * newHeight increases.
Also percent to bottom > .995 is almost never satisfied as infinite scroll adds some height to the dom when you reach the last record.
If I remove that constrain and return the newHeight then it works.

Not sure about the concept of these hardcoded numbers, but from looking at the code I would say that infinite scroll would be improbable to work in a generic scenario.

This code was originally written by @adamdbradley as shown here 7679ac0#diff-58836582aafdd1198e252f7f815195e1R530 2 years ago. Maybe he can shed some light on why that constrain is there and if those numbers should be constant or not.
Note: back then virtualScroll did not support infiniteScroll

faraazc · 2017-09-20T12:03:16Z

@masimplo Thank you so much for so deep dive in to VS.
I tried setting [bufferRatio], with that I could rarely see such issue coming. It may occur with more tests. Does setting [bufferRatio] has any impact?.

for my markup above where approximateitem for each item rendered is 57px and setting [bufferRatio]="10", looks like solving this problem to some extent. Can you put some light on it. also @adamdbradley Can you also help us in getting VS issues fixed, since @masimplo is working actively on it. I am sure community needs VS fixed than any other issues.

faraazc · 2017-09-20T12:43:41Z

@masimplo can we remove those two constrains and return new height and test with infinitescroll ?..May be those constrains are not valid any more?.After removing them you can run all tests in your setup and also provide me an ionic-angular bundle.i will also test if that makes sense?..

masimplo · 2017-09-20T13:05:02Z

@faraazc if you remove the constrains then infinite scroll does work, BUT I think this will come with a performance hit, as it will return a new height always and thus do more calculations than necessary. It doesn't seem to break anything, but I might be missing something important here, so I am not comfortable removing it. If you really want to use the code without that constraint then here you go:
experimental ionic-angular.zip

faraazc · 2017-09-20T13:08:36Z

@masimplo Thank you..I will experiment and get back..

ghenry22 · 2017-09-25T10:03:25Z

@masimplo great work! If this fixes #9655 you'll be my hero! will test it out when I get back from travelling later this week and provide feedback.

manucorporat · 2017-09-25T10:38:05Z

@masimplo awesome work! I currently working in ionic-core, but I will ask to focus in getting your PR merged as soon as possible! I haven't reviewed it, but looks like an very good job.

It is complex component, personally I have had a bad time dealing with it :D

HugoHeneault · 2017-09-25T10:55:36Z

@manucorporat So we could hope to have it merge on ionic3? 😲

masimplo · 2017-09-25T11:34:27Z

@manucorporat thanks for the reply, I know you are swamped with stencilJS but virtual scroll is the only component not up to par with the quality of Ionic components and since I really need it working properly I made an effort to fix it as good as I could. Any comments/improvements on the PR will be greatly appreciated whenever you get the time and hopefully this will be merged before v4 hits the shelves.

adamdbradley · 2017-09-25T14:53:47Z

@masimplo awesome thanks for the PR! Yeah we'll run it through our tests and work to get it merged in. Thanks!

manucorporat · 2017-09-29T15:47:43Z

I started reviewing it already, found some issues with existing e2e tests, but not sure yet they are regressions of this PR, still looking at it

masimplo · 2017-09-29T16:09:25Z

Did run the e2e tests while developing, the ones that appear broken (I remember the one with cards with images specifically) behaved the same with original code as well - unless I got confused between commits somewhere. Looking forward to your review.

@manucorporat anything you need let me know, I will be around.

fifafu · 2017-10-04T18:47:32Z

Oh my god, this actually works super well with my pretty complicated endless scroll view (using cards, regular updates, pictures etc.). It also seems to handle edge cases e.g. updates while scrolling or orientation changes successfully. Performance is awesome. (I'm using the 3.7.0 bundle)

This seems to fix all issues I was seeing that basically made it impossible for us to use Ionic 2+ in various apps.

Thank you so much!

manucorporat · 2017-10-06T10:18:09Z

Merged @masimplo !
88b2e83

I merged and squashed it locally and git lost the track of it, I am sorry the commit is not assigned to you :(

HugoHeneault · 2017-10-06T10:21:28Z

Probably the most beautiful Ionic PR this year 😃

masimplo · 2017-10-06T10:26:18Z

Great news @manucorporat it is ok that it is not attributed to me. If it has issues in the future, you will take all the blame :P

@fifafu you are most welcome.
@HugoHeneault thanks for the comment. Appreciate it.

manucorporat · 2017-10-06T10:49:56Z

@HugoHeneault it is!! I officially appoint @masimplo the virtual-scroll maintainer ;) hahaha

manucorporat · 2017-10-06T10:51:49Z

It would be great to learn from our mistakes and create a much better virtual-scroll from the ground up for ionic 4 (ionic-core) that is web component based, the same way we did with the new ion-reorder, MUCH MUCH better in ionic-core.

Virtual-scroll is the hardest component to port.

masimplo · 2017-10-30T22:04:54Z

@manucorporat don't forget to take a look at https://github.com/PolymerElements/iron-list when porting to ionic4. Most issues have been ironed out (pun intended).

sredeker · 2017-11-03T13:48:55Z

@masimplo I see this PR is closed but there are a few others related. What is the expectation of resolving this issue? Is there a targeted release for this fix?

bparvizi · 2017-11-10T20:33:11Z

@masimplo and @manucorporat thanks for all your efforts. I just opened an issue dealing with the combination of filtering and scrolling displaying empty nodes. Project sample and video both available. Please see, #13392. Thanks again.

pablomaurer · 2018-03-09T09:14:14Z

src/components/virtual-scroll/virtual-util.ts


-    for (var i = data.topCell; i < totalCells; i++) {
-      cell = cells[i];
-      if (cell.row !== lastRow) {


The deletion of this, causes the virtual-scroll not to work correctly when using multi-column. Now the PR is already merged and ionic 4 is coming, so this is just a comment to let other people know how they can fix it.

mileEight · 2019-07-24T02:13:39Z

That's good, but there's a problem. When more data is loaded down, the rendered data shouldn't jump automatically. So what's the solution?

masimplo added 14 commits September 15, 2017 14:08

Convert queue consts to enum

4cf1f39

Initialize differ with trackByFunction as ngForOf does

fdd9bd1

Throw errors not strings

8e5774a

Simplify addCell function

ad9c6d9

Simplify changes calculation

a716e58

Make debug logs consistent

7d3b7c4

Stop searching nodes if cell is already rendered

21a7ce9

Change TemplateType consts to enum

ebef282

Stop looking for available nodes after one is found

75aed51

Change comment

a13184f

Return records from VirtualScroll getter

e06d757

Stop clearing DOM in slow path

8590b9b

Update existing nodes by matching cells

acec8ef

Fix top and bottom cells special cases

c8d5e09

masimplo mentioned this pull request Sep 18, 2017

fix(virtual-scroll) many fixes #12194

Closed

11 tasks

masimplo added 2 commits September 18, 2017 17:49

Use recordSize consistently

9b3b0db

Simplify adjustRendered function

07e15d7

masimplo added 2 commits September 18, 2017 19:06

Remove console.warn

84d402d

Remove needClean param from populateNodeData

bfa44b9

Recalculated nodes once more on scrollEnd

564ef41

Use const enums to reduce generated code

6eaf7d1

Fix virtualScroll e2e infiniteScroll test

c6a2a97

brandyscarney requested a review from manucorporat September 29, 2017 15:31

brandyscarney added this to the 3.7.2 milestone Sep 29, 2017

HugoHeneault mentioned this pull request Oct 5, 2017

Ionic2 Segment and Virtual Scroll don't work together properly #7734

Closed

manucorporat closed this Oct 6, 2017

HugoHeneault mentioned this pull request Oct 29, 2017

RC4 ion-img doesn't correctly work with virtualScroll #9660

Closed

This was referenced Dec 1, 2017

VirtualScroll Scrolling / display issues when filtering list #9935

Closed

Virtual Scroll- Combination of Scrolling and Filtering Results in Empty Nodes #13392

Closed

pablomaurer reviewed Mar 9, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(virtualScroll) flickering issue fixes. No need for re-rendering the whole DOM #12917

fix(virtualScroll) flickering issue fixes. No need for re-rendering the whole DOM #12917

masimplo commented Sep 18, 2017 •

edited

Loading

HugoHeneault commented Sep 18, 2017

masimplo commented Sep 18, 2017 •

edited

Loading

faraazc commented Sep 18, 2017

masimplo commented Sep 18, 2017 •

edited

Loading

masimplo commented Sep 19, 2017

masimplo commented Sep 19, 2017

faraazc commented Sep 20, 2017 •

edited

Loading

masimplo commented Sep 20, 2017 •

edited

Loading

masimplo commented Sep 20, 2017 •

edited

Loading

faraazc commented Sep 20, 2017

faraazc commented Sep 20, 2017

masimplo commented Sep 20, 2017

faraazc commented Sep 20, 2017

ghenry22 commented Sep 25, 2017

manucorporat commented Sep 25, 2017 •

edited

Loading

HugoHeneault commented Sep 25, 2017

masimplo commented Sep 25, 2017

adamdbradley commented Sep 25, 2017

manucorporat commented Sep 29, 2017

masimplo commented Sep 29, 2017 •

edited

Loading

fifafu commented Oct 4, 2017 •

edited

Loading

manucorporat commented Oct 6, 2017

HugoHeneault commented Oct 6, 2017 •

edited

Loading

masimplo commented Oct 6, 2017

manucorporat commented Oct 6, 2017

manucorporat commented Oct 6, 2017 •

edited

Loading

masimplo commented Oct 30, 2017

sredeker commented Nov 3, 2017

bparvizi commented Nov 10, 2017

pablomaurer Mar 9, 2018

mileEight commented Jul 24, 2019

fix(virtualScroll) flickering issue fixes. No need for re-rendering the whole DOM #12917

fix(virtualScroll) flickering issue fixes. No need for re-rendering the whole DOM #12917

Conversation

masimplo commented Sep 18, 2017 • edited Loading

Short description of what this resolves:

Changes proposed in this pull request:

HugoHeneault commented Sep 18, 2017

masimplo commented Sep 18, 2017 • edited Loading

faraazc commented Sep 18, 2017

masimplo commented Sep 18, 2017 • edited Loading

masimplo commented Sep 19, 2017

masimplo commented Sep 19, 2017

faraazc commented Sep 20, 2017 • edited Loading

masimplo commented Sep 20, 2017 • edited Loading

masimplo commented Sep 20, 2017 • edited Loading

faraazc commented Sep 20, 2017

faraazc commented Sep 20, 2017

masimplo commented Sep 20, 2017

faraazc commented Sep 20, 2017

ghenry22 commented Sep 25, 2017

manucorporat commented Sep 25, 2017 • edited Loading

HugoHeneault commented Sep 25, 2017

masimplo commented Sep 25, 2017

adamdbradley commented Sep 25, 2017

manucorporat commented Sep 29, 2017

masimplo commented Sep 29, 2017 • edited Loading

fifafu commented Oct 4, 2017 • edited Loading

manucorporat commented Oct 6, 2017

HugoHeneault commented Oct 6, 2017 • edited Loading

masimplo commented Oct 6, 2017

manucorporat commented Oct 6, 2017

manucorporat commented Oct 6, 2017 • edited Loading

masimplo commented Oct 30, 2017

sredeker commented Nov 3, 2017

bparvizi commented Nov 10, 2017

pablomaurer Mar 9, 2018

Choose a reason for hiding this comment

mileEight commented Jul 24, 2019

masimplo commented Sep 18, 2017 •

edited

Loading

masimplo commented Sep 18, 2017 •

edited

Loading

masimplo commented Sep 18, 2017 •

edited

Loading

faraazc commented Sep 20, 2017 •

edited

Loading

masimplo commented Sep 20, 2017 •

edited

Loading

masimplo commented Sep 20, 2017 •

edited

Loading

manucorporat commented Sep 25, 2017 •

edited

Loading

masimplo commented Sep 29, 2017 •

edited

Loading

fifafu commented Oct 4, 2017 •

edited

Loading

HugoHeneault commented Oct 6, 2017 •

edited

Loading

manucorporat commented Oct 6, 2017 •

edited

Loading