InfiniteScroll stops intermittently

[nnbrandon]

The pageHeight is calculated at the beginning of the rendered component but pageHeight is kept constant. A page can have varying content as a user scrolls and pageHeight is still kept the same and can cause the InfiniteScroll to miscalculate the nextEndPage. Rather than using pageHeight, I am proposing the usage of scrollHeight to determine when to call the onMore() function.

Although this works, the scrollHeight doesn't play well with the replace feature, I left a conditional statement for nextEndPage to use the original pageHeight if replace is set to true.

Feedback is very much appreciated.

What does this PR do?

Introduce using scrollHeight rather than using the pageHeight as a user scrolls.

Where should the reviewer start?

src/js/components/InfiniteScroll/InfiniteScroll.js

What testing has been done on this PR?

Before using InfiniteScroll from master: https://codesandbox.io/s/green-water-n7e21?file=/src/App.js

After using InfiniteScroll from this PR: https://codesandbox.io/s/blue-tree-q4d1o?file=/src/App.js

How should this be manually tested?

Scroll through the contents of the table until you are no longer able to. The maximum index number to be reached at is 999.

What are the relevant issues?

#4337 & #3106

Do the grommet docs need to be updated?

No

[halocline]

Just quickly glancing to get an understanding to your approach and noticed a typo:

Suggested change

	document.documentElement.scrolHeight || document.body.scrollHeight;
	document.documentElement.scrollHeight || document.body.scrollHeight;

Going to continue studying.

[nnbrandon]

Thank you for pointing this out. Fixed

[IanKBovard]

It's hard for me to accept any solution when the problem is not able to be reproduced consistently. I'm able to get into the high 2000's(or until my computer crashes) on grommet master, quite consistently, when scrolling. I'll have to investigate further, to see if this is the best solution. Regardless of the solution, i do think it should work with replace.

[nnbrandon]

It's hard for me to accept any solution when the problem is not able to be reproduced consistently. I'm able to get into the high 2000's(or until my computer crashes) on grommet master, quite consistently, when scrolling. I'll have to investigate further, to see if this is the best solution. Regardless of the solution, i do think it should work with replace.

It is impossible to reproduce the problem on the standard storybook and default code sandbox for InfiniteScroll on master with the given examples because the pageHeight will always be the same for the pages used in the storybook and sandbox.

I created a code sandbox that consistently reproduces the problem: https://codesandbox.io/s/green-water-n7e21?file=/src/App.js:1025-1046

Depending on the monitor/screen size, it will stop scrolling with the new pages. With my Macbook 13 inch, I am unable to scroll past index 159.

In this code sandbox, it contains a processors.json file which holds a members array of strings. Some strings are the same and some aren't but as you scroll and receive a page of 80 members from the array, the content will always be different from the page before. The pageHeight is captured but never changed at the beginning of the render of this component. nextEndPage is calculated incorrectly because of the pageHeight captured in the beginning of the render with the different page results.

I created another sandbox that contains the code changes I made on this PR: https://codesandbox.io/s/blue-tree-q4d1o?file=/src/App.js.

On my 13 inch Macbook, I am able to continously keep scrolling to the end of the array at index 999.

[IanKBovard]

@nnbrandon ah, i gotcha. thanks for clearing that up. sorry, i neglected to test it on my laptop. I can reproduce it consistently now. Will continue to test more.

[IanKBovard]

Hey @nnbrandon just to let you know i'm still looking at this issue. Just got done with a conversation with @halocline who is also working on infinite scroll. We noticed a few other problems, and a general lack of testing for the component. For now we'd like to to build up the testing library, to add stability, and to make sure your pr doesn't cause any breaking changes.

I've looked through your pr pretty thoroughly, my main concern is that infinite scroll has a lot of complexity, and you're conditional adds extra complexity. This isn't a bad thing, but I would like to add a couple tests that we can run your pr through before we can merge it in. Since, backwards compatibility is very important.

Our goal is, by mid next week we'll have enough tests. I will update you as soon as that is the case.

[halocline]

Hey @nnbrandon just to let you know i'm still looking at this issue. Just got done with a conversation with another dev who is also working on infinite scroll. We noticed a few other problems, and a general lack of testing for the component. For now we'd like to to build up the testing library, to add stability, and to make sure your pr doesn't cause any breaking changes.

