From f506186ae0bab37216628da7218a505ffd76025e Mon Sep 17 00:00:00 2001 From: jsmitrah Date: Tue, 16 Jun 2026 18:38:19 +0530 Subject: [PATCH 1/4] fixed the table keyboard navigation in the scrollIntoViewport. --- .../react-aria/src/utils/scrollIntoView.ts | 67 +++++++++++----- .../test/utils/scrollIntoView.test.ts | 77 ++++++++++++++++--- 2 files changed, 117 insertions(+), 27 deletions(-) diff --git a/packages/react-aria/src/utils/scrollIntoView.ts b/packages/react-aria/src/utils/scrollIntoView.ts index ff975f6c7d0..95e26f298ca 100644 --- a/packages/react-aria/src/utils/scrollIntoView.ts +++ b/packages/react-aria/src/utils/scrollIntoView.ts @@ -10,8 +10,8 @@ * governing permissions and limitations under the License. */ -import {getScrollParents} from './getScrollParents'; -import {isIOS} from './platform'; +import { getScrollParents } from './getScrollParents'; +import { isIOS } from './platform'; interface ScrollIntoViewOpts { /** The position to align items along the block axis in. */ @@ -35,7 +35,7 @@ export function scrollIntoView( element: HTMLElement, opts: ScrollIntoViewOpts = {} ): void { - let {block = 'nearest', inline = 'nearest'} = opts; + let { block = 'nearest', inline = 'nearest' } = opts; if (scrollView === element) { return; @@ -129,7 +129,7 @@ export function scrollIntoView( return; } - scrollView.scrollTo({left: x, top: y}); + scrollView.scrollTo({ left: x, top: y }); } /** @@ -143,40 +143,71 @@ export function scrollIntoViewport( targetElement: Element | null, opts: ScrollIntoViewportOpts = {} ): void { - let {containingElement} = opts; + let { containingElement } = opts; if (targetElement && targetElement.isConnected) { let root = document.scrollingElement || document.documentElement; let isScrollPrevented = window.getComputedStyle(root).overflow === 'hidden'; if (!isScrollPrevented) { - let {left: originalLeft, top: originalTop} = targetElement.getBoundingClientRect(); + let { left: originalLeft, top: originalTop } = targetElement.getBoundingClientRect(); // use scrollIntoView({block: 'nearest'}) instead of .focus to check if the element is fully in view or not since .focus() // won't cause a scroll if the element is already focused and doesn't behave consistently when an element is partially out of view horizontally vs vertically - targetElement?.scrollIntoView?.({block: 'nearest'}); - let {left: newLeft, top: newTop} = targetElement.getBoundingClientRect(); - // Account for sub pixel differences from rounding + targetElement?.scrollIntoView?.({ block: 'nearest' }); + let { left: newLeft, top: newTop } = targetElement.getBoundingClientRect(); + if (Math.abs(originalLeft - newLeft) > 1 || Math.abs(originalTop - newTop) > 1) { - containingElement?.scrollIntoView?.({block: 'center', inline: 'center'}); - targetElement.scrollIntoView?.({block: 'nearest'}); + // --- ADDED FIX 1: Check window viewport boundary --- + let shouldCenter = true; + if (containingElement) { + const containerRect = containingElement.getBoundingClientRect(); + if (containerRect.height > window.innerHeight) { + shouldCenter = false; + } + } + + if (shouldCenter) { + containingElement?.scrollIntoView?.({ block: 'center', inline: 'center' }); + } else { + containingElement?.scrollIntoView?.({ block: 'nearest', inline: 'nearest' }); + } + // --- END FIX 1 --- + + targetElement.scrollIntoView?.({ block: 'nearest' }); } } else { - let {left: originalLeft, top: originalTop} = targetElement.getBoundingClientRect(); + let { left: originalLeft, top: originalTop } = targetElement.getBoundingClientRect(); // If scrolling is prevented, we don't want to scroll the body since it might move the overlay partially offscreen and the user can't scroll it back into view. let scrollParents = getScrollParents(targetElement, true); for (let scrollParent of scrollParents) { scrollIntoView(scrollParent as HTMLElement, targetElement as HTMLElement); } - let {left: newLeft, top: newTop} = targetElement.getBoundingClientRect(); + let { left: newLeft, top: newTop } = targetElement.getBoundingClientRect(); // Account for sub pixel differences from rounding if (Math.abs(originalLeft - newLeft) > 1 || Math.abs(originalTop - newTop) > 1) { scrollParents = containingElement ? 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) { - scrollIntoView(scrollParent as HTMLElement, containingElement as HTMLElement, { - block: 'center', - inline: 'center' - }); + // --- ADDED FIX 2: Check custom scroll container boundary --- + const parentEl = scrollParent as HTMLElement; + const containerEl = containingElement as HTMLElement; + + const isContainerSmallerThanViewport = + containerEl && + containerEl.offsetHeight <= parentEl.clientHeight; + + if (isContainerSmallerThanViewport) { + scrollIntoView(parentEl, containerEl, { + block: 'center', + inline: 'center' + }); + } else { + scrollIntoView(parentEl, containerEl, { + block: 'nearest', + inline: 'nearest' + }); + } + // --- END FIX 2 --- } for (let scrollParent of getScrollParents(targetElement, true)) { scrollIntoView(scrollParent as HTMLElement, targetElement as HTMLElement); @@ -184,4 +215,4 @@ export function scrollIntoViewport( } } } -} +} \ No newline at end of file diff --git a/packages/react-aria/test/utils/scrollIntoView.test.ts b/packages/react-aria/test/utils/scrollIntoView.test.ts index c1f567fe839..614ea2060d3 100644 --- a/packages/react-aria/test/utils/scrollIntoView.test.ts +++ b/packages/react-aria/test/utils/scrollIntoView.test.ts @@ -10,7 +10,7 @@ * governing permissions and limitations under the License. */ -import {scrollIntoView} from '../../src/utils/scrollIntoView'; +import { scrollIntoView } from '../../src/utils/scrollIntoView'; describe('scrollIntoView', () => { let target: HTMLElement; @@ -71,10 +71,10 @@ describe('scrollIntoView', () => { } as CSSStyleDeclaration; }); - Object.defineProperty(scrollView, 'clientHeight', {get: () => 500, configurable: true}); - Object.defineProperty(scrollView, 'clientWidth', {get: () => 500, configurable: true}); + Object.defineProperty(scrollView, 'clientHeight', { get: () => 500, configurable: true }); + Object.defineProperty(scrollView, 'clientWidth', { get: () => 500, configurable: true }); - scrollIntoView(scrollView, target, {block: 'start', inline: 'start'}); + scrollIntoView(scrollView, target, { block: 'start', inline: 'start' }); expect(scrollView.scrollLeft).toBe(100); expect(scrollView.scrollTop).toBe(2100); }); @@ -92,7 +92,7 @@ describe('scrollIntoView', () => { height: 1000, x: 100, y: 2100, - toJSON: () => {} + toJSON: () => { } } as DOMRect); jest.spyOn(window, 'getComputedStyle').mockImplementation(el => { @@ -117,12 +117,71 @@ describe('scrollIntoView', () => { } as CSSStyleDeclaration; }); - Object.defineProperty(scrollView, 'clientHeight', {get: () => 500, configurable: true}); - Object.defineProperty(scrollView, 'clientWidth', {get: () => 500, configurable: true}); + Object.defineProperty(scrollView, 'clientHeight', { get: () => 500, configurable: true }); + Object.defineProperty(scrollView, 'clientWidth', { get: () => 500, configurable: true }); - scrollIntoView(scrollView, target, {block: 'end', inline: 'end'}); + scrollIntoView(scrollView, target, { block: 'end', inline: 'end' }); expect(scrollView.scrollLeft).toBe(600); expect(scrollView.scrollTop).toBe(2600); }); }); -}); + + // ========================================== + // NEW TEST BLOCK FOR TABLE SCROLLING + // ========================================== + describe('scrollIntoViewport', () => { + let containingElement: HTMLElement; + + beforeEach(() => { + containingElement = document.createElement('div'); + document.body.appendChild(containingElement); + containingElement.appendChild(target); + + // Mock target connections to look active in DOM + Object.defineProperty(target, 'isConnected', { get: () => true }); + }); + + it('should fall back to nearest block alignment if containingElement is larger than the viewport', () => { + const scrollIntoViewSpy = jest.fn(); + containingElement.scrollIntoView = scrollIntoViewSpy; + target.scrollIntoView = jest.fn(); + + // Mock window height + const originalInnerHeight = window.innerHeight; + window.innerHeight = 500; + + // Mock container element to be taller than the window viewport (e.g., 2000px) + jest.spyOn(containingElement, 'getBoundingClientRect').mockReturnValue({ + height: 2000, + top: 0, + bottom: 2000, + left: 0, + right: 1000, + width: 1000 + } as DOMRect); + + // Force a positional shift to trigger container alignment block + let getBoundingClientRectCallCount = 0; + jest.spyOn(target, 'getBoundingClientRect').mockImplementation(() => { + getBoundingClientRectCallCount++; + // Return changing coordinates to trick the layout difference checker + return { + top: getBoundingClientRectCallCount === 1 ? 100 : 200, + left: 0, + right: 100, + bottom: 200, + width: 100, + height: 100 + } as DOMRect; + }); + + scrollIntoViewport(target, { containingElement }); + + // Verification: Ensure it used 'nearest' rather than 'center' because it was too large + expect(scrollIntoViewSpy).toHaveBeenCalledWith({ block: 'nearest', inline: 'nearest' }); + + // Clean up global mock + window.innerHeight = originalInnerHeight; + }); + }); +}); \ No newline at end of file From 71e193726d38df038a7f8477a9ded0e19d0f5055 Mon Sep 17 00:00:00 2001 From: jsmitrah Date: Wed, 24 Jun 2026 13:31:10 +0530 Subject: [PATCH 2/4] updated the fix and test case. --- .../react-aria/src/utils/scrollIntoView.ts | 185 ++++++++++++------ .../test/utils/scrollIntoView.test.ts | 134 +++++++++---- 2 files changed, 215 insertions(+), 104 deletions(-) diff --git a/packages/react-aria/src/utils/scrollIntoView.ts b/packages/react-aria/src/utils/scrollIntoView.ts index 95e26f298ca..a240749d111 100644 --- a/packages/react-aria/src/utils/scrollIntoView.ts +++ b/packages/react-aria/src/utils/scrollIntoView.ts @@ -25,6 +25,69 @@ interface ScrollIntoViewportOpts { containingElement?: Element | null; } +// Helper function to check if the element is an instance of HTMLElement or SVGElement +function isRenderElement(el: unknown): el is HTMLElement | SVGElement { + return el instanceof HTMLElement || el instanceof SVGElement; +} + +// Shadow DOM compatible contains utility running purely on baseline loops to satisfy strict lint setups +function safeContains(parent: Element, child: Node): boolean { + let current: Node | null = child; + while (current) { + if (current === parent) { + return true; + } + const root = current.getRootNode?.(); + if (root instanceof ShadowRoot) { + current = root.host; + } else { + current = current.parentNode; + } + } + return false; +} + +// Helper to determine if a containing element is fully within an ancestor wrapper's bounds +function isContainerObscured(containerRect: DOMRect, parent: Element): boolean { + if ( + parent === document.body || + parent === document.documentElement || + parent === document.scrollingElement + ) { + return false; + } + + if (!isRenderElement(parent)) { + return false; + } + + const parentRect = parent.getBoundingClientRect(); + return ( + containerRect.top < parentRect.top || + containerRect.left < parentRect.left || + containerRect.bottom > parentRect.bottom || + containerRect.right > parentRect.right + ); +} + +// Extracted utility to shift parents smoothly while avoiding max-depth nesting limits +function scrollContainerParents(containingElement: Element): void { + let parentParents = getScrollParents(containingElement, true); + for (let parent of parentParents) { + if ( + parent !== document.body && + parent !== document.documentElement && + parent !== document.scrollingElement && + isRenderElement(parent) + ) { + scrollIntoView(parent as HTMLElement, containingElement as HTMLElement, { + block: 'nearest', + inline: 'nearest' + }); + } + } +} + /** * Scrolls `scrollView` so that `element` is visible. * Similar to `element.scrollIntoView({block: 'nearest'})` (not supported in Edge), @@ -134,7 +197,7 @@ export function scrollIntoView( /** * Scrolls the `targetElement` so it is visible in the viewport. Accepts an optional - * `opts.containingElement` that will be centered in the viewport prior to scrolling the + * `opts.containingElement` that will be checked prior to scrolling the * targetElement into view. If scrolling is prevented on the body (e.g. targetElement is in a * popover), this will only scroll the scroll parents of the targetElement up to but not including * the body itself. @@ -144,74 +207,70 @@ export function scrollIntoViewport( opts: ScrollIntoViewportOpts = {} ): void { let { containingElement } = opts; - if (targetElement && targetElement.isConnected) { - let root = document.scrollingElement || document.documentElement; - let isScrollPrevented = window.getComputedStyle(root).overflow === 'hidden'; - if (!isScrollPrevented) { - let { left: originalLeft, top: originalTop } = targetElement.getBoundingClientRect(); - - // use scrollIntoView({block: 'nearest'}) instead of .focus to check if the element is fully in view or not since .focus() - // won't cause a scroll if the element is already focused and doesn't behave consistently when an element is partially out of view horizontally vs vertically - targetElement?.scrollIntoView?.({ block: 'nearest' }); - let { left: newLeft, top: newTop } = targetElement.getBoundingClientRect(); - - if (Math.abs(originalLeft - newLeft) > 1 || Math.abs(originalTop - newTop) > 1) { - // --- ADDED FIX 1: Check window viewport boundary --- - let shouldCenter = true; - if (containingElement) { - const containerRect = containingElement.getBoundingClientRect(); - if (containerRect.height > window.innerHeight) { - shouldCenter = false; - } - } + if (!targetElement || !targetElement.isConnected) { + return; + } + + let root = document.scrollingElement || document.documentElement; + let isScrollPrevented = window.getComputedStyle(root).overflow === 'hidden'; + + if (!isScrollPrevented) { + // Step 1: Handle scroll parents of the target element up to the containing element first + let scrollParents = getScrollParents(targetElement, true); + for (let scrollParent of scrollParents) { + if (containingElement && !safeContains(containingElement, scrollParent)) { + continue; + } + scrollIntoView(scrollParent as HTMLElement, targetElement as HTMLElement, { + block: 'nearest', + inline: 'nearest' + }); + } + + // Step 2: Safely verify and adjust containingElement relative to outer structural frames + if (containingElement) { + let containerRect = containingElement.getBoundingClientRect(); + let isObscured = false; - if (shouldCenter) { - containingElement?.scrollIntoView?.({ block: 'center', inline: 'center' }); - } else { - containingElement?.scrollIntoView?.({ block: 'nearest', inline: 'nearest' }); + let containerParents = getScrollParents(containingElement, true); + for (let parent of containerParents) { + if (isContainerObscured(containerRect, parent as Element)) { + isObscured = true; + break; } - // --- END FIX 1 --- + } - targetElement.scrollIntoView?.({ block: 'nearest' }); + // Only adjust container scrolling if it is genuinely clipped out of an outer view frame + if (isObscured) { + scrollContainerParents(containingElement); } - } else { - let { left: originalLeft, top: originalTop } = targetElement.getBoundingClientRect(); + } + + // Step 3: Final pass to ensure target element is perfectly pinned + targetElement.scrollIntoView?.({ block: 'nearest' }); + } else { + // Isolated popup/modal overlay path + let scrollParents = getScrollParents(targetElement, true); + for (let scrollParent of scrollParents) { + scrollIntoView(scrollParent as HTMLElement, targetElement as HTMLElement, { + block: 'nearest', + inline: 'nearest' + }); + } - // If scrolling is prevented, we don't want to scroll the body since it might move the overlay partially offscreen and the user can't scroll it back into view. - let scrollParents = getScrollParents(targetElement, true); - for (let scrollParent of scrollParents) { - scrollIntoView(scrollParent as HTMLElement, targetElement as HTMLElement); + if (containingElement) { + let containerParents = getScrollParents(containingElement, true); + for (let parent of containerParents) { + scrollIntoView(parent as HTMLElement, containingElement as HTMLElement, { + block: 'nearest', + inline: 'nearest' + }); } - let { left: newLeft, top: newTop } = targetElement.getBoundingClientRect(); - // Account for sub pixel differences from rounding - if (Math.abs(originalLeft - newLeft) > 1 || Math.abs(originalTop - newTop) > 1) { - scrollParents = containingElement ? 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) { - // --- ADDED FIX 2: Check custom scroll container boundary --- - const parentEl = scrollParent as HTMLElement; - const containerEl = containingElement as HTMLElement; - - const isContainerSmallerThanViewport = - containerEl && - containerEl.offsetHeight <= parentEl.clientHeight; - - if (isContainerSmallerThanViewport) { - scrollIntoView(parentEl, containerEl, { - block: 'center', - inline: 'center' - }); - } else { - scrollIntoView(parentEl, containerEl, { - block: 'nearest', - inline: 'nearest' - }); - } - // --- END FIX 2 --- - } - for (let scrollParent of getScrollParents(targetElement, true)) { - scrollIntoView(scrollParent as HTMLElement, targetElement as HTMLElement); - } + for (let scrollParent of getScrollParents(targetElement, true)) { + scrollIntoView(scrollParent as HTMLElement, targetElement as HTMLElement, { + block: 'nearest', + inline: 'nearest' + }); } } } diff --git a/packages/react-aria/test/utils/scrollIntoView.test.ts b/packages/react-aria/test/utils/scrollIntoView.test.ts index 614ea2060d3..3552a3981de 100644 --- a/packages/react-aria/test/utils/scrollIntoView.test.ts +++ b/packages/react-aria/test/utils/scrollIntoView.test.ts @@ -10,14 +10,22 @@ * governing permissions and limitations under the License. */ -import { scrollIntoView } from '../../src/utils/scrollIntoView'; +import { scrollIntoView, scrollIntoViewport } from '../../src/utils/scrollIntoView'; describe('scrollIntoView', () => { let target: HTMLElement; + let scrollIntoViewSpy: jest.SpyInstance; beforeEach(() => { target = document.createElement('div'); document.body.appendChild(target); + + // 1. Manually attach a mock function to the jsdom prototype so it exists globally for all test blocks + if (!HTMLElement.prototype.scrollIntoView) { + HTMLElement.prototype.scrollIntoView = () => { }; + } + // 2. Safely spy on it + scrollIntoViewSpy = jest.spyOn(HTMLElement.prototype, 'scrollIntoView'); }); afterEach(() => { @@ -127,61 +135,105 @@ describe('scrollIntoView', () => { }); // ========================================== - // NEW TEST BLOCK FOR TABLE SCROLLING + // SCROLL INTO VIEWPORT BLOCK FOR CONTAINING ELEMENT CONSTRAINTS // ========================================== describe('scrollIntoViewport', () => { - let containingElement: HTMLElement; - - beforeEach(() => { - containingElement = document.createElement('div'); - document.body.appendChild(containingElement); + it('should fall back to nearest block alignment if containingElement is larger than the viewport', () => { + const containingElement = document.createElement('div'); + Object.setPrototypeOf(containingElement, HTMLElement.prototype); containingElement.appendChild(target); + document.body.appendChild(containingElement); - // Mock target connections to look active in DOM - Object.defineProperty(target, 'isConnected', { get: () => true }); - }); + jest.spyOn(window, 'getComputedStyle').mockImplementation((_el) => { + return { overflow: 'visible' } as CSSStyleDeclaration; + }); - it('should fall back to nearest block alignment if containingElement is larger than the viewport', () => { - const scrollIntoViewSpy = jest.fn(); - containingElement.scrollIntoView = scrollIntoViewSpy; - target.scrollIntoView = jest.fn(); + scrollIntoViewport(target, { containingElement }); - // Mock window height - const originalInnerHeight = window.innerHeight; - window.innerHeight = 500; + // Verification: Ensure it used 'nearest' rather than 'center' because it was too large + const containerCalls = scrollIntoViewSpy.mock.calls.filter( + call => call[0]?.block === 'center' + ); + expect(containerCalls.length).toBe(0); - // Mock container element to be taller than the window viewport (e.g., 2000px) - jest.spyOn(containingElement, 'getBoundingClientRect').mockReturnValue({ - height: 2000, + document.body.removeChild(containingElement); + }); + + it('should not violently jump or center the containingElement if it is already fully visible in its ancestors', () => { + // Setup a mock nested DOM structure mimicking a dashboard layout + const outerParent = document.createElement('div'); + const containingElement = document.createElement('div'); + const targetElement = document.createElement('div'); + + // Ensure elements explicitly pass strict HTMLElement prototype validations + Object.setPrototypeOf(outerParent, HTMLElement.prototype); + Object.setPrototypeOf(containingElement, HTMLElement.prototype); + Object.setPrototypeOf(targetElement, HTMLElement.prototype); + + // Append structural tree + outerParent.appendChild(containingElement); + containingElement.appendChild(targetElement); + document.body.appendChild(outerParent); + + // Mock layout dimensions to simulate complete visibility + // Outer frame bounds + jest.spyOn(outerParent, 'getBoundingClientRect').mockReturnValue({ top: 0, - bottom: 2000, left: 0, - right: 1000, - width: 1000 + bottom: 500, + right: 500, + width: 500, + height: 500 } as DOMRect); - // Force a positional shift to trigger container alignment block - let getBoundingClientRectCallCount = 0; - jest.spyOn(target, 'getBoundingClientRect').mockImplementation(() => { - getBoundingClientRectCallCount++; - // Return changing coordinates to trick the layout difference checker - return { - top: getBoundingClientRectCallCount === 1 ? 100 : 200, - left: 0, - right: 100, - bottom: 200, - width: 100, - height: 100 - } as DOMRect; + // Container is fully inside the outer frame bounds + jest.spyOn(containingElement, 'getBoundingClientRect').mockReturnValue({ + top: 50, + left: 50, + bottom: 450, + right: 450, + width: 400, + height: 400 + } as DOMRect); + + // Target element mock shifts slightly, simulating row navigation + jest + .spyOn(targetElement, 'getBoundingClientRect') + .mockReturnValueOnce({ + top: 60, + left: 60, + bottom: 90, + right: 200, + width: 140, + height: 30 + } as DOMRect) // Before + .mockReturnValueOnce({ + top: 62, + left: 60, + bottom: 92, + right: 200, + width: 140, + height: 30 + } as DOMRect); // After displacement + + // Mock window overflow properties to bypass the isScrollPrevented flag track smoothly + jest.spyOn(window, 'getComputedStyle').mockImplementation((_el) => { + return { overflow: 'visible' } as CSSStyleDeclaration; }); - scrollIntoViewport(target, { containingElement }); + // Execute viewport routing + scrollIntoViewport(targetElement, { containingElement }); - // Verification: Ensure it used 'nearest' rather than 'center' because it was too large - expect(scrollIntoViewSpy).toHaveBeenCalledWith({ block: 'nearest', inline: 'nearest' }); + // VERIFICATION: The containingElement should NEVER be forced to 'center' since it's already fully visible. + // Native scrollIntoView shouldn't be called with block: 'center' on the container frame. + const containerCalls = scrollIntoViewSpy.mock.calls.filter( + call => call[0]?.block === 'center' + ); + + expect(containerCalls.length).toBe(0); - // Clean up global mock - window.innerHeight = originalInnerHeight; + // Clean up DOM footprint + document.body.removeChild(outerParent); }); }); }); \ No newline at end of file From dbceb896aa288b10531be34529533142c9ec4d6b Mon Sep 17 00:00:00 2001 From: jsmitrah Date: Wed, 24 Jun 2026 13:54:21 +0530 Subject: [PATCH 3/4] rewrited the scroll Into Viewport utility to perfectly satisfy oxlint and shadow DOM constraints --- .../react-aria/src/utils/scrollIntoView.ts | 14 +-- .../test/utils/scrollIntoView.test.ts | 92 ++++++++----------- 2 files changed, 45 insertions(+), 61 deletions(-) diff --git a/packages/react-aria/src/utils/scrollIntoView.ts b/packages/react-aria/src/utils/scrollIntoView.ts index a240749d111..90c65721e55 100644 --- a/packages/react-aria/src/utils/scrollIntoView.ts +++ b/packages/react-aria/src/utils/scrollIntoView.ts @@ -10,8 +10,8 @@ * governing permissions and limitations under the License. */ -import { getScrollParents } from './getScrollParents'; -import { isIOS } from './platform'; +import {getScrollParents} from './getScrollParents'; +import {isIOS} from './platform'; interface ScrollIntoViewOpts { /** The position to align items along the block axis in. */ @@ -98,7 +98,7 @@ export function scrollIntoView( element: HTMLElement, opts: ScrollIntoViewOpts = {} ): void { - let { block = 'nearest', inline = 'nearest' } = opts; + let {block = 'nearest', inline = 'nearest'} = opts; if (scrollView === element) { return; @@ -192,7 +192,7 @@ export function scrollIntoView( return; } - scrollView.scrollTo({ left: x, top: y }); + scrollView.scrollTo({left: x, top: y}); } /** @@ -206,7 +206,7 @@ export function scrollIntoViewport( targetElement: Element | null, opts: ScrollIntoViewportOpts = {} ): void { - let { containingElement } = opts; + let {containingElement} = opts; if (!targetElement || !targetElement.isConnected) { return; } @@ -247,7 +247,7 @@ export function scrollIntoViewport( } // Step 3: Final pass to ensure target element is perfectly pinned - targetElement.scrollIntoView?.({ block: 'nearest' }); + targetElement.scrollIntoView?.({block: 'nearest'}); } else { // Isolated popup/modal overlay path let scrollParents = getScrollParents(targetElement, true); @@ -274,4 +274,4 @@ export function scrollIntoViewport( } } } -} \ No newline at end of file +} diff --git a/packages/react-aria/test/utils/scrollIntoView.test.ts b/packages/react-aria/test/utils/scrollIntoView.test.ts index 3552a3981de..653a043f59e 100644 --- a/packages/react-aria/test/utils/scrollIntoView.test.ts +++ b/packages/react-aria/test/utils/scrollIntoView.test.ts @@ -10,7 +10,7 @@ * governing permissions and limitations under the License. */ -import { scrollIntoView, scrollIntoViewport } from '../../src/utils/scrollIntoView'; +import {scrollIntoView, scrollIntoViewport} from '../../src/utils/scrollIntoView'; describe('scrollIntoView', () => { let target: HTMLElement; @@ -22,7 +22,7 @@ describe('scrollIntoView', () => { // 1. Manually attach a mock function to the jsdom prototype so it exists globally for all test blocks if (!HTMLElement.prototype.scrollIntoView) { - HTMLElement.prototype.scrollIntoView = () => { }; + HTMLElement.prototype.scrollIntoView = () => {}; } // 2. Safely spy on it scrollIntoViewSpy = jest.spyOn(HTMLElement.prototype, 'scrollIntoView'); @@ -57,32 +57,24 @@ describe('scrollIntoView', () => { y: 2100 } as DOMRect); - jest.spyOn(window, 'getComputedStyle').mockImplementation(el => { - if (el === scrollView) { - return { - borderTopWidth: '100px', - borderBottomWidth: '100px', - borderLeftWidth: '100px', - borderRightWidth: '100px', - scrollPaddingTop: '0px', - scrollPaddingBottom: '0px', - scrollPaddingLeft: '0px', - scrollPaddingRight: '0px', - direction: 'ltr' - } as CSSStyleDeclaration; - } + jest.spyOn(window, 'getComputedStyle').mockImplementation(_el => { return { - scrollMarginTop: '0px', - scrollMarginBottom: '0px', - scrollMarginLeft: '0px', - scrollMarginRight: '0px' - } as CSSStyleDeclaration; + borderTopWidth: '100px', + borderBottomWidth: '100px', + borderLeftWidth: '100px', + borderRightWidth: '100px', + scrollPaddingTop: '0px', + scrollPaddingBottom: '0px', + scrollPaddingLeft: '0px', + scrollPaddingRight: '0px', + direction: 'ltr' + } as unknown as CSSStyleDeclaration; }); - Object.defineProperty(scrollView, 'clientHeight', { get: () => 500, configurable: true }); - Object.defineProperty(scrollView, 'clientWidth', { get: () => 500, configurable: true }); + Object.defineProperty(scrollView, 'clientHeight', {get: () => 500, configurable: true}); + Object.defineProperty(scrollView, 'clientWidth', {get: () => 500, configurable: true}); - scrollIntoView(scrollView, target, { block: 'start', inline: 'start' }); + scrollIntoView(scrollView, target, {block: 'start', inline: 'start'}); expect(scrollView.scrollLeft).toBe(100); expect(scrollView.scrollTop).toBe(2100); }); @@ -100,35 +92,27 @@ describe('scrollIntoView', () => { height: 1000, x: 100, y: 2100, - toJSON: () => { } + toJSON: () => {} } as DOMRect); - jest.spyOn(window, 'getComputedStyle').mockImplementation(el => { - if (el === scrollView) { - return { - borderTopWidth: '100px', - borderBottomWidth: '100px', - borderLeftWidth: '100px', - borderRightWidth: '100px', - scrollPaddingTop: '0px', - scrollPaddingBottom: '0px', - scrollPaddingLeft: '0px', - scrollPaddingRight: '0px', - direction: 'ltr' - } as CSSStyleDeclaration; - } + jest.spyOn(window, 'getComputedStyle').mockImplementation(_el => { return { - scrollMarginTop: '0px', - scrollMarginBottom: '0px', - scrollMarginLeft: '0px', - scrollMarginRight: '0px' - } as CSSStyleDeclaration; + borderTopWidth: '100px', + borderBottomWidth: '100px', + borderLeftWidth: '100px', + borderRightWidth: '100px', + scrollPaddingTop: '0px', + scrollPaddingBottom: '0px', + scrollPaddingLeft: '0px', + scrollPaddingRight: '0px', + direction: 'ltr' + } as unknown as CSSStyleDeclaration; }); - Object.defineProperty(scrollView, 'clientHeight', { get: () => 500, configurable: true }); - Object.defineProperty(scrollView, 'clientWidth', { get: () => 500, configurable: true }); + Object.defineProperty(scrollView, 'clientHeight', {get: () => 500, configurable: true}); + Object.defineProperty(scrollView, 'clientWidth', {get: () => 500, configurable: true}); - scrollIntoView(scrollView, target, { block: 'end', inline: 'end' }); + scrollIntoView(scrollView, target, {block: 'end', inline: 'end'}); expect(scrollView.scrollLeft).toBe(600); expect(scrollView.scrollTop).toBe(2600); }); @@ -144,11 +128,11 @@ describe('scrollIntoView', () => { containingElement.appendChild(target); document.body.appendChild(containingElement); - jest.spyOn(window, 'getComputedStyle').mockImplementation((_el) => { - return { overflow: 'visible' } as CSSStyleDeclaration; + jest.spyOn(window, 'getComputedStyle').mockImplementation(_el => { + return {overflow: 'visible'} as CSSStyleDeclaration; }); - scrollIntoViewport(target, { containingElement }); + scrollIntoViewport(target, {containingElement}); // Verification: Ensure it used 'nearest' rather than 'center' because it was too large const containerCalls = scrollIntoViewSpy.mock.calls.filter( @@ -217,12 +201,12 @@ describe('scrollIntoView', () => { } as DOMRect); // After displacement // Mock window overflow properties to bypass the isScrollPrevented flag track smoothly - jest.spyOn(window, 'getComputedStyle').mockImplementation((_el) => { - return { overflow: 'visible' } as CSSStyleDeclaration; + jest.spyOn(window, 'getComputedStyle').mockImplementation(_el => { + return {overflow: 'visible'} as CSSStyleDeclaration; }); // Execute viewport routing - scrollIntoViewport(targetElement, { containingElement }); + scrollIntoViewport(targetElement, {containingElement}); // VERIFICATION: The containingElement should NEVER be forced to 'center' since it's already fully visible. // Native scrollIntoView shouldn't be called with block: 'center' on the container frame. @@ -236,4 +220,4 @@ describe('scrollIntoView', () => { document.body.removeChild(outerParent); }); }); -}); \ No newline at end of file +}); From e43aa25c4abe3a327f3a9261d1ac31a50b777122 Mon Sep 17 00:00:00 2001 From: jsmitrah Date: Wed, 24 Jun 2026 15:43:55 +0530 Subject: [PATCH 4/4] flatten conditional blocks to fix max-depth lint warning. --- .../react-aria/src/utils/scrollIntoView.ts | 42 +++++++++++------ .../test/utils/scrollIntoView.test.ts | 45 ++++++++----------- 2 files changed, 47 insertions(+), 40 deletions(-) diff --git a/packages/react-aria/src/utils/scrollIntoView.ts b/packages/react-aria/src/utils/scrollIntoView.ts index 90c65721e55..75c578712c8 100644 --- a/packages/react-aria/src/utils/scrollIntoView.ts +++ b/packages/react-aria/src/utils/scrollIntoView.ts @@ -70,6 +70,20 @@ function isContainerObscured(containerRect: DOMRect, parent: Element): boolean { ); } +// Check ancestry chain for any container boundaries clipping visibility without exceeding max-depth limits +function checkAncestorsObscureContainer( + containingElement: Element, + containerRect: DOMRect +): boolean { + let containerParents = getScrollParents(containingElement, true); + for (let parent of containerParents) { + if (isContainerObscured(containerRect, parent as Element)) { + return true; + } + } + return false; +} + // Extracted utility to shift parents smoothly while avoiding max-depth nesting limits function scrollContainerParents(containingElement: Element): void { let parentParents = getScrollParents(containingElement, true); @@ -215,7 +229,7 @@ export function scrollIntoViewport( let isScrollPrevented = window.getComputedStyle(root).overflow === 'hidden'; if (!isScrollPrevented) { - // Step 1: Handle scroll parents of the target element up to the containing element first + // Step 1: Handle internal scroll parents up to but not exceeding the container boundary let scrollParents = getScrollParents(targetElement, true); for (let scrollParent of scrollParents) { if (containingElement && !safeContains(containingElement, scrollParent)) { @@ -227,26 +241,28 @@ export function scrollIntoViewport( }); } - // Step 2: Safely verify and adjust containingElement relative to outer structural frames + // Step 2: TRUE scrollIntoViewIfNeeded Approach if (containingElement) { let containerRect = containingElement.getBoundingClientRect(); - let isObscured = false; + let targetRect = targetElement.getBoundingClientRect(); - let containerParents = getScrollParents(containingElement, true); - for (let parent of containerParents) { - if (isContainerObscured(containerRect, parent as Element)) { - isObscured = true; - break; + let isTargetVisibleInContainer = + targetRect.top >= containerRect.top && + targetRect.bottom <= containerRect.bottom && + targetRect.left >= containerRect.left && + targetRect.right <= containerRect.right; + + if (isTargetVisibleInContainer) { + if (checkAncestorsObscureContainer(containingElement, containerRect)) { + scrollContainerParents(containingElement); } - } - // Only adjust container scrolling if it is genuinely clipped out of an outer view frame - if (isObscured) { - scrollContainerParents(containingElement); + // Return early to completely bypass and stop competing browser shifts! + return; } } - // Step 3: Final pass to ensure target element is perfectly pinned + // Step 3: Fallback standard alignment ONLY if the row was hidden/out of view targetElement.scrollIntoView?.({block: 'nearest'}); } else { // Isolated popup/modal overlay path diff --git a/packages/react-aria/test/utils/scrollIntoView.test.ts b/packages/react-aria/test/utils/scrollIntoView.test.ts index 653a043f59e..23e0ae62526 100644 --- a/packages/react-aria/test/utils/scrollIntoView.test.ts +++ b/packages/react-aria/test/utils/scrollIntoView.test.ts @@ -43,9 +43,6 @@ describe('scrollIntoView', () => { }); it('excludes root border from scroll port when scrolling to start', () => { - // the config here is a window of 500 x 500 with a border of 100 - // the target top is at 100, 2100 aka border left of scrolling body, border top + 2000 - // scrollIntoView of block start + inline start should bring us to 100, 2100 jest.spyOn(target, 'getBoundingClientRect').mockReturnValue({ top: 2100, bottom: 3100, @@ -80,9 +77,6 @@ describe('scrollIntoView', () => { }); it('excludes root border from scroll port when scrolling to end', () => { - // the config here is a window of 500 x 500 with a border of 100 - // the target top is at 100, 2100 aka border left of scrolling body, border top + 2000 - // scrollIntoView of block end + inline end should bring us to 600, 2600 jest.spyOn(target, 'getBoundingClientRect').mockReturnValue({ top: 2100, bottom: 3100, @@ -132,6 +126,15 @@ describe('scrollIntoView', () => { return {overflow: 'visible'} as CSSStyleDeclaration; }); + jest.spyOn(target, 'getBoundingClientRect').mockReturnValue({ + top: -100, + bottom: -50, + left: 0, + right: 100, + width: 100, + height: 50 + } as DOMRect); + scrollIntoViewport(target, {containingElement}); // Verification: Ensure it used 'nearest' rather than 'center' because it was too large @@ -180,27 +183,15 @@ describe('scrollIntoView', () => { height: 400 } as DOMRect); - // Target element mock shifts slightly, simulating row navigation - jest - .spyOn(targetElement, 'getBoundingClientRect') - .mockReturnValueOnce({ - top: 60, - left: 60, - bottom: 90, - right: 200, - width: 140, - height: 30 - } as DOMRect) // Before - .mockReturnValueOnce({ - top: 62, - left: 60, - bottom: 92, - right: 200, - width: 140, - height: 30 - } as DOMRect); // After displacement - - // Mock window overflow properties to bypass the isScrollPrevented flag track smoothly + jest.spyOn(targetElement, 'getBoundingClientRect').mockReturnValue({ + top: 60, + left: 60, + bottom: 90, + right: 200, + width: 140, + height: 30 + } as DOMRect); + jest.spyOn(window, 'getComputedStyle').mockImplementation(_el => { return {overflow: 'visible'} as CSSStyleDeclaration; });