Open cph0 opened 1 year ago
In your example each item places at two rows. And two different items placed at one row at the same time. It should not work in latest stable release too. And It's hard to support this case in future.
About maximim update depth... I was afraid of this issue when I move logic to pure react way with useMemo and resubscribe in useEffect.
@cph0 I revert new observing logic in 7.0.0-beta.2. Should work same as 6.3.0. let me know if you still have your issue in 7.0.0-beta.2. At least "maximum update depth warning" should be left.
I'm also thinking about how to avoid index change loop for some rare cases such a css transform (it affects getBoundingClientRect results but not affects a layout). One of the possible solution is not change indexes while the difference between old rendered index and new rendered index will be lower than some threshold value - new items will be appear, but already rendered items outside viewport will not be hidden. This behaviour can also improve performance theoretically since we will keep old nodes in DOM a little bit longer and delete them by chunks. Another way is some smart "loop" detection.
I have tried version '7.0.0-beta.2', the warning has gone but items are still being added/removed endlessly. It was already a problem in version '6.3.0'.
Some sort of loop detection would be the ideal solution I think. An index change threshold could work, I'd have to set it to a high enough value such that any data wouldn't cause the columns then row heights to change enough to trigger the loop. Although continuously scrolling really slowly could cause all items to be rendered and so freeze the page.
Currently I've got a tweak in the source code to not update the indexes if the new index is equal to the 2nd iteration ago. However I must have done something wrong as occasionally some list items are not rendered at all.
const indexChanges = useRef<number[]>([]);
...
if (indexChanges.current[indexChanges.current.length - 2] === nextStartIndex)
return;
...
if (indexChanges.current.length >= 2)
indexChanges.current.splice(0, indexChanges.current.length - 2);
indexChanges.current.push(nextStartIndex);
setIndexes([nextStartIndex, nextEndIndex]);
@cph0
I don't understand how you "change" the size of the cell to trigger loop.
CodeSendBox above trigger loop because every time when I hide item at the top of the list layout shifts and I need to render one more item again.
So you should guarantee that list can replace any item by spacer and other elements will be at the same place after some item was replaced. Other elements should not change their heights at this moment. If some of the item change the height, list will recalculate indexes to fill all spaces not covered by items. But, if some item change height again, and new spaces from top or bottom appear, list should recalculate again and again.
So I want to see exactly your case because loop can be a result of "unsupported for virtualization" code.
Loops avoiding is a hack and those cases with loops will not work fully correctly even with this hack.
I have created another sandbox with a realistic example. The grid is valid, no items span multiple rows.
https://codesandbox.io/s/cranky-bas-5e4e8f?file=/src/App.js:4130-4381
I haven't managed to get the data such that scrolling with the mouse wheel will cause the problem (I have seen it with data at my job). However if you drag the scroll bar quickly with the mouse (from the top to the middle) the items will disappear and the scroll bar will flicker up and down continuously.
@cph0, oh, first, thank you for reproduction - it's a really huge help, nice work! This case works fully incorrect. It can't compensate scroll/compute item size incorrectly/can't scroll if you scrolled down to list bottom. The most funny thing is this case is easiest for virtualization since all cell sizes are equal. I'm working on it
First thoughts -
This is how one item is rendered. Item margin and item height cumpute incorrect. align-content: 'start' (or 'flex-start') should be used to render one item properly. Try it.
it's not a fix for issue but it's a proper way to render this type of grid.
@cph0 I rewrote scroll compensation logic. Now I remember a distance between viewport top (top limit of viewport) and top of the first item (delta). After render I checked the difference between delta before render and delta after render and set viewport scrollTop by deltas diff. This is a regular/usual logic for infinite lists and other stuff where we need to simulate overflow-anchor logic.
The new logic comes with 7.0.0-beta.3 which I released now. Try it. I'm waiting for your awesome feedback!
I have updated the sandbox with '7.0.0-beta.3'. Scrolling with the mouse wheel from the bottom now works and doesn't get stuck. But dragging the scrollbar with the mouse quickly (actually always did it slowly) from top to middle or bottom to middle still makes the items disappear and the scrollbar to flicker continuously.
It's much better without 'overflow-anchor: none;' but I had other problems with 'auto'. The list would sometimes scroll by itself until it reached the top or bottom. You can still cause the items to disappear (and the scrollbar to flicker) but it really takes some fast dragging to get it to happen.
@cph0 Looks like this is a result of wrong estimation for fast scrolling. Did you try scrollThreshold prop? This prop can help with fast scroll optimisation. If scroll difference more than this prop value index change is skipped. It's not the best ux because user see "no items" before fast scroll end, but it's a compromise to provide good fast scroll ux. (1000, 2000 is a good start points for scrollThreshold prop)
Also in your case (when you know items size or margin) you can provide itemSize and itemMargin props directly. It's always better if you know this item dimensions.
I have updated the sandbox with the scrollThreshold prop set to 2000. Dragging the scrollbar slowly still causes the items to disappear and the scrollbar to flicker continuously.
Yes! I can reproduce it. For me only SUPER fast scrolling with wheel cause a loop (I have a mx anywhere mause with infinite wheel)
@cph0 I released experimentral (and maybe unstable) changes to prevent loop and improve fast scrolling in 7.0.0-beta.4
Before, I perform scroll compensation only for "small scroll" - if [start, end] and [nextStart, nextEnd] arrays are intersected). Now I run scroll compensation any time when start index was changed.
I have updated the sandbox with '7.0.0-beta.4'. Dragging the scrollbar slowly (with the mouse cursor) still causes the items to disappear and the scrollbar to flicker continuously.
Which browser are you using? In Chrome it happens with slow scrolling.
I have updated the sandbox with '7.0.0-beta.4'. Dragging the scrollbar slowly (with the mouse cursor) still causes the items to disappear and the scrollbar to flicker continuously.
Which browser are you using? In Chrome it happens with slow scrolling.
I'm using a Chrome, not in a sandbox. I reproduce this bug but only with fast wheel scrolling (and it was fixed for me), I can't cause an issue with dragging scrollbar.
I'm facing the same problem, any solution to solve it or any progress to fix this?
I'm facing the same problem, any solution to solve it or any progress to fix this?
@zerosoul Did you try 7.0.0-beta.4?
Facing the same problem. Any updates ?
Facing the same problem. Any updates ?
Hi, @yaswanthmaddula-sureify , no any updates. I added a small fix and now I can't reproduce it with sandbox above. Need more information/reproductions (os, browser, mouse, trackpad or touch device, maybe screen zoom)
I've done a workaround with the table. When scrolling we freeze the columns so they don't change width. This has solved the loop in this case.
Hi, So I was able to reproduce this issue. Here is the link to the codesandbox - https://codesandbox.io/s/react-viewport-list-forked-5f5mhx?file=/src/Item.js.
In this sandbox the last item is flickering instead of scrollbar.
I was using react fragments inside the Item
component and seems like that caused this issue. But even after adding a div
, I observed in my actual code that the scrollbar continuously scrolls to one of the end once I start scrolling.
@oleggrishechkin
I was using react fragments inside the Item...
You should guarantee that items has a <div>
or any other root element, you can't use fragments, arrays - item must be only one DOM element, because VIewportList traverse by DOM siblings internally.
And when I add div instead of fragment your sandbox works correctly for me.
Loop can cause when next item render incorrectly - has a broken getBoundingClientRect (display: contents for example), has a fragment outside, render at the same row as another item, change a height of siblings (if you use flex and items flex-shrink is 1), parent uses stretch for justify or align... And so on. Sometimes inconsistent margin can cause loop too.
Thank you @oleggrishechkin
Here is a quick hack if you are not aware of the number of children your component returns.
import React, { Children } from "react";
import { ViewportList } from "react-viewport-list";
import ListItem from "./Somewhere";
const ListContainer = () => {
// Array of list item objects
const listItems = [];
return (
<ViewportList
items={Children.toArray(
listItems.map((item) => (
<div>
<ListItem item={item} />
</div>
))
)}
>
{(child) => child}
</ViewportList>
);
};
export default ListContainer;
@yaswanthmaddula-sureify
Example below should works better:
(don't forget a react key for each item)
import React, { Children } from "react";
import { ViewportList } from "react-viewport-list";
import ListItem from "./Somewhere";
const ListContainer = () => {
// Array of list item objects
const listItems = [];
return (
<ViewportList
items={listItems}
>
{(item) => (
<div key={item.id}>
<ListItem item={item} />
</div>
)}
</ViewportList>
);
};
export default ListContainer;
Technically seems correct. Will check this. Thanks @oleggrishechkin
Hello, still facing this issue on 7.1.1
. Not really able to reproduce it on CodeSandbox but it happens when the scroll position makes the last row half visible and it then enters a loop of mounting/unmounting the next element.
It ends up making the whole page sluggish and somewhat render blocking.
Hello, still facing this issue on
7.1.1
. Not really able to reproduce it on CodeSandbox but it happens when the scroll position makes the last row half visible and it then enters a loop of mounting/unmounting the next element.It ends up making the whole page sluggish and somewhat render blocking.
Thanks for the video. One of the reason why loop issue still exists is no stable reproduction. If you can create a reproduction demo (code, repo) it will be really helpful. Sometimes loop issue happened because wrong margin and flex styles was applied.
Sometimes the list will get stuck in a scroll loop. Where it renders another item but this changes the dimensions which means that it then doesn't render the item and so it keeps doing this. Also throws a maximum update depth exceeded error.
I have created a code sandbox which demonstrates this. It's not a realistic example but I was struggling to get the data that makes the columns change size in such a way that it loops.
https://codesandbox.io/s/blue-cloud-lpzrw7?file=/src/App.js