Skip to content

Commit e23354b

Browse files
hariombalharadevin-ai-integration[bot]
hariombalhara
and
devin-ai-integration[bot]
authored
fix: ESC key handler only works once in embed modal (calcom#25615)
* fix: ESC key handler only works once in embed modal Co-Authored-By: hariom@cal.com <hariombalhara@gmail.com> * Fix bug found by cubic and add tests --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
1 parent 0b7f514 commit e23354b

4 files changed

Lines changed: 172 additions & 13 deletions

File tree

‎packages/embeds/embed-core/src/EmbedElement.test.ts‎

Lines changed: 106 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,12 @@ import inlineHTML from "./Inline/inlineHtml";
99
import { EMBED_DARK_THEME_CLASS, EMBED_LIGHT_THEME_CLASS } from "./constants";
1010
import { getColorSchemeDarkQuery } from "./ui-utils";
1111

12+
type EmbedElementWithPrivateMethodsAccess = {
13+
boundResizeHandler: () => void;
14+
boundPrefersDarkThemeChangedHandler: (e: MediaQueryListEvent) => void;
15+
boundEnsureContainerTakesSkeletonHeightWhenVisible: () => void;
16+
}
17+
1218
(function defineEmbedTestElement() {
1319
class TestEmbedElement extends EmbedElement {
1420
constructor({
@@ -31,6 +37,75 @@ import { getColorSchemeDarkQuery } from "./ui-utils";
3137
customElements.define("test-embed", TestEmbedElement);
3238
})();
3339

40+
function mockWindowEventListeners() {
41+
const eventListenerCallbacks = new Map<string, EventListenerOrEventListenerObject>();
42+
const animationFrameCallbacks: Map<number, FrameRequestCallback> = new Map();
43+
const colorSchemeListenerCallbacks: Map<string, EventListenerOrEventListenerObject> = new Map();
44+
const colorSchemeQuery = getColorSchemeDarkQuery();
45+
let nextAnimationFrameId = 1;
46+
47+
vi
48+
.spyOn(window, "addEventListener")
49+
.mockImplementation((event: string, callback: EventListenerOrEventListenerObject) => {
50+
eventListenerCallbacks.set(event, callback);
51+
});
52+
53+
vi
54+
.spyOn(window, "removeEventListener")
55+
.mockImplementation((event: string, callback: EventListenerOrEventListenerObject) => {
56+
const registeredCallback = eventListenerCallbacks.get(event);
57+
expect(registeredCallback).toBe(callback);
58+
eventListenerCallbacks.delete(event);
59+
});
60+
61+
vi
62+
.spyOn(window, "requestAnimationFrame")
63+
.mockImplementation((callback: FrameRequestCallback) => {
64+
const id = nextAnimationFrameId++;
65+
animationFrameCallbacks.set(id, callback);
66+
return id;
67+
});
68+
69+
vi
70+
.spyOn(window, "cancelAnimationFrame")
71+
.mockImplementation((id: number) => {
72+
animationFrameCallbacks.delete(id);
73+
});
74+
75+
vi
76+
.spyOn(colorSchemeQuery, "addEventListener")
77+
.mockImplementation((event: string, callback: EventListenerOrEventListenerObject) => {
78+
colorSchemeListenerCallbacks.set(event, callback);
79+
});
80+
81+
vi
82+
.spyOn(colorSchemeQuery, "removeEventListener")
83+
.mockImplementation((event: string, callback: EventListenerOrEventListenerObject) => {
84+
colorSchemeListenerCallbacks.delete(event);
85+
});
86+
87+
return {
88+
expectListenerToBeRegistered: (event: string, callback: EventListenerOrEventListenerObject) => {
89+
expect(eventListenerCallbacks.get(event)).toBe(callback);
90+
},
91+
expectListenerToBeUnregistered: (event: string, callback: EventListenerOrEventListenerObject) => {
92+
expect(eventListenerCallbacks.get(event)).not.toBe(callback);
93+
},
94+
expectAnimationFrameListenerToBeRegistered: (rafId: number, callback: FrameRequestCallback) => {
95+
expect(animationFrameCallbacks.get(rafId)).toBe(callback);
96+
},
97+
expectAnimationFrameListenerToBeUnregistered: (rafId: number, callback: FrameRequestCallback) => {
98+
expect(animationFrameCallbacks.get(rafId)).not.toBe(callback);
99+
},
100+
expectColorSchemeListenerToBeRegistered: (callback: (e: MediaQueryListEvent) => void) => {
101+
expect(colorSchemeListenerCallbacks.get("change")).toBe(callback);
102+
},
103+
expectColorSchemeListenerToBeUnregistered: (callback: (e: MediaQueryListEvent) => void) => {
104+
expect(colorSchemeListenerCallbacks.get("change")).not.toBe(callback);
105+
},
106+
};
107+
}
108+
34109
function buildMediaQueryListEvent({ type, matches }: { type: string; matches: boolean }) {
35110
return {
36111
type,
@@ -143,7 +218,9 @@ describe("EmbedElement", () => {
143218
if (!element) {
144219
throw new Error("`element` not defined");
145220
}
146-
document.body.removeChild(element);
221+
if (element.parentNode) {
222+
document.body.removeChild(element);
223+
}
147224
vi.restoreAllMocks();
148225
});
149226

@@ -358,5 +435,33 @@ describe("EmbedElement", () => {
358435
expectLayoutToBe("month_view", element);
359436
});
360437
});
438+
439+
describe("Cleanup Behavior", () => {
440+
it("should clean up all resources when element is disconnected", () => {
441+
const { expectListenerToBeRegistered, expectListenerToBeUnregistered, expectAnimationFrameListenerToBeRegistered, expectAnimationFrameListenerToBeUnregistered, expectColorSchemeListenerToBeRegistered, expectColorSchemeListenerToBeUnregistered } = mockWindowEventListeners();
442+
443+
element = createTestEmbedElement({
444+
dataset: { pageType: "user.event.booking.slots" },
445+
});
446+
447+
448+
449+
const internalEmbed = element as unknown as EmbedElementWithPrivateMethodsAccess;
450+
451+
const boundResizeHandler = internalEmbed.boundResizeHandler;
452+
const boundPrefersDarkThemeChangedHandler = internalEmbed.boundPrefersDarkThemeChangedHandler;
453+
const boundEnsureContainerTakesSkeletonHeightWhenVisible = internalEmbed.boundEnsureContainerTakesSkeletonHeightWhenVisible;
454+
455+
expectListenerToBeRegistered("resize", boundResizeHandler);
456+
expectColorSchemeListenerToBeRegistered(boundPrefersDarkThemeChangedHandler);
457+
expectAnimationFrameListenerToBeRegistered(element.skeletonContainerHeightTimer!, boundEnsureContainerTakesSkeletonHeightWhenVisible);
458+
459+
document.body.removeChild(element);
460+
461+
expectListenerToBeUnregistered("resize", boundResizeHandler);
462+
expectColorSchemeListenerToBeUnregistered(boundPrefersDarkThemeChangedHandler);
463+
expectAnimationFrameListenerToBeUnregistered(element.skeletonContainerHeightTimer!, boundEnsureContainerTakesSkeletonHeightWhenVisible);
464+
});
465+
});
361466
});
362467
});

