Skip to content

feat: rework scroll utilities and fix layout bugs - #10556

Open
nwidynski wants to merge 3 commits into
adobe:mainfrom
nwidynski:fix-scrollable
Open

nwidynski wants to merge 3 commits into
adobe:mainfrom
nwidynski:fix-scrollable

Conversation

@nwidynski

@nwidynski nwidynski commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

@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

Copy link
Copy Markdown
Member

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

nwidynski commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants