Jumping back during scroll when rows have different height

[arudnev]

If we take the chat demo and change it to generate / show rows of different height then when you try to scroll up slowly it will load and show older messages, but then will jump back to more recent ones, making it impossible to scroll up.

I've cloned ui-scroll repository, made couple of changes in styles (to have multi-line rows of different height) and changed generation of text for the chat items - http://rawgit.com/armortext/ui-scroll/master/demo/chat/chat.html

To reproduce scroll all the way up, then all the way down and up again, then around item 80 it will consistently start jumping back and forward while you are trying to scroll up, producing something like this in the console log:

index = -71; count = 10
index = -82; count = 10
index = -92; count = 10
index = -79; count = 10
index = -91; count = 10
index = -78; count = 10
index = -90; count = 10
index = -76; count = 10
index = -88; count = 10
index = -74; count = 10
index = -87; count = 10
index = -71; count = 10
index = -82; count = 10

It has similar behavior when you set minIndex / maxIndex on the datasource.

[arudnev]

I've created another example which might be related to #105 - https://jsfiddle.net/yr3wj3cy/

It's based mostly on the chat example with modifications that mentioned earlier, but it adds this:

    $timeout(function () {
        datasource.minIndex = Server.first - Server.max + 1;
    });

It shows related, but probably separate defect (might make sense to track it separately) - if minIndex is specified on the datasource then on initial load or reload the viewport is positioned not to the startIndex element, but rather to some element closer to the minIndex, all after doing multiple requests for more data from the datasource. So, instead of showing item 1 on load it would show something like item 94 by default.
This might be a special case because adjustScrollTopAfterMinIndexSet is not called until after the loading is done, yet adjustPadding relies on current value of minIndexUser, so scroll and padding are out sync while reload is being processed.
On other hand, if we consider adjustBuffer and adjustScrollTop* as something that need to work in tandem to ensure that viewport.scrollTo is maintained correctly, then it can be treated as parts of the same defect.
It seems that logic in adjustScrollTopAfterPrepend is a bit off for some cases when topPadding.height() == 0 already, i.e. for the case when we already reached bof and need to adjust viewport.scrollTo() after elements for newly added items are resized. In case when we are at bof we are already have all the items added to DOM, resized and properly scrolled to, so there is no need to change viewport.scrollTo().

[dhilt]

I've pushed some changes into 'master' and I beleive that the situation with different item heights becames better. Also I have tried to inject the latest sources into your demo and did not see the issue. Could you please double check it?

[arudnev]

It fixes the issue in those samples, but I tried it in a real app and it's still happening, even though not as often and repeatable as it was before.
Unfortunately, another issue is introduced with those latest changes.
Now there is this jump due to temporary (sub-second) changes in padding (unbound / bound padding adjustments), which visually looks like scroll to different viewport page and back, with all kinds of changes in colors, flash of images, etc, all in addition to jump of scroller.

[dhilt]

@arudnev thanks, now I see, looks like we need to think about refactoring of scroll/paddings adjustment process in general... or even clipping/inserting process itself. Btw I'm going to try to fix this issue over old code base once again.

[arudnev]

I was doing debugging and ran into something unexpected that explained at least some of the weird jumps.
I'll see if I can come up with samples to reproduce it (need to check if it's in any way caused by layout / css used by the the item templates), but here is quick summary for now.
When we scroll up it gets more data to be prepended to the buffer, then does clipBottom, then prepends the results, then checks if it needs to load more data and as part of that call to shouldLoadBottom sometimes returns true (strange, we just clipped bottom), it fetches more data to append, then does clipTop and appends new items, etc.
I thought that shouldLoadBottom returns true shorty after the clipBottom while we are scrolling up due to some flaws in logic in adjustBuffer / adjustPadding / adjustScrollTop*, but it looks like it might be caused by some incorrect assumption in clipBottom / clipTop.
In those operations the overageHeight is calculated, the height of bottomPadding / topPadding is incremented by that value and overage items are removed from the buffer. In theory it should result in unchanged positions of items that are currently in visible area, but apparently they get visually shifted a bit in some cases.

The following extra logging in clipTop should demonstrate that change in visual position:

          if (overage > 0) {
            console.log(`clipping top: overage = ${overage}, overageHeight: ${overageHeight}`);
            console.log(`before: scrollTop = ${viewport.scrollTop()}, bottomDataPos = ${viewport.bottomDataPos()}`);
            // we need to adjust top padding element before items are removed from top
            // to avoid strange behavior of scroll bar during remove top items when we are at the very bottom
            topPadding.height(topPadding.height() + overageHeight);
            buffer.bof = false;
            buffer.remove(0, overage);
            buffer.first += overage;
            console.log(`after: scrollTop = ${viewport.scrollTop()}, bottomDataPos = ${viewport.bottomDataPos()}`);
          }

Instead of bottomDataPos we could check for scrollTop of the top visible element, but in any case it should not change when we clip overage items from the buffer and increment top padding height:

clipping top: overage = 15, overageHeight: 1586
before: scrollTop = 2215, bottomDataPos = 4009
after: scrollTop = 2215, bottomDataPos = 3964

[arudnev]

It seems that this particular miscalculation in clipTop / clipBottom comes down to value of line-height.

If we set line-height: normal then the following discrepancy is equal to 1:

buffer[i + 1].element[0].offsetTop - buffer[i].element[0].offsetTop - itemHeight

For line-height: 1 there is no discrepancy between difference of offsetTop of items and sum of buffer[i].element.outerHeight(true)

Here is default value in bootstrap css:

body {
...
  line-height: 1.42857143;
...
}

So, the discrepancy in that case (most probably can be observed in demo code) is around 3px per item (4009 - 3964 / 15 = 3) .

It probably affects most of the other calculations with paddings / scroll / offsets where element.outerHeight(true) is used.

[dhilt]

@arudnev I've done with paddings/scrollTop adjustments refactoring. One of the important idea is to hide unbound elements with no adjustments and then to show them after the data is bound. Also the mechanism of such adjustments has been refactored itself. A common scenario with different item heights passes successfully. But may I ask you to check this on your side?

Regarding to line-height it would be greate to make sure that there is a problem and make a separate issue with repro or even PR...

[arudnev]

Great improvement, the trick with ng-hide is very clever, it seems to be mostly fixing the original issue, so I'm going to close this ticket.
I still see small but consistent jumps in scroll position after new data is fetched.
When bigger jumps are happening when I slowly scroll up I also see in the logs it fetches top, then quickly bottom and top again:

ds.get(-229, 10, ...)
ds.get(-239, 10, ...)
ds.get(-220, 10, ...)
ds.get(-239, 10, ...)

I believe those are related to the issue with clipTop / clipBottom / line-height that I mentioned earlier, so I'll see it I can make it easily reproducible and open separate ticket, if so.