From 0523043958d96fa1dfdbbe9fb714b092063d7d26 Mon Sep 17 00:00:00 2001 From: jack shelton Date: Mon, 22 Apr 2024 11:15:17 -0500 Subject: [PATCH 1/4] refactor: initial popover tests --- .../src/components/popover/popover.driver.ts | 20 +++- .../src/components/popover/popover.test.ts | 91 ++++++++++--------- 2 files changed, 65 insertions(+), 46 deletions(-) diff --git a/packages/kit-headless/src/components/popover/popover.driver.ts b/packages/kit-headless/src/components/popover/popover.driver.ts index d496b5656..139d1041d 100644 --- a/packages/kit-headless/src/components/popover/popover.driver.ts +++ b/packages/kit-headless/src/components/popover/popover.driver.ts @@ -1,7 +1,9 @@ -import { type Locator, type Page } from '@playwright/test'; +import { expect, type Locator, type Page } from '@playwright/test'; export type DriverLocator = Locator | Page; +type OpenKeys = 'Enter' | 'Space'; + export function createTestDriver(rootLocator: T) { const getPopover = () => { return rootLocator.locator('[popover]'); @@ -11,6 +13,21 @@ export function createTestDriver(rootLocator: T) { return rootLocator.locator('[popovertarget]'); }; + const openPopover = async (key: OpenKeys | 'click', index?: number) => { + const action = key === 'click' ? 'click' : 'press'; + const trigger = index !== undefined ? getTrigger().nth(index) : getTrigger(); + const popover = index !== undefined ? getPopover().nth(index) : getPopover(); + + if (action === 'click') { + await trigger.click({ position: { x: 1, y: 1 } }); // Modified line + } else { + await trigger.press(key); + } + + // Needed because Playwright doesn't wait for the listbox to be visible + await expect(popover).toBeVisible(); + }; + const getAllPopovers = () => { return getPopover().all(); }; @@ -30,6 +47,7 @@ export function createTestDriver(rootLocator: T) { getAllPopovers, getTrigger, getAllTriggers, + openPopover, getProgrammaticButtonTrigger, }; } diff --git a/packages/kit-headless/src/components/popover/popover.test.ts b/packages/kit-headless/src/components/popover/popover.test.ts index a5623344f..c00bd7105 100644 --- a/packages/kit-headless/src/components/popover/popover.test.ts +++ b/packages/kit-headless/src/components/popover/popover.test.ts @@ -21,7 +21,7 @@ test('@Visual diff', async ({ page }) => { }); test.describe('Mouse Behavior', () => { - test(`GIVEN a closed hero popover + test(`GIVEN a closed popover WHEN clicking on the trigger THEN the popover should be opened `, async ({ page }) => { const { driver: d } = await setup(page, 'hero'); @@ -32,7 +32,7 @@ test.describe('Mouse Behavior', () => { await expect(d.getPopover()).toBeVisible(); }); - test(`GIVEN an open hero popover + test(`GIVEN an open popover WHEN clicking elsewhere on the page THEN the popover should close`, async ({ page }) => { const { driver: d } = await setup(page, 'hero'); @@ -40,32 +40,33 @@ test.describe('Mouse Behavior', () => { await expect(d.getPopover()).toBeHidden(); await d.getTrigger().click(); - // If I use `toBeVisible` here, the test fails that the `toBeHidden` check below???? - await expect(d.getPopover()).toHaveCSS('opacity', '1'); + await expect(d.getPopover()).toBeVisible(); await page.mouse.click(0, 0); await expect(d.getPopover()).toBeHidden(); }); - test(`GIVEN an open auto popover + 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 }) => { const { driver: d } = await setup(page, 'auto'); - //ask shai: is it good to use nth here??? - const [firstPopOver, secondPopOver] = await d.getAllPopovers(); - const [firstPopoverTrigger, secondPopoverTrigger] = await d.getAllTriggers(); - await expect(firstPopOver).toBeHidden(); - await expect(secondPopOver).toBeHidden(); + 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 firstPopoverTrigger.click({ position: { x: 1, y: 1 } }); - await expect(firstPopOver).toBeVisible(); + await expect(firstPopover).toBeHidden(); + await expect(secondPopover).toBeHidden(); - await secondPopoverTrigger.click({ position: { x: 1, y: 1 } }); - await expect(secondPopOver).toBeVisible(); + await firstTrigger.click({ position: { x: 1, y: 1 } }); + await expect(firstPopover).toBeVisible(); - await expect(firstPopOver).toBeHidden(); + await secondTrigger.click({ position: { x: 1, y: 1 } }); + await expect(secondPopover).toBeVisible(); + + await expect(firstPopover).toBeHidden(); }); test(`GIVEN a pair of manual popovers @@ -73,18 +74,19 @@ test.describe('Mouse Behavior', () => { THEN then both popovers should be opened`, async ({ page }) => { const { driver: d } = await setup(page, 'manual'); - //ask shai: is it good to use nth here??? - const [firstPopOver, secondPopOver] = await d.getAllPopovers(); - const [firstPopoverTrigger, secondPopoverTrigger] = await d.getAllTriggers(); + 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 expect(firstPopover).toBeHidden(); + await expect(secondPopover).toBeHidden(); - await firstPopoverTrigger.click({ position: { x: 1, y: 1 } }); - await secondPopoverTrigger.click({ position: { x: 1, y: 1 } }); + await firstTrigger.click({ position: { x: 1, y: 1 } }); + await secondTrigger.click({ position: { x: 1, y: 1 } }); - await expect(firstPopOver).toBeVisible(); - await expect(secondPopOver).toBeVisible(); + await expect(firstPopover).toBeVisible(); + await expect(secondPopover).toBeVisible(); }); test(`GIVEN a pair of manual opened popovers @@ -92,30 +94,28 @@ test.describe('Mouse Behavior', () => { 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.click({ position: { x: 1, y: 1 } }); - await secondPopoverTrigger.click({ position: { x: 1, y: 1 } }); + 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('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. - await firstPopoverTrigger.click({ position: { x: 1, y: 1 } }); - await secondPopoverTrigger.click({ position: { x: 1, y: 1 } }); + await firstTrigger.click({ position: { x: 1, y: 1 } }); + await secondTrigger.click({ position: { x: 1, y: 1 } }); // Assert - await expect(firstPopOver).toBeHidden(); - await expect(secondPopOver).toBeHidden(); + await expect(firstPopover).toBeHidden(); + await expect(secondPopover).toBeHidden(); }); - test(`GIVEN a popover with placement set to top - WHEN opening the popover - THEN the popover should appear to the right of the trigger`, async ({ page }) => { + test(`GIVEN a popover with placement set to right + WHEN hovering over the popover + THEN the popover should appear to the right of the trigger`, async ({ page }) => { const { driver: d } = await setup(page, 'placement'); const popover = d.getPopover(); @@ -128,10 +128,11 @@ test.describe('Mouse Behavior', () => { const popoverBoundingBox = await popover.boundingBox(); const triggerBoundingBox = await trigger.boundingBox(); - expect(popoverBoundingBox?.x).toBeGreaterThan( + const triggerRightEdge = (triggerBoundingBox?.x ?? Number.MAX_VALUE) + - (triggerBoundingBox?.width ?? Number.MAX_VALUE), - ); + (triggerBoundingBox?.width ?? Number.MAX_VALUE); + + expect(popoverBoundingBox?.x).toBeGreaterThan(triggerRightEdge); }); test(`GIVEN a popover with a gutter configured @@ -149,10 +150,10 @@ test.describe('Mouse Behavior', () => { const popoverBoundingBox = await popover.boundingBox(); const triggerBoundingBox = await trigger.boundingBox(); - expect( + const gutterSpace = (triggerBoundingBox?.y ?? 0) - - ((popoverBoundingBox?.y ?? 0) + (popoverBoundingBox?.height ?? 0)), - ).toBe(40); + ((popoverBoundingBox?.y ?? 0) + (popoverBoundingBox?.height ?? 0)); + expect(gutterSpace).toBe(40); }); // test(`GIVEN a combobox with a flip configured From c3c2a9e75809e8713e3c48beaf6d24552426b3b3 Mon Sep 17 00:00:00 2001 From: jack shelton Date: Mon, 22 Apr 2024 12:39:08 -0500 Subject: [PATCH 2/4] refactor: improve middle popover tests --- .../src/components/popover/popover.driver.ts | 4 +- .../src/components/popover/popover.test.ts | 82 ++++++++----------- 2 files changed, 36 insertions(+), 50 deletions(-) diff --git a/packages/kit-headless/src/components/popover/popover.driver.ts b/packages/kit-headless/src/components/popover/popover.driver.ts index 139d1041d..080b3eeca 100644 --- a/packages/kit-headless/src/components/popover/popover.driver.ts +++ b/packages/kit-headless/src/components/popover/popover.driver.ts @@ -2,7 +2,7 @@ import { expect, type Locator, type Page } from '@playwright/test'; export type DriverLocator = Locator | Page; -type OpenKeys = 'Enter' | 'Space'; +export type PopoverOpenKeys = 'Enter' | 'Space'; export function createTestDriver(rootLocator: T) { const getPopover = () => { @@ -13,7 +13,7 @@ export function createTestDriver(rootLocator: T) { return rootLocator.locator('[popovertarget]'); }; - const openPopover = async (key: OpenKeys | 'click', index?: number) => { + const openPopover = async (key: PopoverOpenKeys | 'click', index?: number) => { const action = key === 'click' ? 'click' : 'press'; const trigger = index !== undefined ? getTrigger().nth(index) : getTrigger(); const popover = index !== undefined ? getPopover().nth(index) : getPopover(); diff --git a/packages/kit-headless/src/components/popover/popover.test.ts b/packages/kit-headless/src/components/popover/popover.test.ts index c00bd7105..06ab91b10 100644 --- a/packages/kit-headless/src/components/popover/popover.test.ts +++ b/packages/kit-headless/src/components/popover/popover.test.ts @@ -1,5 +1,5 @@ import { expect, test, type Page } from '@playwright/test'; -import { createTestDriver } from './popover.driver'; +import { PopoverOpenKeys, createTestDriver } from './popover.driver'; async function setup(page: Page, exampleName: string) { await page.goto(`/headless/popover/${exampleName}`); @@ -184,44 +184,37 @@ test.describe('Mouse Behavior', () => { }); test.describe('Keyboard Behavior', () => { - for (const key of ['Enter', 'Space']) { - test(`GIVEN a closed hero popover - WHEN focusing on the button and pressing the '${key}' key - THEN the popover should open`, async ({ page }) => { + for (const key of ['Enter', 'Space'] as PopoverOpenKeys[]) { + test(`GIVEN a closed popover + WHEN focusing on the button and pressing the '${key}' key + THEN the popover should open`, async ({ page }) => { const { driver: d } = await setup(page, 'hero'); await expect(d.getPopover()).toBeHidden(); await d.getTrigger().press(key); - await expect(d.getPopover()).toBeVisible(); }); - test(`GIVEN a open hero popover - WHEN focusing on the button and pressing the '${key}' key - THEN the popover should close`, async ({ page }) => { + test(`GIVEN an open popover + WHEN focusing on the button and pressing the '${key}' key + THEN the popover should close`, async ({ page }) => { const { driver: d } = await setup(page, 'hero'); // Open the popover - await d.getTrigger().press(key); + await d.openPopover(key); - await expect(d.getPopover()).toBeVisible(); - - // Close the popover await d.getTrigger().press(key); - await expect(d.getPopover()).toBeHidden(); }); } - test(`GIVEN a open hero popover - WHEN focusing on the button and pressing the 'Escape' key - THEN the popover should close and the trigger be focused`, async ({ page }) => { + 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 }) => { const { driver: d } = await setup(page, 'hero'); // Open the popover - await d.getTrigger().press('Enter'); - - await expect(d.getPopover()).toBeVisible(); + await d.openPopover('Enter'); // Close the popover page.keyboard.press('Escape'); @@ -230,9 +223,9 @@ test.describe('Keyboard Behavior', () => { await expect(d.getTrigger()).toBeFocused(); }); - test(`GIVEN a programmatic popover with a programmatic trigger - WHEN focusing on the button and pressing the 'o' key - THEN the popover should open`, async ({ page }) => { + test(`GIVEN a popover + WHEN focusing on the button and pressing the 'o' key + THEN the popover should be programmatically opened`, async ({ page }) => { const { driver: d } = await setup(page, 'programmatic'); await expect(d.getPopover()).toBeHidden(); @@ -249,25 +242,28 @@ test.describe('Keyboard Behavior', () => { await expect(d.getPopover()).toBeVisible(); }); test(`GIVEN an open auto popover - WHEN the first trigger open and the focus changes to the second popover - THEN the first popover should close and the second one appear`, async ({ page }) => { + 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, secondPopOver] = await d.getAllPopovers(); - const [firstPopoverTrigger, secondPopoverTrigger] = await d.getAllTriggers(); + 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 expect(firstPopover).toBeHidden(); + await expect(secondPopover).toBeHidden(); - await firstPopoverTrigger.press('Enter'); - await expect(firstPopOver).toBeVisible(); - await firstPopoverTrigger.press('Tab'); - await expect(secondPopoverTrigger).toBeFocused(); + await d.openPopover('Enter', 0); + await firstTrigger.press('Tab'); - await secondPopoverTrigger.press('Enter'); - await expect(secondPopOver).toBeVisible(); + await expect(secondTrigger).toBeFocused(); + await d.openPopover('Enter', 1); - await expect(firstPopOver).toBeHidden(); + await expect(firstPopover).toBeHidden(); }); test(`GIVEN a pair of manual popovers @@ -275,18 +271,8 @@ test.describe('Keyboard Behavior', () => { THEN then both popovers should be opened`, async ({ page }) => { const { driver: d } = await setup(page, 'manual'); - const [firstPopOver, secondPopOver] = await d.getAllPopovers(); - const [firstPopoverTrigger, secondPopoverTrigger] = await d.getAllTriggers(); - - await expect(firstPopOver).toBeHidden(); - await expect(secondPopOver).toBeHidden(); - - await firstPopoverTrigger.press('Enter'); - - await secondPopoverTrigger.press('Enter'); - - await expect(firstPopOver).toBeVisible(); - await expect(secondPopOver).toBeVisible(); + await d.openPopover('click', 0); + await d.openPopover('click', 1); }); test(`GIVEN a pair of manual opened popovers From 64c18cee5a2c071d0c2c9bd3541306440e31e55a Mon Sep 17 00:00:00 2001 From: jack shelton Date: Mon, 22 Apr 2024 15:50:13 -0500 Subject: [PATCH 3/4] add changeset --- .changeset/blue-spiders-flash.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/blue-spiders-flash.md diff --git a/.changeset/blue-spiders-flash.md b/.changeset/blue-spiders-flash.md new file mode 100644 index 000000000..d777a2955 --- /dev/null +++ b/.changeset/blue-spiders-flash.md @@ -0,0 +1,5 @@ +--- +'@qwik-ui/headless': patch +--- + +fix: race condition in the popover when programmatically opening on supported browsers From 1ff2d380dd011920706e3d15bec27f3a9c07a67a Mon Sep 17 00:00:00 2001 From: jack shelton Date: Mon, 22 Apr 2024 16:09:43 -0500 Subject: [PATCH 4/4] docs: improved popover intro --- .../docs/headless/popover/examples/basic.tsx | 14 ++++++++++++++ .../docs/headless/popover/examples/hero.tsx | 19 ++++++++++++++----- .../routes/docs/headless/popover/index.mdx | 14 ++++++++++++-- .../src/components/popover/popover.test.ts | 12 ++++++------ 4 files changed, 46 insertions(+), 13 deletions(-) create mode 100644 apps/website/src/routes/docs/headless/popover/examples/basic.tsx diff --git a/apps/website/src/routes/docs/headless/popover/examples/basic.tsx b/apps/website/src/routes/docs/headless/popover/examples/basic.tsx new file mode 100644 index 000000000..dedb43a8e --- /dev/null +++ b/apps/website/src/routes/docs/headless/popover/examples/basic.tsx @@ -0,0 +1,14 @@ +import { component$ } from '@builder.io/qwik'; +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 dedb43a8e..1571e4f1d 100644 --- a/apps/website/src/routes/docs/headless/popover/examples/hero.tsx +++ b/apps/website/src/routes/docs/headless/popover/examples/hero.tsx @@ -1,13 +1,22 @@ -import { component$ } from '@builder.io/qwik'; +import { component$, useSignal } from '@builder.io/qwik'; import { Popover, PopoverTrigger } from '@qwik-ui/headless'; + export default component$(() => { + const triggerRef = useSignal(); + return ( <> - - Popover Trigger + + Click me - - My Hero! + + I am anchored to the popover trigger! ); diff --git a/apps/website/src/routes/docs/headless/popover/index.mdx b/apps/website/src/routes/docs/headless/popover/index.mdx index f745e65e4..b7f4badae 100644 --- a/apps/website/src/routes/docs/headless/popover/index.mdx +++ b/apps/website/src/routes/docs/headless/popover/index.mdx @@ -23,9 +23,17 @@ A non-modal primitive with overlays that open and close around a DOM element. ]} /> + + You are about to see a fixed scrolling popover. Yes, this behavior is intended. It is + part of the native popover API! If you want to anchor items, there is a floating section + that covers this behavior. + + + + The Qwik UI Popover component is designed to align with the [HTML Popover API Specification](https://developer.mozilla.org/en-US/docs/Web/API/Popover_API). As of now, native support for this API is around `82.5%`, and in almost every browser. -To ensure consistent behavior across all browsers, Qwik UI provides a `polyfill` for browser versions that do not support the API natively. +To ensure consistent behavior across all browsers, Qwik UI provides a `polyfill` with feature parity for browser versions that do not support the API natively.
@@ -296,7 +304,9 @@ useTask$(async ({ track }) => { To use the popover API with floating elements, you can add the `floating={true}` prop to the Popover component. This API enables the use of JavaScript to dynamically position the listbox using the `top` & `left` absolute properties. -> Enabling the `floating={true}` property will introduce a slight increase in code size, as we currently utilize JavaScript to implement floating items. We've strived to keep it as minimal as possible, but powerful in building complex components in anticipation of the forthcoming anchor API. +Enabling the `floating={true}` property will introduce a slight increase in code size, as we currently utilize JavaScript to implement floating items. We've strived to keep it as minimal as possible, but powerful in building complex components. + +> Once the native anchor API is available in all browsers, you can remove the floating prop and use CSS instead! Smaller bundle size for the win! To float an element, it must have an anchored element to latch onto. This can be done with the `anchorRef` prop. diff --git a/packages/kit-headless/src/components/popover/popover.test.ts b/packages/kit-headless/src/components/popover/popover.test.ts index 06ab91b10..f5485ad91 100644 --- a/packages/kit-headless/src/components/popover/popover.test.ts +++ b/packages/kit-headless/src/components/popover/popover.test.ts @@ -12,7 +12,7 @@ async function setup(page: Page, exampleName: string) { } test('@Visual diff', async ({ page }) => { - const { driver: d } = await setup(page, 'hero'); + const { driver: d } = await setup(page, 'basic'); await expect(page).toHaveScreenshot('closed popover.png'); await d.getTrigger().click(); @@ -24,7 +24,7 @@ test.describe('Mouse Behavior', () => { test(`GIVEN a closed popover WHEN clicking on the trigger THEN the popover should be opened `, async ({ page }) => { - const { driver: d } = await setup(page, 'hero'); + const { driver: d } = await setup(page, 'basic'); await expect(d.getPopover()).toBeHidden(); await d.getTrigger().click(); @@ -35,7 +35,7 @@ test.describe('Mouse Behavior', () => { test(`GIVEN an open popover WHEN clicking elsewhere on the page THEN the popover should close`, async ({ page }) => { - const { driver: d } = await setup(page, 'hero'); + const { driver: d } = await setup(page, 'basic'); await expect(d.getPopover()).toBeHidden(); await d.getTrigger().click(); @@ -188,7 +188,7 @@ test.describe('Keyboard Behavior', () => { test(`GIVEN a closed popover WHEN focusing on the button and pressing the '${key}' key THEN the popover should open`, async ({ page }) => { - const { driver: d } = await setup(page, 'hero'); + const { driver: d } = await setup(page, 'basic'); await expect(d.getPopover()).toBeHidden(); await d.getTrigger().press(key); @@ -198,7 +198,7 @@ test.describe('Keyboard Behavior', () => { test(`GIVEN an open popover WHEN focusing on the button and pressing the '${key}' key THEN the popover should close`, async ({ page }) => { - const { driver: d } = await setup(page, 'hero'); + const { driver: d } = await setup(page, 'basic'); // Open the popover await d.openPopover(key); @@ -211,7 +211,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 }) => { - const { driver: d } = await setup(page, 'hero'); + const { driver: d } = await setup(page, 'basic'); // Open the popover await d.openPopover('Enter');