WIP: virtual scroll

[ajoslin]

@robdmoore if you wanna test, here it is so far

Test in /test/html/list-fit.html

TODO:

• unit tests
• docs
• grid
• cleanup on $destroy
• one more run of optimizations
• errors on bad developer usage
• allow user to input item size
• fix visual bug while scrolling back up/left
• fix transclusion bug while scrolling down

[adamdbradley]

related #65

[robdmoore]

Awesome! I'll give it a go and let you know.

[robdmoore]

Hey,

Just tried it out. I probably have a fairly complex case, which I guess is good for testing purposes, but it didn't really work yet unfortunately.

My list datasource is populated after the controller loads since it's loaded via the then of a promise (a HTTP request to an API).

This mean that initially I got an error because the datasource was undefined, so I set it to an empty array when the controller is instantiated and that error went away, but when the data was loaded the list didn't update. Also, with an empty array the list showed up one item with no content in it.

Another thing with my list is that I'm using bindonce for performance reasons (once loaded my items don't really change). I removed this to make sure that it wasn't interacting with the virtual scroll and confirmed it had the same issues.

Let me know if you need a plunkr to reproduce or want me to do more testing :)

Thanks

[robdmoore]

Oh and the other thing - at the moment I'm passing a few filters into the data source so that sorting and categorisation can be applied on the fly without having to update the datasource in the controller. Will the current implementation work for that? I also removed that for my first round of testing to make sure it wasn't that.

[ajoslin]

All those use cases sound like they should work - it's getting a lot more polished now, should be done or nearly done today.

[robdmoore]

OK cool - I'll wait to hear back from you when I should test again :)

[ajoslin]

Thanks for the help so far!

[ajoslin]

@robdmoore would you be able to try it out again? It's a lot closer now to where we want it.

Syntax:

<something scroll-collection-repeat="item in items" scroll-item-size="52">
</something>

The size is a pixel amount.

Note: keep your dom elements simple for this. As you scroll down, it compiles new items and inserts them into the DOM. I've only been able to get it to perform great when my items are relatively simple.

[ajoslin]

OK, serious updates happenin'. It's about as ready as it's gonna be, I think.

@robdmoore and everyone, it's ready to test.

1. Pull down this branch (wip-virtual-scroll)
2. Load test/html/list-fit.html, try it out, see the usage
3. Copy that API into your app and see how it works.

Warning: keep your dom as simple as possible. Even the example in list-fit.html janks a tiny bit on older android phones because it has to compile three new dom elements every time the user scrolls down.

Please lemme know any problems you find.

Now to just finish the unit tests, add errors for user misbehavior, document it, add anymore optimizations we can, and merge!

[mlynch]

@ajoslin, awesome work. I noticed a few things I think we should tweak or think about before release:

First of all, it's fricking fast as all hell on my iPhone 5. Crazy! People are going to love this.

• Mousewheel scrolling does not animate (inertial scrolling after stopping) like it does in the normal case. Not sure how big a deal this is though.
• I would still like to see a rectangular grid layout support that is not row based. Right now we just have row swapping, which is awesome, but I want to make sure we can add arbitrary grid layout support as well.
  • When I say "grid", I don't mean grid in the responsive sense, I mean grid in the float sense. Trying to build a grid of photos and then group them into rows of three is a pain. It's much easier to just iterate through them all and float left or display: inline-block and have them wrap.
  • I know this might be slower on older Androids, but practically everything is. It could just be a layout option they supply, like this: <div scroll-collection-layout="grid">, but the default can be row.
• Any thoughts on how this works combined with the infinite scroll system?

Of course, if we ship this today as-is, it will be a big step forward, so maybe these can wait?

Thoughts?

[ajoslin]

1. Mousewheel: I'll check it out, not sure.
2. Rectangular grid: OK, can be done. You're right, it's really annoying for the dev to change his data structure. I'll try to figure out a good api and post my ideas here later
3. It will work with any of our current scroll systems - because the scrollView thinks it's just a normal content area with X height. scrollDelegate, pull to refresh, infinite scroll, bounce, etc work as normal :-)

I will try and implement the grid before releasing, incase it requires a breaking api change.

[ajoslin]

Also, one bug: the items are transformed wrong for one frame when scrolling up, causing a tiny flicker (you can see it if you scroll slowly). Some bad math on the rendering I think.

[mlynch]

