Scroll into view with sticky elements and page scrolling can cause bad-large jumps

[lukpsaxo]

Provide a general summary of the issue here

We have a table with the scrolling element above the table (now supported by your virtualizer, thanks!) and some sticky elements.

changing the focussed row down and up works, but when you press up going from the top table row to the table header, the page jumps and focus is now out of view.

🤔 Expected Behavior?

no significant jump when moving through rows

😯 Current Behavior

a big jump moving from the top table row to the table header.

💁 Possible Solution

Take this with a pinch of salt - this is what AI generated as a fix - it appears to work though:

+++ b/node_modules/react-aria/dist/private/utils/scrollIntoView.cjs
@@ -123,10 +123,17 @@ function $9a1324d6ffd8bbb0$export$c826860796309d1b(targetElement, opts = {}) {
             if (Math.abs(originalLeft - newLeft) > 1 || Math.abs(originalTop - newTop) > 1) {
                 scrollParents = containingElement ? (0, $e2a63525475bbd9f$exports.getScrollParents)(containingElement, true) : [];
                 // scroll containing element into view first, then rescroll target element into view like the non chrome flow above
-                for (let scrollParent of scrollParents)$9a1324d6ffd8bbb0$export$53a0910f038337bd(scrollParent, containingElement, {
-                    block: 'center',
-                    inline: 'center'
-                });
+                for (let scrollParent of scrollParents){
+                    // Saxo patch (react-spectrum #9780 edge case): only re-center the containing
+                    // element when it fits within the scroll parent's viewport. Centering a
+                    // containing element larger than the viewport (e.g. a full-height virtualized
+                    // grid using window/parent scrolling) scrolls to its middle and moves a
+                    // sticky focus target unexpectedly, causing the page to jump.
+                    if (containingElement && containingElement.offsetHeight <= scrollParent.clientHeight && containingElement.offsetWidth <= scrollParent.clientWidth) $9a1324d6ffd8bbb0$export$53a0910f038337bd(scrollParent, containingElement, {
+                        block: 'center',
+                        inline: 'center'
+                    });
+                }
                 for (let scrollParent of (0, $e2a63525475bbd9f$exports.getScrollParents)(targetElement, true))$9a1324d6ffd8bbb0$export$53a0910f038337bd(scrollParent, targetElement);
             }
         }

🔦 Context

We have this situation in our app with a table and some sticky elements, so keyboard support for the table appears broken and inaccessible.

🖥️ Steps to Reproduce

Go here:
https://codesandbox.io/p/sandbox/gwl9xx

1. Click setup repro
2. press up on the keyboard

or manually:

1. focus the table
2. press down until the header is out of view
3. press up until the top row is in view
4. press up again to go to the table header <-- Unexpected behaviour

Version

1.18.0

What browsers are you seeing the problem on?

Chrome

If other, please specify.

No response

What operating system are you using?

windows

🧢 Your Company/Team

Saxo Bank

🕷 Tracking Issue

No response

[lukpsaxo]

alternative solution we think would be to go to nearest rather than center..

[lukpsaxo]

related and possibly fixed by (maybe?!) - https://github.com/adobe/react-spectrum/pull/9780/changes

[LFDanLu]

reproduced this locally, it isn't fixed by #9780 unfortunately (which was released as a part of 1.18). Ideally instead of doing the scrollIntoView center we'd have similar behavior to scrollIntoViewIfNeeded. I don't think skipping the target element's scroll container's scroll into view is quite right since the container itself still could be out of view (e.g. an scrollable overlay with a really large table that is out of view or something), but nearest doesn't quite to be quite right when I test locally (it is definitely an improvement over the center case here but causes large jumps when navigating to a row that is out of view)

[nwidynski]

@LFDanLu Will be addressed by the coming refactor, once I find the time to wrap the work up. It will always use our shim, which implements scrollIntoViewIfNeeded - similarly btw, I noticed that the subpixel hack makes no sense at all for the shim, since again, it implements if needed behavior already 🤣

[LFDanLu]

nice, thanks for looking into it!