Skip to content

fix(ScrollView): don't report empty visible rect when the content suspends #8215

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(ScrollView): don't report empty visible rect when the content suspends #8215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
89c6870
fix(ScrollView): don't report empty visible rect when the content sus…
alirezamirian May 10, 2025
f9cfb09
Merge branch 'main' into 8214-virtualizer-suspense-issue
LFDanLu May 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions packages/@react-aria/virtualizer/src/ScrollView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ export function useScrollView(props: ScrollViewProps, ref: RefObject<HTMLElement
// adjusted space. In very specific cases this might result in the scrollbars disappearing
// again, resulting in extra padding. We stop after a maximum of two layout passes to avoid
// an infinite loop. This matches how browsers behavior with native CSS grid layout.
if (!isTestEnv && clientWidth !== dom.clientWidth || clientHeight !== dom.clientHeight) {
Copy link
Contributor Author

@alirezamirian alirezamirian May 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this change, clientWidth !== dom.clientWidth || clientHeight !== dom.clientHeight missed parenthesis for the if condition to make more sense.

if (!isTestEnv && (dom.checkVisibility?.() ?? true) && (clientWidth !== dom.clientWidth || clientHeight !== dom.clientHeight)) {
Copy link
Contributor Author

@alirezamirian alirezamirian May 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checkVisibility is well supported across browsers. Not sure if true is a better fallback or false.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to use isElementVisible() as the fallback. I'm wondering whether it may even make sense to refactor it, now that checkVisibility() is broadly available.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isElementVisible is currently a non-exported function in @react-aria/focus, so not sure.
Another fallback could be dom.clientWidth > 0 || dom.clientHeight > 0. Maybe even that alone and without checkVisibility is good enough, but checking on visibility is more correct, considering the fact that react hides the suspended UI.

Copy link
Contributor

@nwidynski nwidynski May 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to move isElementVisible into @react-aria/utils and export it (see #8146 (comment)) at this point, but let's wait what the core maintainers have to say. Strictly speaking, it would be enough to check display: hidden and whether the elements bounding rect is inside the viewport, since this is what React is using to render suspended content.

state.width = dom.clientWidth;
state.height = dom.clientHeight;
flush(() => {
Expand All @@ -198,7 +198,7 @@ export function useScrollView(props: ScrollViewProps, ref: RefObject<HTMLElement

// Update visible rect when the content size changes, in case scrollbars need to appear or disappear.
let lastContentSize = useRef<Size | null>(null);
let [update, setUpdate] = useState({});
let [update, setUpdate] = useState<{} | false>(false);
useLayoutEffect(() => {
if (!isUpdatingSize.current && (lastContentSize.current == null || !contentSize.equals(lastContentSize.current))) {
// React doesn't allow flushSync inside effects, so queue a microtask.
Expand All @@ -224,7 +224,9 @@ export function useScrollView(props: ScrollViewProps, ref: RefObject<HTMLElement

// Will only run in tests, needs to be in separate effect so it is properly run in the next render in strict mode.
useLayoutEffect(() => {
updateSize(fn => fn());
if (update) {
updateSize(fn => fn());
}
}, [update]);

let onResize = useCallback(() => {
Expand Down
34 changes: 34 additions & 0 deletions packages/@react-spectrum/list/chromatic/ListView.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,3 +168,37 @@ export const Empty = {
render: TemplateEmptyState,
name: 'empty state'
};

export const SuspendedItem = {
render: (args) => {
return (
<React.Suspense fallback="loading...">
<div style={{width: 200}}>
<ListView {...args}>
<Item>
<Text>
<WithSuspense>Item 1</WithSuspense>
</Text>
</Item>
<Item>Item 2</Item>
</ListView>
</div>
</React.Suspense>
);
},
name: 'suspended item'
};
let data: string;
const promise = new Promise((resolve) => setTimeout(resolve, 500)).then(() => {
data = 'data';
});
const useData = () => {
if (!data) {
throw promise;
}
return data;
};
function WithSuspense({children}: { children: React.ReactNode }) {
useData();
return <>{children}</>;
}