angular-ui / ui-scroll

Unlimited bidirectional scrolling over a limited element buffer for AngularJS applications
http://angular-ui.github.io/ui-scroll/demo/
MIT License
327 stars 107 forks source link

Fixed row height optimization #221

Open dhilt opened 5 years ago

dhilt commented 5 years ago

Based on issue https://github.com/angular-ui/ui-scroll/issues/220.

  1. Visibility-watcher feature might be optional.
  2. When a row height is fixed, the performance could be improved.
priandsf commented 5 years ago

@dhilt We are intensively testing this optimization and found an issue with the allow-visibilty-watch flag.

=> How it manifests: Scrolling with the mouse wheel does not work when the scroller is first rendered. After we scroll once using the scrollbar, then it starts to work properly

=> Why it behaves like this: Because the wheelHandler() event handler checks for scrollTop === 0 && !buffer.bof. At that time, bof==false and it prevents the scroll (both directions).

=> Why it works with the visibility watcher Because visibilityWatcher() calls doAdjust() which fetches the rows before first (negative index in my case). This sets bof=true. It seems to be a side effect here that makes the wheel work.

I'm not sure what wheelHandler() is for. I suppressed it for testing and didn't see an issue. But I bet there is a good reason for it :-) A solution is to detect the scroll direction and add that to each condition, like bellow:

const deltaY = event.deltaY;
if ((deltaY<0 && scrollTop === 0 && !buffer.bof) || (deltaY>0 && scrollTop === yMax && !buffer.eof)) {

But checking deltaY to find the direction is not reliable, according to MDN.

Any idea?

dhilt commented 5 years ago

@priandsf There was the issue years ago (https://github.com/angular-ui/ui-utils/issues/239) and it was fixed with PR https://github.com/Hill30/NGScroller/pull/39, which is responsible for wheelHandler code. We also have demo http://localhost:5005/scrollBubblingPrevent/scrollBubblingPrevent.html and test case ("prevent unwanted scroll bubbling").

Well, I took v1.7.4, commented out

wrapper.unregisterVisibilityWatcher = wrapper.scope.$watch(() => visibilityWatcher(wrapper));

line and run the demo app. And I didn't see your problem with Mac's track pad (wheelHandler did work). I will try to reproduce your issue, but at first glance it's not so obvious, and it would be helpful if you might say how to reproduce it with minimal changes over the latest version (1.7.4).

priandsf commented 5 years ago

@dhilt Ok I narrowed down the problem in our app. I cannot reproduce it yet on one of the samples ;-(

To make a long story short, it happens because buffer.effectiveHeight in enqueueFetch returns 0. It returns 0 because the outerHeight of every element is 0, although the elements are properly inserted in the DOM but not yet "digested" (resolved: the ng-repeat is not yet executed). Hard to understand why, but our data retrieval code triggers some $digest which led to this situation.

Nevertheless, I checked-in a fix that makes it works in our case: when row-height is provided, the buffer uses it to calculate the effective height. This is consistent with rest of the code.

priandsf commented 5 years ago

@dhilt Hi Denis, I'll be happy if you can have a look at the row-height. I put many optimizations into it:

priandsf commented 5 years ago

@dhilt I uploaded our latest code to my branch if you want to give a look. We did sone extensive testing on our application and we haven't found any major issue.

dhilt commented 5 years ago

@priandsf I will take a look when I have time, especially I think I'm most excited about the performance demo!