fix: properly scroll body if keyboard focusing a item with no other scroll parents by LFDanLu · Pull Request #9780 · adobe/react-spectrum

LFDanLu · 2026-03-11T20:30:13Z

#8977 was fixed by #9784, but this has some extra fixes to the scroll into view calculations

✅ 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

📝 Test Instructions:

Test https://codesandbox.io/p/sandbox/2x4wtp?file=%2Findex.html%3A14%2C16 which should have the same changes that were added to this PR. Clicking the various buttons at the top should scroll the various blocking into view

Go to "Table with React Transition" story in the RAC storybook and make sure the window is small enough that the body is scrollable. Keyboard navigate downwards through the items and make sure they get scrolled into view properly. This was fixed by a separate PR as a side effect but double check it still works

🧢 Your Project:

RSP

…croll parents

LFDanLu · 2026-03-11T20:32:15Z

packages/react-aria/src/utils/getScrollParents.ts

@@ -16,12 +16,15 @@ export function getScrollParents(node: Element, checkForOverflow?: boolean): Ele
  let parentElements: Element[] = [];
  let root = document.scrollingElement || document.documentElement;

-  do {


old logic wasn't including the root element except if the initial node provided was the root. isScrollable should properly check if scrolling is being prevented on the root so we don't actually need the node ! == root check I believe

Meh, our test suite was exactly calling with root as the container. Sorry about that.

all good haha, I didn't catch this either on my original review. So many different scenarios to test too

LFDanLu · 2026-03-11T20:34:22Z

packages/react-aria/src/utils/scrollIntoView.ts

@@ -73,7 +73,7 @@ export function scrollIntoView(scrollView: HTMLElement, element: HTMLElement, op
  let scrollBarOffsetX = scrollView === root ? 0 : borderLeftWidth + borderRightWidth;
  let scrollBarOffsetY = scrollView === root ? 0 : borderTopWidth + borderBottomWidth;
  let scrollBarWidth = scrollView.offsetWidth - scrollView.clientWidth - scrollBarOffsetX;
-  let scrollBarHeight = scrollView.offsetHeight - scrollView.clientHeight - scrollBarOffsetY;
+  let scrollBarHeight = scrollView === root ? 0 : scrollView.offsetHeight - scrollView.clientHeight - scrollBarOffsetY;


we already use scrollView.clientHeight when calculating viewBottom as used below. That omits the scroll bar already if scrollView is the root so we can set to 0 here

I think we would want to do that for scrollBarWidth too, right? In case the root is horizontally scrollable.

Also I think we need to adjust the scrollPort for the root element too actually. Since the border may not be in view, it might not make sense to always add/subtract it, right?

hm, good points I'll see about trying that soon

actually for the scrollPort do we need to accommodate for that? The scrollPort values should always be the coordinates of the scroll region we are currently positioning the target with respect to so it shouldn't care about the actual viewport and thus should always take the border and scrollPadding into view right? The border of the scroll region/port being not in view should get handled by scrolling all parents up to the root in scrollIntoViewport?

For instance take a table with a border that scrolled partially out of view such that its border isn't in view: Scrolling a table row into view would be to first scroll the table itself into view, then the item into view within the table right so we wouldn't need to accommodate for the border being in view or not?

Hm, I think for normal containers that's right, but not for the root element. I will have to test this more in depth, but here is my thinking:

The Scrollport is defined as the unclipped rect of a scroll container, adjusted by scroll-padding. For the root element, this is always the viewport, hence why we set the view to (0, 0, clientWidth, clientHeight). Here, clientWidth and clientHeight correspond to viewport - scrollbar, while offsetHeight continues to be the "full" rect, including borders.

The client values on the root are unaffected when a border is added/removed on the root. Hence why I think we need to adjust when the border is in view vs not. On normal elements, a border wouldn't affect the inner scroll offset, but on the root it does.

Edit: Confirmed via sandbox.

oh I see, in that case I think we can adjust the scrollPort values to include the border if we are currently scrolling w/ respect to the root?

let scrollPortTop = viewTop + (isRoot ? 0 : borderTopWidth) + scrollPaddingTop; let scrollPortBottom = viewBottom - (isRoot ? 0 : borderBottomWidth) - scrollPaddingBottom - scrollBarHeight; let scrollPortLeft = viewLeft + (isRoot ? 0 : borderLeftWidth) + scrollPaddingLeft; let scrollPortRight = viewRight - (isRoot ? 0 : borderRightWidth) - scrollPaddingRight;

That way we remain in the same coordinate system w/ regards to the scrollPort values and the scrollArea values.

https://codesandbox.io/p/sandbox/2x4wtp?file=%2Findex.html%3A14%2C16 seems to work well now with those changes

Yeah, that's what my first thought was too, but I don't think that works fully - scrolling both to red start and red end is broken. I haven't fully grasped why, because I haven't spent time on this today, but I think its because the border falls within the target area, which is what I meant by it being visible or not.

I might be overlooking some thing but broken in what way? Seems to scroll to the top/bottom appropriately regardless of where I'm scrolled:

Screen.Recording.2026-03-16.at.10.21.42.AM.mov

If it is the horizontal scrolling not moving to start all the time, I think that is because triggerScroll is only applying its options to scrollIntoView's block and not inline

Nvm, it's because I forgot we implemented IfNeeded behavior and my screen is large enough that green was always visible, hence why it would only scroll horizontally. Btw curiously enough, native scrollIntoView seems to fail this test in all browsers (try putting focus on the button while its out of view, without pressing). Now I'm wondering whether we even want this 🤣

I think I prefer the way this behaves with our current changes though - more consistent and we are not aiming for strict parity anyways.

LFDanLu · 2026-03-11T20:35:11Z

packages/react-aria/src/utils/scrollIntoView.ts

+        for (let scrollParent of getScrollParents(targetElement, true)) {
+          scrollIntoView(scrollParent as HTMLElement, targetElement as HTMLElement);
+        }


mirrors the flow above, we need to first scroll the containing element into view if necessary, then re-scroll the target element into view to compensate. There is some logic in useSelectableCollection that revealed that this was a problem because we do two separate scroll in views (one via scrollIntoView and one via scrollIntoViewport ) and the second of which scrolls the containing element into view first

rspbot · 2026-03-11T20:35:15Z

Build successful! 🎉

rspbot · 2026-03-13T23:56:11Z

Build successful! 🎉

…ession

rspbot · 2026-04-07T00:12:44Z

Build successful! 🎉

fix: properly scroll body if keyboard focusing a item with no other s…

82a863e

…croll parents

github-actions bot added the RAC label Mar 11, 2026

LFDanLu commented Mar 11, 2026

View reviewed changes

LFDanLu mentioned this pull request Mar 11, 2026

Collections are not scrolling if scroll on body in the Chrome browser #8977

Closed

nwidynski mentioned this pull request Mar 12, 2026

feat: Automatically bound virtualizer visible rectangle to window viewport #9784

Merged

LFDanLu added 2 commits March 13, 2026 16:07

fix scrollbar width case and add basic tests

b4130a1

tentative fix to accomodate for borders on root when scrolling into view

200a2c5

LFDanLu added 2 commits April 6, 2026 17:01

Merge branch 'main' of github.com:adobe/react-spectrum into 8977_regr…

d5cff0c

…ession

fix import

3372f70

github-actions bot removed the RAC label Apr 7, 2026

LFDanLu changed the title ~~fix: (WIP) properly scroll body if keyboard focusing a item with no other scroll parents~~ fix: properly scroll body if keyboard focusing a item with no other scroll parents Apr 7, 2026

LFDanLu marked this pull request as ready for review April 7, 2026 00:06

LFDanLu added ready for review RAC labels Apr 7, 2026

Conversation

LFDanLu commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nwidynski Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nwidynski Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LFDanLu Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rspbot commented Mar 11, 2026

Uh oh!

rspbot commented Mar 13, 2026

Uh oh!

rspbot commented Apr 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LFDanLu commented Mar 11, 2026 •

edited

Loading

nwidynski Mar 12, 2026 •

edited

Loading

nwidynski Mar 16, 2026 •

edited

Loading

LFDanLu Mar 11, 2026 •

edited

Loading