diff --git a/.changeset/six-cheetahs-bow.md b/.changeset/six-cheetahs-bow.md new file mode 100644 index 000000000..78f579579 --- /dev/null +++ b/.changeset/six-cheetahs-bow.md @@ -0,0 +1,5 @@ +--- +'@qwik-ui/headless': patch +--- + +fix: popover opening immediately in CSR diff --git a/.changeset/thirty-carrots-sip.md b/.changeset/thirty-carrots-sip.md new file mode 100644 index 000000000..13aa76fa0 --- /dev/null +++ b/.changeset/thirty-carrots-sip.md @@ -0,0 +1,5 @@ +--- +'@qwik-ui/headless': patch +--- + +fix: regression with popover polyfill executing unnecessary code diff --git a/apps/website/src/routes/docs/headless/popover/examples/basic.tsx b/apps/website/src/routes/docs/headless/popover/examples/basic.tsx index dedb43a8e..f2f88d212 100644 --- a/apps/website/src/routes/docs/headless/popover/examples/basic.tsx +++ b/apps/website/src/routes/docs/headless/popover/examples/basic.tsx @@ -3,10 +3,10 @@ import { Popover, PopoverTrigger } from '@qwik-ui/headless'; export default component$(() => { return ( <> - + Popover Trigger - + My Hero! diff --git a/apps/website/src/routes/docs/headless/popover/examples/hero.tsx b/apps/website/src/routes/docs/headless/popover/examples/hero.tsx index 1571e4f1d..cfdf24129 100644 --- a/apps/website/src/routes/docs/headless/popover/examples/hero.tsx +++ b/apps/website/src/routes/docs/headless/popover/examples/hero.tsx @@ -6,14 +6,14 @@ export default component$(() => { return ( <> - + Click me I am anchored to the popover trigger! diff --git a/apps/website/src/routes/docs/headless/popover/examples/hide.tsx b/apps/website/src/routes/docs/headless/popover/examples/hide.tsx new file mode 100644 index 000000000..b825fe181 --- /dev/null +++ b/apps/website/src/routes/docs/headless/popover/examples/hide.tsx @@ -0,0 +1,17 @@ +import { component$ } from '@builder.io/qwik'; +import { Popover, PopoverTrigger, usePopover } from '@qwik-ui/headless'; +export default component$(() => { + const { hidePopover } = usePopover(`hide-id`); + + return ( + <> + + + Click me + + + My Hero! + + + ); +}); diff --git a/apps/website/src/routes/docs/headless/popover/examples/show.tsx b/apps/website/src/routes/docs/headless/popover/examples/show.tsx new file mode 100644 index 000000000..a5dda8db9 --- /dev/null +++ b/apps/website/src/routes/docs/headless/popover/examples/show.tsx @@ -0,0 +1,14 @@ +import { component$ } from '@builder.io/qwik'; +import { Popover, usePopover } from '@qwik-ui/headless'; +export default component$(() => { + const { showPopover } = usePopover(`show-id`); + + return ( + <> + + + My Hero! + + + ); +}); diff --git a/apps/website/src/routes/docs/headless/popover/examples/toggle.tsx b/apps/website/src/routes/docs/headless/popover/examples/toggle.tsx new file mode 100644 index 000000000..ee9f61ed1 --- /dev/null +++ b/apps/website/src/routes/docs/headless/popover/examples/toggle.tsx @@ -0,0 +1,14 @@ +import { component$ } from '@builder.io/qwik'; +import { Popover, usePopover } from '@qwik-ui/headless'; +export default component$(() => { + const { togglePopover } = usePopover(`toggle-id`); + + return ( + <> + + + My Hero! + + + ); +}); diff --git a/packages/kit-headless/src/components/popover/popover-trigger.tsx b/packages/kit-headless/src/components/popover/popover-trigger.tsx index 2a9f9378c..f3b627533 100644 --- a/packages/kit-headless/src/components/popover/popover-trigger.tsx +++ b/packages/kit-headless/src/components/popover/popover-trigger.tsx @@ -21,6 +21,7 @@ export function usePopover(popovertarget: string) { const didInteractSig = useSignal(false); const popoverSig = useSignal(null); const initialClickSig = useSignal(false); + const isCSRSig = useSignal(false); const loadPolyfill$ = $(async () => { await import('@oddbird/popover-polyfill'); @@ -43,7 +44,13 @@ export function usePopover(popovertarget: string) { didInteractSig.value = true; }); - useTask$(async ({ track }) => { + useTask$(() => { + if (isBrowser) { + isCSRSig.value = true; + } + }); + + useTask$(({ track }) => { track(() => didInteractSig.value); if (!isBrowser) return; @@ -51,9 +58,15 @@ export function usePopover(popovertarget: string) { // get popover if (!popoverSig.value) { popoverSig.value = document.getElementById(popovertarget); + } - if (!initialClickSig.value) { - popoverSig.value?.showPopover(); + // so it only runs once on click for supported browsers + if (isSupportedSig.value) { + if (!popoverSig.value) return; + + if (!initialClickSig.value && !isCSRSig.value) { + /* opens manual on any event */ + popoverSig.value.showPopover(); } } }); @@ -69,10 +82,7 @@ export function usePopover(popovertarget: string) { // calls code in here twice for some reason, we think it's because of the client re-render, but it still works // so it only runs once on first click - if ( - !popoverSig.value.classList.contains(':popover-open') && - !isSupportedSig.value - ) { + if (!popoverSig.value.classList.contains(':popover-open')) { popoverSig.value.showPopover(); } }), diff --git a/packages/kit-headless/src/components/popover/popover.test.ts b/packages/kit-headless/src/components/popover/popover.test.ts index f5485ad91..b32c45300 100644 --- a/packages/kit-headless/src/components/popover/popover.test.ts +++ b/packages/kit-headless/src/components/popover/popover.test.ts @@ -37,61 +37,58 @@ test.describe('Mouse Behavior', () => { THEN the popover should close`, async ({ page }) => { const { driver: d } = await setup(page, 'basic'); - await expect(d.getPopover()).toBeHidden(); - await d.getTrigger().click(); - - await expect(d.getPopover()).toBeVisible(); + await d.openPopover('click'); await page.mouse.click(0, 0); await expect(d.getPopover()).toBeHidden(); }); - test(`GIVEN an open popover - WHEN clicking the first trigger on the page and then clicking the second trigger - THEN the first popover should close and the second one appear`, async ({ page }) => { + test(`GIVEN a pair of popovers in auto mode + WHEN clicking the first trigger + AND clicking the second trigger + THEN the first popover should close and the second one appear`, async ({ + page, + }) => { const { driver: d } = await setup(page, 'auto'); const firstPopover = d.getPopover().nth(0); const secondPopover = d.getPopover().nth(1); - const firstTrigger = d.getTrigger().nth(0); - const secondTrigger = d.getTrigger().nth(1); - - await expect(firstPopover).toBeHidden(); - await expect(secondPopover).toBeHidden(); - - await firstTrigger.click({ position: { x: 1, y: 1 } }); - await expect(firstPopover).toBeVisible(); - await secondTrigger.click({ position: { x: 1, y: 1 } }); - await expect(secondPopover).toBeVisible(); + await d.openPopover('click', 0); + await d.openPopover('click', 1); await expect(firstPopover).toBeHidden(); + await expect(secondPopover).toBeVisible(); }); - test(`GIVEN a pair of manual popovers - WHEN clicking the first trigger on the page and then clicking the second trigger - THEN then both popovers should be opened`, async ({ page }) => { + test(`GIVEN an open manual popover + WHEN clicking elsewhere on the page + THEN the popover should remain open`, async ({ page }) => { const { driver: d } = await setup(page, 'manual'); - const firstPopover = d.getPopover().nth(0); - const secondPopover = d.getPopover().nth(1); - const firstTrigger = d.getTrigger().nth(0); - const secondTrigger = d.getTrigger().nth(1); + // initial open + await d.openPopover('click', 0); - await expect(firstPopover).toBeHidden(); - await expect(secondPopover).toBeHidden(); + await page.mouse.click(0, 0); - await firstTrigger.click({ position: { x: 1, y: 1 } }); - await secondTrigger.click({ position: { x: 1, y: 1 } }); + await expect(d.getPopover().nth(0)).toBeVisible(); + }); - await expect(firstPopover).toBeVisible(); - await expect(secondPopover).toBeVisible(); + test(`GIVEN a pair of manual popovers + WHEN clicking the first trigger on the page + AND then clicking the second trigger + THEN then both popovers should be opened`, async ({ page }) => { + const { driver: d } = await setup(page, 'manual'); + + await d.openPopover('click', 0); + await d.openPopover('click', 1); }); test(`GIVEN a pair of manual opened popovers - WHEN clicking the first trigger on the page and then clicking the second trigger - THEN then both popovers should be closed`, async ({ page }) => { + WHEN clicking the first trigger on the page + AND clicking the second trigger + THEN both popovers should be closed`, async ({ page }) => { const { driver: d } = await setup(page, 'manual'); const firstPopover = d.getPopover().nth(0); @@ -102,9 +99,8 @@ test.describe('Mouse Behavior', () => { await d.openPopover('click', 0); await d.openPopover('click', 1); - // Need to be explicit about where we're clicking. By default - // the click action tries to click the center of the element - // but in this case, the popover is covering it. + // Explicitly specifying click positions due to default behavior targeting the element's center, + // which is obscured by the popover in this scenario. await firstTrigger.click({ position: { x: 1, y: 1 } }); await secondTrigger.click({ position: { x: 1, y: 1 } }); @@ -136,8 +132,8 @@ test.describe('Mouse Behavior', () => { }); test(`GIVEN a popover with a gutter configured - WHEN opening the popover - THEN the popover should be spaced 40px from the popover`, async ({ page }) => { + WHEN opening the popover + THEN the popover should be spaced 40px from the popover`, async ({ page }) => { const { driver: d } = await setup(page, 'gutter'); const popover = d.getPopover(); @@ -210,7 +206,7 @@ test.describe('Keyboard Behavior', () => { test(`GIVEN an open popover WHEN focusing on the button and pressing the 'Escape' key - THEN the popover should close and the trigger be focused`, async ({ page }) => { + THEN the popover should close and the trigger focused`, async ({ page }) => { const { driver: d } = await setup(page, 'basic'); // Open the popover @@ -241,100 +237,83 @@ test.describe('Keyboard Behavior', () => { await expect(d.getPopover()).toBeVisible(); }); - test(`GIVEN an open auto popover - WHEN the first trigger opens - AND the focus changes to the second popover - THEN the first popover should close and the second one appear`, async ({ - page, - }) => { - const { driver: d } = await setup(page, 'auto'); - const firstPopover = d.getPopover().nth(0); - const secondPopover = d.getPopover().nth(1); - const firstTrigger = d.getTrigger().nth(0); - const secondTrigger = d.getTrigger().nth(1); + test.describe('auto', () => { + test(`GIVEN a pair of auto popovers + WHEN one popover is open with the enter key + AND another popover is open with the enter key + THEN the first popover should close and the second one appear`, async ({ page }) => { + const { driver: d } = await setup(page, 'auto'); - await expect(firstPopover).toBeHidden(); - await expect(secondPopover).toBeHidden(); + const firstPopover = d.getPopover().nth(0); + const secondPopover = d.getPopover().nth(1); - await d.openPopover('Enter', 0); - await firstTrigger.press('Tab'); - - await expect(secondTrigger).toBeFocused(); - await d.openPopover('Enter', 1); + await expect(firstPopover).toBeHidden(); + await expect(secondPopover).toBeHidden(); - await expect(firstPopover).toBeHidden(); - }); + await d.openPopover('Enter', 0); + await d.openPopover('Enter', 1); - test(`GIVEN a pair of manual popovers - WHEN clicking the first trigger on the page and then clicking the second trigger - THEN then both popovers should be opened`, async ({ page }) => { - const { driver: d } = await setup(page, 'manual'); - - await d.openPopover('click', 0); - await d.openPopover('click', 1); + await expect(firstPopover).toBeHidden(); + }); }); test(`GIVEN a pair of manual opened popovers - WHEN activating the first trigger on the page and then activating the second trigger - THEN then both popovers should be closed`, async ({ page }) => { + WHEN pressing enter on the first trigger on the page + AND the same on the second trigger + THEN then both popovers should be closed`, async ({ page }) => { const { driver: d } = await setup(page, 'manual'); - const [firstPopOver, secondPopOver] = await d.getAllPopovers(); - const [firstPopoverTrigger, secondPopoverTrigger] = await d.getAllTriggers(); - - // Arrange - await firstPopoverTrigger.press('Enter'); - - await secondPopoverTrigger.press('Enter'); + const firstPopover = d.getPopover().nth(0); + const secondPopover = d.getPopover().nth(1); + const firstTrigger = d.getTrigger().nth(0); + const secondTrigger = d.getTrigger().nth(1); - await expect(firstPopOver).toBeVisible(); - await expect(secondPopOver).toBeVisible(); + await d.openPopover('Enter', 0); + await d.openPopover('Enter', 1); - // Act - await secondPopoverTrigger.press('Enter'); - await expect(secondPopOver).toBeHidden(); + await secondTrigger.press('Enter'); + await expect(secondPopover).toBeHidden(); // Assert - await firstPopoverTrigger.press('Enter'); - await expect(firstPopOver).toBeHidden(); + await firstTrigger.press('Enter'); + await expect(firstPopover).toBeHidden(); }); - test(`GIVEN a programmatic popover - WHEN focusing the button on the page and then typing 'o' - THEN the popover should open`, async ({ page }) => { + test(`GIVEN a popover + WHEN programmatically toggling the popover + THEN the popover should open`, async ({ page }) => { const { driver: d } = await setup(page, 'programmatic'); const popover = d.getPopover(); - const programmaticButtonTrigger = d.getProgrammaticButtonTrigger(); + const programmaticTrigger = page.getByRole('button'); await expect(popover).toBeHidden(); - await programmaticButtonTrigger.press('o'); + await programmaticTrigger.press('o'); await expect(popover).toBeVisible(); }); - test(`GIVEN an open programmatic popover - WHEN focusing the button on the page and then typing 'o' - THEN the popover should close`, async ({ page }) => { + test(`GIVEN a popover + WHEN programmatically toggling the popover + THEN the popover should close`, async ({ page }) => { const { driver: d } = await setup(page, 'programmatic'); + // initial open const popover = d.getPopover(); - const programmaticButtonTrigger = d.getProgrammaticButtonTrigger(); - - await programmaticButtonTrigger.press('o'); - + const programmaticTrigger = page.getByRole('button'); + await programmaticTrigger.press('o'); await expect(popover).toBeVisible(); - await programmaticButtonTrigger.press('o'); + await programmaticTrigger.press('o'); await expect(popover).toBeHidden(); }); test(`GIVEN a popover with placement set to top - WHEN opening the popover using the keyboard - THEN the popover should appear to the right of the trigger`, async ({ page }) => { + WHEN opening the popover using the keyboard + THEN the popover should appear to the right of the trigger`, async ({ page }) => { const { driver: d } = await setup(page, 'placement'); const popover = d.getPopover(); @@ -353,3 +332,43 @@ test.describe('Keyboard Behavior', () => { ); }); }); + +test.describe('Programmatic', () => { + // test(`GIVEN a programmatic popover + // WHEN the showPopover function is called + // THEN the popover should be open`, async ({ page }) => { + // const { driver: d } = await setup(page, 'show'); + // await expect(d.getPopover()).toBeHidden(); + // const programmaticTrigger = page.getByRole('button', { name: 'show popover' }); + // await programmaticTrigger.click(); + // await expect(d.getPopover()).toBeVisible(); + // }); + // test(`GIVEN an open programmatic popover + // WHEN the hidePopover function is called + // THEN the popover should be hidden`, async ({ page }) => { + // const { driver: d } = await setup(page, 'hide'); + // const programmaticTrigger = page.getByRole('button', { name: 'hide popover' }); + // // initial open + // await d.openPopover('click'); + // await programmaticTrigger.click({ position: { x: 0, y: 0 } }); + // await expect(d.getPopover()).toBeHidden(); + // }); + // test(`GIVEN a programmatic popover + // WHEN the togglePopover function is called + // THEN the popover should be open`, async ({ page }) => { + // const { driver: d } = await setup(page, 'toggle'); + // const programmaticTrigger = page.getByRole('button', { name: 'toggle popover' }); + // await programmaticTrigger.click(); + // await expect(d.getPopover()).toBeVisible(); + // }); + // test(`GIVEN an open programmatic popover + // WHEN the togglePopover function is called + // THEN the popover should be closed`, async ({ page }) => { + // const { driver: d } = await setup(page, 'toggle'); + // const programmaticTrigger = page.getByRole('button', { name: 'toggle popover' }); + // await programmaticTrigger.click(); + // await expect(d.getPopover()).toBeVisible(); + // await programmaticTrigger.click(); + // await expect(d.getPopover()).toBeHidden(); + // }); +});