‎packages/embeds/embed-core/src/EmbedElement.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ export class EmbedElement extends HTMLElement {
2929

3030
private boundResizeHandler: () => void;
3131
private boundPrefersDarkThemeChangedHandler: (e: MediaQueryListEvent) => void;
32+
private boundEnsureContainerTakesSkeletonHeightWhenVisible: () => void;
33+
3234
private isSkeletonSupportedPageType() {
3335
const pageType = this.getPageType();
3436
// Any pageType being set is considered as skeleton supported. There is always a fallback skeleton loader if no direct match for a skeleton loader is found based on pageType
@@ -99,7 +101,7 @@ export class EmbedElement extends HTMLElement {
99101
return;
100102
}
101103
}
102-
const rafId = requestAnimationFrame(this.ensureContainerTakesSkeletonHeightWhenVisible.bind(this));
104+
const rafId = requestAnimationFrame(this.boundEnsureContainerTakesSkeletonHeightWhenVisible);
103105
this.skeletonContainerHeightTimer = rafId;
104106
return rafId;
105107
}
@@ -158,6 +160,7 @@ export class EmbedElement extends HTMLElement {
158160
this.getSkeletonData = data.getSkeletonData;
159161
this.boundResizeHandler = this.resizeHandler.bind(this);
160162
this.boundPrefersDarkThemeChangedHandler = this.prefersDarkThemeChangedHandler.bind(this);
163+
this.boundEnsureContainerTakesSkeletonHeightWhenVisible = this.ensureContainerTakesSkeletonHeightWhenVisible.bind(this);
161164
}
162165

