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 all 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 @@ -11,6 +11,7 @@
- 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))
- Improved pagination in `EuiBasicTable`. `paginationBar` is hidden when there is no data and `EuiPagination` is displayed even when there is only one page ([#2598](https://github.com/elastic/eui/pull/#2598))
- 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
58 changes: 58 additions & 0 deletions src/components/popover/popover.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,64 @@ describe('EuiPopover', () => {
});
});
});

describe('listener cleanup', () => {
let _raf: typeof window['requestAnimationFrame'];
let _caf: typeof window['cancelAnimationFrame'];
beforeAll(() => {
jest.useFakeTimers();
_raf = window.requestAnimationFrame;
_caf = window.cancelAnimationFrame;

const activeAnimationFrames = new Map<number, number>();
let nextAnimationFrameId = 0;
window.requestAnimationFrame = fn => {
const animationFrameId = nextAnimationFrameId++;
activeAnimationFrames.set(animationFrameId, setTimeout(fn));
return animationFrameId;
};
window.cancelAnimationFrame = (id: number) => {
const timeoutId = activeAnimationFrames.get(id);
if (timeoutId) {
clearTimeout(timeoutId);
activeAnimationFrames.delete(id);
}
};
});

afterAll(() => {
jest.useRealTimers();
window.requestAnimationFrame = _raf;
window.cancelAnimationFrame = _caf;
});

it('cleans up timeouts and rAFs on unmount', () => {
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.error calls, like
// React's setState on an unmounted component warning
// to be future proof, verify that's still the case
expect(() => {
console.error('This is a test');
}).toThrow();

// execute any pending timeouts or animation frame callbacks
// and validate the timeout/rAF clearing done by EuiPopover
jest.advanceTimersByTime(10);
});
});
});

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