[nwidynski]

Closes no issues directly, because this PR is intentionally limited to additions only. Migrating call sites to the new utilities and signatures is to be done in chore follow-ups, because it would otherwise be rather hard to review what changed here.

From a high level, this is the first round of changes for a set of layout- and drastically improved scroll utilities. Here is what this PR does today:

1.) Fix issues in getScrollParent and getScrollParents by merging their implementation into a new generator
2.) Fix various bugs in isScrollable and expand its scope for window scrolling
3.) Introduce modalities and axis isolation into scroll utilities (pointer-> skip hidden, virtual -> incl. hidden)

The rest is just chores of moving code blocks from calculatePosition and domHelpers.

✅ 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).
• Filled out test instructions.
• Updated documentation (if it already exists for this component).
• Looked at the Accessibility Practices for this feature - Aria Practices
• I understand every change in this PR and can explain why it's there.
• If AI-assisted, I followed our AI contribution guidance and pointed my assistant at CLAUDE.md.

📝 Test Instructions:

🧢 Your Project:

[nwidynski]

@LFDanLu Do you want to give this a DNM build as well? This PR only overlaps with #10558 in its utils but is otherwise self-contained and feature-complete.

[snowystinger]

Here is what this PR does today:

Can we have test instructions for the before and after's for each of these? There are no tests or stories or instructions for existing stories, so it's a bit hard to tell what all these changes are for or how they are improvements.

[nwidynski]

@snowystinger Yes, absolutely. I can come back with a test suite, but I haven't really yet gotten a definitive signal about whether or not these PRs are even something you want merged.

I understand the dilemma from a reviewers perspective though. It's hard to tell which bugs this fixes without tests, but its also too much work to add tests for something you might not even want 😅

As a general rule of thumb, what this PR does is bring react-aria reasonably up to par with https://github.com/scroll-into-view/compute-scroll-into-view in terms of edge case handling. The biggest issues and features are:

For isScrollable:

1. Iframes were not supported.
2. checkForOverflow wasn't axis-aware. This meant a container with scrollHeight > clientHeight and overflow-y: hidden would still return true, if it had overflow-x: scroll.
3. Overflow propagation was completely missing. This meant that a body would come back as scrollable, even though it actually controlled the overflow for the viewport.
4. Nodes above the scrolling element would be incorrectly asserted.
5. clip overflow was not supported.

For getScrollParent and getScrollParents:

1. ShadowDOM was not supported.
2. Positioned ancestors were not supported.
3. Nodes would include themselves as a scrollParent.

Then for the primarily new features:

1. Modalities are introduced for native parity of scrollIntoView and to fix bugs whenever getScrollParent is used to find the nearest potentially-scrollable container, i.e. Table, auto-scroll, etc.
2. getMaxScrollTop/Left are introduced for scrollIntoView and AI components, especially under flex-reverse and rtl rendering scenarios.
3. BoundingNode signature is now supported for parity with the layout PR, because viewport and elements are generally very different when it comes to layout.

In terms of a very high-level roadmap, you have to imagine any PR I'm opening as work towards getting a Navigation Component/Carousel into React Aria. I'm not giving up until that's done, haha.

[nwidynski]

@snowystinger I added a test suite now. This was a ton of work, because no fixtures for any of this existed, so don't be surprised by the size increase (it's nearly 10x the original change size). I've tried making it easy for you guys to get a before/after by implementing the test suite in a way in which it will skip tests for new features when you swap the underlying implementations of isScrollable, getScrollParent & getScrollParents back to their original - just make sure to roll them back all at once. TS will complain but running the suite will work.

PS: AI was used for the repetitive parts of mapping different permutations. The test setup and the native comparison helpers were done manually, and I've also given each test case a review so they actually add distinct value.

[nwidynski]

@snowystinger Is there any chance we can close this out? Let me know if there’s anything I can take off your plate elsewhere to free up time for it.

[snowystinger]

Hey, I've been trying to find ways to split the PR up into more manageable chunks with clear goals, the tests are helpful, so thank you.
Otherwise I just need to set aside a large chunk of time to get my head around it.
If the code reorganisation is not strictly necessary, it may be easier to put it back. Otherwise I was thinking that would be a good thing to pull out into a PR first, even if it doesn't "solve" anything, it'd make it a lot easier to focus on what is actually changing.

[snowystinger]

whoops, left two comments in a partial review i had started

[snowystinger]

? should we remove?

[nwidynski]

Not sure I understand what you're trying to get at. Are you asking why this is a bug? It's because the fallback escapes the isScrollable check. This can, for example, cause a scroll prevented window scroll.

I had given all call sites a quick look before writing this comment, and remember there being better alternatives than using getScrollParent for most of them. In fact, they often have bugs precisely because they misuse it. What they usually want, is to reference back to the "state" component, e.g. useSelectableItem -> useSelectableCollection. The almost universally better way to do that is to hand down the ref via WeakMap or props.

For the remaining cases (iirc only 1 or 2) returning nullable should be fine. It has been a while since this code was written though, so this is stuff to figure out in the mentioned chore follow-ups.

[snowystinger]

yeah, i was just asking if "later" had occurred or if we still needed to do the audit

also, unrelated, but just a heads up that I'll be out the coming week

[nwidynski]

Ah alrighty, then yes, still needs to be done. This is after this PR ships though. You will see this TODO on some tests as well because they temporarily assert wrongly. Otherwise I wish you a great time off!

[snowystinger]

what happened here?

[nwidynski]

IIRC, tests were flaky with it on. I haven't gotten to the bottom of why just yet, this is TBD. It had to do with the time it took wheel events to cross the CDP process boundary.