Merge pull request #792 from warmbly/fix/menus-close-on-email-body-tap

feat: close every dashboard dropdown on a tap inside a same-origin iframe such as the unibox email body
This commit is contained in:
Matthew Meszaros
2026-10-02 05:37:50 +00:00
committed by GitHub
3 changed files with 55 additions and 3 deletions
+1 -1
View File
@@ -306,7 +306,7 @@ Everything in the dashboard must use our own theme, not browser/library defaults
- Row interactions: list rows behave like the campaigns list — clicking anywhere on a row opens that item's detail (drawer or page); right-side action buttons (3-dots / "More") either open a relevant detail/tab or drop a short menu of the actions for that row (the mailbox 3-dots menus Settings and Disconnect). A destructive action belongs in that menu as a `danger` item as well as in the detail's own danger zone, because the selection bar is not where anyone looks to remove one row. Inner interactive controls (checkbox, dropdown trigger, action buttons) must `e.stopPropagation()` so they don't also fire the row's open handler.
- Prefer realtime over polling: subscribe to the socket and `queryClient.invalidateQueries(...)` on the relevant event instead of `refetchInterval` where an event exists (see `useRealtimeEvents` / `RealtimeManager`).
- Interaction details are part of "done". Before calling a dashboard change finished, walk the small things a user hits in the first minute, because these are what make the product feel broken even when the data flow is right:
- every dropdown / popover / picker closes on click-away and on Escape, including when it sits inside a dialog or drawer. Dialog cards stop `mousedown` propagation so the backdrop does not close them; React's `stopPropagation` also stops the native event, so never hand-roll a click-outside listener: every floating layer closes through `useClickOutside` (`@/hooks/useClickOutside`, which `PopoverMenu` uses too). It listens for `pointerdown` in the capture phase, treats a `[data-floating]` layer it opened as inside but the floating panel or dialog holding it as outside, closes on focus moving into an iframe, and takes Escape for the innermost layer only, stopping it there and handing focus back to the trigger. A dialog's own Escape handler still bails out while a `[data-floating]` popover or the `[role="alertdialog"]` confirm is on screen
- every dropdown / popover / picker closes on click-away and on Escape, including when it sits inside a dialog or drawer. Dialog cards stop `mousedown` propagation so the backdrop does not close them; React's `stopPropagation` also stops the native event, so never hand-roll a click-outside listener: every floating layer closes through `useClickOutside` (`@/hooks/useClickOutside`, which `PopoverMenu` uses too). It listens for `pointerdown` in the capture phase, treats a `[data-floating]` layer it opened as inside but the floating panel or dialog holding it as outside, closes on focus moving into an iframe or a tap landing in a same-origin one (a phone moves no focus), and takes Escape for the innermost layer only, stopping it there and handing focus back to the trigger. A dialog's own Escape handler still bails out while a `[data-floating]` popover or the `[role="alertdialog"]` confirm is on screen
- toggles are the shared `Toggle` (sky pill, 32x18) from `campaigns/preferences/components/CampaignPreferenceBoolBox`; never hand-roll a switch. If a whole row toggles on click, the switch itself must `stopPropagation` so it does not toggle twice, and a `<label htmlFor>` pointing at the switch would double-fire too, so use a plain element for the row title
- **a page is not shipped until it is routed, linked and titled.** Three lists have to agree, and nothing fails the build when they do not: the route table in `web/src/main.tsx`, the nav that links to it (`AppNav`, `settings/layout.tsx`), and the title map in `web/src/hooks/useDocumentTitle.ts` (static pathnames in `ROUTE_TITLES`, `:id` routes as a regex in `PARAM_ROUTES`). A nav entry with no route renders nothing; a page component with no route is dead code nobody can reach; a route with no title falls through to the literal `"Page not found | Warmbly"` in the tab, which reads as a broken app on a page that works. All three drift silently, so check them together, and after a merge that touched routing check the settings nav against `main.tsx` specifically
- every detail page reachable from a list has a way back on all viewports: a "← Section" link above the title (see `campaigns/[id]/layout.tsx`) or a back arrow in its header (see `AutomationFlow`); the header breadcrumb is desktop-only and its crumbs must stay clickable, so it does not count as the only route back
+33 -1
View File
@@ -1,5 +1,5 @@
// useClickOutside is how every dropdown closes: a press outside it, Escape (the
// innermost one only), or focus moving into an iframe.
// innermost one only), or focus moving into or a tap landing in an iframe.
import React from "react";
import { createPortal } from "react-dom";
@@ -140,4 +140,36 @@ describe("useClickOutside", () => {
await settle();
expect(isOpen("A")).toBe(false);
});
// A tap on a phone moves no focus, so only the frame's own document hears it.
it("closes on a tap inside a same-origin frame that never takes focus", () => {
render(
<>
<Drop name="A" />
<iframe title="body" />
</>,
);
const frameDoc = (screen.getByTitle("body") as HTMLIFrameElement).contentDocument!;
toggle("A");
fireEvent.pointerDown(frameDoc.body);
expect(isOpen("A")).toBe(false);
// And the listener goes with the layer: reopening still works once.
toggle("A");
expect(isOpen("A")).toBe(true);
fireEvent.pointerDown(frameDoc.body);
expect(isOpen("A")).toBe(false);
});
it("stays open on a press inside a frame it holds", () => {
render(
<Drop name="A">
<iframe title="preview" />
</Drop>,
);
toggle("A");
const frameDoc = (screen.getByTitle("preview") as HTMLIFrameElement).contentDocument!;
fireEvent.pointerDown(frameDoc.body);
expect(isOpen("A")).toBe(true);
});
});
+21 -1
View File
@@ -1,5 +1,5 @@
// The one way a dropdown, popover or picker closes itself: a press anywhere
// outside it, Escape, or another one opening. Every floating layer in the
// outside it (an email body's frame included), Escape, or another one opening. Every floating layer in the
// dashboard goes through this so they all behave the same.
import { useEffect, useRef, type RefObject } from "react";
@@ -79,6 +79,24 @@ export default function useClickOutside(open: boolean, onClose: () => void, insi
if (document.activeElement?.tagName === "IFRAME") self.close();
});
};
// A tap on a phone does not move focus into the frame either, so a
// same-origin frame's own document is listened to as well. A frame
// (re)loading while open is picked up on its load.
const frameDocs = new Set<Document>();
const onFramePress = () => self.close();
const watchFrame = (frame: HTMLIFrameElement) => {
if (isInside(frame)) return;
const doc = frame.contentDocument;
if (!doc || frameDocs.has(doc)) return;
doc.addEventListener("pointerdown", onFramePress, true);
frameDocs.add(doc);
};
const frames = Array.from(document.querySelectorAll("iframe"));
const onFrameLoad = (e: Event) => watchFrame(e.currentTarget as HTMLIFrameElement);
for (const frame of frames) {
watchFrame(frame);
frame.addEventListener("load", onFrameLoad);
}
// Capture phase: dialogs stop mousedown propagation on their card so the
// backdrop does not close them, which would otherwise swallow this too.
// Pointer events so a tap closes it on touch screens as well.
@@ -87,6 +105,8 @@ export default function useClickOutside(open: boolean, onClose: () => void, insi
window.addEventListener("blur", onBlur);
return () => {
clearTimeout(blurTimer);
for (const doc of frameDocs) doc.removeEventListener("pointerdown", onFramePress, true);
for (const frame of frames) frame.removeEventListener("load", onFrameLoad);
document.removeEventListener("pointerdown", onPointerDown, true);
document.removeEventListener("keydown", onKey, true);
window.removeEventListener("blur", onBlur);