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.

Sign up for GitHub

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

Jump to bottom

fix(focus-monitor): implement OnDestroy logic #9305

Merged
merged 1 commit into from
Jan 23, 2018
Merged

fix(focus-monitor): implement OnDestroy logic #9305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 16 additions & 2 deletions src/cdk/a11y/focus-monitor.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,14 +186,28 @@ describe('FocusMonitor', () => {
fixture.detectChanges();
tick();

expect(buttonElement.classList.length)
.toBe(2, 'button should have exactly 2 focus classes');
expect(buttonElement.classList.length).toBe(2, 'button should have exactly 2 focus classes');

focusMonitor.stopMonitoring(buttonElement);
fixture.detectChanges();

expect(buttonElement.classList.length).toBe(0, 'button should not have any focus classes');
}));

it('should remove classes when destroyed', fakeAsync(() => {
buttonElement.focus();
fixture.detectChanges();
tick();

expect(buttonElement.classList.length).toBe(2, 'button should have exactly 2 focus classes');

// Destroy manually since destroying the fixture won't do it.
focusMonitor.ngOnDestroy();
fixture.detectChanges();

expect(buttonElement.classList.length).toBe(0, 'button should not have any focus classes');
}));

});


Expand Down
12 changes: 8 additions & 4 deletions src/cdk/a11y/focus-monitor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ type MonitoredElementInfo = {

/** Monitors mouse and keyboard events to determine the cause of focus events. */
@Injectable()
export class FocusMonitor {
export class FocusMonitor implements OnDestroy {
/** The focus origin that the next focus event is a result of. */
private _origin: FocusOrigin = null;

Expand All @@ -58,8 +58,8 @@ export class FocusMonitor {
/** The timeout id of the touch timeout, used to cancel timeout later. */
private _touchTimeout: number;

/** Weak map of elements being monitored to their info. */
private _elementInfo = new WeakMap<Element, MonitoredElementInfo>();
/** Map of elements being monitored to their info. */
private _elementInfo = new Map<HTMLElement, MonitoredElementInfo>();
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: I would prefer to keep this as a WeakMap, but we need a Map in order to be able to iterate over the elements. Alternatively, if we make an assumption that all the monitored elements will be removed on destroy, we could remove only the global listeners.

Copy link
Member

Choose a reason for hiding this comment

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

This is a tricky trade-off. We don't have any data to know for sure, but I expect most apps will end up with FocusMonitor provided at the root, meaning that using Map could end up making elements remain in memory after they'd otherwise be removed.

Do you think it would be common that monitored elements would remain while the module with the FocusMonitor would be unloaded?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe the most common case would be that the elements are removed while the monitor remains, in which case the responsibility for stopping the monitoring is on the consumer.

Regarding using the Map, I have a hunch (haven't verified it) that the current setup would retain references to the elements as well. While we use the element as a key, we also have a reference to it in the unlisten function which is inside the value. I think that this would retain the reference until the item is removed from the map.

Copy link
Member

Choose a reason for hiding this comment

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

You're right, the unlisten would preserve the element reference already, so switching to a Map won't change anything


/** A map of global objects to lists of current listeners. */
private _unregisterGlobalListeners = () => {};
Expand Down Expand Up @@ -135,7 +135,7 @@ export class FocusMonitor {
* @param element The element to stop monitoring.
*/
stopMonitoring(element: HTMLElement): void {
let elementInfo = this._elementInfo.get(element);
const elementInfo = this._elementInfo.get(element);

if (elementInfo) {
elementInfo.unlisten();
Expand All @@ -157,6 +157,10 @@ export class FocusMonitor {
element.focus();
}

ngOnDestroy() {
this._elementInfo.forEach((_info, element) => this.stopMonitoring(element));
}

/** Register necessary event listeners on the document and window. */
private _registerGlobalListeners() {
// Do nothing if we're not on the browser platform.
Expand Down