163166
public isSkeletonLoaderVisible() {

‎packages/embeds/embed-core/src/ModalBox/ModalBox.test.ts‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
import "../../test/__mocks__/windowMatchMedia";
2+
3+
import { describe, it, expect, beforeEach, vi, afterEach } from "vitest";
4+
5+
import { EmbedElement } from "../EmbedElement";
6+
import { ModalBox } from "./ModalBox";
7+
8+
function setupWindowCal() {
9+
// Mock window.Cal.__css which is required by ModalBox constructor
10+
if (!window.Cal) {
11+
// @ts-expect-error - Test setup
12+
window.Cal = {};
13+
}
14+
window.Cal.__css = "/* mock css */";
15+
}
16+
17+
// Register ModalBox as a custom element for testing
18+
if (!customElements.get("cal-modal-box")) {
19+
customElements.define("cal-modal-box", ModalBox);
20+
}
21+
22+
describe("ModalBox", () => {
23+
let modalBox: ModalBox;
24+
25+
beforeEach(() => {
26+
setupWindowCal();
27+
});
28+
29+
afterEach(() => {
30+
if (modalBox && modalBox.parentNode) {
31+
document.body.removeChild(modalBox);
32+
}
33+
vi.restoreAllMocks();
34+
});
35+
36+
it("should verify that connectedCallback and disconnectedCallback of ModalBox call the EmbedElement's methods", () => {
37+
const embedElementConnectedCallbackSpy = vi.spyOn(EmbedElement.prototype, "connectedCallback");
38+
const embedElementDisconnectedCallbackSpy = vi.spyOn(EmbedElement.prototype, "disconnectedCallback");
39+
40+
modalBox = new ModalBox();
41+
42+
document.body.appendChild(modalBox);
43+
44+
expect(embedElementConnectedCallbackSpy).toHaveBeenCalledTimes(1);
45+
46+
document.body.removeChild(modalBox);
47+
48+
expect(embedElementDisconnectedCallbackSpy).toHaveBeenCalledTimes(1);
49+
});
50+
});

‎packages/embeds/embed-core/src/ModalBox/ModalBox.ts‎

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,12 @@ import modalBoxHtml, { getSkeletonData } from "./ModalBoxHtml";
66
export class ModalBox extends EmbedElement {
77
static htmlOverflow: string;
88

9+
private escHandler = (e: KeyboardEvent) => {
10+
if (e.key === "Escape") {
11+
this.close();
12+
}
13+
};
14+
915
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
1016
//@ts-ignore
1117
static get observedAttributes() {
@@ -220,17 +226,7 @@ export class ModalBox extends EmbedElement {
220226
super.connectedCallback();
221227
this.assertHasShadowRoot();
222228
const closeEl = this.shadowRoot.querySelector<HTMLElement>(".close");
223-
document.addEventListener(
224-
"keydown",
225-
(e) => {
226-
if (e.key === "Escape") {
227-
this.close();
228-
}
229-
},
230-
{
231-
once: true,
232-
}
233-
);
229+
document.addEventListener("keydown", this.escHandler);
234230

235231
// The backdrop is inside the host element, and a click on host element is only possible if the user clicks outside the iframe.
236232
// So, it is backdrop click handler
@@ -245,6 +241,11 @@ export class ModalBox extends EmbedElement {
245241
}
246242
}
247243

244+
disconnectedCallback() {
245+
super.disconnectedCallback();
246+
document.removeEventListener("keydown", this.escHandler);
247+
}
248+
248249
constructor() {
249250
super({ isModal: true, getSkeletonData });
250251
const modalHtml = `<style>${window.Cal.__css}</style><style>${loaderCss}</style>${modalBoxHtml({

0 commit comments

Comments
 (0)