refactor(web): the six click-outside menus dismiss through the shared clickOutside primitive, so no menu carries a hand-rolled window click closer (BUG-3231) (#1580)

Six menus closed on a window or document click whose target lay outside
them: the FieldEditor select dropdown, the BoardView lane menu,
EmojiPickerButton, ReactionPicker, the TopBar workspace overflow menu and
the console mobile nav. Each now uses lib/utils/clickOutside.ts, which
decides on a capture-phase press outside the node and its extras, and the
hand-rolled handlers are gone.

No bug claim: the drag-out symptom the BUG-3229 census predicted for these
menus did not reproduce in Chromium on main (real-mouse e2e, two menus).

The TopBar overflow menu exempts the visible workspace pills, whose press may
start a drag into it, and handleWsClick closes it for every real pill click,
modifier-clicks included.

Claude-Session: https://claude.ai/code/session_01NidgPePY7UvHcEJCMkD2Qz
This commit is contained in:
xarmian
2026-09-26 14:08:07 -04:00
committed by GitHub
parent bda30156b3
commit ccd5febfff
10 changed files with 276 additions and 112 deletions
@@ -0,0 +1,85 @@
import { test, expect } from './fixtures';
import { browserLogin } from './lib/collab-helpers';
import { authJson } from './lib/attachment-viewer';
import type { Locator, Page } from '@playwright/test';
/**
* BUG-3231: menus that closed on a window CLICK outside them closed when a drag
* started inside and was released outside, because that click lands on a
* common ancestor outside the menu. They now dismiss through `clickOutside`,
* which decides on the press.
*
* Each leg drags with a real mouse from inside the menu to a point outside it
* and asserts the menu is still open, then presses that point and asserts it
* closes, so a menu that simply stopped closing fails the second half.
*/
const DESKTOP = { width: 1280, height: 800 };
const OUTSIDE = { x: 640, y: DESKTOP.height - 20 };
async function dragOut(page: Page, from: Locator) {
const box = await from.boundingBox();
if (!box) throw new Error('drag source has no box');
await page.mouse.move(box.x + 4, box.y + box.height / 2);
await page.mouse.down();
await page.mouse.move(box.x + box.width / 2, box.y + box.height / 2, { steps: 4 });
await page.mouse.move(OUTSIDE.x, OUTSIDE.y, { steps: 10 });
await page.mouse.up();
}
test.describe('BUG-3231: a drag started inside a menu and released outside does not close it', () => {
test.beforeEach(async ({ page }, testInfo) => {
test.skip(testInfo.project.name !== 'desktop-chromium', 'a mouse-drag concern; one desktop browser is enough');
await page.setViewportSize(DESKTOP);
await browserLogin(page);
});
test('field select dropdown (FieldEditor)', async ({ page, fixture, request }) => {
const stamp = Date.now();
const resp = await request.post(`/api/v1/workspaces/${fixture.workspaceSlug}/collections/tasks/items`, {
headers: authJson(fixture),
data: { title: `Menu drag ${stamp}`, content: '', fields: JSON.stringify({ status: 'open' }) },
});
expect(resp.ok(), await resp.text()).toBeTruthy();
const { slug } = (await resp.json()) as { slug: string };
await page.goto(`/${fixture.adminUsername}/${fixture.workspaceSlug}/tasks/${slug}`);
const trigger = page.locator('.select-trigger', { hasText: 'Open' }).first();
await expect(trigger).toBeVisible({ timeout: 15_000 });
await trigger.click();
const list = page.locator('.select-dropdown[role="listbox"]');
await expect(list).toBeVisible();
await dragOut(page, list.getByRole('option').first());
await expect(list).toBeVisible();
await page.mouse.click(OUTSIDE.x, OUTSIDE.y);
await expect(list).toBeHidden();
});
test('board lane actions menu, tag input (BoardView)', async ({ page, fixture, request }) => {
const stamp = Date.now();
const resp = await request.post(`/api/v1/workspaces/${fixture.workspaceSlug}/collections/tasks/items`, {
headers: authJson(fixture),
data: { title: `Lane drag ${stamp}`, content: '', fields: JSON.stringify({ status: 'open' }) },
});
expect(resp.ok(), await resp.text()).toBeTruthy();
await page.goto(`/${fixture.adminUsername}/${fixture.workspaceSlug}/tasks?view=board`);
const kebab = page.locator('.lane-menu-btn').first();
await expect(kebab).toBeVisible({ timeout: 15_000 });
await kebab.click();
const first = page.getByRole('menuitem').first();
await expect(first).toBeVisible();
// Press a menu row and release outside: the click lands on a common
// ancestor outside the menu, which the old window closer took for an
// outside click. (A drag out of the tag INPUT does not reproduce in
// Chromium, whose text-selection drag keeps the release on the input.)
await dragOut(page, first);
await expect(first).toBeVisible();
await page.mouse.click(OUTSIDE.x, OUTSIDE.y);
await expect(first).toBeHidden();
});
});
@@ -28,6 +28,7 @@
import EmptyState from '../common/EmptyState.svelte';
import LaneActionsMenu from './LaneActionsMenu.svelte';
import { viewport } from '$lib/stores/breakpoint.svelte';
import { clickOutside } from '$lib/utils/clickOutside';
import { draftKey, lostLaneLabel, type DraftSaveTarget } from '$lib/collections/laneDrafts';
@@ -237,12 +238,6 @@
// Dismiss the open lane menu on any click outside it (mirrors the
// QuickActionsMenu pattern). The menu markup lives under
// `.lane-menu-wrap`, so clicks there don't close it.
function handleWindowClick(e: MouseEvent) {
if (openMenuColumn === null) return;
const target = e.target as HTMLElement | null;
if (!target) return;
if (!target.closest('.lane-menu-wrap')) closeMenu();
}
const flipDurationMs = 200;
const touchDragDelayMs = 500;
@@ -785,7 +780,6 @@
}
</script>
<svelte:window onclick={handleWindowClick} />
{#if items.length === 0}
<EmptyState {collection} {wsSlug} {oncreate} />
@@ -917,7 +911,11 @@
<!-- Kebab shows for create OR any non-empty lane (sort is
always available, even to viewers — TASK-1673). -->
{#if onCreateInColumn || colItems.length > 0}
<div class="lane-menu-wrap">
<!-- `clickOutside` dismisses only on a press that STARTS outside (BUG-3231): a drag begun inside and released outside is not an outside click. -->
<div
class="lane-menu-wrap"
use:clickOutside={{ enabled: openMenuColumn === colValue, onOutside: closeMenu }}
>
<button
class="lane-btn lane-menu-btn"
title="Lane actions"
@@ -136,11 +136,13 @@
return v.replace(/_/g, ' ').replace(/\b\w/g, (c) => c.toUpperCase());
}
// stopPropagation on every click: a click that mutates state (drill-down,
// archive confirm) re-renders and detaches the clicked node before the
// event bubbles to BoardView's <svelte:window> outside-click handler,
// where closest() on the orphan returns null and slams the menu shut.
// Same Svelte 5 same-click issue documented in console/+layout.svelte.
// stopPropagation on every click. It was needed while BoardView closed this
// menu from a <svelte:window> click handler: a click that mutates state
// (drill-down, archive confirm) re-renders and detaches the clicked node
// before the event bubbled there, closest() on the orphan returned null, and
// the menu slammed shut. BoardView now uses `clickOutside`, which decides on
// the press before any click re-renders (BUG-3231); the guard stays as
// belt-and-braces. Same Svelte 5 issue documented in console/+layout.svelte.
function run(e: MouseEvent, fn: () => void) {
e.stopPropagation();
fn();
@@ -2,6 +2,7 @@
import EmojiPicker from '$lib/components/common/EmojiPicker.svelte';
import BottomSheet from '$lib/components/common/BottomSheet.svelte';
import { viewport } from '$lib/stores/breakpoint.svelte';
import { clickOutside } from '$lib/utils/clickOutside';
interface Props {
value: string;
@@ -45,17 +46,6 @@
};
}
function handleWindowClick(e: MouseEvent) {
// On mobile the BottomSheet owns dismissal (backdrop tap + Escape +
// close button) — skip the outside-click handler so it doesn't race.
if (viewport.isMobile) return;
if (open) {
const target = e.target as HTMLElement;
if (!target?.closest('.emoji-picker-button') && !target?.closest('.epb-dropdown')) {
open = false;
}
}
}
function toggleOpen() {
// Mobile branch skips the positioning math — BottomSheet docks to
@@ -83,7 +73,6 @@
}
</script>
<svelte:window onclick={handleWindowClick} />
<div class="emoji-picker-button" class:size-md={size === 'md'}>
<button
@@ -104,7 +93,12 @@
</div>
</BottomSheet>
{:else if open}
<div class="epb-dropdown" use:portal style="position:absolute; z-index:99999; left:{dropdownX}px; top:{dropdownY}px;">
<!-- `clickOutside` dismisses only on a press that STARTS outside (BUG-3231): a drag begun inside and released outside is not an outside click. Desktop only: on mobile the BottomSheet owns dismissal. -->
<div
class="epb-dropdown"
use:portal
use:clickOutside={{ onOutside: () => (open = false), extra: () => [triggerEl] }}
style="position:absolute; z-index:99999; left:{dropdownX}px; top:{dropdownY}px;">
<EmojiPicker selected={value} onselect={handleSelect} />
</div>
{/if}
@@ -37,6 +37,7 @@ handlers — onchange is never called.
import { shouldOpenInPane } from '$lib/components/collections/itemCardClick';
import BottomSheet from '$lib/components/common/BottomSheet.svelte';
import { viewport } from '$lib/stores/breakpoint.svelte';
import { clickOutside } from '$lib/utils/clickOutside';
import { canonicalValueColor, formatFieldLabel as formatLabel } from '$lib/utils/fieldColors';
interface Props {
@@ -916,20 +917,12 @@ handlers — onchange is never called.
}
}
function handleWindowClick(e: MouseEvent) {
// On mobile the BottomSheet owns dismissal (backdrop tap + Escape +
// close button) — skip this handler so it doesn't race the sheet.
if (viewport.isMobile) return;
if (
dropdownOpen &&
triggerEl &&
dropdownEl &&
!triggerEl.contains(e.target as Node) &&
!dropdownEl.contains(e.target as Node)
) {
dropdownOpen = false;
}
}
// The desktop dropdown closes through `clickOutside`, which dismisses only on a press that STARTS outside (BUG-3231): a drag begun inside and released outside is not an outside click.
// On mobile the BottomSheet branch renders instead and owns dismissal.
const dropdownOutside = {
onOutside: () => (dropdownOpen = false),
extra: () => [triggerEl],
};
// ── Input handlers ─────────────────────────────────────────────────────
//
@@ -1351,7 +1344,6 @@ handlers — onchange is never called.
</script>
<svelte:window onclick={handleWindowClick} />
{#snippet relationChip(raw: string)}
<!--
@@ -1562,6 +1554,7 @@ handlers — onchange is never called.
{:else if dropdownOpen && field.options}
<div
bind:this={dropdownEl}
use:clickOutside={dropdownOutside}
class="select-dropdown"
role="listbox"
aria-label="{field.label} options"
@@ -1880,6 +1873,7 @@ handlers — onchange is never called.
{:else if dropdownOpen && multiChoices.length}
<div
bind:this={dropdownEl}
use:clickOutside={dropdownOutside}
class="select-dropdown"
role="listbox"
aria-multiselectable="true"
+32 -40
View File
@@ -16,6 +16,7 @@
import MenuItem from '$lib/components/common/MenuItem.svelte';
import { workspaceRestoreTarget } from '$lib/utils/workspace-route';
import { isBlockedByModal } from '$lib/a11y/viewerBackdrop';
import { clickOutside } from '$lib/utils/clickOutside';
let { mobile = false }: { mobile?: boolean } = $props();
@@ -560,9 +561,13 @@
e.preventDefault();
return;
}
// A pill is exempt from the overflow menu's outside-press dismissal (a
// press may start a drag into the menu), so every real click on one
// closes the menu here, the modifier-click (new tab/window) included, as
// the window click closer it replaced did (BUG-3231).
closeOverflow();
if (e.metaKey || e.ctrlKey || e.shiftKey || e.altKey || e.button !== 0) return;
e.preventDefault();
closeOverflow();
const target =
ws.slug === currentSlug
? `/${ws.owner_username}/${ws.slug}`
@@ -632,48 +637,21 @@
</script>
<!--
This window handler now serves ONLY the dnd-entangled workspace
overflow menu. The user menu moved onto the shared Menu primitive
(PLAN-2290 Phase 2 / TASK-2292), which owns its own instance-scoped
pointerdown outside-click.
The overflow menu deliberately stays on window `click` with the
`!isDragging` guard rather than the primitive's pointerdown-based
outside-close:
• pointerdown fires at drag START — mousedown on a workspace pill
(outside the menu) is exactly how a reorder drag begins, so a
pointerdown-based close would tear the menu down as the user
starts dragging toward it.
• the `click` fired on mouseup at drag END must NOT close the menu
either — finalize handlers are still wiring up state at that
point (drop animation / finalize ordering), hence the
`!isDragging` guard below.
The overflow menu's outside dismissal is `clickOutside` on the menu
(BUG-3231), which decides on the press, so a drag begun inside the menu and
released outside no longer closes it. It used to be a window `click` handler
here, for two reasons that still hold and are now met differently:
• pointerdown fires at drag START, and a press on a visible workspace
pill (outside the menu) is how a reorder drag toward the menu begins.
The pills are therefore `extra` containers; a plain or modifier click
on a pill closes the menu itself (handleWsClick).
• nothing may close it mid-drag while finalize handlers are wiring up
state: `suppress: () => isDragging`.
The user menu is on the shared Menu primitive (PLAN-2290 Phase 2 /
TASK-2292). This window handler keeps only the drag disarm and the keys.
-->
<svelte:window
onmouseup={disarmMenu}
onclick={(e) => {
const target = e.target as HTMLElement;
// DELIBERATELY UNGUARDED by `isBlockedByModal` (TASK-2430). Unlike the
// keydown handler above, this is a pointer-DISMISSER: its only effect is
// to close the menu, i.e. to tear down LOWER UI. A click inside a
// frontmost viewer closing chrome that is already obscured and inert is
// the correct outcome, not a leak — the same reasoning that keeps
// `clickOutside.ts`, the emoji/reaction pickers and the sidebar's
// collection picker unguarded. Adding a gate here would leave a stale
// menu open behind the viewer instead.
//
// Don't close on outside-click during a drag — the click event
// fired on mouseup at drag end would otherwise tear down the
// menu while finalize handlers are still wiring up state.
if (
overflowOpen &&
!isDragging &&
!target.closest('#workspace-overflow-menu') &&
!target.closest('.overflow-trigger-wrap')
) {
closeOverflow();
}
}}
onkeydown={handleOverflowKeydown}
/>
@@ -928,6 +906,20 @@
<!-- svelte-ignore a11y_no_static_element_interactions -->
<div
bind:this={overflowMenuEl}
use:clickOutside={{
// dismisses only on a press that STARTS outside (BUG-3231): a drag begun inside and released outside is not an outside click.
// Unguarded by isBlockedByModal on purpose (TASK-2430): a dismisser only
// tears down lower UI. A press on a visible pill is exempt: it may be the
// start of a drag into this menu, and a plain click on a pill closes the
// menu itself (handleWsClick). Not during a drag either.
enabled: overflowOpen,
onOutside: closeOverflow,
extra: () => [
overflowTriggerEl?.closest('.overflow-trigger-wrap'),
...(rowEl?.querySelectorAll('a.workspace-item') ?? []),
],
suppress: () => isDragging,
}}
id="workspace-overflow-menu"
class="overflow-menu"
class:open={menuVisible}
@@ -1,6 +1,7 @@
<script lang="ts">
import BottomSheet from '$lib/components/common/BottomSheet.svelte';
import { viewport } from '$lib/stores/breakpoint.svelte';
import { clickOutside } from '$lib/utils/clickOutside';
interface Props {
onSelect: (emoji: string) => void;
@@ -26,22 +27,8 @@
onSelect(emoji);
}
function handleClickOutside(event: MouseEvent) {
if (pickerEl && !pickerEl.contains(event.target as Node)) {
open = false;
}
}
$effect(() => {
// On mobile the BottomSheet owns dismissal (backdrop tap + Escape +
// close button) — skip the outside-click listener so it doesn't race.
if (open && !viewport.isMobile) {
document.addEventListener('click', handleClickOutside, true);
return () => {
document.removeEventListener('click', handleClickOutside, true);
};
}
});
// Outside dismissal is `clickOutside` on the root, which dismisses only on a press that STARTS outside (BUG-3231): a drag begun inside and released outside is not an outside click.
// On mobile the BottomSheet owns dismissal, so it is off there.
</script>
{#snippet emojiGrid()}
@@ -58,7 +45,11 @@
</div>
{/snippet}
<div class="reaction-picker" bind:this={pickerEl}>
<div
class="reaction-picker"
bind:this={pickerEl}
use:clickOutside={{ enabled: open && !viewport.isMobile, onOutside: () => (open = false) }}
>
<button class="trigger" type="button" onclick={toggle} title="Add reaction">
<svg width="16" height="16" viewBox="0 0 16 16" fill="none" aria-hidden="true">
<circle cx="8" cy="8" r="7" stroke="currentColor" stroke-width="1.5" />
@@ -0,0 +1,71 @@
import { describe, it, expect, vi, afterEach } from 'vitest';
import { clickOutside } from './clickOutside';
// BUG-3231: the shared outside-dismissal primitive decides on the PRESS. A
// drag that starts inside the node and is released outside produces a click
// on a common ancestor outside the node; a click-based closer took that for an
// outside click. This one never sees it, because the press was inside.
function setup(opts: Partial<Parameters<typeof clickOutside>[1]> = {}) {
const outside = document.body.appendChild(document.createElement('div'));
const node = document.body.appendChild(document.createElement('div'));
const inside = node.appendChild(document.createElement('span'));
const extraEl = document.body.appendChild(document.createElement('button'));
const onOutside = vi.fn();
const action = clickOutside(node, { onOutside, ...opts });
return { outside, node, inside, extraEl, onOutside, action };
}
const down = (el: Element) => el.dispatchEvent(new Event('pointerdown', { bubbles: true }));
const up = (el: Element) => el.dispatchEvent(new Event('pointerup', { bubbles: true }));
const click = (el: Element) => el.dispatchEvent(new MouseEvent('click', { bubbles: true }));
afterEach(() => {
document.body.innerHTML = '';
});
describe('clickOutside (BUG-3231)', () => {
it('a press outside dismisses', () => {
const { outside, onOutside } = setup();
down(outside);
expect(onOutside).toHaveBeenCalledTimes(1);
});
it('a drag that starts inside and is released outside does not dismiss', () => {
const { inside, outside, onOutside } = setup();
down(inside);
up(outside);
click(document.body); // the common ancestor of the press and the release
expect(onOutside).not.toHaveBeenCalled();
});
it('a press inside does not dismiss', () => {
const { inside, onOutside } = setup();
down(inside);
expect(onOutside).not.toHaveBeenCalled();
});
it('a press on an extra container does not dismiss', () => {
const { extraEl, onOutside } = setup({ extra: () => [document.querySelector('button')] });
down(extraEl);
expect(onOutside).not.toHaveBeenCalled();
});
it('enabled=false and suppress hold it off', () => {
const a = setup({ enabled: false });
down(a.outside);
expect(a.onOutside).not.toHaveBeenCalled();
a.action.destroy();
document.body.innerHTML = '';
const b = setup({ suppress: () => true });
down(b.outside);
expect(b.onOutside).not.toHaveBeenCalled();
});
it('destroy removes the listener', () => {
const { outside, onOutside, action } = setup();
action.destroy();
down(outside);
expect(onOutside).not.toHaveBeenCalled();
});
});
@@ -0,0 +1,38 @@
import { describe, it, expect } from 'vitest';
import { readFileSync } from 'node:fs';
import { resolve } from 'node:path';
// BUG-3231: six menus closed on a window/document CLICK whose target was not
// inside them, so a drag begun inside and released outside closed them. Each
// now dismisses through `clickOutside`, which decides on the press. This pins
// the population the census found (BUG-3229 trail): each file uses the action
// and no longer carries a click-based outside closer. A new window click
// closer elsewhere is not caught here; the census instruments on the trail are
// how to look for one.
const SRC = resolve(__dirname, '..', '..');
const POPULATION = [
'lib/components/fields/FieldEditor.svelte',
'lib/components/collections/BoardView.svelte',
'lib/components/common/EmojiPickerButton.svelte',
'lib/components/timeline/ReactionPicker.svelte',
'lib/components/layout/TopBar.svelte',
'routes/console/+layout.svelte',
];
// A click listener on the window or document: the closer shape this replaced.
const CLICK_CLOSER = [
/<svelte:window[^>]*\bonclick=/s,
/<svelte:document[^>]*\bonclick=/s,
/(?:window|document)\.addEventListener\(\s*['"]click['"]/,
];
describe('the click-outside menu population dismisses on the press (BUG-3231)', () => {
for (const f of POPULATION) {
it(`${f} uses clickOutside and has no window/document click closer`, () => {
const source = readFileSync(resolve(SRC, f), 'utf8');
expect(source).toMatch(/use:clickOutside=\{/);
for (const re of CLICK_CLOSER) expect(source).not.toMatch(re);
});
}
});
+13 -14
View File
@@ -5,6 +5,7 @@
import { api } from '$lib/api/client';
import { onMount } from 'svelte';
import { isBlockedByModal } from '$lib/a11y/viewerBackdrop';
import { clickOutside } from '$lib/utils/clickOutside';
let { children } = $props();
let ready = $state(false);
@@ -69,14 +70,6 @@
}
}
function handleWindowClick(event: MouseEvent) {
if (!mobileMenuOpen) return;
const target = event.target as HTMLElement | null;
if (!target) return;
if (!target.closest('.console-nav')) {
mobileMenuOpen = false;
}
}
let initial = $derived(
authStore.user?.name?.charAt(0)?.toUpperCase() ??
@@ -84,17 +77,21 @@
);
</script>
<svelte:window onkeydown={handleWindowKeydown} onclick={handleWindowClick} />
<svelte:window onkeydown={handleWindowKeydown} />
{#if ready}
<div class="console-layout">
<nav class="console-nav">
<!-- `clickOutside` dismisses only on a press that STARTS outside (BUG-3231): a drag begun inside and released outside is not an outside click. -->
<nav class="console-nav" use:clickOutside={{ enabled: mobileMenuOpen, onOutside: () => (mobileMenuOpen = false) }}>
<div class="nav-left">
<a href="/console" class="nav-logo">Pad</a>
<!--
stopPropagation: this toggle click MUST NOT reach
handleWindowClick. See the BUG-1330 note on the SVG below
for the full explanation. The CSS `pointer-events: none`
stopPropagation: this toggle click once had to be kept from
a window click handler (BUG-1330, the note on the SVG below).
Outside dismissal is now `clickOutside` on the nav, which
reads the pointerdown before any click re-renders the icon,
and the toggle is inside the nav anyway (BUG-3231); the guard
stays as belt-and-braces. The CSS `pointer-events: none`
already moves the click target onto the button, but
stopping propagation is belt-and-braces — if a future
change adds another element inside the button without the
@@ -280,7 +277,9 @@
fires, and the menu slams shut as fast as it opened. Forcing the
hamburger SVG and its children to be pointer-event-transparent
makes the button the click target, which is never re-rendered.
Verified with a Svelte 5 playground repro.
Verified with a Svelte 5 playground repro. (Since BUG-3231 the
outside dismissal is `clickOutside`, which decides on pointerdown,
before the swap, so the hazard no longer applies. The rule is kept.)
*/
.mobile-hamburger svg,
.mobile-hamburger svg * {