I've looked through your pr pretty thoroughly, my main concern is that infinite scroll has a lot of complexity, and you're conditional adds extra complexity. This isn't a bad thing, but I would like to add a couple tests that we can run your pr through before we can merge it in. Since, backwards compatibility is very important.

Our goal is, by mid next week we'll have enough test. I will update you as soon as that is the case.

@IanKBovard, well said. @nnbrandon, thanks for your contribution and patience on this -- very helpful. We'll get tests in place so we are able to confidently scale and enhance.

[nnbrandon]

@IanKBovard @halocline It is very much understandable to have more tests around InfiniteScroll. Thank you for keeping me updated and will be looking forward to the additional tests for InfiniteScroll.

[ShimiSun]

Where are we with this PR @IanKBovard ?

[IanKBovard]

The tests needed to run this pr against arent currently ready. I believe tests for show and step were just recently merged in. The next step would be to fix the issues with show and step(which, i believe halocline has a fix for). Once that is merged in, we can use step and show to create a test for onmore, and other scroll functionality that we can run against this pr. @halocline can correct me i'm wrong, but i believe these are the next steps.

[halocline]

The tests needed to run this pr against arent currently ready. I believe tests for show and step were just recently merged in. The next step would be to fix the issues with show and step(which, i believe halocline has a fix for). Once that is merged in, we can use step and show to create a test for onmore, and other scroll functionality that we can run against this pr. @halocline can correct me i'm wrong, but i believe these are the next steps.

Yes, that's the order in which we need to progress here.

[IanKBovard]

@nnbrandon Just updating you at where we're at right now with this pr. This was just merged in #4426
Show was an important prop that needed to be fixed in order for us to start writing tests for InfiniteScroll's onmore function; which we can run alongside this pr. I'll have to check with @halocline, but I believe we can start doing that now.

[nnbrandon]

@nnbrandon Just updating you at where we're at right now with this pr. This was just merged in #4426
Show was an important prop that needed to be fixed in order for us to start writing tests for InfiniteScroll's onmore function; which we can run alongside this pr. I'll have to check with @halocline, but I believe we can start doing that now.

Sounds great! Thank you for keeping me updated Ian.

[IanKBovard]

@nnbrandon While we were able to increase the testing coverage for InfiniteScroll, we were unable to create tests that directly test the main functionality of the component(scrolling). Which, is unrealistic without end to end testing, but I was hopeful. For now we will just have to settle for extensive manual testing.

This pr looks good, but have you tried this solution instead of leveraging scrollHeight? Just want to make sure we explore everything.

const nextEndPage = Math.min(
  lastPage,
  Math.max(
    (!replace && endPage) || 0,
    multiColumn
      ? Math.ceil(((top + height + offset) * width) / pageArea)
      : Math.floor((top + height + offset) / pageHeight + 1), //added +1 to this calculation.
  ),
);

This seems to work with your example on my end. I just added + 1 to the final line. I don't, however, know every use case for inifiniteScroll, and would like your input.

[nnbrandon]

@nnbrandon While we were able to increase the testing coverage for InfiniteScroll, we were unable to create tests that directly test the main functionality of the component(scrolling). Which, is unrealistic without end to end testing, but I was hopeful. For now we will just have to settle for extensive manual testing.

This pr looks good, but have you tried this solution instead of leveraging scrollHeight? Just want to make sure we explore everything.

const nextEndPage = Math.min(
  lastPage,
  Math.max(
    (!replace && endPage) || 0,
    multiColumn
      ? Math.ceil(((top + height + offset) * width) / pageArea)
      : Math.floor((top + height + offset) / pageHeight + 1), //added +1 to this calculation.
  ),
);

This seems to work with your example on my end. I just added + 1 to the final line. I don't, however, know every use case for inifiniteScroll, and would like your input.

@IanKBovard, I tested the +1 to the final line on my example. Although this solves the problem of varying and different pageHeights, it does seem as if that the nextEndPage gets calculated largely before the scrollbar is reached to the bottom of the page. Is this the kind of behavior that the InfiniteScroll should have for non-multiColumn data?

With scrollHeight, I am able to see that the nextEndPage is calculated as the scrollbar reaches the end of the page. I am fine with either solutions but I do want to hear what the correct behavior should be for the InfiniteScroll.

[IanKBovard]

