Skip to content

Commit 4062ddf

Browse files
nilbuildnilbuild
committed
Fix document event listener leak in onDriverClick (#452)
1 parent 431c036 commit 4062ddf

4 files changed

Lines changed: 188 additions & 18 deletions

File tree

‎src/events.ts‎

Lines changed: 29 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,10 @@ function onKeyup(e: KeyboardEvent) {
6464
}
6565
}
6666

67+
const DRIVER_CLICK_EVENTS = ["pointerdown", "mousedown", "pointerup", "mouseup", "click"] as const;
68+
69+
const driverClickHandlers = new WeakMap<Element, (e: MouseEvent | PointerEvent) => void>();
70+
6771
/**
6872
* Attaches click handler to the elements created by driver.js. It makes
6973
* sure to give the listener the first chance to handle the event, and
@@ -79,7 +83,9 @@ export function onDriverClick(
7983
listener: (pointer: MouseEvent | PointerEvent) => void,
8084
shouldPreventDefault?: (target: HTMLElement) => boolean
8185
) {
82-
const listenerWrapper = (e: MouseEvent | PointerEvent, listener?: (pointer: MouseEvent | PointerEvent) => void) => {
86+
destroyDriverClick(element);
87+
88+
const handler = (e: MouseEvent | PointerEvent) => {
8389
const target = e.target as HTMLElement;
8490
if (!element.contains(target)) {
8591
return;
@@ -91,26 +97,32 @@ export function onDriverClick(
9197
e.stopImmediatePropagation();
9298
}
9399

94-
listener?.(e);
100+
if (e.type === "click") {
101+
listener?.(e);
102+
}
95103
};
96104

97105
// We want to be the absolute first one to hear about the event
98106
const useCapture = true;
99107

100-
// Events to disable
101-
document.addEventListener("pointerdown", listenerWrapper, useCapture);
102-
document.addEventListener("mousedown", listenerWrapper, useCapture);
103-
document.addEventListener("pointerup", listenerWrapper, useCapture);
104-
document.addEventListener("mouseup", listenerWrapper, useCapture);
105-
106-
// Actual click handler
107-
document.addEventListener(
108-
"click",
109-
e => {
110-
listenerWrapper(e, listener);
111-
},
112-
useCapture
113-
);
108+
for (const type of DRIVER_CLICK_EVENTS) {
109+
document.addEventListener(type, handler, useCapture);
110+
}
111+
112+
driverClickHandlers.set(element, handler);
113+
}
114+
115+
export function destroyDriverClick(element: Element) {
116+
const handler = driverClickHandlers.get(element);
117+
if (!handler) {
118+
return;
119+
}
120+
121+
for (const type of DRIVER_CLICK_EVENTS) {
122+
document.removeEventListener(type, handler, true);
123+
}
124+
125+
driverClickHandlers.delete(element);
114126
}
115127

116128
export function initEvents() {
@@ -122,6 +134,7 @@ export function initEvents() {
122134

123135
export function destroyEvents() {
124136
window.removeEventListener("keyup", onKeyup);
137+
window.removeEventListener("keydown", trapFocus);
125138
window.removeEventListener("resize", requireRefresh);
126139
window.removeEventListener("scroll", requireRefresh);
127140
}

‎src/overlay.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { easeInOutQuad } from "./utils";
2-
import { onDriverClick } from "./events";
2+
import { destroyDriverClick, onDriverClick } from "./events";
33
import { emit } from "./emitter";
44
import { getConfig } from "./config";
55
import { getState, setState } from "./state";
@@ -173,6 +173,7 @@ function generateStageSvgPathString(stage: StageDefinition) {
173173
export function destroyOverlay() {
174174
const overlaySvg = getState("__overlaySvg");
175175
if (overlaySvg) {
176+
destroyDriverClick(overlaySvg);
176177
overlaySvg.remove();
177178
}
178179
}

‎src/popover.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { Config, DriverHook, getConfig, getCurrentDriver } from "./config";
22
import { Driver, DriveStep } from "./driver";
33
import { emit } from "./emitter";
4-
import { onDriverClick } from "./events";
4+
import { destroyDriverClick, onDriverClick } from "./events";
55
import { repositionPopover } from "./position";
66
import { getState, setState, State } from "./state";
77
import { bringInView, getFocusableElements } from "./utils";
@@ -63,6 +63,7 @@ export function hidePopover() {
6363
export function renderPopover(element: Element, step: DriveStep) {
6464
let popover = getState("popover");
6565
if (popover) {
66+
destroyDriverClick(popover.wrapper);
6667
document.body.removeChild(popover.wrapper);
6768
}
6869

@@ -331,5 +332,6 @@ export function destroyPopover() {
331332
return;
332333
}
333334

335+
destroyDriverClick(popover.wrapper);
334336
popover.wrapper.parentElement?.removeChild(popover.wrapper);
335337
}

‎tests/events.test.ts‎

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
import { afterEach, describe, expect, it, vi } from "vitest";
2+
import { createDriver, navButton, nextFrame, SAMPLE_STEPS, useDriverHarness } from "./utils";
3+
4+
useDriverHarness();
5+
6+
afterEach(() => {
7+
vi.restoreAllMocks();
8+
});
9+
10+
const DRIVER_EVENT_TYPES = ["click", "pointerdown", "mousedown", "pointerup", "mouseup"];
11+
12+
type ListenerRecord = {
13+
type: string;
14+
listener: EventListenerOrEventListenerObject;
15+
capture: boolean;
16+
};
17+
18+
function normalizeCapture(options?: boolean | AddEventListenerOptions | EventListenerOptions): boolean {
19+
if (typeof options === "boolean") {
20+
return options;
21+
}
22+
23+
return !!options?.capture;
24+
}
25+
26+
function trackDocumentListeners() {
27+
const added: ListenerRecord[] = [];
28+
const removed: ListenerRecord[] = [];
29+
30+
const originalAdd = document.addEventListener.bind(document);
31+
const originalRemove = document.removeEventListener.bind(document);
32+
33+
vi.spyOn(document, "addEventListener").mockImplementation((type, listener, options) => {
34+
if (DRIVER_EVENT_TYPES.includes(type)) {
35+
added.push({
36+
type,
37+
listener: listener as EventListenerOrEventListenerObject,
38+
capture: normalizeCapture(options),
39+
});
40+
}
41+
42+
return originalAdd(type, listener as EventListener, options);
43+
});
44+
45+
vi.spyOn(document, "removeEventListener").mockImplementation((type, listener, options) => {
46+
if (DRIVER_EVENT_TYPES.includes(type)) {
47+
removed.push({
48+
type,
49+
listener: listener as EventListenerOrEventListenerObject,
50+
capture: normalizeCapture(options),
51+
});
52+
}
53+
54+
return originalRemove(type, listener as EventListener, options);
55+
});
56+
57+
function liveCount(): number {
58+
const outstanding = [...removed];
59+
let live = 0;
60+
61+
for (const record of added) {
62+
const idx = outstanding.findIndex(
63+
candidate =>
64+
candidate.type === record.type &&
65+
candidate.listener === record.listener &&
66+
candidate.capture === record.capture
67+
);
68+
69+
if (idx === -1) {
70+
live++;
71+
} else {
72+
outstanding.splice(idx, 1);
73+
}
74+
}
75+
76+
return live;
77+
}
78+
79+
return { liveCount };
80+
}
81+
82+
describe("document listener cleanup", () => {
83+
it("attaches document click listeners while the tour is active", () => {
84+
const tracker = trackDocumentListeners();
85+
const d = createDriver({ animate: false, steps: SAMPLE_STEPS });
86+
d.drive();
87+
88+
expect(tracker.liveCount()).toBeGreaterThan(0);
89+
});
90+
91+
it("removes every document click listener once the tour is destroyed", () => {
92+
const tracker = trackDocumentListeners();
93+
const d = createDriver({ animate: false, steps: SAMPLE_STEPS });
94+
d.drive();
95+
d.destroy();
96+
97+
expect(tracker.liveCount()).toBe(0);
98+
});
99+
100+
it("does not accumulate document listeners as the tour moves between steps", () => {
101+
const tracker = trackDocumentListeners();
102+
const d = createDriver({ animate: false, steps: SAMPLE_STEPS });
103+
d.drive();
104+
105+
const afterFirstStep = tracker.liveCount();
106+
107+
navButton("next")?.click();
108+
navButton("next")?.click();
109+
110+
expect(tracker.liveCount()).toBe(afterFirstStep);
111+
});
112+
113+
it("removes the keydown focus-trap listener when destroyed", () => {
114+
const removeSpy = vi.spyOn(window, "removeEventListener");
115+
const d = createDriver({ animate: false, steps: SAMPLE_STEPS });
116+
d.drive();
117+
d.destroy();
118+
119+
expect(removeSpy.mock.calls.some(([type]) => type === "keydown")).toBe(true);
120+
});
121+
});
122+
123+
describe("overlay pointer suppression", () => {
124+
it("prevents default on pointer events over the overlay to block page interaction", async () => {
125+
const d = createDriver({ animate: false, steps: SAMPLE_STEPS });
126+
d.drive();
127+
await nextFrame();
128+
129+
const path = document.querySelector(".driver-overlay path");
130+
expect(path).not.toBeNull();
131+
132+
for (const type of ["pointerdown", "mousedown", "pointerup", "mouseup"]) {
133+
const event = new MouseEvent(type, { bubbles: true, cancelable: true });
134+
path!.dispatchEvent(event);
135+
expect(event.defaultPrevented).toBe(true);
136+
}
137+
});
138+
139+
it("still routes overlay clicks to the click handler", async () => {
140+
const onNextClick = vi.fn();
141+
const d = createDriver({
142+
animate: false,
143+
overlayClickBehavior: "nextStep",
144+
steps: SAMPLE_STEPS,
145+
onNextClick,
146+
});
147+
d.drive();
148+
await nextFrame();
149+
150+
document.querySelector(".driver-overlay path")?.dispatchEvent(new MouseEvent("click", { bubbles: true }));
151+
152+
expect(onNextClick).toHaveBeenCalledTimes(1);
153+
});
154+
});

0 commit comments

Comments
 (0)