diff --git a/.changeset/swift-forks-sneeze.md b/.changeset/swift-forks-sneeze.md new file mode 100644 index 00000000000..709edbe0aa1 --- /dev/null +++ b/.changeset/swift-forks-sneeze.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +PageLayout: Move pane and sidebar viewport clamping to CSS so window resize stays smooth and JS sync runs only after resizing ends diff --git a/packages/react/src/PageLayout/PageLayout.module.css b/packages/react/src/PageLayout/PageLayout.module.css index 526fa6d8ee5..d019a2ae779 100644 --- a/packages/react/src/PageLayout/PageLayout.module.css +++ b/packages/react/src/PageLayout/PageLayout.module.css @@ -31,10 +31,23 @@ --pane-width-small: 100%; --pane-width-medium: 100%; --pane-width-large: 100%; + --page-layout-viewport-width: 100vw; /* NOTE: This value is exported via :export for use in usePaneWidth.ts */ --pane-max-width-diff: 511px; /* Sidebar uses a smaller diff since it doesn't need to reserve space for a file tree pane */ --sidebar-max-width-diff: 256px; + --pane-viewport-max: max( + var(--pane-min-width, 0px), + calc(var(--page-layout-viewport-width) - var(--pane-max-width-diff)) + ); + --sidebar-viewport-max: max( + var(--pane-min-width, 0px), + calc(var(--page-layout-viewport-width) - var(--sidebar-max-width-diff)) + ); + + @supports (width: 100dvw) { + --page-layout-viewport-width: 100dvw; + } @media screen and (min-width: 768px) { --pane-width-small: 240px; @@ -602,11 +615,13 @@ width: 100%; @media screen and (min-width: 768px) { - /* - * --pane-max-width is set by JS on mount and updated on resize (debounced). - * JS calculates viewport - margin to avoid scrollbar discrepancy with 100vw. - */ - width: clamp(var(--pane-min-width), var(--pane-width), var(--pane-max-width)); + width: clamp(var(--pane-min-width), var(--pane-width), min(var(--pane-max-width), var(--pane-viewport-max))); + } + + &:where([data-constrain-to-viewport='false']) { + @media screen and (min-width: 768px) { + width: clamp(var(--pane-min-width), var(--pane-width), var(--pane-max-width)); + } } } } @@ -846,7 +861,17 @@ width: 100%; @media screen and (min-width: 768px) { - width: clamp(var(--pane-min-width), var(--pane-width), var(--pane-max-width)); + width: clamp( + var(--pane-min-width), + var(--pane-width), + min(var(--pane-max-width), var(--sidebar-viewport-max)) + ); + } + + &:where([data-constrain-to-viewport='false']) { + @media screen and (min-width: 768px) { + width: clamp(var(--pane-min-width), var(--pane-width), var(--pane-max-width)); + } } } diff --git a/packages/react/src/PageLayout/PageLayout.tsx b/packages/react/src/PageLayout/PageLayout.tsx index bd1ff29733a..b1f5a29b328 100644 --- a/packages/react/src/PageLayout/PageLayout.tsx +++ b/packages/react/src/PageLayout/PageLayout.tsx @@ -897,11 +897,12 @@ const Pane = React.forwardRef { }), ) - expect(result.current.currentWidth).toBe(400) + // The trailing mount sync now clamps restored widths to the current viewport max: + // 1280px viewport - 959px wide diff = 321px. + expect(result.current.currentWidth).toBe(321) }) it('should not restore from localStorage when not resizable', () => { @@ -654,7 +656,6 @@ describe('usePaneWidth', () => { }), ) - // Adds resize listener for throttled CSS updates and debounced state sync expect(addEventListenerSpy).toHaveBeenCalledWith('resize', expect.any(Function)) addEventListenerSpy.mockRestore() }) @@ -678,6 +679,25 @@ describe('usePaneWidth', () => { addEventListenerSpy.mockRestore() }) + it('should add resize listener for viewport-constrained custom widths', () => { + const addEventListenerSpy = vi.spyOn(window, 'addEventListener') + const refs = createMockRefs() + + renderHook(() => + usePaneWidth({ + width: {min: '150px', default: '300px', max: '700px'}, + minWidth: 256, + resizable: true, + constrainToViewport: true, + widthStorageKey: 'test-constrained-custom', + ...refs, + }), + ) + + expect(addEventListenerSpy).toHaveBeenCalledWith('resize', expect.any(Function)) + addEventListenerSpy.mockRestore() + }) + it('should clamp ref when viewport shrinks', async () => { vi.useFakeTimers() vi.stubGlobal('innerWidth', 1280) @@ -714,10 +734,11 @@ describe('usePaneWidth', () => { vi.useRealTimers() }) - it('should throttle CSS variable update', async () => { + it('should not write pane CSS variables during window resize', async () => { vi.useFakeTimers() vi.stubGlobal('innerWidth', 1280) const refs = createMockRefs() + const setPropertySpy = vi.spyOn(refs.paneRef.current!.style, 'setProperty') renderHook(() => usePaneWidth({ @@ -729,28 +750,21 @@ describe('usePaneWidth', () => { }), ) - // Initial --pane-max-width should be set on mount (1280 - 959 wide diff = 321) - expect(refs.paneRef.current?.style.getPropertyValue('--pane-max-width')).toBe('321px') + setPropertySpy.mockClear() - // Shrink viewport (crosses 1280 breakpoint, diff switches to 511) vi.stubGlobal('innerWidth', 1000) - - // Fire resize - with throttle, first update happens immediately (if THROTTLE_MS passed) window.dispatchEvent(new Event('resize')) - // Since Date.now() starts at 0 and lastUpdateTime is 0, first update should happen immediately - // but it's in rAF, so we need to advance through rAF await act(async () => { await vi.runAllTimersAsync() }) - // CSS variable should now be updated: 1000 - 511 = 489 - expect(refs.paneRef.current?.style.getPropertyValue('--pane-max-width')).toBe('489px') + expect(setPropertySpy).not.toHaveBeenCalled() vi.useRealTimers() }) - it('should update ARIA attributes after throttle', async () => { + it('should update ARIA attributes after resize debounce', async () => { vi.useFakeTimers() vi.stubGlobal('innerWidth', 1280) const refs = createMockRefs() @@ -765,32 +779,27 @@ describe('usePaneWidth', () => { }), ) - // Initial ARIA max should be set on mount (1280 - 959 wide diff = 321) expect(refs.handleRef.current?.getAttribute('aria-valuemax')).toBe('321') - // Shrink viewport (crosses 1280 breakpoint, diff switches to 511) vi.stubGlobal('innerWidth', 900) - - // Fire resize - with throttle, update happens via rAF window.dispatchEvent(new Event('resize')) - // Wait for rAF to complete + expect(refs.handleRef.current?.getAttribute('aria-valuemax')).toBe('321') + await act(async () => { await vi.runAllTimersAsync() }) - // ARIA should now be updated: 900 - 511 = 389 expect(refs.handleRef.current?.getAttribute('aria-valuemax')).toBe('389') vi.useRealTimers() }) - it('should throttle full sync on rapid resize', async () => { + it('should coalesce rapid resize events into one trailing sync', async () => { vi.useFakeTimers() vi.stubGlobal('innerWidth', 1280) const refs = createMockRefs() - - const setPropertySpy = vi.spyOn(refs.paneRef.current!.style, 'setProperty') + const setAttributeSpy = vi.spyOn(refs.handleRef.current!, 'setAttribute') renderHook(() => usePaneWidth({ @@ -802,43 +811,30 @@ describe('usePaneWidth', () => { }), ) - // Clear mount calls - setPropertySpy.mockClear() + setAttributeSpy.mockClear() + startTransitionSpy.mockClear() - // Fire resize events rapidly vi.stubGlobal('innerWidth', 1100) window.dispatchEvent(new Event('resize')) - - // With throttle, CSS should update immediately or via rAF - await act(async () => { - await vi.runAllTimersAsync() - }) - - // First update should have happened: 1100 - 511 = 589 - expect(setPropertySpy).toHaveBeenCalledWith('--pane-max-width', '589px') - - // Clear for next test - setPropertySpy.mockClear() - - // Fire more resize events rapidly (within throttle window) for (let i = 0; i < 3; i++) { vi.stubGlobal('innerWidth', 1000 - i * 50) window.dispatchEvent(new Event('resize')) } - // Should schedule via rAF + expect(setAttributeSpy).not.toHaveBeenCalled() + expect(startTransitionSpy).not.toHaveBeenCalled() + await act(async () => { await vi.runAllTimersAsync() }) - // Now CSS and ARIA should be synced with final viewport value (900) - expect(setPropertySpy).toHaveBeenCalledWith('--pane-max-width', '389px') // 900 - 511 + expect(startTransitionSpy).toHaveBeenCalledTimes(1) expect(refs.handleRef.current?.getAttribute('aria-valuemax')).toBe('389') vi.useRealTimers() }) - it('should update React state via startTransition after throttle', async () => { + it('should update React state via startTransition after resize debounce', async () => { vi.useFakeTimers() vi.stubGlobal('innerWidth', 1280) const refs = createMockRefs() @@ -853,19 +849,18 @@ describe('usePaneWidth', () => { }), ) - // Initial maxPaneWidth state (1280 - 959 wide diff = 321) + startTransitionSpy.mockClear() expect(result.current.maxPaneWidth).toBe(321) - // Shrink viewport (crosses 1280 breakpoint, diff switches to 511) vi.stubGlobal('innerWidth', 800) window.dispatchEvent(new Event('resize')) - // After throttle (via rAF), state updated via startTransition + expect(startTransitionSpy).not.toHaveBeenCalled() + await act(async () => { await vi.runAllTimersAsync() }) - // State now reflects new max: 800 - 511 = 289 expect(result.current.maxPaneWidth).toBe(289) vi.useRealTimers() @@ -942,7 +937,7 @@ describe('usePaneWidth', () => { addEventListenerSpy.mockRestore() }) - it('should apply and remove containment attributes during resize', async () => { + it('should not toggle containment attributes during window resize', async () => { vi.useFakeTimers() vi.stubGlobal('innerWidth', 1280) const refs = createMockRefs() @@ -957,65 +952,16 @@ describe('usePaneWidth', () => { }), ) - // Initially no data-dragging attribute expect(refs.paneRef.current?.hasAttribute('data-dragging')).toBe(false) expect(refs.contentWrapperRef.current?.hasAttribute('data-dragging')).toBe(false) - // Fire resize vi.stubGlobal('innerWidth', 1000) window.dispatchEvent(new Event('resize')) - // Attribute should be applied immediately on first resize - expect(refs.paneRef.current?.hasAttribute('data-dragging')).toBe(true) - expect(refs.contentWrapperRef.current?.hasAttribute('data-dragging')).toBe(true) - - // Fire another resize event immediately (simulating continuous resize) - vi.stubGlobal('innerWidth', 900) - window.dispatchEvent(new Event('resize')) - - // Attribute should still be present (containment stays on during continuous resize) - expect(refs.paneRef.current?.hasAttribute('data-dragging')).toBe(true) - expect(refs.contentWrapperRef.current?.hasAttribute('data-dragging')).toBe(true) - - // Wait for the debounce timeout (150ms) to complete after resize stops await act(async () => { - await vi.advanceTimersByTimeAsync(150) + await vi.runAllTimersAsync() }) - // Attribute should be removed after debounce completes - expect(refs.paneRef.current?.hasAttribute('data-dragging')).toBe(false) - expect(refs.contentWrapperRef.current?.hasAttribute('data-dragging')).toBe(false) - - vi.useRealTimers() - }) - - it('should cleanup containment attributes on unmount during resize', async () => { - vi.useFakeTimers() - vi.stubGlobal('innerWidth', 1280) - const refs = createMockRefs() - - const {unmount} = renderHook(() => - usePaneWidth({ - width: 'medium', - minWidth: 256, - resizable: true, - widthStorageKey: 'test-cleanup-containment', - ...refs, - }), - ) - - // Fire resize - vi.stubGlobal('innerWidth', 1000) - window.dispatchEvent(new Event('resize')) - - // Attribute should be applied - expect(refs.paneRef.current?.hasAttribute('data-dragging')).toBe(true) - expect(refs.contentWrapperRef.current?.hasAttribute('data-dragging')).toBe(true) - - // Unmount immediately (before debounce timer fires) - unmount() - - // Attribute should be cleaned up on unmount regardless of timing expect(refs.paneRef.current?.hasAttribute('data-dragging')).toBe(false) expect(refs.contentWrapperRef.current?.hasAttribute('data-dragging')).toBe(false) @@ -1076,6 +1022,27 @@ describe('usePaneWidth', () => { vi.stubGlobal('innerWidth', 500) expect(result.current.getMaxPaneWidth()).toBe(400) }) + + it('should cap viewport-constrained custom widths using the sidebar viewport diff', () => { + vi.stubGlobal('innerWidth', 600) + const refs = createMockRefs() + + const {result} = renderHook(() => + usePaneWidth({ + width: {min: '150px', default: '300px', max: '700px'}, + minWidth: 256, + resizable: true, + constrainToViewport: true, + widthStorageKey: 'test-sidebar-max', + ...refs, + }), + ) + + expect(result.current.getMaxPaneWidth()).toBe(344) + + vi.stubGlobal('innerWidth', 1200) + expect(result.current.getMaxPaneWidth()).toBe(700) + }) }) describe('resizable toggling', () => { @@ -1125,9 +1092,10 @@ describe('usePaneWidth', () => { rerender({resizable: false}) expect(result.current.currentWidth).toBe(400) - // Toggle back to resizable — picks up the preserved width + // Toggle back to resizable — the trailing sync re-applies the viewport clamp: + // 1280px viewport - 959px wide diff = 321px. rerender({resizable: true}) - expect(result.current.currentWidth).toBe(400) + expect(result.current.currentWidth).toBe(321) }) it('should preserve controlled width across resizable toggle', () => { diff --git a/packages/react/src/PageLayout/usePaneWidth.ts b/packages/react/src/PageLayout/usePaneWidth.ts index f0c12b57e11..e5914f9c95c 100644 --- a/packages/react/src/PageLayout/usePaneWidth.ts +++ b/packages/react/src/PageLayout/usePaneWidth.ts @@ -184,9 +184,7 @@ export function usePaneWidth({ minWidth, resizable, widthStorageKey, - paneRef, handleRef, - contentWrapperRef, constrainToViewport = false, onResizeEnd, currentWidth: controlledWidth, @@ -207,7 +205,9 @@ export function usePaneWidth({ }) // Cache the CSS variable value to avoid getComputedStyle during drag (causes layout thrashing) // Updated on mount and resize when breakpoints might change - const maxWidthDiffRef = React.useRef(constrainToViewport ? DEFAULT_SIDEBAR_MAX_WIDTH_DIFF : DEFAULT_MAX_WIDTH_DIFF) + const maxWidthDiffRef = React.useRef( + constrainToViewport ? DEFAULT_SIDEBAR_MAX_WIDTH_DIFF : getMaxWidthDiffFromViewport(), + ) // Calculate max width constraint - for custom widths this is capped to viewport bounds // when constrainToViewport is set (e.g., Sidebar), otherwise it uses the custom max directly. @@ -329,131 +329,54 @@ export function usePaneWidth({ getMaxPaneWidthRef.current = getMaxPaneWidth }) - // Update CSS variable, refs, and ARIA on mount and window resize. - // Strategy: Only sync when resize stops (debounced) to avoid layout thrashing on large DOMs + // Sync refs, ARIA, and React state on mount plus once after window resize ends. + // Visual clamping is handled by CSS so the resize listener can stay trailing-only. useIsomorphicLayoutEffect(() => { if (!resizable) return - let lastViewportWidth = window.innerWidth - - // Full sync of refs, ARIA, and state (debounced, runs when resize stops) - const syncAll = () => { - const currentViewportWidth = window.innerWidth - - // Only update the cached diff value if we crossed the breakpoint - const crossedBreakpoint = - (lastViewportWidth < DEFAULT_PANE_MAX_WIDTH_DIFF_BREAKPOINT && - currentViewportWidth >= DEFAULT_PANE_MAX_WIDTH_DIFF_BREAKPOINT) || - (lastViewportWidth >= DEFAULT_PANE_MAX_WIDTH_DIFF_BREAKPOINT && - currentViewportWidth < DEFAULT_PANE_MAX_WIDTH_DIFF_BREAKPOINT) - lastViewportWidth = currentViewportWidth - - if (crossedBreakpoint) { - maxWidthDiffRef.current = getMaxWidthDiffFromViewport() - } - + const syncOnce = () => { + maxWidthDiffRef.current = constrainToViewport ? DEFAULT_SIDEBAR_MAX_WIDTH_DIFF : getMaxWidthDiffFromViewport() const actualMax = getMaxPaneWidthRef.current() + const clamped = Math.min(currentWidthRef.current, actualMax) + const wasClamped = clamped !== currentWidthRef.current - // Update CSS variable for visual clamping (may already be set by throttled update) - paneRef.current?.style.setProperty('--pane-max-width', `${actualMax}px`) - - // Track if we clamped current width - const wasClamped = currentWidthRef.current > actualMax - if (wasClamped) { - currentWidthRef.current = actualMax - paneRef.current?.style.setProperty('--pane-width', `${actualMax}px`) - } - - // Update ARIA via DOM - cheap, no React re-render - updateAriaValues(handleRef.current, {max: actualMax, current: currentWidthRef.current}) + currentWidthRef.current = clamped + updateAriaValues(handleRef.current, {min: minPaneWidth, max: actualMax, current: clamped}) - // Only trigger React re-render if values actually changed. - // startTransition doesn't bail out on same-value updates like normal setState, - // so we guard explicitly to avoid unnecessary re-renders on every resize tick. (#7801) - const maxChanged = actualMax !== maxPaneWidthRef.current - if (maxChanged || wasClamped) { + if (actualMax !== maxPaneWidthRef.current || wasClamped) { maxPaneWidthRef.current = actualMax startTransition(() => { setMaxPaneWidth(actualMax) if (wasClamped) { - setCurrentWidthState(actualMax) + setCurrentWidthState(clamped) } }) } } - // Initial calculation on mount — use viewport-based lookup to avoid - // getComputedStyle which forces a synchronous layout recalc on the - // freshly-committed DOM tree (measured at ~614ms on large pages). - maxWidthDiffRef.current = getMaxWidthDiffFromViewport() - const initialMax = getMaxPaneWidthRef.current() - maxPaneWidthRef.current = initialMax - setMaxPaneWidth(initialMax) - paneRef.current?.style.setProperty('--pane-max-width', `${initialMax}px`) - updateAriaValues(handleRef.current, {min: minPaneWidth, max: initialMax, current: currentWidthRef.current}) + syncOnce() // For custom widths that aren't viewport-constrained, max is fixed - no need to listen to resize if (customMaxWidth !== null && !constrainToViewport) return - // Throttle approach for window resize - provides immediate visual feedback for small DOMs - // while still limiting update frequency - const THROTTLE_MS = 16 // ~60fps - const DEBOUNCE_MS = 150 // Delay before removing containment after resize stops - let lastUpdateTime = 0 - let pendingUpdate = false - let rafId: number | null = null + const DEBOUNCE_MS = 150 let debounceId: ReturnType | null = null - let isResizing = false - - const startResizeOptimizations = () => { - if (isResizing) return - isResizing = true - paneRef.current?.setAttribute('data-dragging', 'true') - contentWrapperRef.current?.setAttribute('data-dragging', 'true') - } - - const endResizeOptimizations = () => { - if (!isResizing) return - isResizing = false - paneRef.current?.removeAttribute('data-dragging') - contentWrapperRef.current?.removeAttribute('data-dragging') - } const handleResize = () => { - // Apply containment on first resize event (stays applied until resize stops) - startResizeOptimizations() - - const now = Date.now() - if (now - lastUpdateTime >= THROTTLE_MS) { - lastUpdateTime = now - syncAll() - } else if (!pendingUpdate) { - pendingUpdate = true - rafId = requestAnimationFrame(() => { - pendingUpdate = false - rafId = null - lastUpdateTime = Date.now() - syncAll() - }) - } - - // Debounce the cleanup — remove containment after resize stops if (debounceId !== null) clearTimeout(debounceId) debounceId = setTimeout(() => { debounceId = null - endResizeOptimizations() + syncOnce() }, DEBOUNCE_MS) } // eslint-disable-next-line github/prefer-observers -- Uses window resize events instead of ResizeObserver to avoid INP issues. ResizeObserver on document.documentElement fires on any content change (typing, etc), while window resize only fires on actual viewport changes. window.addEventListener('resize', handleResize) return () => { - if (rafId !== null) cancelAnimationFrame(rafId) if (debounceId !== null) clearTimeout(debounceId) - endResizeOptimizations() window.removeEventListener('resize', handleResize) } - }, [resizable, customMaxWidth, constrainToViewport, minPaneWidth, paneRef, handleRef, contentWrapperRef]) + }, [resizable, customMaxWidth, constrainToViewport, minPaneWidth, handleRef]) return { currentWidth,