Skip to content

Commit 3fda36b

Browse files
FHachezclaudemaiieul
authored
fix(modal): prevent onClose$ from firing on initial mount when show is false (#1175)
* fix(modal): prevent onClose$ from firing on initial mount when show is false onClose$ should only be called after an open→close transition, not during the initial useTask$ run when the modal starts in a closed state. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(modal): add E2E tests for onShow$/onClose$ callback lifecycle Covers the bug where onClose$ was called on initial mount when the modal starts closed. Tests verify callbacks fire only after real open/close transitions and accumulate correctly across multiple cycles. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore: changeset --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: maiieul <45822175+maiieul@users.noreply.github.com>
1 parent a1d548b commit 3fda36b

4 files changed

Lines changed: 103 additions & 3 deletions

File tree

.changeset/floppy-donuts-begin.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@qwik-ui/headless': patch
3+
---
4+
5+
fix: onClose$ now doesn't fire if the show prop is set to false by default
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
import { component$, useSignal, useStyles$ } from '@builder.io/qwik';
2+
import { Modal } from '@qwik-ui/headless';
3+
4+
export default component$(() => {
5+
useStyles$(styles);
6+
const isOpen = useSignal(false);
7+
const showCount = useSignal(0);
8+
const closeCount = useSignal(0);
9+
10+
return (
11+
<>
12+
<button onClick$={() => (isOpen.value = true)} class="modal-trigger">
13+
Open Modal
14+
</button>
15+
<p data-testid="show-count">onShow count: {showCount.value}</p>
16+
<p data-testid="close-count">onClose count: {closeCount.value}</p>
17+
<Modal.Root
18+
bind:show={isOpen}
19+
onShow$={() => showCount.value++}
20+
onClose$={() => closeCount.value++}
21+
>
22+
<Modal.Panel class="modal-panel">
23+
<Modal.Title>Callback Test Modal</Modal.Title>
24+
<Modal.Close class="modal-close">Close</Modal.Close>
25+
</Modal.Panel>
26+
</Modal.Root>
27+
</>
28+
);
29+
});
30+
31+
// internal
32+
import styles from '../snippets/modal.css?inline';

packages/kit-headless/src/components/modal/modal-panel.tsx

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ export const HModalPanel = component$((props: PropsOf<'dialog'>) => {
4040
const context = useContext(modalContextId);
4141

4242
const panelRef = useSignal<HTMLDialogElement>();
43+
const wasOpenSig = useSignal(false);
4344

4445
useTask$(async function toggleModal({ track, cleanup }) {
4546
const isOpen = track(() => context.showSig.value);
@@ -69,11 +70,12 @@ export const HModalPanel = component$((props: PropsOf<'dialog'>) => {
6970
});
7071

7172
useTask$(async ({ track }) => {
72-
track(() => context.showSig.value);
73+
const isOpen = track(() => context.showSig.value);
7374

74-
if (context.showSig.value) {
75+
if (isOpen) {
76+
wasOpenSig.value = true;
7577
await context.onShow$?.();
76-
} else {
78+
} else if (wasOpenSig.value) {
7779
await context.onClose$?.();
7880
}
7981
});

packages/kit-headless/src/components/modal/modal.test.ts

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,67 @@ test.describe('Nested Modals', () => {
234234
});
235235
});
236236

237+
test.describe('Callbacks', () => {
238+
test(`GIVEN a modal with bind:show defaulting to false
239+
WHEN the page loads
240+
THEN onClose$ should NOT be called`, async ({ page }) => {
241+
await setup(page, 'test-callbacks');
242+
243+
await expect(page.getByTestId('close-count')).toHaveText('onClose count: 0');
244+
});
245+
246+
test(`GIVEN a modal with bind:show defaulting to false
247+
WHEN the page loads
248+
THEN onShow$ should NOT be called`, async ({ page }) => {
249+
await setup(page, 'test-callbacks');
250+
251+
await expect(page.getByTestId('show-count')).toHaveText('onShow count: 0');
252+
});
253+
254+
test(`GIVEN a closed modal
255+
WHEN the modal is opened
256+
THEN onShow$ should be called once`, async ({ page }) => {
257+
const { driver: d } = await setup(page, 'test-callbacks');
258+
259+
await d.getTrigger().click();
260+
await expect(d.getModal()).toBeVisible();
261+
262+
await expect(page.getByTestId('show-count')).toHaveText('onShow count: 1');
263+
});
264+
265+
test(`GIVEN an open modal
266+
WHEN the modal is closed
267+
THEN onClose$ should be called once`, async ({ page }) => {
268+
const { driver: d } = await setup(page, 'test-callbacks');
269+
270+
await d.getTrigger().click();
271+
await expect(d.getModal()).toBeVisible();
272+
273+
await page.getByRole('button', { name: 'Close' }).click();
274+
await expect(d.getModal()).toBeHidden();
275+
276+
await expect(page.getByTestId('close-count')).toHaveText('onClose count: 1');
277+
});
278+
279+
test(`GIVEN a modal opened and closed twice
280+
WHEN the modal is opened and closed a second time
281+
THEN onShow$ and onClose$ should each have been called twice`, async ({
282+
page,
283+
}) => {
284+
const { driver: d } = await setup(page, 'test-callbacks');
285+
286+
for (let i = 0; i < 2; i++) {
287+
await d.getTrigger().click();
288+
await expect(d.getModal()).toBeVisible();
289+
await page.getByRole('button', { name: 'Close' }).click();
290+
await expect(d.getModal()).toBeHidden();
291+
}
292+
293+
await expect(page.getByTestId('show-count')).toHaveText('onShow count: 2');
294+
await expect(page.getByTestId('close-count')).toHaveText('onClose count: 2');
295+
});
296+
});
297+
237298
test.describe('A11y', () => {
238299
test('Axe Validation Test', async ({ page }) => {
239300
const { driver: d } = await setup(page, 'hero');

0 commit comments

Comments
 (0)