diff --git a/packages/components/src/navigation/Stack.test.tsx b/packages/components/src/navigation/Stack.test.tsx index 7b3a76aeca..4fc32c4e75 100644 --- a/packages/components/src/navigation/Stack.test.tsx +++ b/packages/components/src/navigation/Stack.test.tsx @@ -42,7 +42,7 @@ beforeAll(() => { describe('stack push and pop tests', () => { let texts: string[]; - let stackItems: React.ReactNode[]; + let stackItems: React.ReactElement[]; let rerender: (ui: React.ReactElement) => void; function renderStack(stackCount: number, itemCount = 5) { @@ -130,3 +130,170 @@ describe('stack push and pop tests', () => { expectStack([0]); }); }); + +describe('key-based view tracking', () => { + function runTimers() { + act(() => { + jest.runOnlyPendingTimers(); + }); + } + + /** Builds a stack item identified by `key` but with arbitrary content. */ + function keyedItem(key: string, content: string) { + return ( +
+ {content} +
+ ); + } + + it('keeps content live when re-rendered with new props for the same key', () => { + // The element instance changes but the key does not - this mimics a parent + // (e.g. IrisGrid) re-rendering with fresh children for the same logical + // view. The rendered view should reflect the latest props, and no slide + // animation should be triggered. + const { rerender } = render({[keyedItem('a', 'v1')]}); + expect(screen.getByText('v1')).toBeTruthy(); + + rerender({[keyedItem('a', 'v2')]}); + + expect(screen.getByText('v2')).toBeTruthy(); + expect(screen.queryByText('v1')).toBeNull(); + // No push/pop animation should have started. + expect(document.querySelector('.pushing-view')).toBeNull(); + expect(document.querySelector('.popping-view')).toBeNull(); + }); + + it('does not churn or remount when repeatedly re-rendered with new instances for the same keys', () => { + // Regression: a host (e.g. IrisGrid) re-renders frequently, handing the + // stack brand-new element objects that represent the same logical views. + // Tracking by `key` means these re-renders must not trigger a push/pop + // animation, and the mounted view must stay mounted (same DOM node). + const { rerender } = render( + {[keyedItem('a', 'A'), keyedItem('b', 'B')]} + ); + const mountedView = screen.getByText('B'); + + for (let i = 0; i < 5; i += 1) { + // Fresh element instances, identical keys. + rerender({[keyedItem('a', 'A'), keyedItem('b', 'B')]}); + + // The same DOM node remains mounted - it was never unmounted/remounted. + expect(screen.getByText('B')).toBe(mountedView); + // No slide animation was triggered by the re-render. + expect(document.querySelector('.pushing-view')).toBeNull(); + expect(document.querySelector('.popping-view')).toBeNull(); + } + + // Running any timers must not flush a pending animation - there is none. + runTimers(); + expect(screen.getByText('B')).toBe(mountedView); + expect(screen.queryByText('A')).toBeNull(); + }); + + it('does not retrigger the push animation when re-rendered mid-push with the same top key', () => { + const { rerender } = render({[keyedItem('a', 'A')]}); + rerender({[keyedItem('a', 'A'), keyedItem('b', 'B')]}); + // Mid-push: main 'A' plus pushing 'B'. + expect(screen.getByText('A')).toBeTruthy(); + expect(screen.getByText('B')).toBeTruthy(); + + // Re-render with brand-new element instances for the same keys/length. + rerender({[keyedItem('a', 'A'), keyedItem('b', 'B')]}); + // Still mid-push to the same view - nothing changes. + expect(screen.getByText('A')).toBeTruthy(); + expect(screen.getByText('B')).toBeTruthy(); + + runTimers(); + // Push completes - 'B' becomes the main view. + expect(screen.getByText('B')).toBeTruthy(); + expect(screen.queryByText('A')).toBeNull(); + }); + + it('redirects the push to a new view when a different view is pushed mid-push', () => { + const { rerender } = render({[keyedItem('a', 'A')]}); + rerender({[keyedItem('a', 'A'), keyedItem('b', 'B')]}); + expect(screen.getByText('B')).toBeTruthy(); + + // Push a third view before the first push completes. + rerender( + + {[keyedItem('a', 'A'), keyedItem('b', 'B'), keyedItem('c', 'C')]} + + ); + // The push now targets 'C' instead of 'B'. + expect(screen.getByText('A')).toBeTruthy(); + expect(screen.getByText('C')).toBeTruthy(); + expect(screen.queryByText('B')).toBeNull(); + + runTimers(); + expect(screen.getByText('C')).toBeTruthy(); + }); + + it('redirects the main view when the top changes mid-pop', () => { + const { rerender } = render( + {[keyedItem('a', 'A'), keyedItem('b', 'B')]} + ); + expect(screen.getByText('B')).toBeTruthy(); + + // Pop down to 'A' - starts the pop animation for 'B'. + rerender({[keyedItem('a', 'A')]}); + expect(screen.getByText('A')).toBeTruthy(); + expect(screen.getByText('B')).toBeTruthy(); + + // While 'B' is still popping out, swap the remaining view to 'C'. + rerender({[keyedItem('c', 'C')]}); + expect(screen.getByText('C')).toBeTruthy(); + expect(screen.queryByText('A')).toBeNull(); + // 'B' is still animating out. + expect(screen.getByText('B')).toBeTruthy(); + + runTimers(); + expect(screen.getByText('C')).toBeTruthy(); + expect(screen.queryByText('B')).toBeNull(); + }); + + it('does not churn the main view when re-rendered mid-pop with the same remaining view', () => { + const { rerender } = render( + {[keyedItem('a', 'A'), keyedItem('b', 'B')]} + ); + + // Pop down to 'A' - starts the pop animation for 'B'. + rerender({[keyedItem('a', 'A')]}); + expect(screen.getByText('A')).toBeTruthy(); + expect(screen.getByText('B')).toBeTruthy(); + + // Re-render mid-pop with a new element instance for the same key 'A'. + // The top key is unchanged, so the main view is left untouched. + rerender({[keyedItem('a', 'A2')]}); + expect(screen.getByText('A2')).toBeTruthy(); + // 'B' is still animating out. + expect(screen.getByText('B')).toBeTruthy(); + + runTimers(); + expect(screen.getByText('A2')).toBeTruthy(); + expect(screen.queryByText('B')).toBeNull(); + }); + + it('swaps the main view without animating when the top key changes at the same depth', () => { + // Same stack length but a different top key - the main view is replaced + // directly, with no push/pop animation. + const { rerender } = render({[keyedItem('a', 'A')]}); + expect(screen.getByText('A')).toBeTruthy(); + + rerender({[keyedItem('b', 'B')]}); + expect(screen.getByText('B')).toBeTruthy(); + expect(screen.queryByText('A')).toBeNull(); + expect(document.querySelector('.pushing-view')).toBeNull(); + expect(document.querySelector('.popping-view')).toBeNull(); + }); + + it('renders nothing when there are no children', () => { + const { container } = render({[]}); + const mainView = container.querySelector('.main-view'); + expect(mainView).not.toBeNull(); + expect(mainView).toBeEmptyDOMElement(); + expect(document.querySelector('.pushing-view')).toBeNull(); + expect(document.querySelector('.popping-view')).toBeNull(); + }); +}); diff --git a/packages/components/src/navigation/Stack.tsx b/packages/components/src/navigation/Stack.tsx index 40bfc8b458..a423199511 100644 --- a/packages/components/src/navigation/Stack.tsx +++ b/packages/components/src/navigation/Stack.tsx @@ -4,10 +4,52 @@ import './Stack.scss'; import { SlideTransition } from '../transitions'; export type StackProps = { - children: React.ReactNode | React.ReactNode[]; + /** + * Stack views to animate between. Each child must be a React element with a + * stable `key`; views are tracked by that key across re-renders (primitive + * nodes such as strings/numbers are not supported). + */ + children: React.ReactElement | React.ReactElement[]; 'data-testid'?: string; }; +/** + * Returns the React `key` for an element, or `null` for non-elements. + * + * The parent that renders the stack may re-render frequently (e.g. an + * `IrisGrid` re-renders on every table tick) and hand us brand-new element + * objects that represent the same logical view. Tracking the animating views + * by `key` (rather than by element reference) means those re-renders neither + * retrigger the slide animation nor churn state every frame. + * + * Children are always run through `React.Children.toArray`, which assigns a + * stable key to every element, so element views always have a key. + */ +function getViewKey(node: React.ReactNode): React.Key | null { + return React.isValidElement(node) ? node.key : null; +} + +/** + * Finds the child that currently represents the view identified by `key`. + * + * Returning the *current* element (instead of a stored copy) keeps the rendered + * view's content live: when a parent re-renders with a new element instance for + * the same logical view, the view's latest props are forwarded to the mounted + * component. This matters for prop-driven panels such as the "Organize Columns" + * (`VisibilityOrderingBuilder`) sidebar, whose list contents and Undo/Redo state + * arrive entirely via props from the grid - freezing those props would stop the + * panel from reflecting moves, hides, groupings, and undo/redo. + */ +function findViewByKey( + views: readonly React.ReactNode[], + key: React.Key | null +): React.ReactNode { + if (key == null) { + return null; + } + return views.find(view => getViewKey(view) === key) ?? null; +} + /** * Pass a full navigation stack of children, and then automatically does a sliding animation when the stack changes. * Adding items to the stack will do a "push" animation, and removing items from the stack will do a "pop" animation. @@ -21,11 +63,21 @@ export function Stack({ [children] ); const prevChildrenArray = usePrevious(childrenArray); - const [mainView, setMainView] = useState( - childrenArray[childrenArray.length - 1] + + // The animating views are tracked by `key`, not element identity, so that + // frequent parent re-renders (which produce new-but-equivalent elements) + // neither retrigger animations nor freeze content. The element actually + // rendered is always looked up from the latest `childrenArray` via + // `findViewByKey`, keeping each view's props live. + const [mainViewKey, setMainViewKey] = useState(() => + getViewKey(childrenArray[childrenArray.length - 1]) ); - const [pushingView, setPushingView] = useState(null); + const [pushingViewKey, setPushingViewKey] = useState(null); + + // The popping view has already been removed from `children`, so we keep the + // element instance itself - it is only animating out and its content does not + // need to stay live. const [poppingView, setPoppingView] = useState(null); /** @@ -36,6 +88,10 @@ export function Stack({ * * When the `pushingView` or `poppingView` is set, that will kick off their animation. * Completion of the animation is handled in `pushComplete` or `popComplete`, and then the stack is in an idle state again. + * + * Views are compared by `key` (see `getViewKey`) so that a parent that + * re-renders with new element instances for the same logical view does not + * retrigger animations or churn state on every render. */ useEffect( function initAnimation() { @@ -45,42 +101,51 @@ export function Stack({ ) { return; } - const topChild = childrenArray[childrenArray.length - 1]; - if ( + const topChildKey = getViewKey(childrenArray[childrenArray.length - 1]); + + if (pushingViewKey !== null || poppingView !== null) { + // We're mid-animation. Keep the animating view pointed at the current + // top view, but don't start a new animation. + if (pushingViewKey !== null) { + if (topChildKey !== pushingViewKey) { + // A different view was pushed mid-animation - animate to it instead + setPushingViewKey(topChildKey); + } + } else if (topChildKey !== mainViewKey) { + setMainViewKey(topChildKey); + } + } else if ( childrenArray.length === prevChildrenArray.length || - prevChildrenArray.length === 0 || - pushingView !== null || - poppingView !== null + prevChildrenArray.length === 0 ) { - // 1) Stack is the same size, we've just mounted, or we're already in an animation - just update the view - if (pushingView !== null && topChild !== pushingView) { - // Stack was updated mid animation - setPushingView(topChild); - } else if (topChild !== poppingView && topChild !== mainView) { - // Replace the current view - setMainView(topChild); + // Stack is the same size or we've just mounted - just update the view + if (topChildKey !== mainViewKey) { + setMainViewKey(topChildKey); } } else if (childrenArray.length > prevChildrenArray.length) { - // 2) Stack has grown - start the push animation - setPushingView(topChild); - } else if (childrenArray.length < prevChildrenArray.length) { - // 3) Stack has shrunk - start the pop animation - setMainView(topChild); + // Stack has grown - start the push animation + setPushingViewKey(topChildKey); + } else { + // Stack has shrunk - start the pop animation + setMainViewKey(topChildKey); setPoppingView(prevChildrenArray[prevChildrenArray.length - 1]); } }, - [childrenArray, prevChildrenArray, pushingView, poppingView, mainView] + [childrenArray, prevChildrenArray, pushingViewKey, poppingView, mainViewKey] ); const pushComplete = useCallback(() => { - setMainView(pushingView); - setPushingView(null); - }, [pushingView]); + setMainViewKey(pushingViewKey); + setPushingViewKey(null); + }, [pushingViewKey]); const popComplete = useCallback(() => { setPoppingView(null); }, []); + const mainView = findViewByKey(childrenArray, mainViewKey); + const pushingView = findViewByKey(childrenArray, pushingViewKey); + return (
diff --git a/packages/components/src/transitions/SlideTransition.test.tsx b/packages/components/src/transitions/SlideTransition.test.tsx new file mode 100644 index 0000000000..573675097b --- /dev/null +++ b/packages/components/src/transitions/SlideTransition.test.tsx @@ -0,0 +1,85 @@ +import React from 'react'; +import { act, render } from '@testing-library/react'; +import SlideTransition from './SlideTransition'; + +beforeEach(() => { + jest.useFakeTimers(); +}); + +afterEach(() => { + act(() => { + jest.runOnlyPendingTimers(); + }); + jest.useRealTimers(); +}); + +it('renders its children', () => { + const { getByTestId } = render( + +
content
+
+ ); + + expect(getByTestId('child')).toBeTruthy(); +}); + +it('points the transition ref at the first child element, applying slide classes to it', () => { + // The child only exists while `in` is true, mimicking how `Stack` renders the + // popping/pushing view conditionally. Re-reading `firstElementChild` when + // `in` toggles is what lets CSSTransition animate the child once it appears. + function Wrapper({ on }: { on: boolean }): JSX.Element { + return ( + + {/* eslint-disable-next-line react/jsx-no-useless-fragment */} + <>{on &&
content
} +
+ ); + } + + const { rerender, queryByTestId, getByTestId } = render( + + ); + // Child is not rendered while `in` is false. + expect(queryByTestId('child')).toBeNull(); + + act(() => { + rerender(); + }); + + // The newly-appeared child receives the slide transition classes, proving the + // ref was re-attached to `firstElementChild` when `in` toggled. + const child = getByTestId('child'); + expect(child.className).toContain('slide-left'); +}); + +it('applies the slide-right class when direction is right', () => { + // CSSTransition only adds enter classes when `in` transitions to true, so we + // toggle `in` to trigger the animation rather than mounting with `in` true. + function Wrapper({ on }: { on: boolean }): JSX.Element { + return ( + + {/* eslint-disable-next-line react/jsx-no-useless-fragment */} + <>{on &&
content
} +
+ ); + } + + const { rerender, getByTestId } = render(); + + act(() => { + rerender(); + }); + + expect(getByTestId('child').className).toContain('slide-right'); +}); + +it('unmounts cleanly, detaching the ref', () => { + const { unmount } = render( + +
content
+
+ ); + + // Unmounting invokes the ref callback with `null`. + expect(() => unmount()).not.toThrow(); +}); diff --git a/packages/components/src/transitions/SlideTransition.tsx b/packages/components/src/transitions/SlideTransition.tsx index 9d2e526893..4869ed1c01 100644 --- a/packages/components/src/transitions/SlideTransition.tsx +++ b/packages/components/src/transitions/SlideTransition.tsx @@ -45,11 +45,19 @@ function SlideTransition({ }: SlideTransitionProps): JSX.Element { const nodeRef = useRef(null); - // Mimics findDOMNode for CSSTransition - // The ref should be set before CSSTransition does anything with it - const setRef = useCallback((node: HTMLElement | null) => { - nodeRef.current = node?.firstElementChild as HTMLElement; - }, []); + // Mimics findDOMNode for CSSTransition. + // Keying on `in` rather than `children` avoids re-creating the ref callback + // on every render, which would needlessly detach/re-attach the ref. + const setRef = useCallback( + (node: HTMLElement | null) => { + nodeRef.current = (node?.firstElementChild as HTMLElement | null) ?? null; + }, + // `inProp` is intentionally a dependency: toggling `in` must re-create the + // callback so the ref re-attaches and re-reads `firstElementChild` when the + // child appears/disappears. + // eslint-disable-next-line react-hooks/exhaustive-deps + [props.in] + ); return ( { >
- - this.handleMenuSelect(optionItems[i])} - items={optionItems} - /> - - {openOptionsStack.map((option, i) => ( + {[ - {option} - - ))} + this.handleMenuSelect(optionItems[i])} + items={optionItems} + /> + , + ...openOptionsStack.map((option, i) => ( + + {option} + + )), + ]}