From 7aa5fa67f7cbdf2a2da0e27135a34f588a333a07 Mon Sep 17 00:00:00 2001 From: xarmian Date: Tue, 6 Oct 2026 10:59:53 -0400 Subject: [PATCH] fix(web): an open mobile sheet stops page pull-to-refresh, and a pull from its list top closes it (BUG-3386) (#1830) With the mobile Workspace sheet open, a downward swipe in the collection list did not close it, and at the list's top it pull-to-refreshed the page. DockedSheet put overscroll-behavior: contain on .ds-panel, which never scrolls, so it did nothing: the list (.ds-content) is the scroller, and its overscroll chained to the page. - .ds-content gets overscroll-behavior: contain; - while any DockedSheet is open the root is overscroll-behavior-y: none (html/body :has(.ds-panel)), so a drag on the backdrop, the grip, or a list too short to scroll cannot refresh the page either; - a downward drag that starts in the list with the list at its top closes the sheet, the grip's existing close gesture continued into the content. A drag on a scrolled list, or one that moves up first, stays a scroll. Applies to all three DockedSheets (Workspace, You, Quick capture). Unit tests cover the content pull (close, snap back, scrolled list, up-first, viewer in front); mutants removing the handler, the scrollTop check and the up-first check each turn one red. The mobile-chromium e2e drives a real CDP touch drag: red on main (root overscroll still auto under the open sheet) and red on a mutant without the content handler (the sheet stays open). Claude-Session: https://claude.ai/code/session_01Q9WssCjFuHjSfLABoW4tmS --- .../bug-3386-sheet-pull-to-refresh.spec.ts | 59 ++++++++++++ .../lib/components/layout/DockedSheet.svelte | 65 ++++++++++++- .../layout/DockedSheet.svelte.test.ts | 91 +++++++++++++++++++ 3 files changed, 214 insertions(+), 1 deletion(-) create mode 100644 web/e2e/bug-3386-sheet-pull-to-refresh.spec.ts diff --git a/web/e2e/bug-3386-sheet-pull-to-refresh.spec.ts b/web/e2e/bug-3386-sheet-pull-to-refresh.spec.ts new file mode 100644 index 000000000..36e4abd33 --- /dev/null +++ b/web/e2e/bug-3386-sheet-pull-to-refresh.spec.ts @@ -0,0 +1,59 @@ +import { test, expect } from './fixtures'; +import { browserLogin } from './lib/collab-helpers'; +import { CdpTouch } from './lib/attachment-viewer'; + +/** + * BUG-3386 (Dave's report): with the mobile Workspace sheet open, a downward + * swipe in the collection list did not close it; at the list's top it + * pull-to-refreshed the page instead. + * + * Two halves, both on the Pixel 7 project with real touch through CDP + * (Playwright's touchscreen can only tap): + * - no page pull-to-refresh while a sheet is open. Headless Chromium draws no + * pull-to-refresh, so what this pins is the property that controls it: + * the ROOT's computed overscroll-behavior-y is `none` while the sheet is + * open (and `auto` again once it closes), and the list itself is `contain`, + * so a pull at its top cannot chain to the page; + * - a downward drag that starts in the list, with the list at its top, closes + * the sheet, as the grip's drag always did (the existing close gesture). + */ + +async function openWorkspaceSheet(page: import('@playwright/test').Page) { + await page.locator('.bottom-nav .bn-item', { hasText: 'Workspace' }).click(); + await expect(page.locator('.ds-panel[aria-label="Workspace"]')).toBeVisible(); +} + +const rootOverscroll = (page: import('@playwright/test').Page) => + page.evaluate(() => [getComputedStyle(document.documentElement).overscrollBehaviorY, getComputedStyle(document.body).overscrollBehaviorY]); + +test('BUG-3386: no page pull-to-refresh while the Workspace sheet is open, and a pull from the list top closes it', async ({ + page, + fixture, +}, testInfo) => { + test.skip(testInfo.project.name !== 'mobile-chromium', 'needs a touch device'); + await browserLogin(page); + await page.goto(`/${fixture.adminUsername}/${fixture.workspaceSlug}`); + await expect(page.locator('.bottom-nav')).toBeVisible(); + + // Closed: the page keeps its normal overscroll (pull-to-refresh allowed). + expect(await rootOverscroll(page)).toEqual(['auto', 'auto']); + + await openWorkspaceSheet(page); + expect(await rootOverscroll(page), 'the page can still pull-to-refresh under the open sheet').toEqual(['none', 'none']); + const list = page.locator('.ds-panel[aria-label="Workspace"] .ds-content'); + expect(await list.evaluate((el) => getComputedStyle(el).overscrollBehaviorY)).toBe('contain'); + expect(await list.evaluate((el) => el.scrollTop)).toBe(0); + + // A real downward drag that starts inside the list, at its top. + const box = (await list.boundingBox())!; + const x = box.x + box.width / 2; + const y0 = box.y + 40; + const touch = await CdpTouch.attach(page); + await touch.down(0, x, y0); + for (let dy = 10; dy <= 220; dy += 15) await touch.move(0, x, y0 + dy); + await touch.lift(0); + await expect(page.locator('.ds-panel[aria-label="Workspace"]')).toHaveCount(0); + + // Closed again: the page's overscroll is back. + await expect.poll(() => rootOverscroll(page)).toEqual(['auto', 'auto']); +}); diff --git a/web/src/lib/components/layout/DockedSheet.svelte b/web/src/lib/components/layout/DockedSheet.svelte index 5c7a13b12..b902255f0 100644 --- a/web/src/lib/components/layout/DockedSheet.svelte +++ b/web/src/lib/components/layout/DockedSheet.svelte @@ -35,6 +35,7 @@ const DISMISS_PX = 90; let panelEl = $state(null); + let contentEl = $state(null); /** * TASK-2430 — this sheet is a GLOBAL Escape/gesture owner (three instances @@ -106,6 +107,48 @@ dragY = 0; } + /* + * BUG-3386: a downward drag in the CONTENT closes the sheet too, when the + * content is already scrolled to its top. That is the grip's gesture + * continued into the list, the way a native sheet behaves; before, the + * same drag pulled the page underneath to refresh. A drag that starts + * with the list scrolled, or that moves up first, is a scroll and is + * left alone. CONTENT_SLOP keeps a tap or a jitter from engaging it. + */ + const CONTENT_SLOP = 8; + let contentArmed = false; + function onContentTouchStart(e: TouchEvent) { + contentArmed = false; + if (blockedByFrontLayer()) return; + if (!contentEl || contentEl.scrollTop > 0) return; + startY = e.touches[0].clientY; + contentArmed = true; + } + function onContentTouchMove(e: TouchEvent) { + if (dragging) { + onTouchMove(e); + return; + } + if (!contentArmed || !contentEl) return; + const dy = e.touches[0].clientY - startY; + if (dy < 0 || contentEl.scrollTop > 0) { + contentArmed = false; // a scroll, not a pull + return; + } + if (dy > CONTENT_SLOP) { + if (blockedByFrontLayer()) { + contentArmed = false; + return; + } + startY = e.touches[0].clientY; + dragging = true; + } + } + function onContentTouchEnd() { + contentArmed = false; + if (dragging) onTouchEnd(); + } + function onKeydown(e: KeyboardEvent) { if (!open || e.key !== 'Escape') return; // A HELD Escape fires many auto-repeat keydowns, and each is a FRESH @@ -160,7 +203,15 @@ > -
+ +
{@render children()}
@@ -207,10 +258,22 @@ border-radius: 999px; background: var(--border); } + /* The list is the scroller, so it is the element that has to stop scroll + chaining (BUG-3386): `contain` on .ds-panel, which never scrolls, did + nothing, and a pull at the list's top chained to the page and + pull-to-refreshed it. */ .ds-content { overflow-y: auto; + overscroll-behavior: contain; padding: 0 0 var(--space-3); flex: 1 1 auto; min-height: 0; } + /* And no page pull-to-refresh at all while a sheet is open (BUG-3386): + a drag on the backdrop, on the grip, or on a list too short to scroll + never reaches a scroller of the sheet's, and would reach the page. */ + :global(html:has(.ds-panel)), + :global(body:has(.ds-panel)) { + overscroll-behavior-y: none; + } diff --git a/web/src/lib/components/layout/DockedSheet.svelte.test.ts b/web/src/lib/components/layout/DockedSheet.svelte.test.ts index 7dd3b002c..e780848dd 100644 --- a/web/src/lib/components/layout/DockedSheet.svelte.test.ts +++ b/web/src/lib/components/layout/DockedSheet.svelte.test.ts @@ -342,3 +342,94 @@ describe('DockedSheet — swipe-to-dismiss (TASK-2430)', () => { expect(onclose).not.toHaveBeenCalled(); }); }); + +function content(): HTMLElement { + const el = document.querySelector('.ds-content'); + if (!el) throw new Error('.ds-content not found'); + return el as HTMLElement; +} + +describe('DockedSheet — pull down in the content (BUG-3386)', () => { + it('a pull from the TOP of the list closes the sheet, like the grip', async () => { + const onclose = vi.fn(); + render(DockedSheet, { props: baseProps({ onclose }) }); + await tick(); + flushSync(); + + const c = content(); + c.dispatchEvent(touch('touchstart', 100)); + c.dispatchEvent(touch('touchmove', 120)); // past the slop: engages at y=120 + c.dispatchEvent(touch('touchmove', 320)); + flushSync(); + expect(panel().style.transform).toBe('translateY(200px)'); + c.dispatchEvent(touch('touchend', 320)); + expect(onclose).toHaveBeenCalledTimes(1); + }); + + it('a short pull from the top snaps back without closing', async () => { + const onclose = vi.fn(); + render(DockedSheet, { props: baseProps({ onclose }) }); + await tick(); + flushSync(); + + const c = content(); + c.dispatchEvent(touch('touchstart', 100)); + c.dispatchEvent(touch('touchmove', 120)); + c.dispatchEvent(touch('touchmove', 150)); + c.dispatchEvent(touch('touchend', 150)); + flushSync(); + expect(onclose).not.toHaveBeenCalled(); + expect(panel().style.transform).toBe(''); + }); + + it('a pull in a list that is SCROLLED is a scroll: the sheet does not move', async () => { + const onclose = vi.fn(); + render(DockedSheet, { props: baseProps({ onclose }) }); + await tick(); + flushSync(); + + const c = content(); + c.scrollTop = 40; // jsdom keeps the value it is given + c.dispatchEvent(touch('touchstart', 100)); + // Two moves: a pull that engaged at the first would translate at the second. + c.dispatchEvent(touch('touchmove', 140)); + c.dispatchEvent(touch('touchmove', 400)); + flushSync(); + expect(panel().style.transform).toBe(''); + c.dispatchEvent(touch('touchend', 400)); + expect(onclose).not.toHaveBeenCalled(); + }); + + it('a drag that moves UP first is a scroll, even if it then comes back down', async () => { + const onclose = vi.fn(); + render(DockedSheet, { props: baseProps({ onclose }) }); + await tick(); + flushSync(); + + const c = content(); + c.dispatchEvent(touch('touchstart', 300)); + c.dispatchEvent(touch('touchmove', 250)); + c.dispatchEvent(touch('touchmove', 320)); + c.dispatchEvent(touch('touchmove', 500)); + flushSync(); + expect(panel().style.transform).toBe(''); + c.dispatchEvent(touch('touchend', 500)); + expect(onclose).not.toHaveBeenCalled(); + }); + + it('does not engage while a viewer lease is frontmost', async () => { + const onclose = vi.fn(); + render(DockedSheet, { props: baseProps({ onclose }) }); + await tick(); + flushSync(); + + acquire(mountViewer()); + const c = content(); + c.dispatchEvent(touch('touchstart', 100)); + c.dispatchEvent(touch('touchmove', 400)); + flushSync(); + expect(panel().style.transform).toBe(''); + c.dispatchEvent(touch('touchend', 400)); + expect(onclose).not.toHaveBeenCalled(); + }); +});