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

[LFDanLu]

#8977 was fixed by #9784, but this has some extra fixes to the scroll into view calculations. Specifically this fixes the case where the scrolling body had a border applied to it which caused scroll into view to not fully scroll a item into view. Also fixes some erroneous getScrollParent cases where it wasn't including the root, see the tests

✅ 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 blocks 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

[LFDanLu]

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

[nwidynski]

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

[LFDanLu]

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

[LFDanLu]

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

[nwidynski]

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?

[LFDanLu]

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

[LFDanLu]

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?

[nwidynski]

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.

[LFDanLu]

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

[nwidynski]

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.

[LFDanLu]

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

[nwidynski]

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]

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]

Build successful! 🎉

• View the storybook
• View the S2 storybook
• View the S2 docs
• View the React Aria docs

[rspbot]

Build successful! 🎉

• View the storybook
• View the S2 storybook
• View the S2 docs
• View the React Aria docs

[rspbot]

Build successful! 🎉

• View the storybook
• View the S2 storybook
• View the S2 docs
• View the React Aria docs

[nwidynski]

@LFDanLu LGTM! One more thing I noticed is that we probably want to switch to parseFloat, since most values we parse have subpixel precision.

[rspbot]

Build successful! 🎉

• View the storybook
• View the S2 storybook
• View the S2 docs
• View the React Aria docs

[rspbot]

Build successful! 🎉

• View the storybook
• View the S2 storybook
• View the S2 docs
• View the React Aria docs

[rspbot]

Build successful! 🎉

• View the storybook
• View the S2 storybook
• View the S2 docs
• View the React Aria docs

[nwidynski]

@LFDanLu I found a couple more issues in certain edge cases (no regressions, they were largely also broken before the large changeset). Do you want me to list them here, or should I take care of those? I'm working on a PR for reverse flex support in (virtualized) lists and would just include it there.

[LFDanLu]

@nwidynski thanks for letting us know! If you already have fixes for them, feel free to include them in your larger PR or split it out separately. IMO since they aren't regressions, less pressure to get them into the upcoming release. Thanks!

[nwidynski]

@LFDanLu Gotcha, will split it into separate prep-work before the main PR then. If you got a minute btw, could you look at getScrollParent(s) and let me know whether it was intended to include the element itself in the resulting list of parents? Seems like a bug but weird that both methods appear to do the same 😅

[LFDanLu]

@nwidynski with the changes in this PR, getScrollParents shouldn't include the node passed into it in the parent list. getScrollParent also shouldn't already though, regardless of whether or not the node provided is scrollable it will hit a case where scrollableNode gets updated to the node's parent element. Are you seeing otherwise locally?

[nwidynski]

@LFDanLu I should have clarified Im comparing to main branch instead of this one but frankly I cant even tell how the changes on this branch are supposed to prevent the node itself from being included in the results. Im on mobile atm, so visibility is a bit challenging 😅

From what I can tell, the first iteration on the while loop of getScrollParents will include the node itself if it is scrollable. And in getScrollParent that scenario is possible if the node itself is not and the immediate parent is scrollable, in which case it will never enter the while loop and thereby return itself.

PS: I would expect this behavior when the root element is passed, but not for any arbitrary scroll container.

[LFDanLu]

@nwidynski Ah my mistake with getScrollParents, I had misremembered the original change and mixed it up with some prior discussions from a while ago. I personally think getScrollParents should only return the parent elements and not the node itself unless it is the root as you mentioned, but that change would be problematic for people who may have been using this internal util before and relying on that behavior. I could be persuaded either way though tbh.

As for getScrollParent though, in the case you mentioned wouldn't we hit

react-spectrum/packages/react-aria/src/utils/getScrollParent.ts

Lines 21 to 23 in 12bd517

	while (scrollableNode && !isScrollable(scrollableNode, checkForOverflow)) {
	scrollableNode = scrollableNode.parentElement;
	}

and enter the while loop since the node isn't scrollable as you mentioned, thus setting scrollableNode = scrollableNode.parentElement; and returning that?

[nwidynski]

@LFDanLu Huh, I guess it was already past bedtime, haha. You are right of course, thanks for pointing it out. I will align them on parents only, since breaking changes should come expected with use of internals.

If there is any remaining concern as to why to keep, we can raise it in the coming PR

[rspbot]

Build successful! 🎉

• View the storybook
• View the S2 storybook
• View the S2 docs
• View the React Aria docs