fix(virtual-scroll) many fixes

[pablomaurer]

Ionic Version: 3.x

Short description of what this resolves:

• init virtual-scroll with null/undefined gave errors VirtualScroll Can't read property length of null #11439
• changes made to the array, caused a flickering in the list virtualScroll flashes "wrong" items and in inverse order on init/reinit #12047 Virtual Scroll filtering causes a lot of flicker and performance degradation #12035
• works again when wrapped in a *ngIf Conditional on VirtualScroll causes no content to be displayed in any case #9655
• did not clear elements when array got cleared Virtual Scroll does not clear elements when items are cleared and throws error #12034

maybe also #10971

Influences / How I worked

I went through the history of virtual-scroll.ts and took an older version where people said it was better. Then I added the changes slowly again to find out where some bugs where introduced.

I also compared stuff with angular's ngForOf implementation because ionic also did this in the start, but with time ionic drifted more and more off.

Changes proposed in this pull request:

• never update the _differ, create it only if it does not already exists. don't use the setter instead uses the ngOnChanges. why? ngFor is also like this. May solves some init problems. Easier to reason about.
• removed the check if there are records in _changes(). why? because if we clear the items array with setting it to null, the clearing get's never proceeded.
• only get records length if there are records. why? because with the above change it's now possible that _records is null.
• removed the wrapping this._plt.raf(() in renderVirtual(). why? because this caused the flickering while updating the list. caution I'm not sure why or what this._plt.raf(() did, so may review this point a bit deeper.
• includes PR fix(virtualScroll) remove items if array is null #12134 there is also a bit a description.

How the issues came and what we should care in future.

I think a lot issues came with changes to get virtual-scroll with InfiniteScroll together working, but I think it's more important to get virtual-scroll battle tested before getting it to work with other stuff which may introduces new bugs. PS: I didn't test the changes InfiniteScroll.

Don't care about everything below, you are free to merge..

My WIP Notes - for even further improvements

• fixes virtualScroll stops rendering after a certain amount of scroll - solution #11798
• maybe #11788
• maybe Virtual scroll not working child component without <ion-content> #9424
• If you have a ion-button above vs the calcDimensions gets 16px too much? Would not matter so much, if the next point would be resolved.
• because calcDimensions(); get's only executed if needsClean=true, if you push an item the viewHeight get's not updated and so you will not see the newly added items until another change causes needsClean to be true.

---

Semantic issues

Would look better in a separate PR, so this PR, stays clean with just fixes.

rename

• processRecords() rename to addCells() why? Nobody know's what it's gonna process.
• addCell() rename tocreateCell() why? it doesn't add anything, it just returns a cell.
• readElements() rename tocalcCellDimension() keep consistent with calcDimensions. And readElements doesn't give any hint about, that it will change any data.

modified by reference

• processRecords() modifies _cells by reference and doesn't return anything, looks a bit weird.
• calcDimension() modifies _data by reference and doesn't return anything, looks a bit weird.

more

• in renderVirtual() remove resetting topCell/bottomCell, because they get overwritten anyway in the following adjustRendering().
• before everywriteUpdate() you call readUpdate() or updateDimensions() which is contained in readUpdate(). Just put readUpdate as first line into writeUpdate().
• before every adjustRendered() you call updateDimensions() so put updateDimensions() in adjustRendered().
• updateDimensions() would split it into calcCellDimensions() and updateEstimatedDimensions().
• virtual-util.ts contains a lot var replace with let?
• readElements() here you update cell.left but in updateDimensions() it will get overwritten anyway.

---

Overview helps me finding out what is going on when..

init/data changes

• readUpdate()->calcDimension() updates heigth/width/top/left of data and guesses itmWidth/itmHeigth
• writeUpdate()->processRecords() records to Cells
• writeUpdate()->renderVirtual()->updateDimensions() calcCellDimensions
• writeUpdate()->renderVirtual()->adjustRendered() set topCell/bottomCell in data finds out which cells should be rendered.
• writeUpdate()->renderVirtual()->populateNodeData() assign cells to nodes.
• writeUpdate()->renderVirtual()->initReadNodes() update first cell with position of first node and updateDimensions() again?
• writeUpdate()->renderVirtual()->updateNodeContext() updates positions of the nodes?
• writeUpdate()->renderVirtual()->_stepChangeDetection() detect changes in nodes?
• writeUpdate()->renderVirtual()->_stepDOMWrite() set positions of nodes on the dom

scroll

• _stepNoChanges()->processRecords() records to Cells
• _stepNoChanges()->updateDimensions() calcCellDimensions
• _stepNoChanges()->adjustRendered() set topCell/bottomCell in data finds out which cells should be rendered.
• _stepNoChanges()->populateNodeData() assign cells to nodes.
• _stepNoChanges()->updateNodeContext() updates positions of the nodes?

what's the difference of stepsNoChanges and writeUpdate?
what's the difference of writeUpdate and _stepDOMWrite

[pablomaurer]

Ionic-team please decide between if I should go for Case 2 or Case 3

In virtual-util.ts there is calcDimensions() with this line:

const viewportElement = virtualScrollElement.parentElement;
data.viewHeight = viewportElement.offsetHeight;
data.renderHeight = (data.viewHeight * bufferRatio);

So this code, get's the dimensions for virtual-scroll before it will render the new nodes. But that it's before causes issues and getting it after is to late.

This causes the following behavior:

Case 1, parent of virtual-scroll is ion-content

Works because, when the parent of virtual-scroll is the ion-content it will get the offset-height of scroll-content which is ever the full-height. Because like this the viewHeight will ever stay the same.

solution: works anyway.

Case 2, parent of virtual-scroll is div with height not set

Fails because, when you put virtual-scroll in a div with no height-specified. It will ever use the height it had before rendering the new nodes. So if you replace and array with 3 items with an array of 50 items. It will use the renderHeight for 3 items, even you are displaying 50 items. which results in only seeing 3 items while scrolling and seeing a lot of blank space.

solution: just act as we where in case 1. I'm new to angular and I don't saw that you ever used something like getElementsBy... So please advice me what to use instead!

let el = document.getElementsByClassName('scroll-content')[0] as HTMLElement;
data.viewHeight = el.offsetHeight;

Case 3, parent of virtual-scroll is div with fixed-height and overflow:scroll

This would also be somehow cool, since it would make support for multiple virtual-scrolls.
Now this is better than case 2, because thanks to the fixes-height we got the correct renderHeight, but the scrollListeners are listening to the ion-content so it would not update while scrolling.

solution: attach scroll-listener to parent element and say people that virtual-scroll must be in a container with a fixed-heigth and a overflow:scroll.

[HugoHeneault]

Hey there! I'd like to implement these fixes in my ionic3 project, but I don't know how to do it. @mnewmedia how can I achieve it? Thanks a lot for your work!

[pablomaurer]

@HugoHeneault There is no easy way I know to implement this fixes in an existing project.

[HugoHeneault]

Okay, so we just have to wait I guess :( Thanks for your reply!

[AmitMY]

@speedtreammanga Please post this as an issue. This does not belong in the PR thread

[HugoHeneault]

If anyone is willing to try this PR easily, you can remplace your ionic-angular by this folder we've built:
ionic-angular.zip

[nilebma]

Thanks for compiling this PR in a new version @HugoHeneault. If by any chance you did the same kind of package for [email protected], I'll be very interested :-)

[beckson]

any news to the virtual-scroll?
I had the same problem, using virtual-scroll like

<ion-content>
  <ion-scroll>
      <ion-list [virtualScroll]="xxx">
      </ion-list>
 </ion-scroll>
 <ion-scroll>
</ion-scroll>
</<ion-content>

[HugoHeneault]

@nilebma I don't have this. But you could clone the ionic repo, then apply the changes of this PR to the current ionic repo, then build it. :)

[tonypig93]

Hi, I've download the folder you built and it works fine, solved the annoying flickers, but I got another problem. After ion-infinite-scroll fetches data several times, the list display blank in the viewport until I continue scroll down then I can see items rendered, actually I've came to this problem before your commit, can you take a look at it? Plus, will this PR be included in the next Ionic release?

[masimplo]

Here is the compiled version for 3.6.0 for whoever needs it.
Whole library: ionic-angular.zip
Only virtual scroll folder: virtual-scroll.zip
Just the patch: virtual-scroll.patch.txt

Please give some attention to virtual scroll, it has been broken for many months now.

[masimplo]

This might not be the correct path to take to fix the issue. Removing RequestAnimationFrame wrapper made virtualScroll not render correctly in actual devices (probably because they are slower and thus queuing system is needed). Besides, completely disposing of nodes every time the dataset changes was always problematic and even if you can fake it, there will always be a performance impact. After extensive tests with this pull request's code and meticulous debugging of the original code I have come up with a pull request that addresses the above issues by changing the core handling of what happens when a change is identified. For more info please see #12917

[DrewLandgrave]

@brandyscarney, @danbucholtz
Has there been any movement on this?

[HugoHeneault]

Yeah, another PR which apparently might be more efficient than this one was merged but not yet released. Probably soon with 3.7.2 :)
88b2e83

[fdambrosio]

a simple virtualscroll is not rendering correctly on iOS9, but it's working on iOS10 & IOS11 & Android.

[Ionitron]

Hello and thank you for contributing to Ionic! We have been working on porting all of the Ionic components to web components and have recently updated master to reflect this. This significant change has caused this pull request to break. While we really appreciate the time and effort you put into creating this, we are not able to merge it because of the newly introduced conflicts. We are extremely sorry about this. We will not be merging any more features in to v3. If this is a feature and you have the time, please resubmit this PR against the master branch. If this is a critical security issue in v3, we would greatly appreciate it if you would resubmit the PR against the new v3 branch. Thanks so much for your time!