mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-10 06:55:40 +00:00
fix(web): wrap the settings tab bar so no tab is hidden at phone width (TASK-2245 / C82) (#1306)
* fix(web): wrap the settings tab bar so no tab is hidden at phone width (TASK-2245 / C82) The five owner tabs are 562px intrinsic and the bar's box is the viewport minus the page's 48px of padding, so below ~610px the row overflowed. With `overflow-x:auto` plus `scrollbar-width:none` it overflowed INVISIBLY: the row ended after a tab with clean trailing whitespace and looked complete. Measured at 390x844 on the unfixed build, Storage was 9.3% visible and Danger Zone 0% — workspace export and deletion reachable only by a swipe nothing advertised. At 320/360 three tabs were clipped. No single-row shape can hold the full labels: 562px does not fit 342px, and dropping the tab padding to 10px still needs two rows. Of the three shapes the item proposed, an edge fade leaves a tab clipped by construction, and a picker keeps four of five labels off screen until a tap — which is the defect itself. Wrapping is the one that makes every label legible at once. Deliberately not inside a media query: `flex-wrap` is inert while the row fits. That is measured, not assumed — at 640/768/1024/1280 the bar stays one row at 35px with the content top unmoved at 184.6, identical to the scrolling build; only 320-430 wrap, at a cost of +38px of content offset at 390 and +76px at 320. Two e2e legs, each with a non-vacuity precondition: the mobile leg asserts nothing is clipped and the bar no longer scrolls, and the desktop leg pins the inertness claim — it fails if the rule is ever widened into an unconditional wrap. Claude-Session: https://claude.ai/code/session_01WS9QAnxk1gA3LBha3PvKVm * test(web): read horizontal page scroll off the real scroll chain (TASK-2245 / C82) The spec's "no horizontal page scroll" oracle read `document.scrollingElement`, but the app scrolls inside `.main-content`, whose `overflow-y:auto` computes `overflow-x:auto`. Overflow is therefore contained there and never reaches the document, so that assertion could not fail — it was inert, not a guard. It now walks the tab bar's ancestors to <html> and asserts none of them scroll horizontally. Verified to discriminate rather than assumed: forcing a 3000px-wide child into `.settings` makes the list `[div.settings, main.main-content]`, which the previous oracle reported as clean. Found by Codex review round 1 (P2). Claude-Session: https://claude.ai/code/session_01WS9QAnxk1gA3LBha3PvKVm * test(web): a scroll-chain oracle must check the container, not just overflow (TASK-2245 / C82) `scrollWidth > clientWidth` is true of any element with a wide descendant, including one whose `overflow-x` is `visible` and which therefore cannot scroll at all. The ancestor walk now requires computed `overflow-x` to be `auto` or `scroll` before treating an element as a scroll container, so a long settings value can no longer fail the leg spuriously. My own negative control had already shown the false positive and I read it as confirmation instead of as the defect it was: forcing a 3000px child into `.settings` listed BOTH `div.settings` and `main.main-content`, and only the second is a scroll container. With the filter the same control lists `main.main-content` alone, which is the claim the comment now makes. Also narrows an overclaim in the CSS comment: "any scrolling shape leaves a tab clipped by construction" is broader than anything measured. What was measured is that a row opening at scrollLeft=0 leaves the later tabs clipped in the initial view. Found by Codex review round 2 (P2 + nit). Claude-Session: https://claude.ai/code/session_01WS9QAnxk1gA3LBha3PvKVm
This commit is contained in:
xarmian
@@ -0,0 +1,121 @@
|
||||
import { expect, type Page } from '@playwright/test';
|
||||
import { test } from './fixtures';
|
||||
|
||||
/**
|
||||
* TASK-2245 / C82 — the workspace settings tab bar scrolled horizontally with
|
||||
* its scrollbar hidden (`overflow-x:auto` + `scrollbar-width:none`), so at
|
||||
* phone widths it ended after a tab with clean trailing whitespace and LOOKED
|
||||
* complete. Measured at 390x844 on the unfixed build: the five owner tabs are
|
||||
* 562px intrinsic in a 342px box, Storage 9.3% visible and Danger Zone 0% —
|
||||
* workspace export and deletion reachable only by a swipe nothing advertised.
|
||||
*
|
||||
* The fix is `flex-wrap: wrap` with no media query. Two legs, because the rule
|
||||
* carries two claims:
|
||||
*
|
||||
* - mobile: nothing is clipped, and the bar does not scroll.
|
||||
* - desktop: the rule is INERT. `flex-wrap` does nothing while the row fits,
|
||||
* so the desktop bar must still be a single row. This is the leg that fails
|
||||
* if someone "simplifies" the fix into something that wraps unconditionally.
|
||||
*
|
||||
* Both legs assert a non-vacuity precondition first: a viewport where the tabs
|
||||
* happen to fit would pass the mobile leg trivially, and one where they never
|
||||
* fit would pass the desktop leg trivially.
|
||||
*/
|
||||
|
||||
type BarProbe = {
|
||||
tabCount: number;
|
||||
rows: number;
|
||||
clientWidth: number;
|
||||
/** Intrinsic width of the row: tab widths plus the gaps between them. */
|
||||
intrinsicWidth: number;
|
||||
barScrolls: boolean;
|
||||
/** Ancestors of the bar (up to <html>) that scroll horizontally. */
|
||||
scrollingAncestors: string[];
|
||||
clipped: string[];
|
||||
};
|
||||
|
||||
async function probeTabBar(page: Page): Promise<BarProbe> {
|
||||
return page.evaluate(() => {
|
||||
const bar = document.querySelector('.tab-bar') as HTMLElement;
|
||||
const barBox = bar.getBoundingClientRect();
|
||||
const gap = parseFloat(getComputedStyle(bar).columnGap || '0') || 0;
|
||||
const tabs = [...bar.querySelectorAll('.tab')].map((t) => {
|
||||
const r = t.getBoundingClientRect();
|
||||
// How much of this tab is inside the bar's own box — clipping does
|
||||
// not shrink getBoundingClientRect, so the overlap is the oracle.
|
||||
const visible = Math.max(0, Math.min(r.right, barBox.right) - Math.max(r.left, barBox.left));
|
||||
return {
|
||||
label: (t.textContent ?? '').trim(),
|
||||
y: Math.round(r.y),
|
||||
width: r.width,
|
||||
pct: (100 * visible) / r.width,
|
||||
};
|
||||
});
|
||||
// "No horizontal page scroll" cannot be read off document.scrollingElement
|
||||
// here: the app scrolls in `.main-content`, whose `overflow-y:auto`
|
||||
// computes `overflow-x:auto`, so overflow is contained there and never
|
||||
// reaches the document. Walk the real chain instead.
|
||||
//
|
||||
// Overflow alone is not the test — an `overflow-x:visible` ancestor
|
||||
// reports scrollWidth > clientWidth for any wide descendant while being
|
||||
// unable to scroll, so the element must ALSO be a scroll container. The
|
||||
// negative control on the trail shows both halves: forcing a 3000px child
|
||||
// into `.settings` lists only `main.main-content` (the real container),
|
||||
// `.settings` itself is filtered out as visible-overflow, and the document
|
||||
// oracle stays silent throughout. The empty list below is a measurement,
|
||||
// not a tautology.
|
||||
const scrollingAncestors: string[] = [];
|
||||
for (let el = bar.parentElement; el; el = el.parentElement) {
|
||||
const overflowX = getComputedStyle(el).overflowX;
|
||||
const canScroll = overflowX === 'auto' || overflowX === 'scroll';
|
||||
if (canScroll && el.scrollWidth > el.clientWidth + 1) {
|
||||
scrollingAncestors.push(`${el.tagName.toLowerCase()}.${el.className || '(no class)'}`);
|
||||
}
|
||||
}
|
||||
return {
|
||||
tabCount: tabs.length,
|
||||
rows: new Set(tabs.map((t) => t.y)).size,
|
||||
clientWidth: bar.clientWidth,
|
||||
intrinsicWidth: tabs.reduce((sum, t) => sum + t.width, 0) + gap * Math.max(0, tabs.length - 1),
|
||||
barScrolls: bar.scrollWidth > bar.clientWidth,
|
||||
scrollingAncestors,
|
||||
clipped: tabs.filter((t) => t.pct < 99.5).map((t) => `${t.label} ${t.pct.toFixed(1)}%`),
|
||||
};
|
||||
});
|
||||
}
|
||||
|
||||
async function openSettings(page: Page, username: string, workspace: string) {
|
||||
await page.goto(`/${username}/${workspace}/settings`);
|
||||
// Danger Zone is owner-only and arrives with /me, so waiting on the fifth
|
||||
// tab is what makes the measurement one of the FULL bar.
|
||||
await expect(page.locator('.tab-bar .tab')).toHaveCount(5);
|
||||
}
|
||||
|
||||
test('TASK-2245 C82: no settings tab is clipped at phone width', async ({ page, fixture }, testInfo) => {
|
||||
test.skip(testInfo.project.name !== 'mobile-chromium', 'the clipping only occurs below ~610px');
|
||||
|
||||
await openSettings(page, fixture.adminUsername, fixture.workspaceSlug);
|
||||
const bar = await probeTabBar(page);
|
||||
|
||||
// Non-vacuous: the tabs genuinely cannot fit on one row at this width.
|
||||
expect(bar.intrinsicWidth).toBeGreaterThan(bar.clientWidth);
|
||||
|
||||
expect(bar.clipped, 'settings tabs clipped out of view').toEqual([]);
|
||||
expect(bar.barScrolls, 'tab bar still scrolls horizontally').toBe(false);
|
||||
expect(bar.scrollingAncestors, 'wrapping pushed horizontal scroll onto an ancestor').toEqual([]);
|
||||
expect(bar.rows).toBeGreaterThan(1);
|
||||
});
|
||||
|
||||
test('TASK-2245 C82: the wrap rule is inert on desktop', async ({ page, fixture }, testInfo) => {
|
||||
test.skip(testInfo.project.name !== 'desktop-chromium', 'this is the desktop control leg');
|
||||
|
||||
await openSettings(page, fixture.adminUsername, fixture.workspaceSlug);
|
||||
const bar = await probeTabBar(page);
|
||||
|
||||
// Non-vacuous: at this width the tabs fit, so a single row is a real claim.
|
||||
expect(bar.intrinsicWidth).toBeLessThanOrEqual(bar.clientWidth);
|
||||
|
||||
expect(bar.rows, 'desktop tab bar wrapped when it did not need to').toBe(1);
|
||||
expect(bar.clipped).toEqual([]);
|
||||
expect(bar.scrollingAncestors).toEqual([]);
|
||||
});
|
||||
@@ -913,16 +913,25 @@
|
||||
.settings-header { margin-bottom: var(--space-4); }
|
||||
.settings-header h1 { font-size: 1.6em; }
|
||||
/* ── Tab bar ──── */
|
||||
/* C82 (TASK-2245): the five owner tabs are 562px intrinsic while the bar's
|
||||
box is viewport minus the page's 48px of padding, so below ~610px the row
|
||||
overflowed — and with the scrollbar hidden it overflowed INVISIBLY. Measured
|
||||
at 390x844 before the fix: Storage 9.3% visible, Danger Zone 0%, i.e. workspace
|
||||
export and deletion reachable only by a swipe nothing advertised. Wrapping is
|
||||
what makes every label legible at once; a scrolling row that opens at
|
||||
scrollLeft=0 leaves the later tabs clipped in the initial view, which is the
|
||||
view the user is given.
|
||||
Deliberately NOT inside a media query: flex-wrap is inert when the row fits, and
|
||||
that is measured, not assumed — the trail carries the counterfactual at eight
|
||||
widths, where 640/768/1024/1280 stay one row at 34px, identical to the scrolling
|
||||
version, and only 320-430 wrap. */
|
||||
.tab-bar {
|
||||
display: flex;
|
||||
flex-wrap: wrap;
|
||||
gap: var(--space-1);
|
||||
border-bottom: 1px solid var(--border);
|
||||
margin-bottom: var(--space-6);
|
||||
overflow-x: auto;
|
||||
scrollbar-width: none;
|
||||
-webkit-overflow-scrolling: touch;
|
||||
}
|
||||
.tab-bar::-webkit-scrollbar { display: none; }
|
||||
.tab {
|
||||
padding: var(--space-2) var(--space-4);
|
||||
font-size: 0.9em;
|
||||
|
||||
Reference in New Issue
Block a user