Merge pull request #715 from thejackshelton/fix-popover

fix: popover flakiness
qwikifiers · Apr 22, 2024 · 5481490 · 5481490
2 parents 6ae0665 + 54c42b6
commit 5481490
Show file tree

Hide file tree

Showing 4 changed files with 22 additions and 18 deletions.
diff --git a/apps/website/src/routes/docs/headless/popover/examples/programmatic.tsx b/apps/website/src/routes/docs/headless/popover/examples/programmatic.tsx
@@ -7,9 +7,9 @@ export default component$(() => {
       <button
         class="popover-invoker"
         preventdefault:click
-        onKeyDown$={(e) => {
+        onKeyDown$={async (e) => {
           if (e.key === 'o') {
-            togglePopover();
+            await togglePopover();
           }
         }}
       >

diff --git a/packages/kit-headless/src/components/popover/popover-impl.tsx b/packages/kit-headless/src/components/popover/popover-impl.tsx
@@ -102,7 +102,7 @@ export const PopoverImpl = component$<PopoverImplProps>((props) => {
         hasTopLayerAncestorSig.value = true;
       }
 
-      document.dispatchEvent(new CustomEvent('showpopover'));
+      document.dispatchEvent(new CustomEvent('showpopoverpoly'));
 
       cleanup(() => popoverRef.value);
     }

diff --git a/packages/kit-headless/src/components/popover/popover-trigger.tsx b/packages/kit-headless/src/components/popover/popover-trigger.tsx
@@ -20,6 +20,7 @@ export function usePopover(popovertarget: string) {
 
   const didInteractSig = useSignal<boolean>(false);
   const popoverSig = useSignal<HTMLElement | null>(null);
+  const initialClickSig = useSignal<boolean>(false);
 
   const loadPolyfill$ = $(async () => {
     await import('@oddbird/popover-polyfill');
@@ -42,30 +43,24 @@ export function usePopover(popovertarget: string) {
     didInteractSig.value = true;
   });
 
-  useTask$(({ track }) => {
+  useTask$(async ({ track }) => {
     track(() => didInteractSig.value);
 
     if (!isBrowser) return;
 
     // get popover
     if (!popoverSig.value) {
       popoverSig.value = document.getElementById(popovertarget);
-    }
-
-    // so it only runs once on click for supported browsers
-    if (isSupportedSig.value) {
-      if (!popoverSig.value) return;
 
-      if (popoverSig.value && popoverSig.value.hasAttribute('popover')) {
-        /* opens manual on any event */
-        popoverSig.value.showPopover();
+      if (!initialClickSig.value) {
+        popoverSig.value?.showPopover();
       }
     }
   });
 
   // event is created after teleported properly
   useOnDocument(
-    'showpopover',
+    'showpopoverpoly',
     $(() => {
       if (!didInteractSig.value) return;
 
@@ -74,7 +69,10 @@ 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')) {
+      if (
+        !popoverSig.value.classList.contains(':popover-open') &&
+        !isSupportedSig.value
+      ) {
         popoverSig.value.showPopover();
       }
     }),
@@ -101,12 +99,12 @@ export function usePopover(popovertarget: string) {
     popoverSig.value?.hidePopover();
   });
 
-  return { showPopover, togglePopover, hidePopover, initPopover$ };
+  return { showPopover, togglePopover, hidePopover, initPopover$, initialClickSig };
 }
 
 export const PopoverTrigger = component$<PopoverTriggerProps>(
   ({ popovertarget, disableClickInitPopover = false, ...rest }: PopoverTriggerProps) => {
-    const { initPopover$ } = usePopover(popovertarget);
+    const { initPopover$, initialClickSig } = usePopover(popovertarget);
 
     return (
       <button
@@ -116,6 +114,7 @@ export const PopoverTrigger = component$<PopoverTriggerProps>(
           rest.onClick$,
           !disableClickInitPopover
             ? $(async () => {
+                initialClickSig.value = true;
                 await initPopover$();
               })
             : undefined,

diff --git a/packages/kit-headless/src/components/popover/popover.test.ts b/packages/kit-headless/src/components/popover/popover.test.ts
@@ -236,9 +236,14 @@ test.describe('Keyboard Behavior', () => {
 
     await expect(d.getPopover()).toBeHidden();
 
+    const programmaticTrigger = page.getByRole('button', {
+      name: "Focus me and press the 'o'",
+    });
+
     // Using `page` here because driver is scoped to the popover
-    await page.getByRole('button', { name: "Focus me and press the 'o'" }).focus();
-    await page.keyboard.type('o');
+    await expect(programmaticTrigger).toBeVisible();
+    await programmaticTrigger.focus();
+    await programmaticTrigger.press('o');
 
     await expect(d.getPopover()).toBeVisible();
   });