@nnbrandon good question. I would defer to @ShimiSun or @ericsoderberghp as to what the correct behavior of nextEndPage should be.

[ShimiSun]

Adding @halocline to this thread.

[nnbrandon]

@IanKBovard @halocline, any updates as to the correct behavior for nextEndPage and whether we should keep pursuing scrollHeight?

[halocline]

Apologies for the delay here. I have worked up a couple diagrams to help me explain how I believe nextEndPage is designed and should behave (sometimes I do better thinking in pictures).

How I believe nextEndPage is designed to behave:

How nextEndPage is behaving when a "step" or "page" of results heights differ from the original:
Specifically, if the height of a subsequent page of results is less than the original pageHeight * 1.25, causes a situation where the nextEndPage never gets incremented.

Simple solution to set endPage +1:
This approach always includes an additional page of results we don't really need. While this works and is simple, it seems like it goes against the intent of step and offset and the intended behavior.

---

Those may or may not be helpful, but they did help provide clarity for my thinking.

Where my thoughts have arrived are:

• If possible, I'd like to try to contain the logic for setting nextEndPage and avoid conditional branching which each have differing sets of rules and logic.
• The scrollHeight approach has merit, however I think we should figure out how it would play with the replace scenario. As is, if replace === true the differing page heights could still occur and this solution would not apply.
• As alternative, it would be worthwhile to explore having the pageHeight hook called whenever endPage or beginPage are updated and recalculate pageHeight.

[halocline]

@IanKBovard @halocline, any updates as to the correct behavior for nextEndPage and whether we should keep pursuing scrollHeight?

Hi @nnbrandon, had a draft of a comment in progress when you sent this. Let me know if my thinking makes sense. Essentially, I think we should explore two paths:

1. Taking the scrollHeight approach the next step, but ensuring it works in the replace and multicolumn scenario (because these can still run into the original issue)
2. Should we be recalculating pageHeight each time beginPage and endPage are updated? I think this is an alternative solution we should consider.

[halocline]

@nnbrandon P.S. A couple of other InfiniteScroll fixes were put into place recently, so make sure to grab the latest master.

[IanKBovard]

Thanks @halocline! love the diagrams, incredibly helpful. The best approach, to me, would be to update page height when we change begin/endpage

[IanKBovard]

been doing some more investigating into this issue.

      const nextEndPage = Math.min(
        lastPage,
        Math.max(
          (!replace && endPage) || 0,
          multiColumn
            ? Math.ceil(((top + height + offset) * width) / pageArea)
            : Math.floor((top + height + offset) / pageHeight), // this will round down even at 1.999999
          show ? Math.floor(show / step) : 0,
        ),
      );

Math.floor((top + height + offset) / pageHeight) will always round down no matter how close (top + height + offset) / pageHeight gets. Would it be better to make a check when calculating nextEndPage? If nextEndPage is less than lastPage and if (top + height + offset) / pageHeight % 1.0 is less than to ~0.8 round up, else round down?

This is super messy, but something like this?

      const end = (top + height + offset) / pageHeight;
      const radix = end % 1.0;
      let nextEndPage;
      if (end < lastPage && radix > 0.8) { 
       // if lastpage has not been reached and top + height + offset) / pageHeight % 1.0 is greater than 0.8
      // round up
        nextEndPage = Math.min(
          lastPage,
          Math.max(
            (!replace && endPage) || 0,
            multiColumn
              ? Math.ceil(((top + height + offset) * width) / pageArea)
              : Math.ceil((top + height + offset) / pageHeight),
            show ? Math.floor(show / step) : 0,
          ),
        );
      } else {
      // normal behavior round down
        nextEndPage = Math.min(
          lastPage,
          Math.max(
            (!replace && endPage) || 0,
            multiColumn
              ? Math.ceil(((top + height + offset) * width) / pageArea)
              : Math.floor((top + height + offset) / pageHeight),
            show ? Math.floor(show / step) : 0,
          ),
        );
      }

[nnbrandon]

been doing some more investigating into this issue.

      const nextEndPage = Math.min(
        lastPage,
        Math.max(
          (!replace && endPage) || 0,
          multiColumn
            ? Math.ceil(((top + height + offset) * width) / pageArea)
            : Math.floor((top + height + offset) / pageHeight), // this will round down even at 1.999999
          show ? Math.floor(show / step) : 0,
        ),
      );

