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

remove handler event listeners when map is removed #9508

Merged
merged 3 commits into from
Apr 8, 2020
Merged

remove handler event listeners when map is removed #9508

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1afdc0a
remove handler listeners when removing map
ansis Apr 7, 2020
7ed23a9
fixup
ansis Apr 8, 2020
e91720c
unset handlers
ansis Apr 8, 2020
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
8 changes: 4 additions & 4 deletions src/ui/control/navigation_control.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,10 +173,10 @@ class MouseRotateWrapper {

move(e: MouseEvent, point: Point) {
const map = this.map;
const r = this.mouseRotate.windowMousemove(e, point);
const r = this.mouseRotate.mousemoveWindow(e, point);
if (r && r.bearingDelta) map.setBearing(map.getBearing() + r.bearingDelta);
if (this.mousePitch) {
const p = this.mousePitch.windowMousemove(e, point);
const p = this.mousePitch.mousemoveWindow(e, point);
if (p && p.pitchDelta) map.setPitch(map.getPitch() + p.pitchDelta);
}
}
Expand Down Expand Up @@ -208,8 +208,8 @@ class MouseRotateWrapper {
}

mouseup(e: MouseEvent) {
this.mouseRotate.windowMouseup(e);
if (this.mousePitch) this.mousePitch.windowMouseup(e);
this.mouseRotate.mouseupWindow(e);
if (this.mousePitch) this.mousePitch.mouseupWindow(e);
this.offTemp();
}

Expand Down
4 changes: 2 additions & 2 deletions src/ui/handler/box_zoom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ class BoxZoomHandler {
this._active = true;
}

windowMousemove(e: MouseEvent, point: Point) {
mousemoveWindow(e: MouseEvent, point: Point) {
if (!this._active) return;

const pos = point;
Expand Down Expand Up @@ -111,7 +111,7 @@ class BoxZoomHandler {
this._box.style.height = `${maxY - minY}px`;
}

windowMouseup(e: MouseEvent, point: Point) {
mouseupWindow(e: MouseEvent, point: Point) {
if (!this._active) return;

if (e.button !== 0) return;
Expand Down
4 changes: 2 additions & 2 deletions src/ui/handler/mouse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class MouseHandler {
this._eventButton = eventButton;
}

windowMousemove(e: MouseEvent, point: Point) {
mousemoveWindow(e: MouseEvent, point: Point) {
const lastPoint = this._lastPoint;
if (!lastPoint) return;
e.preventDefault();
Expand All @@ -58,7 +58,7 @@ class MouseHandler {
return this._move(lastPoint, point);
}

windowMouseup(e: MouseEvent) {
mouseupWindow(e: MouseEvent) {
const eventButton = DOM.mouseButton(e);
if (eventButton !== this._eventButton) return;
if (this._moved) DOM.suppressClick();
Expand Down
101 changes: 61 additions & 40 deletions src/ui/handler_manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import TapDragZoomHandler from './handler/tap_drag_zoom';
import DragPanHandler from './handler/shim/drag_pan';
import DragRotateHandler from './handler/shim/drag_rotate';
import TouchZoomRotateHandler from './handler/shim/touch_zoom_rotate';
import {extend} from '../util/util';
import {bindAll, extend} from '../util/util';
import window from '../util/window';
import Point from '@mapbox/point-geometry';
import assert from 'assert';
Expand Down Expand Up @@ -105,6 +105,7 @@ class HandlerManager {
_changes: Array<[HandlerResult, Object, any]>;
_previousActiveHandlers: { [string]: Handler };
_bearingChanged: boolean;
_listeners: Array<[HTMLElement, string, void | {passive?: boolean, capture?: boolean}]>;

constructor(map: Map, options: { interactive: boolean, pitchWithRotate: boolean, clickTolerance: number, bearingSnap: number}) {
this._map = map;
Expand All @@ -122,45 +123,56 @@ class HandlerManager {

this._addDefaultHandlers(options);

// Bind touchstart and touchmove with passive: false because, even though
// they only fire a map events and therefore could theoretically be
// passive, binding with passive: true causes iOS not to respect
// e.preventDefault() in _other_ handlers, even if they are non-passive
// (see https://bugs.webkit.org/show_bug.cgi?id=184251)
this._addListener(this._el, 'touchstart', {passive: false});
this._addListener(this._el, 'touchmove', {passive: false});
this._addListener(this._el, 'touchend');
this._addListener(this._el, 'touchcancel');

this._addListener(this._el, 'mousedown');
this._addListener(this._el, 'mousemove');
this._addListener(this._el, 'mouseup');

// Bind window-level event listeners for move and up/end events. In the absence of
// the pointer capture API, which is not supported by all necessary platforms,
// window-level event listeners give us the best shot at capturing events that
// fall outside the map canvas element. Use `{capture: true}` for the move event
// to prevent map move events from being fired during a drag.
this._addListener(window.document, 'mousemove', {capture: true}, 'windowMousemove');
this._addListener(window.document, 'mouseup', undefined, 'windowMouseup');

this._addListener(this._el, 'mouseover');
this._addListener(this._el, 'mouseout');
this._addListener(this._el, 'dblclick');
this._addListener(this._el, 'click');

this._addListener(this._el, 'keydown', {capture: false});
this._addListener(this._el, 'keyup');

this._addListener(this._el, 'wheel', {passive: false});
this._addListener(this._el, 'contextmenu');

DOM.addEventListener(window, 'blur', () => this.stop());
bindAll(['handleEvent', 'handleWindowEvent'], this);

const el = this._el;

this._listeners = [
// Bind touchstart and touchmove with passive: false because, even though
// they only fire a map events and therefore could theoretically be
// passive, binding with passive: true causes iOS not to respect
// e.preventDefault() in _other_ handlers, even if they are non-passive
// (see https://bugs.webkit.org/show_bug.cgi?id=184251)
[el, 'touchstart', {passive: false}],
[el, 'touchmove', {passive: false}],
[el, 'touchend', undefined],
[el, 'touchcancel', undefined],

[el, 'mousedown', undefined],
[el, 'mousemove', undefined],
[el, 'mouseup', undefined],

// Bind window-level event listeners for move and up/end events. In the absence of
// the pointer capture API, which is not supported by all necessary platforms,
// window-level event listeners give us the best shot at capturing events that
// fall outside the map canvas element. Use `{capture: true}` for the move event
// to prevent map move events from being fired during a drag.
[window.document, 'mousemove', {capture: true}],
[window.document, 'mouseup', undefined],

[el, 'mouseover', undefined],
[el, 'mouseout', undefined],
[el, 'dblclick', undefined],
[el, 'click', undefined],

[el, 'keydown', {capture: false}],
[el, 'keyup', undefined],

[el, 'wheel', {passive: false}],
[el, 'contextmenu', undefined],

[window, 'blur', undefined]
];

for (const [target, type, listenerOptions] of this._listeners) {
DOM.addEventListener(target, type, target === window.document ? this.handleWindowEvent : this.handleEvent, listenerOptions);
}
}

_addListener(element: Element, eventType: string, options: Object, name_?: string) {
const name = name_ || eventType;
DOM.addEventListener(element, eventType, e => this._processInputEvent(e, name), options);
destroy() {
for (const [target, type, listenerOptions] of this._listeners) {
DOM.removeEventListener(target, type, target === window.document ? this.handleWindowEvent : this.handleEvent, listenerOptions);
}
}

_addDefaultHandlers(options: { interactive: boolean, pitchWithRotate: boolean, clickTolerance: number }) {
Expand Down Expand Up @@ -257,7 +269,16 @@ class HandlerManager {
return false;
}

_processInputEvent(e: InputEvent | RenderFrameEvent, eventName?: string) {
handleWindowEvent(e: InputEvent) {
this.handleEvent(e, `${e.type}Window`);
}

handleEvent(e: InputEvent | RenderFrameEvent, eventName?: string) {

if (e.type === 'blur') {
this.stop();
return;
}

this._updatingCamera = true;
assert(e.timeStamp !== undefined);
Expand Down Expand Up @@ -475,7 +496,7 @@ class HandlerManager {
if (this._frameId === undefined) {
this._frameId = this._map._requestRenderFrame(timeStamp => {
delete this._frameId;
this._processInputEvent(new RenderFrameEvent('renderFrame', {timeStamp}));
this.handleEvent(new RenderFrameEvent('renderFrame', {timeStamp}));
this._applyChanges();
});
}
Expand Down
2 changes: 2 additions & 0 deletions src/ui/map.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2319,6 +2319,8 @@ class Map extends Camera {
}
this._renderTaskQueue.clear();
this.painter.destroy();
this.handlers.destroy();
Copy link
Contributor

Choose a reason for hiding this comment

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

might be worth setting this.handlers = null to break the cycle, since handlers caches a reference to map.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

e91720c

delete this.handlers;
this.setStyle(null);
if (typeof window !== 'undefined') {
window.removeEventListener('resize', this._onWindowResize, false);
Expand Down