From ae85df0dad489160d7af3f504452b05880eb7f9a Mon Sep 17 00:00:00 2001 From: Logan Date: Wed, 19 Aug 2026 21:35:03 -0400 Subject: [PATCH 1/2] fix(shell): give the top bar a measured fit at every width MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bar was 611px inside a 600px viewport at the bottom of the Compact band, and 862px while a scan with a real library's title ran, because `job-indicator` is `hidden` when idle and 235px wide when it is not. `body` is `overflow-x: auto`, so a user got a horizontal scrollbar on a shell #24 promised would not need one — and the band is 600 to 899 with work in flight, not the 600 to 610 the idle measurement suggested. `services/top-bar-fit.ts` is `page-header`'s treatment one bar up: a ResizeObserver, every pass starting from all-visible, hiding the lowest-priority child until it fits. Measured rather than breakpointed because three of the five children are as wide as their content — the library filter by the longest library name, the indicator by the running job's title, the search box by its view-scoped placeholder — so any width picked is right for one library, one job and one view. What yields is decided by #24's own sentence, which rules out the two cheapest candidates in the Direction. Hiding the library filter takes away an action, since it is the only control in the app that selects a library (filed as #148, which is the phone already doing it), and collapsing search to an icon is #57's, which is blocked behind #62. So the wordmark yields first — a brand the window title bar repeats, and visually-hidden rather than `display: none` because that h1 is the document's heading — and then the indicator's label, leaving the ring, which the component already does below 600px and whose live region announces the state either way. "Fits" is the children against the content box, not `scrollWidth` against `clientWidth`: `scrollWidth` counts the left padding and not the right, so the first version read 700/700 with the indicator sitting in the whole right gutter. And the bar does not resize when a job starts, which is the case this is for, so every child is observed too. Pinned before it was fixed, as the issue asks. On the unfixed build the new spec fails at 600 idle and at 600, 800 and 900 with a job, and passes at 390, 899 and 1440; `layout-overflow.spec.ts` gains 600x600 and failed there. That spec asserts on the *shell*, so it was green throughout this defect — the per-child measurement is #69's lesson, and it is what caught the gutter case above. Closes #143 --- e2e/specs/layout-overflow.spec.ts | 42 +++- e2e/specs/top-bar-fit.spec.ts | 207 ++++++++++++++++++ frontend/index.css | 36 +++ frontend/index.ts | 9 + frontend/src/components/jobs/job-indicator.ts | 16 +- frontend/src/services/top-bar-fit.ts | 199 +++++++++++++++++ 6 files changed, 499 insertions(+), 10 deletions(-) create mode 100644 e2e/specs/top-bar-fit.spec.ts create mode 100644 frontend/src/services/top-bar-fit.ts diff --git a/e2e/specs/layout-overflow.spec.ts b/e2e/specs/layout-overflow.spec.ts index c5b6401..eaa1953 100644 --- a/e2e/specs/layout-overflow.spec.ts +++ b/e2e/specs/layout-overflow.spec.ts @@ -33,6 +33,14 @@ const VIEWPORTS = [ // was missing its own worst case. { name: '900×600 (the widest sidebar, so the narrowest content)', width: 900, height: 600 }, { name: `the minimum (${MIN_VIEWPORT.width}×${MIN_VIEWPORT.height})`, ...MIN_VIEWPORT }, + // Below the enforced minimum on purpose, and for the reason 700×480 + // is below it further down: a scaled display or a large system font + // lands the layout here without the window ever being dragged there, + // and 600 is the last width before the phone layout takes over. The + // *narrowest header* is a different question from the narrowest + // content area and has a different answer — this one (#143), where + // the bar was 611px inside 600 sitting still. + { name: '600×600 (the bottom of the Compact band)', width: 600, height: 600 }, ]; /** @@ -145,6 +153,12 @@ test.describe('the app fits in its own window', () => { test.describe('the title block fits its bar', () => { test('the hgroup stays inside the 4em top bar', async ({ app }) => { + // Stated rather than inherited from whatever ran last. Since #143 + // the wordmark is visually hidden at widths where the bar cannot + // afford it, so a test about its *vertical* fit has to say which + // width it is asking about. + await app.setViewportSize({ width: 1440, height: 900 }); + // The state a11y.29 landed in. The pair is flex-centred and a UA // gives an `h1` a 0.67em top margin, so the block measured 67px // inside 64 — pre-existing, and invisible until dropping the h3's @@ -246,16 +260,26 @@ test.describe('the shell reflows rather than hiding what does not fit', () => { test(`no scrollbar appears at ${vp.name}`, async ({ app }) => { await app.setViewportSize({ width: vp.width, height: vp.height }); - const excess = await app.evaluate(() => { - const de = document.documentElement; + // Polled, for the reason the track-row test above is: since #143 + // the top bar's fit is *measured* — a ResizeObserver decides what + // it can afford at this width — so a single read taken straight + // after the resize races the observer and reports the frame + // before it. Read once, this passed alone and failed in the full + // suite, which is the shape of a timing assumption rather than of + // a defect. + // + // The other half of the assertion: at every size this app + // promises, the fix costs nothing. A scrollbar that is always + // there is a worse answer than the clipping it replaced. + await expect + .poll(() => + app.evaluate(() => { + const de = document.documentElement; - return de.scrollWidth - de.clientWidth; - }); - - // The other half: at every size this app promises, the fix costs - // nothing. A scrollbar that is always there is a worse answer - // than the clipping it replaced. - expect(excess).toBe(0); + return de.scrollWidth - de.clientWidth; + }), + ) + .toBe(0); }); } }); diff --git a/e2e/specs/top-bar-fit.spec.ts b/e2e/specs/top-bar-fit.spec.ts new file mode 100644 index 0000000..747fdd4 --- /dev/null +++ b/e2e/specs/top-bar-fit.spec.ts @@ -0,0 +1,207 @@ +import { test, expect } from '../support/fixtures.js'; + +/** + * The top bar fits the window it is in (#143). + * + * **This is measured per child, not on the shell**, which is #69's + * lesson repeated one component over: `layout-overflow.spec.ts` asserts + * the *document* needs no sideways scrolling, and clipping inside a + * component is invisible to it — which is exactly why that spec was + * green throughout this defect. What a user sees is a control rendered + * past the edge of the bar it belongs to, so that is what is asserted. + * + * **And it is measured with a job running**, which is the half the + * original report missed. `job-indicator` is `hidden` while idle and up + * to 235px wide when it is not, so the bar was 611px inside 600 sitting + * still and 862px during a scan — 171 to 262px of overflow, arriving + * exactly when a user has reason to look at that bar. Nothing else in + * this suite has ever measured a layout with work in flight; + * `/__test/emit` stages it without staging the scan. + */ +type Page = import('@playwright/test').Page; + +/** + * The widths this asks about. + * + * 600 is the bottom of the Compact band (#24) and where the defect + * lands; 899 and 900 straddle `nav-history` appearing (68px more to + * find, at the width that just gained the sidebar's labels); 800 is the + * enforced minimum; 390 is a phone, where the answer must be that + * nothing collapses because the media queries already did the work. + */ +const WIDTHS = [390, 600, 800, 899, 900, 1440]; + +/** + * A scan whose title is as long as a real one gets. The label is capped + * at 12rem by the component, so this is the widest the indicator can + * be — measuring with "Scanning" instead reports a bar that fits and a + * defect that is 100px smaller than it is. + */ +const LONG_JOB = { + id: 'top-bar-fit', + kind: 'library-scan', + state: 'running', + title: 'Scanning Music from the external drive', + current: 40, + total: 100, +}; + +/** + * Every child's right edge against the bar's own content box. + * + * The content box, not `clientWidth`: the bar has a 2em right gutter, + * and a control sitting in the padding is already the failure — it is + * simply one that `scrollWidth` under-reports, because `scrollWidth` + * counts the left padding and not the right. + */ +const overflowingChildren = (page: Page) => + page.evaluate(() => { + const bar = document.querySelector('header.top-bar')!; + const style = getComputedStyle(bar); + const box = bar.getBoundingClientRect(); + const left = box.left + parseFloat(style.paddingLeft); + const right = box.right - parseFloat(style.paddingRight); + + return [...bar.children] + .filter((child) => { + const cs = getComputedStyle(child); + + // Out of flow is out of the question: a collapsed wordmark is + // `position: absolute` and 1px wide precisely so it costs the + // row nothing. + if (cs.display === 'none' || cs.position === 'absolute') return false; + + const r = child.getBoundingClientRect(); + + return r.width > 0 && (r.right > right + 0.5 || r.left < left - 0.5); + }) + .map((child) => { + const r = child.getBoundingClientRect(); + + return `${child.tagName.toLowerCase()}: ${Math.round(r.left)}..${Math.round(r.right)} outside ${Math.round(left)}..${Math.round(right)}`; + }); + }); + +/** What the fit pass gave up, read back off the DOM it changed. */ +const collapsed = (page: Page) => + page.evaluate(() => ({ + wordmark: !!document.querySelector('header.top-bar hgroup.yj-collapsed'), + jobLabel: !!document.querySelector('job-indicator[compact]'), + })); + +test.describe('the top bar fits the window', () => { + for (const width of WIDTHS) { + test(`no control sits outside the bar at ${width}px, idle`, async ({ + app, + }) => { + await app.setViewportSize({ width, height: 600 }); + + await expect.poll(() => overflowingChildren(app)).toEqual([]); + }); + + test(`no control sits outside the bar at ${width}px, with a job running`, async ({ + app, + testctl, + }) => { + await app.setViewportSize({ width, height: 600 }); + await testctl.emit('JobsChanged', [LONG_JOB]); + + // The indicator has to actually be up, or this test passes by + // measuring the idle case under another name. + await expect(app.locator('job-indicator')).toBeVisible(); + + await expect.poll(() => overflowingChildren(app)).toEqual([]); + }); + } + + /** + * The other half of "measured, never breakpointed": a rule that + * collapses defensively at every narrow width fits just as well and + * is a worse app. 1440 is roomy at any job title; 899 was measured to + * fit with the longest one, because `nav-history` is not there yet. + */ + test('nothing is given up where there is room for it', async ({ + app, + testctl, + }) => { + await app.setViewportSize({ width: 1440, height: 900 }); + await testctl.emit('JobsChanged', [LONG_JOB]); + await expect(app.locator('job-indicator')).toBeVisible(); + + await expect.poll(() => collapsed(app)).toEqual({ + wordmark: false, + jobLabel: false, + }); + }); + + /** + * And it gives them back. The pass starts from all-visible every + * time, so this is the property that a rule which only ever *added* + * to the collapsed set would fail — the wordmark would be gone for + * the rest of the session after one narrow moment. + */ + test('the wordmark comes back when the window does', async ({ + app, + testctl, + }) => { + await testctl.emit('JobsChanged', [LONG_JOB]); + await app.setViewportSize({ width: 600, height: 600 }); + + await expect.poll(() => collapsed(app)).toEqual({ + wordmark: true, + jobLabel: true, + }); + + await app.setViewportSize({ width: 1440, height: 900 }); + + await expect.poll(() => collapsed(app)).toEqual({ + wordmark: false, + jobLabel: false, + }); + }); + + /** + * The wordmark yields its width and not its existence: `display: + * none` would take the document from one top-level heading to none. + */ + test('the collapsed wordmark is still the document heading', async ({ + app, + testctl, + }) => { + await testctl.emit('JobsChanged', [LONG_JOB]); + await app.setViewportSize({ width: 600, height: 600 }); + + await expect.poll(() => collapsed(app)).toMatchObject({ wordmark: true }); + + await expect( + app.getByRole('heading', { name: 'YellowJacket', level: 1 }), + ).toHaveCount(1); + }); + + /** + * And the indicator keeps saying what it is doing after its visible + * label goes — the `sr-only` live region is what announces the state, + * which is the same argument the phone's own rule was written on. + */ + test('the job indicator still announces its state without its label', async ({ + app, + testctl, + }) => { + await testctl.emit('JobsChanged', [LONG_JOB]); + await app.setViewportSize({ width: 600, height: 600 }); + + await expect.poll(() => collapsed(app)).toMatchObject({ jobLabel: true }); + + const spoken = await app + .locator('job-indicator') + .evaluate( + (el) => + el.shadowRoot?.querySelector('[aria-live]')?.textContent?.trim() ?? '', + ); + + expect(spoken).toContain('Scanning Music from the external drive'); + + // Leave the app as the next spec expects to find it. + await app.setViewportSize({ width: 1440, height: 900 }); + }); +}); diff --git a/frontend/index.css b/frontend/index.css index 44e6a47..4431617 100644 --- a/frontend/index.css +++ b/frontend/index.css @@ -104,6 +104,42 @@ p { flex: 0 1 320px; } +/* What the bar gives up when it does not fit is decided by measuring + it (`services/top-bar-fit.ts`, #143). Two rules here are what make + that measurement mean anything. + + **Nothing but the search box may shrink.** `scrollWidth` reports a + perfect fit while a child quietly truncates -- #69's trap, one + component over -- and the indicator's label is `text-overflow: + ellipsis`, so it would have absorbed the deficit and hidden it. The + search box is exempt because it shrinks between its 320px basis and + the 200px floor its own stylesheet sets, and a narrower input hides + nothing it was showing. */ +.top-bar hgroup, +.top-bar library-filter, +.top-bar job-indicator { + flex-shrink: 0; +} + +/* **The wordmark yields its width, not its existence.** It is the + app's top-level heading as well as its brand, and `display: none` + would take a document from one `h1` to none at exactly the widths + where the view's own header is the only thing left saying where you + are. This is `styles/sr-only.css.ts`'s recipe, written out because + that one is a `CSSResult` for shadow roots and this is the light + DOM. */ +.top-bar hgroup.yj-collapsed { + position: absolute; + width: 1px; + height: 1px; + padding: 0; + margin: -1px; + overflow: hidden; + clip-path: inset(50%); + white-space: nowrap; + border: 0; +} + /* The bar is `justify-content: space-between`, which with four children spreads them evenly and left back/forward floating in the middle of nothing. Collecting the free space *after* this one puts the pair diff --git a/frontend/index.ts b/frontend/index.ts index 3ebfa2b..61602b2 100644 --- a/frontend/index.ts +++ b/frontend/index.ts @@ -54,6 +54,7 @@ import '@store/theme-store'; import './src/services/keyboard-shortcut-service'; import { activateView, deactivateView } from '@utils/view-lifecycle'; import { installLongPressContextMenu } from '@utils/long-press'; +import { installTopBarFit } from './src/services/top-bar-fit'; import { hasTrackPayload, getDragPayload, @@ -73,6 +74,14 @@ registerBundledIcons(); // on `pointerType === 'touch'` only. installLongPressContextMenu(); +// The top bar decides what it can afford to show (#143). Here rather +// than in a component because the bar is light DOM in index.html and +// its children are five separate elements; the shell is the only thing +// that can see all five at once. +const topBar = document.querySelector('header.top-bar'); + +if (topBar) installTopBarFit(topBar); + // --------------------------------------------------------------------------- // View caching navigation system // --------------------------------------------------------------------------- diff --git a/frontend/src/components/jobs/job-indicator.ts b/frontend/src/components/jobs/job-indicator.ts index 0791b04..5d95403 100644 --- a/frontend/src/components/jobs/job-indicator.ts +++ b/frontend/src/components/jobs/job-indicator.ts @@ -155,13 +155,27 @@ export class JobIndicator extends LitElement { goes -- the live region in render() is what announces this, and it is unaffected, so the ring keeps its accessible name and screen readers keep hearing the - state change. */ + state change. + + [compact] is the same removal asked for by measurement + rather than by width, and it is set from outside: the + shell's fit pass (services/top-bar-fit.ts, #143) owns + it, because between 600 and 900 whether this label fits + depends on what else is in the bar and on how long the + running job's title is -- 235px for "Scanning Music from + the external drive" -- rather than on the viewport. Two + triggers, one effect, and the phone's is unconditional + because it was argued and pinned before this existed. */ @media (max-width: 599px) { .label { display: none; } } + :host([compact]) .label { + display: none; + } + .alert-dot { width: 6px; height: 6px; diff --git a/frontend/src/services/top-bar-fit.ts b/frontend/src/services/top-bar-fit.ts new file mode 100644 index 0000000..d253b46 --- /dev/null +++ b/frontend/src/services/top-bar-fit.ts @@ -0,0 +1,199 @@ +/** + * What the top bar drops when it runs out of room (#143). + * + * The bar holds five children — the wordmark, back/forward, the library + * filter, the search box and the job indicator — and at the bottom of + * the Compact band they do not all fit. Measured on `main` at 600×600: + * the bar is 611px inside a 600px viewport sitting still, and **862px + * while a scan with a long title is running**, because `job-indicator` + * is `hidden` when idle and up to 235px wide when it is not. `body` is + * `overflow-x: auto`, so what a user sees is a horizontal scrollbar on + * a shell that #24 promised would not need one. + * + * **The fit is measured, never breakpointed**, which is `page-header`'s + * rule (#69) and applies here for a reason specific to this bar: three + * of its five children are as wide as their *content*. The library + * filter is a `` sized by the longest library name, the +indicator by the running job's title, the search box by its view-scoped +placeholder — so any width picked is right for one library, one job and +one view. Swept with a long-titled scan staged, the bar overflowed at +**every** width from 600 to 899 *and* at 900 where `nav-history` +appears, while 899 fits; a breakpoint fixing "600 to 610" would have +fixed whichever case happened to be idle when it was measured. + +**What yields is decided by the promise above, which rules out the two +cheapest answers.** Hiding the library filter takes away an action — +`library-filter` is the only control in the app that calls +`setSelectedLibrary` — so it trades this promise for the same promise +(#148 is the phone already doing that). Collapsing the search box to an +icon is what #57 wants and #57 is blocked behind #62, so building it +here is building it without the thing that blocks it. The two that +yield are the two that are **not** actions: the wordmark, which the +window's own title bar repeats and which #48 wants down to "YJ" at +every width anyway, and then the job indicator's *label*, leaving the +ring — which is not a new judgement, since the component already drops +it below 600px and its `sr-only` live region is what announces the +state either way. + +**The wordmark yields its width, not its existence.** The collapsed +rule is visually-hidden rather than `display: none`, because that `h1` +is the document's top-level heading as well as the brand. + +**"Fits" is the children against the content box, and `scrollWidth` +cannot express it.** `scrollWidth` counts a box's left padding and not +its right, so with 2em gutters it under-reports by 32px: the first fix +read `700/700` — a perfect fit — with the indicator sitting in the +whole right gutter. Same family as #69's title trap, and found only +because `top-bar-fit.spec.ts` measures **per child**, which is what +`layout-overflow.spec.ts` cannot do and why that spec was green +throughout the defect. + +And **the bar does not resize when a job starts**, which is the case the +whole thing is for — a ResizeObserver on the header alone never fires, +so every element child is observed too. + **900 is the worst desktop width, not the 800×600 minimum.** The sidebar collapses to icons *below* 900, so the main panel is 843px at 899 and 700px at 900 — the narrowest content area any desktop width