diff --git a/packages/eui/.vrt/reference/layout-euiaccordion--strict-mode-initial-open-desktop.png b/packages/eui/.vrt/reference/layout-euiaccordion--strict-mode-initial-open-desktop.png new file mode 100644 index 000000000000..a8e0f23831c8 Binary files /dev/null and b/packages/eui/.vrt/reference/layout-euiaccordion--strict-mode-initial-open-desktop.png differ diff --git a/packages/eui/.vrt/reference/layout-euiaccordion--strict-mode-initial-open-mobile.png b/packages/eui/.vrt/reference/layout-euiaccordion--strict-mode-initial-open-mobile.png new file mode 100644 index 000000000000..2b5a1d62af92 Binary files /dev/null and b/packages/eui/.vrt/reference/layout-euiaccordion--strict-mode-initial-open-mobile.png differ diff --git a/packages/eui/changelogs/upcoming/9029.md b/packages/eui/changelogs/upcoming/9029.md new file mode 100644 index 000000000000..c40a14b6d8a4 --- /dev/null +++ b/packages/eui/changelogs/upcoming/9029.md @@ -0,0 +1,3 @@ +**Bug fixes** + +- Fixed `EuiAccordion` rendering with zero height under React Strict Mode (including when mounted inside `EuiFlyout` with `initialIsOpen`) diff --git a/packages/eui/src/components/accordion/__snapshots__/accordion.test.tsx.snap b/packages/eui/src/components/accordion/__snapshots__/accordion.test.tsx.snap index fd48669ab8f3..44fa34a735c1 100644 --- a/packages/eui/src/components/accordion/__snapshots__/accordion.test.tsx.snap +++ b/packages/eui/src/components/accordion/__snapshots__/accordion.test.tsx.snap @@ -796,7 +796,6 @@ exports[`EuiAccordion props forceState open is rendered 1`] = ` class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isOpen" id="17" role="group" - style="block-size: 0;" >
= { title: 'Layout/EuiAccordion', @@ -41,3 +44,61 @@ export const Playground: Story = { children: 'Accordion content', }, }; + +export const StrictModeInitialOpen: Story = { + args: { + id: 'accordion-strict-mode', + initialIsOpen: true, + buttonContent: 'Open on load', + children: ( +
({ + padding: 16, + background: euiTheme.colors.backgroundBaseSubdued, + })} + > + This content should be visible immediately. +
+ ), + }, + decorators: [ + (Story) => ( + + + + ), + ], +}; + +/** + * Reproduction for https://github.com/elastic/eui/issues/9029 + */ +export const StrictModeInFlyout: Story = { + render: () => ( + + {}} aria-labelledby="flyout-9029-title"> + + +

Flyout with accordion

+ + + + +
({ + padding: 16, + background: euiTheme.colors.backgroundBaseSubdued, + })} + > + Accordion content inside flyout (issue #9029) +
+ + + + + ), +}; diff --git a/packages/eui/src/components/accordion/accordion_children/accordion_children.tsx b/packages/eui/src/components/accordion/accordion_children/accordion_children.tsx index 86ae5dd2c09f..39863d7fa113 100644 --- a/packages/eui/src/components/accordion/accordion_children/accordion_children.tsx +++ b/packages/eui/src/components/accordion/accordion_children/accordion_children.tsx @@ -93,10 +93,19 @@ export const EuiAccordionChildren: FunctionComponent< ({ height }: { height: number }) => setContentHeight(Math.round(height)), [] ); - const heightInlineStyle = useMemo( - () => ({ blockSize: isOpen ? contentHeight : 0 }), - [isOpen, contentHeight] - ); + const heightInlineStyle = useMemo(() => { + if (!isOpen) { + return { blockSize: 0 }; + } + + // Avoid overriding CSS `height: auto` with `0` before the first resize + // measurement (e.g. `initialIsOpen` under React Strict Mode). + if (contentHeight === 0) { + return undefined; + } + + return { blockSize: contentHeight }; + }, [isOpen, contentHeight]); return (
({ + EuiPortal: ({ children }: { children: React.ReactNode }) => children, +})); + +const resizeObserverMockRef: { + current: ReturnType | null; +} = { current: null }; + +jest.mock('../observer/resize_observer/resize_observer.tsx', () => { + const { createResizeObserverMock } = jest.requireActual< + typeof import('../../test/internal') + >('../../test/internal'); + resizeObserverMockRef.current = createResizeObserverMock(); + global.ResizeObserver = resizeObserverMockRef.current.ResizeObserver; + + return jest.requireActual('../observer/resize_observer/resize_observer.tsx'); +}); + +const createMockEntry = ( + element: Element, + { width = 500, height = 100 } = {} +): ResizeObserverEntry => ({ + target: element, + contentRect: new DOMRect(0, 0, width, height), + borderBoxSize: [{ inlineSize: width, blockSize: height }], + contentBoxSize: [], + devicePixelContentBoxSize: [], +}); + +const ISSUE_9029_CONTENT = 'Accordion content inside flyout (issue #9029)'; + +const renderAccordionInFlyout = () => + render( + + {}} aria-label="Flyout"> + + +
{ISSUE_9029_CONTENT}
+ + + + + ); + +describe('EuiAccordion Strict Mode regressions', () => { + beforeEach(() => { + resizeObserverMockRef.current?.ResizeObserver.mockClear(); + }); + + /** + * https://github.com/elastic/eui/issues/9029 + */ + it('renders open accordion content inside EuiFlyout under Strict Mode', () => { + const resizeObserverMock = resizeObserverMockRef.current!; + renderAccordionInFlyout(); + + const content = screen.getByText(ISSUE_9029_CONTENT); + const childWrapper = content.closest( + '.euiAccordion__childWrapper' + ) as HTMLElement; + + expect(childWrapper).toBeInTheDocument(); + expect(childWrapper).not.toHaveStyle({ blockSize: '0px' }); + expect(content).toBeVisible(); + + // Simulate resize notifications after Strict Mode observer reconnect + const observers = resizeObserverMock.ResizeObserver.mock.results.map( + (result) => result.value + ); + const observedElement = childWrapper.querySelector( + '.euiAccordion__children' + ) as HTMLElement; + + observers.forEach((observer) => { + resizeObserverMock.triggerCallback( + [createMockEntry(observedElement)], + observer + ); + }); + + expect(childWrapper).not.toHaveStyle({ blockSize: '0px' }); + expect(content).toBeVisible(); + }); +}); diff --git a/packages/eui/src/components/observer/resize_observer/resize_observer.test.tsx b/packages/eui/src/components/observer/resize_observer/resize_observer.test.tsx index 96292da47761..c8c90cbb2640 100644 --- a/packages/eui/src/components/observer/resize_observer/resize_observer.test.tsx +++ b/packages/eui/src/components/observer/resize_observer/resize_observer.test.tsx @@ -7,16 +7,93 @@ */ import React, { FunctionComponent, PropsWithChildren, useState } from 'react'; -import { EuiResizeObserver, useResizeObserver } from './resize_observer'; import { sleep } from '../../../test'; import { render } from '../../../test/rtl'; import { act } from '@testing-library/react'; +import { createResizeObserverMock } from '../../../test/internal'; +import { EuiResizeObserver, useResizeObserver } from './resize_observer'; + +const resizeObserverMockRef: { + current: ReturnType | null; +} = { current: null }; + +jest.mock('./resize_observer.tsx', () => { + const { createResizeObserverMock } = jest.requireActual< + typeof import('../../../test/internal') + >('../../../test/internal'); + resizeObserverMockRef.current = createResizeObserverMock(); + global.ResizeObserver = resizeObserverMockRef.current.ResizeObserver; + + return jest.requireActual('./resize_observer.tsx'); +}); export async function waitforResizeObserver(period = 30) { // `period` defaults to 30 because its the delay used by the ResizeObserver polyfill await sleep(period); } +const createMockEntry = ( + element: Element, + { width = 500, height = 100 } = {} +): ResizeObserverEntry => ({ + target: element, + contentRect: new DOMRect(), + borderBoxSize: [{ inlineSize: width, blockSize: height }], + contentBoxSize: [], + devicePixelContentBoxSize: [], +}); + +describe('EuiResizeObserver', () => { + beforeEach(() => { + resizeObserverMockRef.current?.ResizeObserver.mockClear(); + }); + + it('calls onResize again after observer reconnects with the same dimensions', () => { + const resizeObserverMock = resizeObserverMockRef.current!; + const onResize = jest.fn(); + + const TestComponent = ({ attached }: { attached: boolean }) => ( + + {(resizeRef) => + attached ? ( +
+ content +
+ ) : null + } + + ); + + const { getByTestId, rerender } = render(); + + const element = getByTestId('target'); + const firstObserver = + resizeObserverMock.ResizeObserver.mock.results[0]?.value; + + resizeObserverMock.triggerCallback( + [createMockEntry(element)], + firstObserver + ); + expect(onResize).toHaveBeenCalledTimes(1); + expect(onResize).toHaveBeenCalledWith({ height: 100, width: 500 }); + + rerender(); + rerender(); + + const secondObserver = + resizeObserverMock.ResizeObserver.mock.results[1]?.value; + + expect(resizeObserverMock.ResizeObserver).toHaveBeenCalledTimes(2); + + resizeObserverMock.triggerCallback( + [createMockEntry(getByTestId('target'))], + secondObserver + ); + expect(onResize).toHaveBeenCalledTimes(2); + expect(onResize).toHaveBeenLastCalledWith({ height: 100, width: 500 }); + }); +}); + // EuiResizeObserver and useResizeObserver do not have a fallback for // environments that do not implement the ResizeObserver API. // jsdom does not implement ResizeObserver and we therefore diff --git a/packages/eui/src/components/observer/resize_observer/resize_observer.tsx b/packages/eui/src/components/observer/resize_observer/resize_observer.tsx index 9b945cf67ed9..307cf6b43469 100644 --- a/packages/eui/src/components/observer/resize_observer/resize_observer.tsx +++ b/packages/eui/src/components/observer/resize_observer/resize_observer.tsx @@ -49,7 +49,14 @@ export const EuiResizeObserver: FunctionComponent = ({ }, []); const beginObserve = useCallback( - (node: Element) => makeResizeObserver(node, resizeCallback), + (node: Element) => { + // Reset cached dimensions so the first callback after (re)connecting + // always fires onResize. Required for React Strict Mode, which preserves + // refs across the simulated unmount/remount while state updates from the + // first mount may be discarded. + sizeRef.current = { height: 0, width: 0 }; + return makeResizeObserver(node, resizeCallback); + }, [resizeCallback] ); diff --git a/packages/eui/src/components/observer/use_observer.test.tsx b/packages/eui/src/components/observer/use_observer.test.tsx index 15a53aeff68e..9c9fa5d8ed77 100644 --- a/packages/eui/src/components/observer/use_observer.test.tsx +++ b/packages/eui/src/components/observer/use_observer.test.tsx @@ -6,7 +6,7 @@ * Side Public License, v 1. */ -import React from 'react'; +import React, { StrictMode } from 'react'; import { render } from '../../test/rtl'; import { useObserver, Observer } from './use_observer'; @@ -106,4 +106,30 @@ describe('useObserver', () => { expect(observer.disconnect).toHaveBeenCalledTimes(1); }); + + (process.env.REACT_VERSION === '18' ? it : it.skip)( + 'reconnects the observer after StrictMode remounts without a ref change', + () => { + const observer1 = makeObserver(); + const observer2 = makeObserver(); + const beginObserve = jest + .fn() + .mockReturnValueOnce(observer1) + .mockReturnValueOnce(observer2); + + const TestComponent = () => { + const updateChildNode = useObserver(beginObserve, 'Test'); + return
; + }; + + render( + + + + ); + + expect(beginObserve).toHaveBeenCalledTimes(2); + expect(observer1.disconnect).toHaveBeenCalledTimes(1); + } + ); }); diff --git a/packages/eui/src/components/observer/use_observer.ts b/packages/eui/src/components/observer/use_observer.ts index e5c96c35a3d3..dba1d6ced390 100644 --- a/packages/eui/src/components/observer/use_observer.ts +++ b/packages/eui/src/components/observer/use_observer.ts @@ -44,16 +44,32 @@ export const useObserver = ( // Empty deps: run only on mount/unmount — componentName is only used for the // error message and changing it must not disconnect/re-connect the observer. useEffect(() => { - if (childNodeRef.current == null) { + if (childNodeRef.current === null) { throw new Error(`${componentNameRef.current} did not receive a ref`); } + + // Strict Mode runs effect cleanup without always calling the ref callback + // again (notably in React 18). Reconnect when the node is still mounted. + if (observerRef.current === null) { + observerRef.current = + beginObserveRef.current(childNodeRef.current) ?? null; + } + return () => { observerRef.current?.disconnect(); + observerRef.current = null; }; }, []); const updateChildNode = useCallback((ref: Element | null) => { - if (childNodeRef.current === ref) return; // node hasn't changed + if (childNodeRef.current === ref) { + // Same node, but the observer may have been disconnected by Strict Mode + // effect cleanup without a matching ref(null) call. + if (ref != null && observerRef.current === null) { + observerRef.current = beginObserveRef.current(ref) ?? null; + } + return; + } // if there's an existing observer disconnect it if (observerRef.current != null) {