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

[alirezamirian]

Closes #8214

✅ Pull Request Checklist:

• Included link to corresponding React Spectrum GitHub Issue.
• Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  It requires in-browser testing environment. Let me know if you do have test setup with Cypress, Playwright or other such tools.
• Filled out test instructions.
• Updated documentation (if it already exists for this component).
  N/A
• Looked at the Accessibility Practices for this feature - Aria Practices
  N/A

📝 Test Instructions:

The reproduction codesandbox link provided in the issue can be used to test the fix..

🧢 Your Project:

https://github.com/alirezamirian/jui

[alirezamirian]

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

[alirezamirian]

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

[nwidynski]

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.

[alirezamirian]

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.

[nwidynski]

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.

[snowystinger]

Thanks for the PR, could you include a test or storybook story so we can reproduce and then verify this?

[alirezamirian]

@snowystinger The code change is in a part which is manually excluded in test environment (see !isTestEnv), so a little tricky to cover with test.

I did attempt now to add a story similar to the provided codesandnbox, and I realized ScrollView code was changed the same day (interesting timing 😀 ), and the updated codepath here no longer runs (see the details below). But the reason for the change here in this PR still applies, if this code path is to run in any other scenario.

Previous to e9bd3a3, a call to updateSize was scheduled in a layoutEffect here:

react-spectrum/packages/@react-aria/virtualizer/src/ScrollView.tsx

Line 218 in c1fd8af

queueMicrotask(() => updateSize(flushSync));

The real flushSync from react-dom is passed to updateSize in this invocation of updateSize.

In e9bd3a3, a new layoutEffect is added that also calls updateSize, but with a noop flush function instead of flushSync:

react-spectrum/packages/@react-aria/virtualizer/src/ScrollView.tsx

Lines 225 to 228 in c1fd8af

	// 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());
	}, [update]);

This call updateSize runs first, updating the width and height to the right values without running flushSync. So, when the queued call to updateSize (from the first layoutEffect) runs, it no longer goes into this if statement:

react-spectrum/packages/@react-aria/virtualizer/src/ScrollView.tsx

Line 175 in c1fd8af

if (state.width !== w || state.height !== h) {

And therefore the code that runs flushSync and measures the size on a potentially hidden element happens to not run.

cc @LFDanLu @devongovett

[snowystinger]

I updated the codesandbox to our latest nightly https://codesandbox.io/p/sandbox/upbeat-kate-f6xtgd and observed that the problem is no longer present. While I'm not against the change, I'd like to have more of an idea of:

if this code path is to run in any other scenario

what other scenarios might be. It seems like an important change and we should have some way of knowing if we broke it in the future.

We can use the storybook play feature to run tests on chromatic to get around the testEnv restriction. See https://github.com/adobe/react-spectrum/blob/main/packages/%40react-spectrum/s2/chromatic/Picker.stories.tsx

This isn't a blocker, at the moment, just trying to make reviewing this as easy as possible for the team. Thanks for all the research into the code path.

[LFDanLu]

@alirezamirian Sorry about the delay. With regards to the layoutEffect that you pointed out

react-spectrum/packages/@react-aria/virtualizer/src/ScrollView.tsx

Lines 226 to 228 in 18912c1

	useLayoutEffect(() => {
	updateSize(fn => fn());
	}, [update]);

, that is actually only supposed to run when in a test environment so we should probably initialize

react-spectrum/packages/@react-aria/virtualizer/src/ScrollView.tsx

Line 201 in 18912c1

let [update, setUpdate] = useState({});

to false instead and add a wrapping if statement around the updateSize call so that it only runs if update is not false, my bad. Hopefully that makes it easier to add the reproduction story (and then we can simply add it as a Chromatic story to catch if it ever regresses) without having to jump through too many hoops

[alirezamirian]

@LFDanLu Will try the suggested change, thanks.

[alirezamirian]

@snowystinger

what other scenarios might be

Not sure. but I think the check on visibility is valid regardless of how we get there based on the explanation on #8214. And if there are no other scenarios that code path run, perhaps the code needs some refactoring.

@LFDanLu
Applied your suggestion, and added a story that renders blank without the added condition and renders correctly with it.
But some tests seem to fail with react 19.

before:

after

Also wanted to add a play function like this, but wasn't sure if you run storybook-test on non-s2 chromatic stories:

  play: async ({canvasElement}) => {
    let body = canvasElement.ownerDocument.body;
    const items = await within(body).findAllByRole('row');
    expect(items).toHaveLength(2);
  }