diff --git a/CLAUDE.md b/CLAUDE.md index 763e8a2..cc8d2f1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1113,9 +1113,9 @@ not the fix and cannot be: that function is the `document` listener for `navigate`, so it is an infinite loop. **It is a store rather than an event, because a component that mounts -after a navigation still has to know.** `bottom-nav`'s "More" drawer +after a navigation still has to know.** `bottom-nav`'s "More" sheet creates its `` on open, and that copy had heard no -`navigate` at all — standing on Albums, the drawer opened highlighting +`navigate` at all — standing on Albums, it opened highlighting Home. An event has no answer for a listener that was not there. **A detail view is not a view here**, so the destination it was opened @@ -1216,7 +1216,7 @@ and then vanishing. than a general rule about phones.** `PHONE_COLUMN_IDS` is the precedent for "what a phone shows is a different question", and it would apply — except that `bottom-nav`'s "More" opens the *same* ``, -which filters, so an unfiltered bar would contradict its own drawer one +which filters, so an unfiltered bar would contradict its own sheet one tap away. Which four tabs is still plan 016's committed subset; this only removes from it, and "More" is never filtered because it is how everything else stays reachable. @@ -1874,7 +1874,7 @@ listing the destinations again — but rendering it unconditionally put a second copy of every `data-testid="nav-*"` in the DOM, and 30 existing specs failed with "strict mode violation: resolved to 2 elements" on a desktop viewport where the element is not even visible. It renders only -while the drawer is open, and `bottom-nav.test.ts` asserts its absence +while the sheet is open, and `bottom-nav.test.ts` asserts its absence before that. **The tab bar is four destinations and a way to the rest.** Three to @@ -1883,6 +1883,33 @@ is 32px each. Which four is plan 016's committed subset, and everything else — Settings included, because a phone still needs it — is behind "More". +**And "More" rises from the bottom, on #60's sheet rather than a +second one** (#71). It was a `wa-drawer placement="start"`: a 200px +column of a 424px screen, opening away from the thumb that asked for +it, with the rest of its 400px band empty. It is the *same element* +with `placement="bottom"` and `without-header`, which is what keeps +the change to where it comes from — `wa-drawer` renders a native +`` and opens it with `showModal()`, so #60's containment +finding carries over with nothing new to prove, and the focus trap, +Escape, tap-outside and `wa-after-hide` all come along. Measured at +424x439: 424 wide, 373 tall (85vh, so there is an outside to tap), +48px rows. + +Three things about it are load-bearing. **The sidebar is mounted +rather than re-listed as data**, which the issue offers as the +alternative: the shell's own `` is `display: none` below +600px rather than removed, so a second list drawing `nav-*` handles is +the duplication above, and it would be a second place to add the next +view to. **There is one scroller, and it is the sheet's body** — the +reported "only part of the screen scrolls under my finger" is three +nested ones (the dialog, its body, and the sidebar's own +`overflow-y: auto` host), so which box a drag moves depends on where +the finger landed; `overscroll-behavior: contain` is the other half. +And **`expanded` means the host owns the box, not just the labels**: +`app-sidebar` writes an *inline* width and caps itself at 400px, which +beats any rule the host could write, so the width, the scrolling and +the mouse-only resize handle all follow that attribute. + **There are three supported size bands, and the queue is part of the promise.** Plan 018 (#24) wrote them down: **Phone** below 600 (bottom nav, reflows, fits 320px exactly), **Compact** 600–899 (icon sidebar), diff --git a/e2e/specs/phone-shell.spec.ts b/e2e/specs/phone-shell.spec.ts index 8969511..7653c34 100644 --- a/e2e/specs/phone-shell.spec.ts +++ b/e2e/specs/phone-shell.spec.ts @@ -98,6 +98,58 @@ test.describe('the shell on a phone', () => { ).toBeVisible(); }); + test('draws "More" as a sheet on the bottom edge (#71)', async ({ app }) => { + await app.getByTestId('tab-more').click(); + await expect(app.getByTestId('nav-drawer').locator('app-sidebar')) + .toBeVisible(); + + // What the report is about is geometry, and geometry is what no + // other assertion here can see: the side drawer was a 200px column + // opening away from the thumb that asked for it, with the rest of + // its 400px band empty. Measured rather than screenshotted, since + // the failure is a number. + // + // Polled, because a sheet *arrives*: the drawer's show animation + // translates it a full height below the fold, so a measurement + // taken the moment its content is visible reports a box hanging + // 412px off the bottom of the screen. Asking for the settled + // number is the assertion; asking once is a race. + const measure = () => app.evaluate(() => { + const nav = document.querySelector('bottom-nav'); + const drawer = nav?.shadowRoot?.querySelector('wa-drawer'); + const dialog = drawer?.shadowRoot?.querySelector('[part~="dialog"]'); + const sidebar = nav?.shadowRoot?.querySelector('app-sidebar'); + const row = sidebar?.shadowRoot?.querySelector('li button'); + const box = dialog?.getBoundingClientRect(); + + return { + left: Math.round(box?.left ?? -1), + right: Math.round(box?.right ?? -1), + bottom: Math.round(box?.bottom ?? -1), + height: Math.round(box?.height ?? -1), + row: Math.round(row?.getBoundingClientRect().height ?? -1), + viewport: [window.innerWidth, window.innerHeight], + }; + }); + + await expect + .poll(async () => (await measure()).bottom) + .toBe(PHONE.height); + + const sheet = await measure(); + + expect(sheet.left).toBe(0); + expect(sheet.right).toBe(sheet.viewport[0]); + + // A surface covering the whole screen is a page, not a sheet -- + // which is also what leaves an outside to tap on, the only pointer + // route out of it (#171 is the same question one surface over). + expect(sheet.height).toBeLessThan(sheet.viewport[1]); + + // 48px rows, from #186's touch floor and #60's context sheet. + expect(sheet.row).toBeGreaterThanOrEqual(48); + }); + for (const vp of [PHONE, SMALL_PHONE]) { test(`does not scroll sideways at ${vp.width}×${vp.height}`, async ({ app }) => { await app.setViewportSize(vp); diff --git a/frontend/src/components/bottom-nav/bottom-nav.ts b/frontend/src/components/bottom-nav/bottom-nav.ts index 4a4637b..439f366 100644 --- a/frontend/src/components/bottom-nav/bottom-nav.ts +++ b/frontend/src/components/bottom-nav/bottom-nav.ts @@ -27,15 +27,43 @@ interface Tab { * three to five items before the targets stop being thumb-sized — * 360 px over eleven sidebar entries is 32 px each — so the four here * are the ones plan 016's subset says a phone is *for*, and "More" - * opens the existing `` in a drawer. That is deliberately + * opens the existing `` in a sheet. That is deliberately * a reuse rather than a second nav: two lists of destinations is two * places to add the next view to, and the sidebar already carries the * drag-to-navigate behaviour, the active state and the labels. * + * **"More" rises from the bottom, and it is the same sheet a context + * menu is** (#71). It was a `wa-drawer` sliding in from the side: a + * 200px column of a 424px screen, opening away from the thumb that + * asked for it, with three nested scrollers in it — the dialog, its + * body, and the sidebar's own `overflow-y: auto` host — which is the + * "only part of the screen scrolls under my finger" in the report. + * + * Three things about the replacement are load-bearing. + * + * **It is the same element with another `placement`, not a new + * surface.** `wa-drawer` renders a native `` and opens it with + * `showModal()`, which is exactly what `menu-surface`'s sheet relies + * on — Chrome 37, the real top layer — so #60's containment finding + * carries over with nothing new to prove, and the focus trap, Escape, + * tap-outside and `wa-after-hide` all come along unchanged. + * + * **The body is the only scroller**, with `overscroll-behavior: + * contain`, and the sidebar is told to stop being one. Nesting them is + * what makes a drag scroll the wrong box. + * + * **The sidebar is still mounted rather than re-listed as data**, + * which the issue offers as an alternative. Its `data-testid` per + * destination is the reason: the shell's own sidebar is `display: + * none` below 600px rather than removed, so a second list drawing + * `nav-*` handles is the duplication this component already renders + * conditionally to avoid — and it would be a second place to add the + * next view to, with its own copy of #25's visibility filter. + * * It emits the same bubbling, composed `navigate` event the sidebar * does, so `index.ts` needs no knowledge of it, and it listens for that * event globally for the same reason the sidebar does: a navigation it - * did not send (a card click, a detail view, the drawer) still has to + * did not send (a card click, a detail view, the sheet) still has to * move the highlight. */ @customElement('bottom-nav') @@ -115,15 +143,50 @@ export class BottomNav extends LitElement { white-space: nowrap; } - wa-drawer::part(body) { - padding: 0; + /* The sheet. --size is the drawer's own API for the axis its + placement uses, so auto is what makes it hug its content + instead of being a fixed 25rem band; the rest is the shape + the menu-surface context sheet already has, so a phone meets + one sheet rather than two. 85vh for its reason too: a surface + covering the whole screen is a page, not a sheet. */ + wa-drawer { + --size: auto; } - app-sidebar { - /* The sidebar sizes itself inline and collapses to icons - below 900px, which is every phone. In the drawer there - is room for the labels, so it is told not to. */ - height: 100%; + wa-drawer::part(dialog) { + max-height: 85vh; + border-radius: 12px 12px 0 0; + /* The sidebar paints its own surface, so the sheet takes + that colour rather than the menus' elevated one: two + greys in one sheet is a seam across the middle of it. */ + background-color: var(--yj-bg-surface, #212529); + /* One scroller, and it is the body below. The dialog's own + overflow: auto is what let the sheet scroll as well as + its content, and it is also what would square off the + corners this rule just rounded. */ + overflow: hidden; + } + + wa-drawer::part(body) { + padding: 0; + /* A scroll that reaches the end of this list must not + become a scroll of the page underneath it. */ + overscroll-behavior: contain; + /* The sheet sits on the bottom edge, so the last + destination would otherwise be under the home indicator + on a gesture-navigation phone -- the same allowance the + bar itself makes above. */ + padding-bottom: env(safe-area-inset-bottom, 0); + } + + /* A sheet is dragged at with a thumb, so it says where its top + edge is. Decorative: the destinations are below it. */ + .grip { + width: 36px; + height: 4px; + margin: 8px auto 4px; + border-radius: 2px; + background: var(--yj-text-tertiary, #888); } `]; @@ -158,7 +221,7 @@ export class BottomNav extends LitElement { private visibilityCtrl = new ViewVisibilityController(this); /** - * Whether the drawer has been asked for. + * Whether the sheet has been asked for. * * The sidebar inside it is rendered only while this is true, and * that is not an optimisation. `app-sidebar` carries a @@ -200,15 +263,17 @@ export class BottomNav extends LitElement { override updated() { // Web Awesome renders its heading into its own shadow root and - // never points aria-labelledby at it, so the drawer would + // never points aria-labelledby at it, so the sheet would // otherwise be announced unnamed -- the same fix, and the same // reason, as every wa-dialog in the app. A drawer's shadow root - // has the same shape, so the helper needs no change. + // has the same shape, so the helper needs no change; under + // `without-header` there is no heading to point at, which is + // that helper's documented `aria-label` path. nameDialog(this.drawer); } private onGlobalNavigate = () => { - // A navigation from inside the drawer is the drawer's job done. + // A navigation from inside the sheet is the sheet's job done. // The highlight is not this listener's business any more. this.drawerOpen = false; }; @@ -274,12 +339,14 @@ export class BottomNav extends LitElement { +
${this.drawerOpen ? html`` : nothing} diff --git a/frontend/src/components/sidebar/app-sidebar.ts b/frontend/src/components/sidebar/app-sidebar.ts index f97b263..55696a7 100644 --- a/frontend/src/components/sidebar/app-sidebar.ts +++ b/frontend/src/components/sidebar/app-sidebar.ts @@ -42,6 +42,26 @@ export class AppSidebar extends LitElement { scrollbar-width: thin; } + /* A host that has made room owns the box, not just the labels + (#71). The bottom-nav sheet is the width of the screen and + provides the one scroll container it needs; left to itself + the sidebar is a 200px column with a second scroller inside + it, which is what a nested scroll region feels like under a + thumb -- part of the surface moves and part of it does not. */ + :host([expanded]) { + max-width: none; + height: auto; + overflow: visible; + } + + /* And the width is not draggable there. It is a mouse + affordance (mousedown, col-resize) sitting on the right edge + of a touch surface, where the compatibility mouse events a + tap synthesises can start a resize nobody asked for. */ + :host([expanded]) .resize-handle { + display: none; + } + .resize-handle { position: absolute; top: 0; @@ -160,6 +180,25 @@ export class AppSidebar extends LitElement { :host(.collapsed) li button wa-icon { font-size: var(--yj-icon-md); } + + /* Below 600px the only place this renders is the bottom-nav + sheet -- the shell's own copy is display: none there -- so + the rows are sized for the thumb that opened it: 48px, which + is #186's floor and the height every row in #60's context + sheet already has. A media query inside a shadow root is + answered by the viewport, so the component states this + itself rather than the sheet reaching in. */ + @media (max-width: 599px) { + ul { + padding: 4px 8px 8px; + } + + li button { + min-height: 48px; + padding: 12px 10px; + gap: 14px; + } + } `]; /** Delay in ms before a drag-hover triggers navigation. */ @@ -188,11 +227,14 @@ export class AppSidebar extends LitElement { /** * Keep the labels regardless of the viewport, for a host that has - * made room for them -- `bottom-nav`'s drawer, which is the whole + * made room for them -- `bottom-nav`'s sheet, which is the whole * screen wide on the phone where this would otherwise auto-collapse * to icons. The auto-collapse is a *width* response to a narrow - * shell, and inside a drawer the shell is not what the sidebar is + * shell, and inside a sheet the shell is not what the sidebar is * sharing space with. + * + * It says the host owns the *box*, not only the labels: the width, + * the scrolling and the resize handle all follow it (#71). */ @property({ type: Boolean, reflect: true }) expanded = false; @@ -365,9 +407,18 @@ export class AppSidebar extends LitElement { * be a media query in the stylesheet. */ private applyViewportWidth() { - const narrow = - !this.expanded && - (this.narrowViewport?.matches ?? false); + // A host that made room decides how much: `bottom-nav`'s sheet + // is the whole screen wide, and the inline width below -- which + // beats any rule the host could write -- would draw the old + // 200px side drawer inside it. + if (this.expanded) { + this.style.width = '100%'; + this.collapsed = false; + + return; + } + + const narrow = this.narrowViewport?.matches ?? false; const width = narrow ? MIN_WIDTH : this.userWidth; diff --git a/frontend/test/components/bottom-nav.test.ts b/frontend/test/components/bottom-nav.test.ts index a8eb66b..1be0549 100644 --- a/frontend/test/components/bottom-nav.test.ts +++ b/frontend/test/components/bottom-nav.test.ts @@ -193,4 +193,119 @@ describe('bottom-nav', () => { expect(shadow(el, 'app-sidebar')?.hasAttribute('expanded')) .toBe(true); }); + + it('draws "More" as a sheet rising from the bottom', async () => { + const el = await fixture