Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Track EuiPopover's setTimeout and requestAnimationFrame and clear them on unmount #2614

Merged
Merged
Show file tree
Hide file tree
Changes from 3 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
- Fixed UX/focus bug in `EuiDataGrid` when using keyboard shortcuts to paginate ([#2602](https://github.com/elastic/eui/pull/2602))
- Fixed `EuiIcon` accessibility by adding a `title` prop and a default `aria-label` ([#2554](https://github.com/elastic/eui/pull/2554))
- Fixed `EuiDataGrid`'s in-memory sorting of numeric columns when the cell data contains multiple digit groups ([#2603](https://github.com/elastic/eui/pull/2603))
- Fixed react-dom warning when `EuiPopover` was unmounted before calls to setState ([#2614](https://github.com/elastic/eui/pull/2614))

## [`17.0.0`](https://github.com/elastic/eui/tree/v17.0.0)

Expand Down
24 changes: 24 additions & 0 deletions src/components/popover/popover.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
} from './popover';

import { keyCodes } from '../../services';
import { sleep } from '../../test';

jest.mock('../portal', () => ({
EuiPortal: ({ children }: { children: ReactNode }) => children,
Expand Down Expand Up @@ -254,6 +255,29 @@ describe('EuiPopover', () => {
});
});
});

it('cleans up timeouts and rAFs on unmount', async () => {
const component = mount(
<EuiPopover
id={getId()}
button={<button />}
closePopover={() => {}}
panelPaddingSize="s"
isOpen={false}
/>
);

component.setProps({ isOpen: true });

component.unmount();

// EUI's jest configuration throws an error if there are any console.warn or console.error calls, like
// React's setState on an unmounted component warning
//
// this await gives the environment enough time to execute any pending timeouts or animation frame callbacks
// and validates the timeout/rAF clearing done by EuiPopover
await sleep(50);
});
});

describe('getPopoverPositionFromAnchorPosition', () => {
Expand Down
22 changes: 15 additions & 7 deletions src/components/popover/popover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,10 @@ export class EuiPopover extends Component<Props, State> {
return null;
}

private respositionTimeout: number | undefined;
private closingTransitionTimeout: number | undefined;
private closingTransitionAnimationFrame: number | undefined;
private updateFocusAnimationFrame: number | undefined;
private button: HTMLElement | null = null;
private panel: HTMLElement | null = null;

Expand Down Expand Up @@ -304,7 +307,7 @@ export class EuiPopover extends Component<Props, State> {

updateFocus() {
// Wait for the DOM to update.
window.requestAnimationFrame(() => {
this.updateFocusAnimationFrame = window.requestAnimationFrame(() => {
if (!this.props.ownFocus || !this.panel) {
return;
}
Expand Down Expand Up @@ -374,11 +377,13 @@ export class EuiPopover extends Component<Props, State> {
clearTimeout(this.closingTransitionTimeout);
// We need to set this state a beat after the render takes place, so that the CSS
// transition can take effect.
window.requestAnimationFrame(() => {
this.setState({
isOpening: true,
});
});
this.closingTransitionAnimationFrame = window.requestAnimationFrame(
() => {
this.setState({
isOpening: true,
});
}
);

// for each child element of `this.panel`, find any transition duration we should wait for before stabilizing
const { durationMatch, delayMatch } = Array.prototype.slice
Expand All @@ -398,7 +403,7 @@ export class EuiPopover extends Component<Props, State> {
{ durationMatch: 0, delayMatch: 0 }
);

setTimeout(() => {
this.respositionTimeout = window.setTimeout(() => {
this.setState({ isOpenStable: true }, () => {
this.positionPopoverFixed();
this.updateFocus();
Expand Down Expand Up @@ -429,7 +434,10 @@ export class EuiPopover extends Component<Props, State> {

componentWillUnmount() {
window.removeEventListener('scroll', this.positionPopoverFixed);
clearTimeout(this.respositionTimeout);
clearTimeout(this.closingTransitionTimeout);
cancelAnimationFrame(this.closingTransitionAnimationFrame!);
cancelAnimationFrame(this.updateFocusAnimationFrame!);
}

onMutation = (records: MutationRecord[]) => {
Expand Down