Skip to content

Commit

Permalink
fix(FocusZone): fix EventListener leak (#14031)
Browse files Browse the repository at this point in the history
* fix(FocusZone): fix EventListener leak

* changelog

* address PR comments

* address PR comments
  • Loading branch information
miroslavstastny committed Jul 15, 2020
1 parent 2d5932c commit 042d257
Show file tree
Hide file tree
Showing 3 changed files with 114 additions and 12 deletions.
8 changes: 8 additions & 0 deletions change/@fluentui-react-focus-2020-07-15-00-07-46-master.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"type": "patch",
"comment": "Fix memory leak in FocusZone",
"packageName": "@fluentui/react-focus",
"email": "mistastn@microsoft.com",
"dependentChangeType": "patch",
"date": "2020-07-14T22:07:46.112Z"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
import * as ReactDOM from 'react-dom';
import * as React from 'react';
import { FocusZone } from './FocusZone';

type EventHandler = {
listener: EventListenerOrEventListenerObject;
options?: boolean | AddEventListenerOptions;
};

function optionsEqual(
a: boolean | AddEventListenerOptions | undefined,
b: boolean | AddEventListenerOptions | undefined,
) {
if (typeof a !== 'object' || typeof b !== 'object') {
return a === b;
}

return a.once === b.once && a.passive === b.passive && a.capture === b.capture;
}

function handlersEqual(a: EventHandler, b: EventHandler) {
return a.listener === b.listener && optionsEqual(a.options, b.options);
}

// HEADS UP: this test is intentionally in a separate file aside from the rest of FocusZone tests
// As it is testing ref counting on a global `window` object it would interfere with other FocusZone tests
// which use ReactTestUtils.renderIntoDocument() which renders FocusZone into a detached DOM node and never unmounts.
describe('FocusZone keydown event handler', () => {
let host: HTMLElement;
let keydownEventHandlers: EventHandler[];

beforeEach(() => {
host = document.createElement('div');
keydownEventHandlers = [];

window.addEventListener = jest.fn((type, listener, options) => {
if (type === 'keydown') {
const eventListener = { listener, options };
if (!keydownEventHandlers.some(item => handlersEqual(item, eventListener))) {
keydownEventHandlers.push(eventListener);
}
}
});

window.removeEventListener = jest.fn((type, listener, options) => {
if (type === 'keydown') {
const eventListener = { listener, options };
const index = keydownEventHandlers.findIndex(item => handlersEqual(item, eventListener));
if (index >= 0) {
keydownEventHandlers.splice(index, 1);
}
}
});
});

it('is added on mount/removed on unmount', () => {
ReactDOM.render(<FocusZone />, host);
expect(keydownEventHandlers.length).toBe(1);

ReactDOM.unmountComponentAtNode(host);
expect(keydownEventHandlers.length).toBe(0);
});

it('is added only once for nested focus zones', () => {
ReactDOM.render(
<div>
<FocusZone>
<FocusZone />
</FocusZone>
</div>,
host,
);
expect(keydownEventHandlers.length).toBe(1);

ReactDOM.unmountComponentAtNode(host);
expect(keydownEventHandlers.length).toBe(0);
});

it('is added only once for sibling focus zones', () => {
ReactDOM.render(
<div>
<FocusZone />
<FocusZone />
</div>,
host,
);
expect(keydownEventHandlers.length).toBe(1);

ReactDOM.unmountComponentAtNode(host);
expect(keydownEventHandlers.length).toBe(0);
});
});
26 changes: 14 additions & 12 deletions packages/react-focus/src/components/FocusZone/FocusZone.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,18 @@ export class FocusZone extends React.Component<IFocusZoneProps> implements IFocu
return _outerZones.size;
}

/**
* Handle global tab presses so that we can patch tabindexes on the fly.
* HEADS UP: This must not be an arrow function in order to be referentially equal among instances
* for ref counting to work correctly!
*/
private static _onKeyDownCapture(ev: KeyboardEvent): void {
// eslint-disable-next-line deprecation/deprecation, @fluentui/deprecated-keyboard-event-props
if (ev.which === KeyCodes.tab) {
_outerZones.forEach((zone: FocusZone) => zone._updateTabIndexes());
}
}

constructor(props: IFocusZoneProps) {
super(props);
// Manage componentRef resolution.
Expand Down Expand Up @@ -153,7 +165,7 @@ export class FocusZone extends React.Component<IFocusZoneProps> implements IFocu
_outerZones.add(this);

if (this._windowElement && _outerZones.size === 1) {
this._windowElement.addEventListener('keydown', this._onKeyDownCapture, true);
this._windowElement.addEventListener('keydown', FocusZone._onKeyDownCapture, true);
}
}

Expand Down Expand Up @@ -210,7 +222,7 @@ export class FocusZone extends React.Component<IFocusZoneProps> implements IFocu

// If this is the last outer zone, remove the keydown listener.
if (this._windowElement && _outerZones.size === 0) {
this._windowElement.removeEventListener('keydown', this._onKeyDownCapture, true);
this._windowElement.removeEventListener('keydown', FocusZone._onKeyDownCapture, true);
}
}

Expand Down Expand Up @@ -487,16 +499,6 @@ export class FocusZone extends React.Component<IFocusZoneProps> implements IFocu
this._setParkedFocus(false);
};

/**
* Handle global tab presses so that we can patch tabindexes on the fly.
*/
private _onKeyDownCapture = (ev: KeyboardEvent): void => {
// eslint-disable-next-line deprecation/deprecation, @fluentui/deprecated-keyboard-event-props
if (ev.which === KeyCodes.tab) {
_outerZones.forEach((zone: FocusZone) => zone._updateTabIndexes());
}
};

private _onMouseDown = (ev: React.MouseEvent<HTMLElement>): void => {
if (this._portalContainsElement(ev.target as HTMLElement)) {
// If the event target is inside a portal do not process the event.
Expand Down

0 comments on commit 042d257

Please sign in to comment.