From 01f31d43013ba7f6f54fd8a36990bbafc3c3cc68 Mon Sep 17 00:00:00 2001 From: Jack Pope Date: Wed, 27 Aug 2025 17:46:03 -0400 Subject: [PATCH] Simplify scrollIntoView algorithm for initial version --- .../fragment-refs/ScrollIntoViewCase.js | 14 +- .../src/client/ReactFiberConfigDOM.js | 206 ++---------------- .../__tests__/ReactDOMFragmentRefs-test.js | 121 +++------- .../src/__tests__/utils/IntersectionMocks.js | 22 -- 4 files changed, 50 insertions(+), 313 deletions(-) diff --git a/fixtures/dom/src/components/fixtures/fragment-refs/ScrollIntoViewCase.js b/fixtures/dom/src/components/fixtures/fragment-refs/ScrollIntoViewCase.js index a01c4e7ef0..3b1f21ef68 100644 --- a/fixtures/dom/src/components/fixtures/fragment-refs/ScrollIntoViewCase.js +++ b/fixtures/dom/src/components/fixtures/fragment-refs/ScrollIntoViewCase.js @@ -90,17 +90,9 @@ export default function ScrollIntoViewCase() {

When the Fragment has children:

- The simple path is that all children are in the same scroll - container. If alignToTop=true|undefined, we will select the first - Fragment host child to call scrollIntoView on. Otherwise we'll call - on the last host child. -

-

- In the case of fixed elements and inserted elements or portals - causing fragment siblings to be in different scroll containers, we - split up the host children into groups of scroll containers. If we - hit a fixed element, we'll always attempt to scroll on the first or - last element of the next group, depending on alignToTop value. + In order to handle the case where children are split between + multiple scroll containers, we call scrollIntoView on each child in + reverse order.

When the Fragment does not have children:

diff --git a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js index 878d39839e..74b03d8fe0 100644 --- a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js +++ b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js @@ -3265,14 +3265,17 @@ if (enableFragmentRefsScrollIntoView) { const children: Array = []; traverseFragmentInstance(this._fragmentFiber, collectChildren, children); + const resolvedAlignToTop = alignToTop !== false; + // If there are no children, we can use the parent and siblings to determine a position if (children.length === 0) { const hostSiblings = getFragmentInstanceSiblings(this._fragmentFiber); - const targetFiber = - (alignToTop === false - ? hostSiblings[0] || hostSiblings[1] - : hostSiblings[1] || hostSiblings[0]) || - getFragmentParentHostFiber(this._fragmentFiber); + const targetFiber = resolvedAlignToTop + ? hostSiblings[1] || + hostSiblings[0] || + getFragmentParentHostFiber(this._fragmentFiber) + : hostSiblings[0] || hostSiblings[1]; + if (targetFiber === null) { if (__DEV__) { console.warn( @@ -3287,197 +3290,14 @@ if (enableFragmentRefsScrollIntoView) { return; } - // If there are children, handle them per scroll container - scrollIntoViewByScrollContainer(children, alignToTop !== false); - }; -} - -const CONTAINER_STD = 0; -const CONTAINER_FIXED = 1; -const CONTAINER_SCROLL = 2; -type ScrollableContainerType = 0 | 1 | 2; -function isInstanceScrollable(inst: Instance): ScrollableContainerType { - const style = getComputedStyle(inst); - - if (style.position === 'fixed') { - return CONTAINER_FIXED; - } - - if ( - style.overflow === 'auto' || - style.overflow === 'scroll' || - style.overflowY === 'auto' || - style.overflowY === 'scroll' || - style.overflowX === 'auto' || - style.overflowX === 'scroll' - ) { - return CONTAINER_SCROLL; - } - - return CONTAINER_STD; -} - -function searchDOMUntilCommonAncestor( - instA: Instance, - instB: Instance, - testFn: (instA: Instance) => T, -): T | null { - // Walk up from instA and count depth - let currentNode: ?Instance = instA; - let depthA = 0; - while (currentNode) { - const result = testFn(currentNode); - if (result) { - return result; - } - depthA++; - currentNode = currentNode.parentElement; - } - - // Walk up from instB and count depth - currentNode = instB; - let depthB = 0; - while (currentNode) { - const result = testFn(currentNode); - if (result) { - return result; - } - - depthB++; - currentNode = currentNode.parentElement; - } - - // Reset currentNode to instA and instB - let nodeA: ?Instance = instA; - let nodeB: ?Instance = instB; - - // Align depths - while (depthA > depthB && nodeA) { - nodeA = nodeA.parentElement; - depthA--; - } - while (depthB > depthA && nodeB) { - nodeB = nodeB.parentElement; - depthB--; - } - - // Walk up both nodes to find common ancestor - while (nodeA && nodeB) { - if (nodeA === nodeB) { - return testFn(nodeA); - } - nodeA = nodeA.parentElement; - nodeB = nodeB.parentElement; - } - - return null; -} - -function maybeScrollContainerIntoView( - currentInstance: Instance, - prevInstance: Instance | null, - alignToTop: boolean, - prevContainerIsFixed: boolean, -): boolean { - if (prevInstance === null || prevContainerIsFixed) { - currentInstance.scrollIntoView(alignToTop); - return true; - } - - const currentRect = currentInstance.getBoundingClientRect(); - const prevRect = prevInstance.getBoundingClientRect(); - - // Check if scrolling to current element would push previous element out of viewport - // alignToTop=true: current goes to top, check if prev would still be visible below - // alignToTop=false: current goes to bottom, check if prev would still be visible above - const canScrollVertical = alignToTop - ? currentRect.top + window.innerHeight > prevRect.top - : currentRect.bottom - window.innerHeight < prevRect.bottom; - const canScrollHorizontal = alignToTop - ? currentRect.left + window.innerWidth > prevRect.left - : currentRect.right - window.innerWidth < prevRect.right; - - if (canScrollVertical && canScrollHorizontal) { - currentInstance.scrollIntoView(alignToTop); - return true; - } - - return false; -} - -function scrollIntoViewByScrollContainer( - children: Array, - alignToTop: boolean, -): void { - // Loop through the children, order dependent on alignToTop - // Each time we reach a new scroll container, we look back at the last one - // and scroll the first or last child in that container, depending on alignToTop - // alignToTop=true means iterate in reverse, scrolling the first child of each container - // alignToTop=false means iterate in normal order, scrolling the last child of each container - let prevScrolledInstance = null; - let prevContainerIsFixed = false; - let i = alignToTop ? children.length - 1 : 0; - // We extend the loop one iteration beyond the actual children to handle the last group - while (i !== (alignToTop ? -2 : children.length + 1)) { - const isLastGroup = i < 0 || i >= children.length; - let isNewScrollContainer: null | ScrollableContainerType = null; - - if (isLastGroup) { - // We're past the end, treat as new scroll container to complete the last group - isNewScrollContainer = CONTAINER_SCROLL; - } else { + let i = resolvedAlignToTop ? children.length - 1 : 0; + while (i !== (resolvedAlignToTop ? -1 : children.length)) { const child = children[i]; const instance = getInstanceFromHostFiber(child); - const prevChild = children[alignToTop ? i + 1 : i - 1]; - - if (prevChild) { - const prevInstance = getInstanceFromHostFiber(prevChild); - if (prevInstance.parentNode === instance.parentNode) { - // If these are DOM siblings, check if either is fixed - isNewScrollContainer = - isInstanceScrollable(prevInstance) === CONTAINER_FIXED || - isInstanceScrollable(instance) === CONTAINER_FIXED - ? CONTAINER_FIXED - : CONTAINER_STD; - } else { - isNewScrollContainer = searchDOMUntilCommonAncestor( - instance, - prevInstance, - isInstanceScrollable, - ); - } - } + instance.scrollIntoView(alignToTop); + i += resolvedAlignToTop ? -1 : 1; } - - if (isNewScrollContainer) { - // We found a new scroll container, so scroll the appropriate child from the previous group - let childToScrollIndex; - if (alignToTop) { - childToScrollIndex = isLastGroup ? 0 : alignToTop ? i + 1 : i - 1; - } else { - childToScrollIndex = alignToTop ? i + 1 : i - 1; - } - - if (childToScrollIndex >= 0 && childToScrollIndex < children.length) { - const childToScroll = children[childToScrollIndex]; - const instanceToScroll = - getInstanceFromHostFiber(childToScroll); - - const didScroll = maybeScrollContainerIntoView( - instanceToScroll, - prevScrolledInstance, - alignToTop, - prevContainerIsFixed, - ); - if (didScroll) { - prevScrolledInstance = instanceToScroll; - prevContainerIsFixed = isNewScrollContainer === CONTAINER_FIXED; - } - } - } - - i += alignToTop ? -1 : 1; - } + }; } export function createFragmentInstance( diff --git a/packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js b/packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js index 1918449b90..35c10fe0f0 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js @@ -20,9 +20,6 @@ let Activity; let mockIntersectionObserver; let simulateIntersection; let setClientRects; -let setViewportSize; -let setScrollContainerHeight; -let setBoundingClientRect; let assertConsoleErrorDev; function Wrapper({children}) { @@ -43,9 +40,6 @@ describe('FragmentRefs', () => { mockIntersectionObserver = IntersectionMocks.mockIntersectionObserver; simulateIntersection = IntersectionMocks.simulateIntersection; setClientRects = IntersectionMocks.setClientRects; - setBoundingClientRect = IntersectionMocks.setBoundingClientRect; - setViewportSize = IntersectionMocks.setViewportSize; - setScrollContainerHeight = IntersectionMocks.setScrollContainerHeight; assertConsoleErrorDev = require('internal-test-utils').assertConsoleErrorDev; @@ -1844,6 +1838,9 @@ describe('FragmentRefs', () => { }); describe('scrollIntoView', () => { + function expectLast(arr, test) { + expect(arr[arr.length - 1]).toBe(test); + } // @gate enableFragmentRefs && enableFragmentRefsScrollIntoView it('does not yet support options', async () => { const fragmentRef = React.createRef(); @@ -1862,7 +1859,7 @@ describe('FragmentRefs', () => { describe('with children', () => { // @gate enableFragmentRefs && enableFragmentRefsScrollIntoView - it('calls scrollIntoView on the first child by default, or if alignToTop=true', async () => { + it('settles scroll on the first child by default, or if alignToTop=true', async () => { const fragmentRef = React.createRef(); const childARef = React.createRef(); const childBRef = React.createRef(); @@ -1879,20 +1876,22 @@ describe('FragmentRefs', () => { , ); }); - childARef.current.scrollIntoView = jest.fn(); - childBRef.current.scrollIntoView = jest.fn(); + + let logs = []; + childARef.current.scrollIntoView = jest.fn().mockImplementation(() => { + logs.push('childA'); + }); + childBRef.current.scrollIntoView = jest.fn().mockImplementation(() => { + logs.push('childB'); + }); // Default call fragmentRef.current.experimental_scrollIntoView(); - expect(childARef.current.scrollIntoView).toHaveBeenCalledTimes(1); - expect(childBRef.current.scrollIntoView).toHaveBeenCalledTimes(0); - - childARef.current.scrollIntoView.mockClear(); - + expectLast(logs, 'childA'); + logs = []; // alignToTop=true fragmentRef.current.experimental_scrollIntoView(true); - expect(childARef.current.scrollIntoView).toHaveBeenCalledTimes(1); - expect(childBRef.current.scrollIntoView).toHaveBeenCalledTimes(0); + expectLast(logs, 'childA'); }); // @gate enableFragmentRefs && enableFragmentRefsScrollIntoView @@ -1910,12 +1909,16 @@ describe('FragmentRefs', () => { ); }); - childARef.current.scrollIntoView = jest.fn(); - childBRef.current.scrollIntoView = jest.fn(); + const logs = []; + childARef.current.scrollIntoView = jest.fn().mockImplementation(() => { + logs.push('childA'); + }); + childBRef.current.scrollIntoView = jest.fn().mockImplementation(() => { + logs.push('childB'); + }); fragmentRef.current.experimental_scrollIntoView(false); - expect(childARef.current.scrollIntoView).toHaveBeenCalledTimes(0); - expect(childBRef.current.scrollIntoView).toHaveBeenCalledTimes(1); + expectLast(logs, 'childB'); }); // @gate enableFragmentRefs && enableFragmentRefsScrollIntoView @@ -1946,13 +1949,17 @@ describe('FragmentRefs', () => { root.render(); }); - childARef.current.scrollIntoView = jest.fn(); - childBRef.current.scrollIntoView = jest.fn(); + const logs = []; + childARef.current.scrollIntoView = jest.fn().mockImplementation(() => { + logs.push('childA'); + }); + childBRef.current.scrollIntoView = jest.fn().mockImplementation(() => { + logs.push('childB'); + }); // Default call fragmentRef.current.experimental_scrollIntoView(); - expect(childARef.current.scrollIntoView).toHaveBeenCalledTimes(1); - expect(childBRef.current.scrollIntoView).toHaveBeenCalledTimes(0); + expectLast(logs, 'childA'); }); // @gate enableFragmentRefs && enableFragmentRefsScrollIntoView @@ -2025,52 +2032,6 @@ describe('FragmentRefs', () => { root.render(); }); - setViewportSize(500, 500); - setBoundingClientRect(headerChildRef.current, { - x: 0, - y: 150, - width: 100, - height: 100, - }); - Object.defineProperty(headerChildRef.current, 'clientHeight', { - value: 100, - writable: true, - }); - setBoundingClientRect(childARef.current, { - x: 0, - y: 600, // outside of initial viewport - width: 100, - height: 100, - }); - Object.defineProperty(childARef.current, 'clientHeight', { - value: 100, - writable: true, - }); - setBoundingClientRect(childBRef.current, { - x: 0, - y: 1200, // outside of viewport after scroll to top of scrollContainerAll - width: 100, - height: 100, - }); - Object.defineProperty(childBRef.current, 'clientHeight', { - value: 100, - writable: true, - }); - setBoundingClientRect(childCRef.current, { - x: 0, - y: 1800, - width: 100, - height: 100, - }); - Object.defineProperty(childCRef.current, 'clientHeight', { - value: 100, - writable: true, - }); - - // Make containers scrollable - setScrollContainerHeight(scrollContainerRef.current, 100, 200); - setScrollContainerHeight(scrollContainerNestedRef.current, 100, 200); - let logs = []; headerChildRef.current.scrollIntoView = jest.fn(() => { logs.push('header'); @@ -2087,31 +2048,17 @@ describe('FragmentRefs', () => { // Default call fragmentRef.current.experimental_scrollIntoView(); - expect(childCRef.current.scrollIntoView).toHaveBeenCalledTimes(1); - // In the same group as A, we use the first child - expect(childBRef.current.scrollIntoView).toHaveBeenCalledTimes(0); - // Scrolling to A would push C out of the viewport, don't scroll - expect(childARef.current.scrollIntoView).toHaveBeenCalledTimes(0); - // Scrolling to header would push C out of the viewport, don't scroll - expect(headerChildRef.current.scrollIntoView).toHaveBeenCalledTimes(0); - expect(logs).toEqual(['C']); + expectLast(logs, 'header'); childARef.current.scrollIntoView.mockClear(); childBRef.current.scrollIntoView.mockClear(); childCRef.current.scrollIntoView.mockClear(); + logs = []; // // alignToTop=false fragmentRef.current.experimental_scrollIntoView(false); - expect(headerChildRef.current.scrollIntoView).toHaveBeenCalledTimes(1); - // In the same group as B, only attempt B which is the last child - expect(childARef.current.scrollIntoView).toHaveBeenCalledTimes(0); - // Previous scroll had fixed parent, scroll to B - // even if it would otherwise push prev out of viewport - expect(childBRef.current.scrollIntoView).toHaveBeenCalledTimes(1); - // Scrolling to C would push A out of the viewport, don't scroll to it - expect(childCRef.current.scrollIntoView).toHaveBeenCalledTimes(0); - expect(logs).toEqual(['header', 'B']); + expectLast(logs, 'C'); }); }); diff --git a/packages/react-dom/src/__tests__/utils/IntersectionMocks.js b/packages/react-dom/src/__tests__/utils/IntersectionMocks.js index f34f58ca0d..d89a683e8e 100644 --- a/packages/react-dom/src/__tests__/utils/IntersectionMocks.js +++ b/packages/react-dom/src/__tests__/utils/IntersectionMocks.js @@ -93,25 +93,3 @@ export function setClientRects(target, rects) { })); }; } - -export function setViewportSize(width, height) { - Object.defineProperty(window, 'innerWidth', { - value: width, - writable: true, - }); - Object.defineProperty(window, 'innerHeight', { - value: height, - writable: true, - }); -} - -export function setScrollContainerHeight(target, clientHeight, scrollHeight) { - Object.defineProperty(target, 'clientHeight', { - value: clientHeight, - writable: true, - }); - Object.defineProperty(target, 'scrollHeight', { - value: scrollHeight, - writable: true, - }); -}