@ajoslin what about the API change would have to break? Instead of item-size being a single value, it could take a comma separated (space-friendly) size: `scroll-item-size="100, 200". Or is it a different part of the API that would change?

[udfalkso]

Hi Guys. I'm excited to see this in action! I had a thought that I'll share in case you think it makes sense. Please take it or leave it as you see fit.

It might be nice to add simple support for ng-repeat's "track by" syntax. If present, you could then skip the hashKey fn call getting a tiny performance gain.

[ajoslin]

Good idea, will add it. The point of of the track by syntax is that we can hash some id to cache an item's value instead of caching the whole object itself - it lets us know better whether we need to re-compile an item, or if we can use a cached one.

[robdmoore]

Wow. That gif is AMAZING. One of the funniest I've seen in a PR thread. impressed. Will check this out with my list and get back to you.

[robdmoore]

OK - I've given this a go with my list. The performance of the page is immeasurably better. Scrolling is smoother and in particular moving on and off the page is super fast!

I did notice a few issues on my list:

1. The items show up blank until you start scrolling (after you have scrolled a few items worth) - this happens on all devices so is different from the similar sounding issue I previously raised (ion-list with deferred item content shows up blank intermittently on iOS #1110)
2. If the items have href then it doesn't work (changing to ng-click did work though)
3. At one point I had a set of items that showed up some way down the list, but didn't bind (i.e. they showed all the angular code, e.g. {{myvariable}}, but I couldn't replicate after refreshing
4. When scrolling down the list for the first time it's a little janky (I have images and I suspect it's the image downloading/rendering that causes it) - scrolling back up and down again is really smooth after scrolling down the first time
5. When changing the list dynamically (e.g. via a filter like I describe above) the items don't change

Great work so far - this is really exciting!

[ajoslin]

Thanks @robdmoore ! Pushed a fix. A small request, could you number your bugs? Makes it easier for us to discuss them as numbers. I edited your comment.

1. Can you post your markup for reproducing the blank items? I'm not experiencing this.
2. href is working great for me.
3. I see these uncompiled items too, not sure about this yet - looking into it.
4. The lag may be due to images, yes. Could you post your markup for these?
5. Fixed the digest not changing the list correctly - it now redraws the visible area when the list changes.

[ajoslin]

Also fixed slight item flicker while scrolling up (was due to some bad math).

[ajoslin]

Fixed the items sometimes looking uncompiled.

[robdmoore]

Awesome! I'll produce a plunkr tomorrow to try and reproduce what I'm seeing and try out the latest changes :)

[ajoslin]

1. Renamed the directive to scroll-item-repeat and attr scroll-item-size.
2. track by syntax added - now using the RegExp that angular uses.
3. The directive errors now if you use it incorrectly

Now to get the grid syntax working, then unit test time!

I'm doing unit tests last here because the inner-workings are in such a state of flux.

[ajoslin]

Grid works now. If you specify two sizes, it will use the second to fill in the secondary direction. In this case, the secondary direction is horizontal because our scroller is vertical.

Using separate repeat-elements in your grid It's slightly slower than the old solution, since it has to compile {{itemsOnRow}} seperate elements each time you scroll. But it's not actually a very noticeable difference at all.

As usual, it's important to remember simpler DOM is always better for your infinite repeater.

Try out the pull-to-refresh as well.

[rotorgames]

Hey. Great job.
But slow, the thing is that dom Components physically change their place, causing reflow and repaint in your browser.
The problem is solved by using "-webkit-transform: translate" and "position: absolute" each element on.
In this case, do not need to physically relocate dom.

Do not think about it?

P.S. Sorry for my bad English.

[ajoslin]

Hi @rotorgames. Yes, it is slow if you are using complicated DOM. Can you post your markup, and your css rules?

Your idea is interesting: taking over the DOM/css's positioning system and position things ourselves.

With your idea for a solution, we would still be inserting and removing elements from the DOM, but not repositioning. It is an interesting idea. I will think about it. It definitely has downsides, not using the CSS/DOM's positioning.

Thanks for the feedback. And please post your HTML/CSS.

[rotorgames]

@ajoslin, tomorrow see your code. I'll try to remake a "-webkit-transform: translate".
rotorgames@de0a00c

[ajoslin]

@rotorgames sounds great, look forward to seeing it.

[ajoslin]

So after a lot of testing on a basic implementation, it is definitely a noticeable boost in performance on older Androids or with complicated DOM.

The downside is it forces people to explicitly set the width of their items in their CSS, due to the items becoming position:absolute. Also, we aren't really using the DOM for positioning anymore - how bad is this actually? I'm not sure yet.

I'll see what you come up with tomorrow @rotorgames and consider implementing it! Thanks for the tip!

[robdmoore]

FYI - I've run out of time for this today unfortunately sorry! Will definitely try and look tomorrow.

[ajoslin]

@udfalkso: You can use a getter to make your first item different:

<div collection-repeat="item in repeatedItems"
  collection-item-height="getHeight(item)">
  <div ng-switch="item.isBig">
    <div ng-switch-when="true">I'm a big offset item</div>
    <div ng-switch-default>{{item}}</div>
  </div>
</div>

//Every time items is changed, update repeatedItems to match
$scope.$watchCollection('items', function() {
  $scope.repeatedItems = [ {isBig: true} ].concat($scope.items);
});
$scope.getItemHeight = function(item) {
  return item.isBig ? 200 : 50;
};

[ajoslin]

@robdmoore working on all this stuff today.

[robdmoore]

Awesome! thanks mate :)

[ajoslin]

Also wow - the latest version with the scope digesting improvement is SO MUCH SMOOTHER! Even on iPad!

exciting :-)

[ajoslin]

Finally finished all unit tests! I should have done that a lot earlier, but we kind of rushed the release of this.

@robdmoore can you try scroll position on back in the latest nightly?

[ajoslin]

..Also, ionItem compile error fixed in latest!

[udfalkso]

Thanks @ajoslin - It wasn't pretty, but I managed to use this approach to get it the result I was looking for. I hope you'll consider incorporating a smoother solution to this in the future. Thanks again for the great work, this feature is huge.

[ajoslin]

Just not sure of a better solution yet. I will think about it.

[robdmoore]

Hi @ajoslin,

I've pulled in the latest nightly and tried it out:

• ionList errors fixed
• Href works
• Scroll position is retained

I've also noticed the following (happy to add a separate issue for any of these you want and if they don't make sense / you can't replicate let me know and I'll try and create a plunkr):

1. Modals no longer seem to have has-header automatically added to the ion-content (I can manually add these in, but not sure if the existing behaviour was meant to be retained)
2. The app can be dragged around such that it has a space on the right hand side, e.g.:
   
3. There is possibly a performance issue when using href instead of $state.go, but it's also possible it was caused by the next issue:
4. When the page first loads the collection-repeat draws every single item in the list all at the top position - if I change the items in the list one of the filters then it redraws correctly

[ajoslin]

@robdmoore:

1. Can you open a separate issue for this with a reproduce case?
2. Interesting, we fixed a similar problem in beta.3. I'm now able to reproduce this on an iPad as well at http://ionicframework.com/demos/collection-repeat - I'll try to narrow it down.
3. Not sure at all
4. Not able to reproduce this - can you post some of your list code?

[ajoslin]

Hmm, for 3. What do you mean by 'performance issue'? Lag in compiling, or scrolling, or loading the next page?

[robdmoore]

Cool will do re: reproducable issues.

re: 3 as in changing from the page to the next page.

[robdmoore]

re: 1 I've figured out it's because we are using <header class="bar bar-header bar-positive">...</header> rather than <ion-header-bar class="bar-positive">...</ion-header-bar>. Is it expected behaviour that it wouldn't add the has-header class unless you use the directives? (If so that's fine, it's just a change from before I think since all my modals lost their top padding when I moved to nightly).

[ajoslin]

Yes, the ion-header-bar directive adds the has-* classes. If you don't use the directive, you'll have to add them on your own.

Also, I saw your other issue - will look into it.

Thanks for so much detailed feedback, Robert!

[robdmoore]

No worries - thanks for being so responsive to the things I notice! It makes all the difference :)

Good to know re: ion-header-bar. I assume the same is true for ion-footer-bar?

[robdmoore]

Hmm I'm struggling to try and replicate my weird problem where all the items show up at the top in a basic plunkr. Will have to try again tomorrow.

[robdmoore]

OK, ruled out a few things:

• Removed all CSS from page - still happens so definitely a JS problem
• Tried out filter and $timeout on a plunkr and it didn't happen

Will keep trying...

[ajoslin]

Is it possible your collection-item-height function is ever returning 0?

[robdmoore]

No I hardcode it to 84.

I think it might be to do with the data somehow. If I comment out the bit that sets the data with a static array of items then it seems to work. There isn't much fancy in the data - just some nested arrays (I just checked if that was the cause and it didn't seem to be)).

[robdmoore]

This could be a red herring, but it seems that if I have a nested array of hashes in my items then it might cause it to sometimes happen. I can't replicate in my plunkr though, hmm how frustrating! OK I'm calling it a night, it's really late haha. Will try again tomorrow.

[ajoslin]

Haha, thanks for looking. Sounds definitely very weird. Goodnight.

[robdmoore]

Hey,

I'm still struggling to replicate this outside of my app. If you were debugging the problem I am describing (it's rendering every item in the list rather than say 5 and they all appear at position 0,0) what part of the ionic code would you put breakpoints in?

I can't publish the code because it's a commercial application - is there any possibility of hooking up a Skype call so you can see it via screen share though?

[ajoslin]

Hi rob,

I wouldn't really know where to start until I saw it.. I guess I would find
the collection repeat render function in the bundle and step through that,
seeing what position it is calling renderItem with.

Just search your ionic.bundle.js for 'render = func'.

I could definitely do a Skype call though. My time zone is gmt-7. Email me
(email in github profile) and we'll figure out a time.
On May 6, 2014 7:32 PM, "Robert Moore" [email protected] wrote:

Hey,

I'm still struggling to replicate this outside of my app. If you were
debugging the problem I am describing (it's rendering every item in the
list rather than say 5 and they all appear at position 0,0) what part of
the ionic code would you put breakpoints in?

I can't publish the code because it's a commercial application - is there
any possibility of hooking up a Skype call so you can see it via screen
share though?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1157#issuecomment-42379953
.

[ajoslin]

Rob's problems fixed, closing.

💃

[robdmoore]

🍰