Skip to content
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/eui/changelogs/upcoming/9029.md

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking:

The filename is supposed to be 9811.md, from the PR number.

Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
**Bug fixes**

- Fixed `EuiAccordion` rendering with zero height under React Strict Mode (including when mounted inside `EuiFlyout` with `initialIsOpen`)
Original file line number Diff line number Diff line change
Expand Up @@ -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;"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down Expand Up @@ -849,7 +848,6 @@ exports[`EuiAccordion props initialIsOpen is rendered 1`] = `
class="euiAccordion__childWrapper emotion-euiAccordion__childWrapper-isOpen-noTransition"
id="12"
role="group"
style="block-size: 0;"
>
<div
class="euiAccordion__children emotion-euiAccordion__children"
Expand Down
61 changes: 61 additions & 0 deletions packages/eui/src/components/accordion/accordion.stories.tsx

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking:

For testing the result it's fine but before I approve, we have to remove these stories. We don't need StrictMode specific ones.

Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
import type { Meta, StoryObj } from '@storybook/react';

import { EuiAccordion, EuiAccordionProps } from './accordion';
import { EuiFlyout, EuiFlyoutBody, EuiFlyoutHeader } from '../flyout';
import { EuiTitle } from '../title';
import React, { StrictMode } from 'react';

const meta: Meta<EuiAccordionProps> = {
title: 'Layout/EuiAccordion',
Expand Down Expand Up @@ -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: (
<div
css={({ euiTheme }) => ({
padding: 16,
background: euiTheme.colors.backgroundBaseSubdued,
})}
>
This content should be visible immediately.
</div>
),
},
decorators: [
(Story) => (
<StrictMode>
<Story />
</StrictMode>
),
],
};

/**
* Reproduction for https://github.com/elastic/eui/issues/9029
*/
export const StrictModeInFlyout: Story = {
render: () => (
<StrictMode>
<EuiFlyout onClose={() => {}} aria-labelledby="flyout-9029-title">
<EuiFlyoutHeader hasBorder>
<EuiTitle size="m">
<h2 id="flyout-9029-title">Flyout with accordion</h2>
</EuiTitle>
</EuiFlyoutHeader>
<EuiFlyoutBody>
<EuiAccordion
id="accordion-in-flyout-9029"
initialIsOpen
buttonContent="Accordion in flyout"
>
<div
css={({ euiTheme }) => ({
padding: 16,
background: euiTheme.colors.backgroundBaseSubdued,
})}
>
Accordion content inside flyout (issue #9029)
</div>
</EuiAccordion>
</EuiFlyoutBody>
</EuiFlyout>
</StrictMode>
),
};
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Comment on lines +96 to +108

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

This is very opinionated but we could tighten this like so:

Suggested change
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]);
const heightInlineStyle = useMemo(() => {
if (!isOpen) return { blockSize: 0 };
// Avoid overriding CSS `height: auto` with `0` before the first resize.
if (contentHeight === 0) return undefined;
return { blockSize: contentHeight };
}, [isOpen, contentHeight]);

instead of having 13 lines we have 6 and the code is equally meaningful.


return (
<div
Expand Down
102 changes: 102 additions & 0 deletions packages/eui/src/components/accordion/accordion_strict_mode.test.tsx

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking:

We can add a small targeted unit test in the existing accordion.test.tsx file. We don't need a separate suite just for Strict Mode 😄

Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import React, { StrictMode } from 'react';
import { render, screen } from '../../test/rtl';
import { createResizeObserverMock } from '../../test/internal';
import { EuiAccordion } from './accordion';
import { EuiFlyout, EuiFlyoutBody } from '../flyout';

jest.mock('../portal', () => ({
EuiPortal: ({ children }: { children: React.ReactNode }) => children,
}));

const resizeObserverMockRef: {
current: ReturnType<typeof createResizeObserverMock> | 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');
});
Comment on lines +23 to +31

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(
<StrictMode>
<EuiFlyout onClose={() => {}} aria-label="Flyout">
<EuiFlyoutBody>
<EuiAccordion
id="accordion-in-flyout-9029"
initialIsOpen
buttonContent="Accordion in flyout"
>
<div>{ISSUE_9029_CONTENT}</div>
</EuiAccordion>
</EuiFlyoutBody>
</EuiFlyout>
</StrictMode>
);

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();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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<typeof createResizeObserverMock> | 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 }) => (
<EuiResizeObserver onResize={onResize}>
{(resizeRef) =>
attached ? (
<div ref={resizeRef} data-testid="target">
content
</div>
) : null
}
</EuiResizeObserver>
);

const { getByTestId, rerender } = render(<TestComponent attached />);

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(<TestComponent attached={false} />);
rerender(<TestComponent attached />);

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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,14 @@ export const EuiResizeObserver: FunctionComponent<EuiResizeObserverProps> = ({
}, []);

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]
);

Expand Down
28 changes: 27 additions & 1 deletion packages/eui/src/components/observer/use_observer.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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',
() => {
Comment on lines +110 to +112
const observer1 = makeObserver();
const observer2 = makeObserver();
const beginObserve = jest
.fn()
.mockReturnValueOnce(observer1)
.mockReturnValueOnce(observer2);

const TestComponent = () => {
const updateChildNode = useObserver(beginObserve, 'Test');
return <div ref={updateChildNode} />;
};

render(
<StrictMode>
<TestComponent />
</StrictMode>
);

expect(beginObserve).toHaveBeenCalledTimes(2);
expect(observer1.disconnect).toHaveBeenCalledTimes(1);
}
);
});
Loading
Loading