diff --git a/packages/react/src/PageLayout/PageLayout.tsx b/packages/react/src/PageLayout/PageLayout.tsx index f2ff4f4bd89..36594e99355 100644 --- a/packages/react/src/PageLayout/PageLayout.tsx +++ b/packages/react/src/PageLayout/PageLayout.tsx @@ -30,8 +30,9 @@ const REGION_ORDER = { footer: 4, } -const ARROW_KEYS = new Set(['ArrowLeft', 'ArrowRight', 'ArrowUp', 'ArrowDown']) -const SHRINK_KEYS = new Set(['ArrowLeft', 'ArrowDown']) +const isArrowKey = (key: string) => + key === 'ArrowLeft' || key === 'ArrowRight' || key === 'ArrowUp' || key === 'ArrowDown' +const isShrinkKey = (key: string) => key === 'ArrowLeft' || key === 'ArrowDown' // eslint-disable-next-line @typescript-eslint/no-unused-vars const SPACING_MAP = { @@ -120,7 +121,7 @@ const Root: React.FC> = ({ ) } -const RootWrapper = memo(function RootWrapper({ +function RootWrapper({ style, padding, children, @@ -139,7 +140,7 @@ const RootWrapper = memo(function RootWrapper({ {children} ) -}) +} Root.displayName = 'PageLayout' @@ -349,11 +350,11 @@ const DragHandle = memo(function DragHandle({ */ const handleKeyDown = React.useCallback( (event: React.KeyboardEvent) => { - if (!ARROW_KEYS.has(event.key)) return + if (!isArrowKey(event.key)) return event.preventDefault() // https://github.com/github/accessibility/issues/5101#issuecomment-1822870655 - const delta = SHRINK_KEYS.has(event.key) ? -ARROW_KEY_STEP : ARROW_KEY_STEP + const delta = isShrinkKey(event.key) ? -ARROW_KEY_STEP : ARROW_KEY_STEP // Only set dragging on first keydown (not repeats) if (!isDraggingRef.current) { @@ -366,7 +367,7 @@ const DragHandle = memo(function DragHandle({ const handleKeyUp = React.useCallback( (event: React.KeyboardEvent) => { - if (!ARROW_KEYS.has(event.key)) return + if (!isArrowKey(event.key)) return event.preventDefault() endDragging() stableOnDragEnd.current() @@ -374,6 +375,16 @@ const DragHandle = memo(function DragHandle({ [endDragging], ) + // Cleanup rAF on unmount to prevent stale callbacks + React.useEffect(() => { + return () => { + if (rafIdRef.current !== null) { + cancelAnimationFrame(rafIdRef.current) + rafIdRef.current = null + } + } + }, []) + return (
> = memo(function Header({ +const Header: FCWithSlotMarker> = function Header({ 'aria-label': label, 'aria-labelledby': labelledBy, padding = 'none', @@ -485,8 +496,7 @@ const Header: FCWithSlotMarker> = /> ) -}) - +} Header.displayName = 'PageLayout.Header' // ---------------------------------------------------------------------------- @@ -524,7 +534,7 @@ const contentWidths = { xlarge: '1280px', } -const Content: FCWithSlotMarker> = memo(function Content({ +const Content: FCWithSlotMarker> = function Content({ as = 'main', 'aria-label': label, 'aria-labelledby': labelledBy, @@ -558,8 +568,7 @@ const Content: FCWithSlotMarker>
) -}) - +} Content.displayName = 'PageLayout.Content' // ---------------------------------------------------------------------------- @@ -888,7 +897,7 @@ export type PageLayoutFooterProps = { style?: React.CSSProperties } -const Footer: FCWithSlotMarker> = memo(function Footer({ +const Footer: FCWithSlotMarker> = function Footer({ 'aria-label': label, 'aria-labelledby': labelledBy, padding = 'none', @@ -941,8 +950,7 @@ const Footer: FCWithSlotMarker> = ) -}) - +} Footer.displayName = 'PageLayout.Footer' // ---------------------------------------------------------------------------- diff --git a/packages/react/src/PageLayout/usePaneWidth.test.ts b/packages/react/src/PageLayout/usePaneWidth.test.ts index e906226d080..801ed59d842 100644 --- a/packages/react/src/PageLayout/usePaneWidth.test.ts +++ b/packages/react/src/PageLayout/usePaneWidth.test.ts @@ -400,10 +400,10 @@ describe('usePaneWidth', () => { // Shrink viewport vi.stubGlobal('innerWidth', 800) - // Wrap resize + debounce in act() since it triggers startTransition state update + // Wrap resize + throttle in act() since it triggers startTransition state update await act(async () => { window.dispatchEvent(new Event('resize')) - await vi.advanceTimersByTimeAsync(150) + await vi.runAllTimersAsync() }) // getMaxPaneWidth now returns 800 - 511 = 289 @@ -414,7 +414,7 @@ describe('usePaneWidth', () => { vi.useRealTimers() }) - it('should debounce CSS variable update (no throttle)', async () => { + it('should throttle CSS variable update', async () => { vi.useFakeTimers() vi.stubGlobal('innerWidth', 1280) const refs = createMockRefs() @@ -424,7 +424,7 @@ describe('usePaneWidth', () => { width: 'medium', minWidth: 256, resizable: true, - widthStorageKey: 'test-css-debounce', + widthStorageKey: 'test-css-throttle', ...refs, }), ) @@ -435,15 +435,13 @@ describe('usePaneWidth', () => { // Shrink viewport vi.stubGlobal('innerWidth', 1000) - // Fire resize - CSS should NOT update immediately (debounce only, no throttle) + // Fire resize - with throttle, first update happens immediately (if THROTTLE_MS passed) window.dispatchEvent(new Event('resize')) - // CSS variable should still be old value (debounced) - expect(refs.paneRef.current?.style.getPropertyValue('--pane-max-width')).toBe('769px') - - // Wait for debounce + // 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.advanceTimersByTimeAsync(150) + await vi.runAllTimersAsync() }) // CSS variable should now be updated: 1000 - 511 = 489 @@ -452,7 +450,7 @@ describe('usePaneWidth', () => { vi.useRealTimers() }) - it('should update ARIA attributes after debounce', async () => { + it('should update ARIA attributes after throttle', async () => { vi.useFakeTimers() vi.stubGlobal('innerWidth', 1280) const refs = createMockRefs() @@ -462,7 +460,7 @@ describe('usePaneWidth', () => { width: 'medium', minWidth: 256, resizable: true, - widthStorageKey: 'test-aria-debounce', + widthStorageKey: 'test-aria-throttle', ...refs, }), ) @@ -473,16 +471,12 @@ describe('usePaneWidth', () => { // Shrink viewport vi.stubGlobal('innerWidth', 900) - // Fire resize but don't wait for debounce + // Fire resize - with throttle, update happens via rAF window.dispatchEvent(new Event('resize')) - await vi.advanceTimersByTimeAsync(50) - // ARIA should NOT be updated yet - expect(refs.handleRef.current?.getAttribute('aria-valuemax')).toBe('769') - - // Wait for debounce + // Wait for rAF to complete await act(async () => { - await vi.advanceTimersByTimeAsync(100) + await vi.runAllTimersAsync() }) // ARIA should now be updated: 900 - 511 = 389 @@ -491,7 +485,7 @@ describe('usePaneWidth', () => { vi.useRealTimers() }) - it('should debounce full sync on rapid resize (no throttle)', async () => { + it('should throttle full sync on rapid resize', async () => { vi.useFakeTimers() vi.stubGlobal('innerWidth', 1280) const refs = createMockRefs() @@ -503,7 +497,7 @@ describe('usePaneWidth', () => { width: 'medium', minWidth: 256, resizable: true, - widthStorageKey: 'test-debounce-only', + widthStorageKey: 'test-throttle', ...refs, }), ) @@ -515,35 +509,36 @@ describe('usePaneWidth', () => { vi.stubGlobal('innerWidth', 1100) window.dispatchEvent(new Event('resize')) - // CSS should NOT update immediately (debounce only, no throttle) - expect(setPropertySpy).not.toHaveBeenCalledWith('--pane-max-width', '589px') + // 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 + // 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')) } - // Advance a bit but not past debounce - no CSS updates yet - await vi.advanceTimersByTimeAsync(50) - expect(setPropertySpy).not.toHaveBeenCalledWith('--pane-max-width', expect.any(String)) - - // ARIA should not be updated yet (debounced) - expect(refs.handleRef.current?.getAttribute('aria-valuemax')).toBe('769') // Still initial - - // Wait for debounce to complete + // Should schedule via rAF await act(async () => { - await vi.advanceTimersByTimeAsync(150) + await vi.runAllTimersAsync() }) - // Now CSS, ARIA and refs are synced with final viewport value (900) + // Now CSS and ARIA should be synced with final viewport value (900) expect(setPropertySpy).toHaveBeenCalledWith('--pane-max-width', '389px') // 900 - 511 expect(refs.handleRef.current?.getAttribute('aria-valuemax')).toBe('389') vi.useRealTimers() }) - it('should update React state via startTransition after debounce', async () => { + it('should update React state via startTransition after throttle', async () => { vi.useFakeTimers() vi.stubGlobal('innerWidth', 1280) const refs = createMockRefs() @@ -565,13 +560,9 @@ describe('usePaneWidth', () => { vi.stubGlobal('innerWidth', 800) window.dispatchEvent(new Event('resize')) - // Before debounce completes, state unchanged - await vi.advanceTimersByTimeAsync(50) - expect(result.current.maxPaneWidth).toBe(769) - - // After debounce, state updated via startTransition + // After throttle (via rAF), state updated via startTransition await act(async () => { - await vi.advanceTimersByTimeAsync(100) + await vi.runAllTimersAsync() }) // State now reflects new max: 800 - 511 = 289 @@ -621,7 +612,7 @@ describe('usePaneWidth', () => { addEventListenerSpy.mockRestore() }) - it('should apply containment styles during resize', async () => { + it('should apply and remove containment styles during resize', async () => { vi.useFakeTimers() vi.stubGlobal('innerWidth', 1280) const refs = createMockRefs() @@ -644,18 +635,15 @@ describe('usePaneWidth', () => { vi.stubGlobal('innerWidth', 1000) window.dispatchEvent(new Event('resize')) - // Containment should be applied immediately - expect(refs.paneRef.current?.style.contain).toBe('layout style paint') - expect(refs.paneRef.current?.style.contentVisibility).toBe('auto') - expect(refs.contentRef.current?.style.contain).toBe('layout style paint') - expect(refs.contentRef.current?.style.contentVisibility).toBe('auto') + // At this point, containment is applied but timing depends on throttle behavior + // The key is that it gets cleaned up after - // Wait for debounce to complete + // Wait for throttle to complete via rAF await act(async () => { - await vi.advanceTimersByTimeAsync(150) + await vi.runAllTimersAsync() }) - // Containment should be removed after debounce + // Containment should be removed after throttle completes expect(refs.paneRef.current?.style.contain).toBe('') expect(refs.paneRef.current?.style.contentVisibility).toBe('') expect(refs.contentRef.current?.style.contain).toBe('') @@ -679,19 +667,18 @@ describe('usePaneWidth', () => { }), ) - // Fire resize to apply containment + // Fire resize vi.stubGlobal('innerWidth', 1000) window.dispatchEvent(new Event('resize')) - // Verify containment is applied - expect(refs.paneRef.current?.style.contain).toBe('layout style paint') - - // Unmount before debounce completes + // Unmount immediately (may or may not have styles depending on throttle timing) unmount() - // Containment should be cleaned up + // Containment should be cleaned up on unmount regardless of timing expect(refs.paneRef.current?.style.contain).toBe('') + expect(refs.paneRef.current?.style.contentVisibility).toBe('') expect(refs.contentRef.current?.style.contain).toBe('') + expect(refs.contentRef.current?.style.contentVisibility).toBe('') vi.useRealTimers() }) diff --git a/packages/react/src/PageLayout/usePaneWidth.ts b/packages/react/src/PageLayout/usePaneWidth.ts index 17dc4f331d5..0b1dd7431d2 100644 --- a/packages/react/src/PageLayout/usePaneWidth.ts +++ b/packages/react/src/PageLayout/usePaneWidth.ts @@ -78,9 +78,8 @@ export const isCustomWidthOptions = (width: PaneWidth | CustomWidthOptions): wid return (width as CustomWidthOptions).default !== undefined } -const PANE_WIDTHS = new Set(['small', 'medium', 'large']) export const isPaneWidth = (width: PaneWidth | CustomWidthOptions): width is PaneWidth => { - return PANE_WIDTHS.has(width as PaneWidth) + return width === 'small' || width === 'medium' || width === 'large' } export const getDefaultPaneWidth = (w: PaneWidth | CustomWidthOptions): number => { @@ -268,11 +267,12 @@ export function usePaneWidth({ // For custom widths, max is fixed - no need to listen to resize if (customMaxWidth !== null) return - // Only sync when resize stops (debounced) - no updates during continuous resize - // CSS handles visual clamping naturally via viewport-relative max values - // Updating --pane-max-width during resize causes expensive layout recalcs on large DOMs - const DEBOUNCE_MS = 150 - let debounceId: ReturnType | null = null + // Throttle approach for window resize - provides immediate visual feedback for small DOMs + // while still limiting update frequency + const THROTTLE_MS = 16 // ~60fps + let lastUpdateTime = 0 + let pendingUpdate = false + let rafId: number | null = null let isResizing = false // Apply containment during resize to reduce layout thrashing on large DOMs @@ -292,24 +292,29 @@ export function usePaneWidth({ } const handleResize = () => { - // Apply containment at start of resize startResizeOptimizations() - // Debounced full sync (refs, ARIA, state) when resize stops - if (debounceId !== null) { - clearTimeout(debounceId) - } - debounceId = setTimeout(() => { - debounceId = null - endResizeOptimizations() + const now = Date.now() + if (now - lastUpdateTime >= THROTTLE_MS) { + lastUpdateTime = now syncAll() - }, DEBOUNCE_MS) + endResizeOptimizations() + } else if (!pendingUpdate) { + pendingUpdate = true + rafId = requestAnimationFrame(() => { + pendingUpdate = false + rafId = null + lastUpdateTime = Date.now() + syncAll() + endResizeOptimizations() + }) + } } // 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 (debounceId !== null) clearTimeout(debounceId) + if (rafId !== null) cancelAnimationFrame(rafId) endResizeOptimizations() window.removeEventListener('resize', handleResize) }