mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-25 14:00:40 +00:00
fix(web): the user menu's Sign out is inside the panel, not below its edge (BUG-2985) (#1355)
The sign-out button was there the whole time. It was rendering 59px below
the bottom of the panel that was supposed to contain it.
Measured in a browser before touching anything, three viewport heights,
identical at all of them:
.user-dropdown height 393
.menu-panel max-height 340, clientH 338, overflow-y auto
Sign out top 409, bottom 443
panel bottom 384
So the panel ends at 384 with a clean edge and no scrollbar — overlay
scrollbars paint nothing until you scroll — and the menu LOOKS complete.
"Connect a project…" is cut off too. Not viewport-dependent, not
cloud-only: that measurement is a self-hosted instance with no Billing
row, so cloud is worse.
The cap stays, since it is what keeps a menu off a short viewport. What
changes is that the panel stops being the SCROLLER for this one menu:
`Menu` takes an opt-in `bodyScroll` that makes it a clipped flex column,
and the dropdown becomes a scrolling body plus a pinned divider and Sign
out. Shortening the menu would only move the cliff — Billing, Admin and
the Resources block all come and go — so the account action is kept out
of the scrolling region entirely. Every other consumer of the primitive
is untouched; the flag defaults false.
`min-height: 0` on both flex boxes is load-bearing: a flex item defaults
to `min-height: auto` and refuses to shrink below its content, which
would push the pinned row back out of the panel — the bug restored, with
the markup looking right.
The behavioural instrument is the e2e leg, and it is deliberately NOT a
`toBeVisible()` check: an element scrolled out of an `overflow: auto`
container still has a box, so the pre-fix menu would have PASSED one. It
asserts the item's bottom edge against the panel's, and it goes red on a
binary built from the unfixed tree — 442.6 vs 384.5, the same 58px.
Claude-Session: https://claude.ai/code/session_019EmDd3Wy8b3cPG1TZBSzBu
This commit is contained in:
@@ -0,0 +1,73 @@
|
||||
import { expect } from '@playwright/test';
|
||||
import { test } from './fixtures';
|
||||
|
||||
/**
|
||||
* BUG-2985 — "Sign out" has to be INSIDE the panel, not merely in the DOM.
|
||||
*
|
||||
* The user menu is taller than the Menu primitive's 340px cap, which used to
|
||||
* be the scroller. The panel's bottom edge then looked like the end of the
|
||||
* menu: overlay scrollbars show nothing until you scroll, so there was no cue
|
||||
* that "Connect a project…" and "Sign out" existed below it. Measured before
|
||||
* the fix at 393px of content in a 338px client box, with Sign out laid out at
|
||||
* y=409 while the box ended at 384 — at every viewport height tested, 900 down
|
||||
* to 600, because the cap is fixed rather than viewport-relative.
|
||||
*
|
||||
* WHY `toBeVisible()` IS NOT THE ASSERTION, which is the whole reason this spec
|
||||
* is geometric: an element scrolled out of an `overflow: auto` container still
|
||||
* has a non-empty box and is not `display:none`, so Playwright calls it
|
||||
* visible. The pre-fix menu would have PASSED a visibility check. What was
|
||||
* wrong is where it sat relative to the panel, so that is what is asserted.
|
||||
*/
|
||||
test('BUG-2985: Sign out renders within the user menu panel, not below its edge', async ({
|
||||
page,
|
||||
request,
|
||||
fixture,
|
||||
}) => {
|
||||
const auth = { Authorization: `Bearer ${fixture.apiToken}` };
|
||||
const slug = `b2985-${test.info().workerIndex}-${Date.now().toString(36)}-${Math.random().toString(36).slice(2, 6)}`;
|
||||
const wsResp = await request.post('/api/v1/workspaces', {
|
||||
headers: auth,
|
||||
data: { name: 'BUG-2985 probe', slug, template: 'startup' },
|
||||
});
|
||||
expect(wsResp.ok(), await wsResp.text()).toBeTruthy();
|
||||
const ws = (await wsResp.json()) as { slug: string };
|
||||
|
||||
await page.goto(`/${fixture.adminUsername}/${ws.slug}/tasks`);
|
||||
|
||||
// SKIPPED BY PROJECT NAME, not by probing the DOM. The first version asked
|
||||
// `trigger.count() === 0` immediately after `goto`, which races hydration:
|
||||
// it read 0 on a page that renders the trigger a moment later and skipped
|
||||
// BOTH projects, so the spec reported `2 skipped` with exit 0 — a green
|
||||
// gate covering nothing. A skip condition has to be something the run
|
||||
// KNOWS, not something it might be early to observe.
|
||||
test.skip(
|
||||
test.info().project.name.includes('mobile'),
|
||||
'desktop top bar only — mobile signs out through YouSheet, a different surface',
|
||||
);
|
||||
|
||||
const trigger = page.locator('[aria-label="User menu"]');
|
||||
await expect(trigger).toBeVisible({ timeout: 15_000 });
|
||||
await trigger.click();
|
||||
|
||||
const panel = page.locator('.menu-panel').filter({ hasText: 'Sign out' }).first();
|
||||
await expect(panel).toBeVisible();
|
||||
const signOut = page.getByRole('menuitem', { name: 'Sign out' });
|
||||
await expect(signOut).toBeVisible();
|
||||
|
||||
const panelBox = await panel.boundingBox();
|
||||
const signOutBox = await signOut.boundingBox();
|
||||
expect(panelBox, 'no panel box').not.toBeNull();
|
||||
expect(signOutBox, 'no sign-out box').not.toBeNull();
|
||||
|
||||
// THE LEG: the item's bottom edge is inside the panel's. A 1px slack for
|
||||
// sub-pixel layout; the pre-fix gap was 59px, so nothing about this is
|
||||
// close to the tolerance.
|
||||
expect(signOutBox!.y + signOutBox!.height).toBeLessThanOrEqual(
|
||||
panelBox!.y + panelBox!.height + 1,
|
||||
);
|
||||
|
||||
// And it WORKS from there — the point is a reachable control, not a
|
||||
// well-placed one. Without this, a fix that pinned an inert row would pass.
|
||||
await signOut.click();
|
||||
await page.waitForURL(/\/login(\?|$)/, { timeout: 15_000 });
|
||||
});
|
||||
@@ -36,6 +36,21 @@
|
||||
* Forwarded to BottomSheet in `sheetOnMobile` mode, which owns focus
|
||||
* there. PLAN-2326 DR-8. */
|
||||
focusKey?: string | number;
|
||||
/**
|
||||
* Let the CONSUMER own scrolling instead of the panel (BUG-2985).
|
||||
*
|
||||
* The panel caps itself at 340px and scrolls, which silently truncates
|
||||
* any menu taller than that: the panel's bottom edge looks like the end
|
||||
* of the menu, and overlay scrollbars mean there is no cue at all. The
|
||||
* user menu lost its "Sign out" that way — measured at 393px of content
|
||||
* in a 338px client box, with Sign out rendering at y=409 and the box
|
||||
* ending at 384.
|
||||
*
|
||||
* With this set, the panel keeps its cap but does NOT scroll; the
|
||||
* consumer lays out a scrolling body and whatever must stay visible
|
||||
* beside it. Opt-in, so every other menu keeps today's behaviour.
|
||||
*/
|
||||
bodyScroll?: boolean;
|
||||
/** Return true to suppress outside-close for this event (e.g. while
|
||||
* a drag interaction is in flight — cf. TopBar pill drags). */
|
||||
suppressOutside?: () => boolean;
|
||||
@@ -55,6 +70,7 @@
|
||||
exempt,
|
||||
suppressOutside,
|
||||
focusKey,
|
||||
bodyScroll = false,
|
||||
children
|
||||
}: Props = $props();
|
||||
|
||||
@@ -193,7 +209,7 @@
|
||||
{:else if open}
|
||||
{#if mode === 'portal'}
|
||||
<div
|
||||
class="menu-panel portal"
|
||||
class="menu-panel portal" class:body-scroll={bodyScroll}
|
||||
style:top="{coords.top}px"
|
||||
style:left="{coords.left}px"
|
||||
style:width="{width}px"
|
||||
@@ -211,6 +227,7 @@
|
||||
<div
|
||||
class="menu-panel anchored"
|
||||
class:align-left={align === 'left'}
|
||||
class:body-scroll={bodyScroll}
|
||||
role="menu"
|
||||
aria-label={ariaLabel}
|
||||
tabindex="-1"
|
||||
@@ -236,6 +253,20 @@
|
||||
z-index: 200;
|
||||
}
|
||||
|
||||
/*
|
||||
* The consumer scrolls its own body instead (BUG-2985). The cap STAYS — it
|
||||
* is what keeps a menu from running off a short viewport — but the panel
|
||||
* stops being the scroller, so a consumer can keep a footer visible while
|
||||
* the middle scrolls. `overflow: hidden` rather than `visible`: content
|
||||
* must still be clipped to the panel, or a too-tall menu would paint over
|
||||
* the page with no way to reach the rest.
|
||||
*/
|
||||
.menu-panel.body-scroll {
|
||||
overflow: hidden;
|
||||
display: flex;
|
||||
flex-direction: column;
|
||||
}
|
||||
|
||||
.anchored {
|
||||
position: absolute;
|
||||
top: calc(100% + 6px);
|
||||
|
||||
@@ -714,8 +714,27 @@
|
||||
mode="anchored"
|
||||
ariaLabel="User menu"
|
||||
suppressOutside={() => isDragging || dragArmed}
|
||||
bodyScroll
|
||||
>
|
||||
<!--
|
||||
SIGN OUT IS PINNED; everything above it scrolls (BUG-2985).
|
||||
|
||||
The panel caps at 340px and used to be the scroller, so this
|
||||
menu — 393px of content — ended at the cap with a clean edge,
|
||||
no scrollbar (overlay scrollbars show nothing until you
|
||||
scroll) and no cue. "Connect a project…" and "Sign out" were
|
||||
below the cut: the menu looked complete and the reporter
|
||||
concluded there was no sign-out at all. Measured before the
|
||||
fix at client height 338 with Sign out rendering at y=409.
|
||||
|
||||
Making the menu shorter would only move the cliff — Billing
|
||||
(cloud), Admin and the Resources block all come and go — so
|
||||
the account action is kept OUT of the scrolling region
|
||||
instead. The middle grows and scrolls; the divider and Sign
|
||||
out are always on screen.
|
||||
-->
|
||||
<div class="user-dropdown">
|
||||
<div class="user-dropdown-body">
|
||||
<div class="user-info">
|
||||
<span class="user-dropdown-name">{authStore.user?.name}</span>
|
||||
<span class="user-dropdown-email">{authStore.user?.email}</span>
|
||||
@@ -772,6 +791,7 @@
|
||||
Connect a project…
|
||||
</MenuItem>
|
||||
{/if}
|
||||
</div>
|
||||
<div class="dropdown-divider"></div>
|
||||
<MenuItem danger onclick={handleLogout}>Sign out</MenuItem>
|
||||
</div>
|
||||
@@ -1529,6 +1549,25 @@
|
||||
.user-dropdown class survives markup-side as a styling hook for
|
||||
UserMenuResources' :global(.user-dropdown) rules. */
|
||||
|
||||
/* BUG-2985: the panel no longer scrolls (Menu's `bodyScroll`), so this
|
||||
column does — the body takes the space that is left and Sign out keeps
|
||||
its own, whatever the body holds. `min-height: 0` is load-bearing: a
|
||||
flex item's default `min-height: auto` refuses to shrink below its
|
||||
content, so without it the body would push the pinned row back out of
|
||||
the panel and restore the bug with extra steps. */
|
||||
.user-dropdown {
|
||||
display: flex;
|
||||
flex-direction: column;
|
||||
min-height: 0;
|
||||
max-height: 100%;
|
||||
}
|
||||
|
||||
.user-dropdown-body {
|
||||
flex: 1 1 auto;
|
||||
min-height: 0;
|
||||
overflow-y: auto;
|
||||
}
|
||||
|
||||
.user-info {
|
||||
padding: var(--space-3) var(--space-4);
|
||||
display: flex;
|
||||
|
||||
@@ -0,0 +1,107 @@
|
||||
// Node-project SOURCE guard: "Sign out" sits OUTSIDE the user menu's scrolling
|
||||
// body (BUG-2985).
|
||||
//
|
||||
// The behavioural instrument for this is the e2e leg
|
||||
// (`e2e/bug-2985-signout-reachable.spec.ts`), which measures the item's box
|
||||
// against the panel's — the only way to catch it, since an element scrolled out
|
||||
// of an `overflow: auto` container is still "visible" to a DOM query and to
|
||||
// Playwright. This guard is the cheap half: it fails the moment the structure
|
||||
// that makes the leg pass is undone, which is the likely regression — a future
|
||||
// edit moving Sign out back into the scrolling region, or dropping `bodyScroll`
|
||||
// so the panel scrolls again.
|
||||
//
|
||||
// WHAT IT CANNOT DO: it checks structure, not geometry. A pinned footer tall
|
||||
// enough to overflow the cap on its own would satisfy every assertion here.
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import { readFileSync } from 'node:fs';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
|
||||
const raw = readFileSync(fileURLToPath(new URL('./TopBar.svelte', import.meta.url)), 'utf8');
|
||||
// Comments quote the markup below, so strip them or the guard passes on its
|
||||
// own prose (the trap `itemDetailLinksSurviveFailedRefresh` records).
|
||||
const code = raw
|
||||
.replace(/<!--[\s\S]*?-->/g, '')
|
||||
.replace(/\/\*[\s\S]*?\*\//g, '')
|
||||
.replace(/(^|\s)\/\/[^\n]*/g, '$1');
|
||||
|
||||
const menuSrc = readFileSync(
|
||||
fileURLToPath(new URL('../common/Menu.svelte', import.meta.url)),
|
||||
'utf8',
|
||||
).replace(/<!--[\s\S]*?-->/g, '').replace(/\/\*[\s\S]*?\*\//g, '');
|
||||
|
||||
|
||||
/**
|
||||
* Index just past the `</div>` that closes the `<div>` containing `from`.
|
||||
*
|
||||
* Counts nesting, because the naive "next `</div>`" reading is what made the
|
||||
* first version of the ordering leg pass against its own mutant. Self-closing
|
||||
* tags do not occur in this block; `<div ... />` would need handling if they
|
||||
* ever did.
|
||||
*/
|
||||
function matchingCloseIndex(src: string, from: number): number {
|
||||
const open = src.lastIndexOf('<div', from);
|
||||
if (open < 0) return -1;
|
||||
let depth = 0;
|
||||
const tag = /<div\b|<\/div>/g;
|
||||
tag.lastIndex = open;
|
||||
let m: RegExpExecArray | null;
|
||||
while ((m = tag.exec(src))) {
|
||||
depth += m[0] === '</div>' ? -1 : 1;
|
||||
if (depth === 0) return m.index + m[0].length;
|
||||
}
|
||||
return -1;
|
||||
}
|
||||
|
||||
describe('the user menu', () => {
|
||||
it('hands scrolling to its own body instead of the panel', () => {
|
||||
expect(code).toContain('bodyScroll');
|
||||
expect(code).toContain('user-dropdown-body');
|
||||
});
|
||||
|
||||
it('closes the scrolling body BEFORE the divider and Sign out', () => {
|
||||
// The order is the fix: Sign out INSIDE `.user-dropdown-body` is the
|
||||
// pre-fix layout with extra markup.
|
||||
//
|
||||
// MATCHED BY NESTING, not by the next `</div>`. The first version of this
|
||||
// leg took `indexOf('</div>', …'Connect a project')` as the body's close
|
||||
// and SURVIVED the mutant that moves Sign out back inside — that index
|
||||
// landed on the divider's own closing tag, which precedes Sign out in
|
||||
// both layouts, so the comparison was true either way. The mutant is
|
||||
// faithful; the instrument was not.
|
||||
const bodyOpen = code.indexOf('class="user-dropdown-body"');
|
||||
expect(bodyOpen, 'no scrolling body').toBeGreaterThan(-1);
|
||||
expect(matchingCloseIndex(code, bodyOpen), 'body never closes').toBeGreaterThan(-1);
|
||||
const signOut = code.indexOf('Sign out');
|
||||
expect(signOut).toBeGreaterThan(bodyOpen);
|
||||
expect(signOut, 'Sign out is inside the scrolling body').toBeGreaterThan(
|
||||
matchingCloseIndex(code, bodyOpen),
|
||||
);
|
||||
});
|
||||
|
||||
it('gives the body the min-height that lets it shrink', () => {
|
||||
// `min-height: 0` is load-bearing: a flex item defaults to `min-height:
|
||||
// auto` and refuses to shrink below its content, which would push the
|
||||
// pinned row back out of the panel — the bug, restored, with the markup
|
||||
// looking correct.
|
||||
expect(code).toMatch(/\.user-dropdown-body\s*\{[^}]*min-height:\s*0/);
|
||||
expect(code).toMatch(/\.user-dropdown\s*\{[^}]*min-height:\s*0/);
|
||||
});
|
||||
});
|
||||
|
||||
describe('the Menu primitive', () => {
|
||||
it('stops scrolling, and still CLIPS, under bodyScroll', () => {
|
||||
// `overflow: hidden`, not `visible`: a too-tall menu must still be
|
||||
// clipped to the panel, or it paints over the page with no way to reach
|
||||
// the rest.
|
||||
expect(menuSrc).toMatch(/\.menu-panel\.body-scroll\s*\{[^}]*overflow:\s*hidden/);
|
||||
expect(menuSrc).toMatch(/\.menu-panel\.body-scroll\s*\{[^}]*display:\s*flex/);
|
||||
});
|
||||
|
||||
it('leaves every other menu scrolling the panel', () => {
|
||||
// The opt-in is the compatibility story: this primitive backs many menus
|
||||
// and none of them was asked to change.
|
||||
expect(menuSrc).toMatch(/\.menu-panel\s*\{[^}]*max-height:\s*340px/);
|
||||
expect(menuSrc).toMatch(/\.menu-panel\s*\{[^}]*overflow-y:\s*auto/);
|
||||
expect(menuSrc).toContain('bodyScroll = false');
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user