Math.floor((top + height + offset) / pageHeight) will always round down no matter how close (top + height + offset) / pageHeight gets. Would it be better to make a check when calculating nextEndPage? If nextEndPage is less than lastPage and if (top + height + offset) / pageHeight % 1.0 is less than to ~0.8 round up, else round down?

This is super messy, but something like this?

      const end = (top + height + offset) / pageHeight;
      const radix = end % 1.0;
      let nextEndPage;
      if (end < lastPage && radix > 0.8) { 
       // if lastpage has not been reached and top + height + offset) / pageHeight % 1.0 is greater than 0.8
      // round up
        nextEndPage = Math.min(
          lastPage,
          Math.max(
            (!replace && endPage) || 0,
            multiColumn
              ? Math.ceil(((top + height + offset) * width) / pageArea)
              : Math.ceil((top + height + offset) / pageHeight),
            show ? Math.floor(show / step) : 0,
          ),
        );
      } else {
      // normal behavior round down
        nextEndPage = Math.min(
          lastPage,
          Math.max(
            (!replace && endPage) || 0,
            multiColumn
              ? Math.ceil(((top + height + offset) * width) / pageArea)
              : Math.floor((top + height + offset) / pageHeight),
            show ? Math.floor(show / step) : 0,
          ),
        );
      }

@halocline @IanKBovard Thank you so much for the great explanation on the nextEndPage behavior and the wonderfully drawn diagrams. I also agree with the proposed solution to recalculate pageHeight when endPage/beginPage is updated.

I finally had the chance to play with InfiniteScroll some more last night and will be pushing up a PR in a bit. Just as what @IanKBovard mentioned, I also found that (top + height + offset) / pageHeight rounded down when it became very close to the next calculated page such as (8.992) when it should be 9.

Before Ian's proposed solution, I was running into an issue where it would stop at a certain page because (top + height + offset) / pageHeight rounded down when it became close. I've tested it on a code sandbox and it is no longer rounding down anymore because of the 0.8 buffer which is helping a lot.

Here's a code sandbox with the changes: https://codesandbox.io/s/inspiring-bohr-x05xn?file=/src/InfiniteScroll.js

[IanKBovard]

@nnbrandon thanks for making those changes. One main nitpick, I would call end scrollPosition or something like that. My apologies, my example was thrown together quickly, and the variable names are not the most descriptive.

I would love some more input on this solution. It seems right, to me, to include some sort of condition to round up instead of down for nextEndPage.

[halocline]

Progress is looking promising. I've created the following temporary Storybook stories to serve as manual tests. The stories recreate the scenario in @nnbrandon's original issue by creating page heights of different sizes.

Can we use these as part of the review for this PR?

Using these, you will see a bit of "jumpiness" when the pageHeight is recalculated and goes from larger to smaller heights.

import React, { useState } from 'react';
import { storiesOf } from '@storybook/react';
import isChromatic from 'chromatic/isChromatic';
import { grommet } from 'grommet/themes';

import { Grommet, Box, InfiniteScroll, Text } from 'grommet';

// import { allItems } from './Basics';
const allItems = Array(240)
  .fill()
  .map((_, i) => i + 1);

const ItemHeights = props => {
  const [items, setItems] = useState(allItems.slice(0, 50));

  const onMore = () => {
    setTimeout(() => {
      setItems(allItems.slice(0, items.length + 50));
    }, 1000);
  };

  return (
    <Grommet theme={grommet}>
      <Box>
        <InfiniteScroll items={items} onMore={onMore} {...props}>
          {item => (
            <Box
              key={item}
              height={item <= 50 ? 'xsmall' : 'xxsmall'}
              pad="medium"
              border={{ side: 'bottom' }}
              align="center"
            >
              <Text>item {item}</Text>
            </Box>
          )}
        </InfiniteScroll>
      </Box>
    </Grommet>
  );
};

if (!isChromatic()) {
  storiesOf('InfiniteScroll', module)
    .add('TEMP', () => <ItemHeights />)
    .add('TEMP2', () => <ItemHeights replace />);
}

[IanKBovard]

Please refer to the new PR #4525 that makes the changes we've all discussed

[nnbrandon]

@IanKBovard @halocline I will go ahead and close this PR since there is work going on in #452. Thank you!

[ShimiSun]

Thank you for driving this contribution @nnbrandon 💟