From 0b16be492b5ea4e3339787789ab9a6fa7d173ed2 Mon Sep 17 00:00:00 2001 From: xarmian Date: Wed, 9 Sep 2026 14:42:47 -0400 Subject: [PATCH] fix(web): wrap the settings tab bar so no tab is hidden at phone width (TASK-2245 / C82) (#1306) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 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 --- web/e2e/task-2245-c82-settings-tabs.spec.ts | 121 ++++++++++++++++++ .../[workspace]/settings/+page.svelte | 17 ++- 2 files changed, 134 insertions(+), 4 deletions(-) create mode 100644 web/e2e/task-2245-c82-settings-tabs.spec.ts diff --git a/web/e2e/task-2245-c82-settings-tabs.spec.ts b/web/e2e/task-2245-c82-settings-tabs.spec.ts new file mode 100644 index 00000000..56184bce --- /dev/null +++ b/web/e2e/task-2245-c82-settings-tabs.spec.ts @@ -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 ) that scroll horizontally. */ + scrollingAncestors: string[]; + clipped: string[]; +}; + +async function probeTabBar(page: Page): Promise { + 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([]); +}); diff --git a/web/src/routes/[username]/[workspace]/settings/+page.svelte b/web/src/routes/[username]/[workspace]/settings/+page.svelte index a040bd22..efbe7ace 100644 --- a/web/src/routes/[username]/[workspace]/settings/+page.svelte +++ b/web/src/routes/[username]/[workspace]/settings/+page.svelte @@ -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;