mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-25 14:00:40 +00:00
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
This commit is contained in:
xarmian
@@ -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']);
|
||||
});
|
||||
@@ -35,6 +35,7 @@
|
||||
const DISMISS_PX = 90;
|
||||
|
||||
let panelEl = $state<HTMLElement | null>(null);
|
||||
let contentEl = $state<HTMLElement | null>(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 @@
|
||||
>
|
||||
<span class="ds-handle" aria-hidden="true"></span>
|
||||
</div>
|
||||
<div class="ds-content">
|
||||
<!-- svelte-ignore a11y_no_static_element_interactions -->
|
||||
<div
|
||||
class="ds-content"
|
||||
bind:this={contentEl}
|
||||
ontouchstart={onContentTouchStart}
|
||||
ontouchmove={onContentTouchMove}
|
||||
ontouchend={onContentTouchEnd}
|
||||
ontouchcancel={onContentTouchEnd}
|
||||
>
|
||||
{@render children()}
|
||||
</div>
|
||||
</div>
|
||||
@@ -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;
|
||||
}
|
||||
</style>
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user