diff --git a/.planning/NOTES.md b/.planning/NOTES.md index f7bfee7..b51bde7 100644 --- a/.planning/NOTES.md +++ b/.planning/NOTES.md @@ -4707,3 +4707,81 @@ measurement taken was of a screen with `job-band` on it and the art at route other than its own** (#175) — it was still up, full-screen and intercepting pointer events, after `AddLibrary` succeeded through the binding, and was gone after a relaunch. Filed. + +## The context menu was clipped on the device, and the fix needed four measurements nothing here could make (measured 2026-08-21, TLP301 / Chrome 113 / 424x439) + +#60 had been diagnosed from the Web Awesome source and was right. What +the device added was the numbers, and three things the reading had not +reached. + +**The clip, reproduced before any code was written.** Long-press on the +lowest visible track row at 424x439: + +| | | +|---|---| +| viewport | 424x439 | +| `.main-panel` | 0 to **318**, computed `contain: content` | +| menu panel | 191 to **401**, 210px tall | +| clipped away | **83px, three of seven items** | +| `wa-popup` computed position | `fixed` | +| `HTMLElement.prototype.hasOwnProperty('popover')` | **false** | +| row height | **29px** (against a 44px floor and a 48px ask) | + +A screenshot shows the menu sliced off flush with the mini player's top +edge. Both halves of the diagnosis are therefore measured, not inferred. + +**"A dialog escapes containment" was the premise, and it was untested.** +Every dialog in this app is mounted in `index.html`, *outside* +`.main-panel` — so nothing here was evidence about a dialog opened from +inside a view, which is what this change needed. A probe `` +appended to `track-list`'s shadow root and `showModal()`n paints to +y=439, over the mini player and the tab bar. A top-layer element's +containing block is the viewport, paint-contained ancestor or not. +Checking that first cost ten minutes and would have cost a rebuild. + +**The UA stylesheet is the thing that makes a naive sheet look wrong.** +That same probe came out **354px wide on a 424px screen**, centred, +because a native `` carries `max-width: calc(100% - 6px - 2em)` +and `margin: auto`. `max-width: none` and explicit margins are four +declarations that are pure undoing. + +**A retry loop cannot win against a steal that happens later.** +`MenuKeyboard` focuses the first item and returns as soon as it lands; +`wa-dialog` then focuses `[autofocus]` or *itself* on the frame after +`showModal()`, and it cannot see our first item to prefer it — the +panel is slotted through `menu-surface`, so the dialog's own +`querySelector` stops at the ``. Measured: the sheet opened with +`document.activeElement` on the `` and every arrow key went +nowhere. Lengthening the retry budget does not help, because the first +attempt *succeeds*. The surface announcing `menu-shown` after +`wa-after-show`, and the keyboard re-asserting, is the fix. + +**The submenu was made worse before it was made better, and only a +measurement caught it.** `#playlist-submenu` is a +`placement="right-start"` flyout anchored to its row. Making the menu a +full-width sheet moved that anchor to x=0, so the flip put the playlist +picker at **x −182 to 0 — entirely off-screen**, and "Add to Playlist" +led nowhere at all. Before the change the anchor row started at x≈245 +and the same flip landed it on screen. It is a `menu-surface` too now +and stacks as a second sheet. Two lessons: a change that moves an +anchor changes every flip decision downstream of it, and *the scope I +declared on the issue was wrong* — I had said I would measure the +submenu and file it, and the measurement said fix it. + +**And the sweep found two call sites the conversion missed.** Twelve +were converted by hand; `menu-surface.test.ts` reads every source file +and fails on a `` outside a three-file allowlist, which +immediately named `queue-panel`'s add-to-playlist popup (a real menu, +converted) and `now-playing`'s cover preview (a hover affordance in the +bottom bar — allowlisted, since a touch device never opens it and +nothing clips it). A thirteenth menu written as a bare popup would pass +every tier here and be clipped on the device, which is precisely why +the guard is a source sweep rather than a rendered assertion. + +**What no tier here can see remains the clip itself.** This runner's +Chromium and CI's WebKit both have the Popover API, so the popup is +top-layered and correct and a "not clipped" assertion passes on the +broken build. The specs assert the *mechanism* — that the surface is a +native `` at phone width — which is the same move +`queue-as-a-screen.spec.ts` makes about containment and for the same +reason. diff --git a/CLAUDE.md b/CLAUDE.md index 4af3651..87510e7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1367,8 +1367,17 @@ against the real components: `wa-dropdown-item` sets its `role` in its *own* first update, so a `[role^="menuitem"]` query at `updateComplete` finds nothing — which reads exactly like a menu that opened and refused to take focus. -- **`focus()` on a popup that has not positioned itself is a silent - no-op**, so the first focus is retried across a few frames. +- **`focus()` on a surface that has not shown itself is a silent + no-op**, so the first focus is retried on a *time* budget rather + than a frame count — the thing being waited for is another + component's animation. And a retry is not enough on its own for the + sheet below: `wa-dialog` focuses `[autofocus]` or *itself* on the + frame after `showModal()`, and it cannot see the first menu item to + prefer it, because the panel is slotted through `menu-surface` and + the dialog's own `querySelector` stops at the ``. The first + attempt therefore *succeeds* and is then overwritten, which no + amount of waiting fixes — so the surface announces `menu-shown` when + it has settled and `MenuKeyboard.refocus()` re-asserts. - **Focus is only taken back if the menu had it.** A click elsewhere closes the menu too, and pulling focus to the row the user right-clicked a moment ago is worse than leaving it. @@ -1376,6 +1385,69 @@ against the real components: moving focus without setting it leaves the highlight on whichever item the mouse last touched. +**And a menu is drawn where it fits: a popup on a desktop, a bottom +sheet on a phone** (#60). `components/menu-surface/` is that one +decision. The host renders the panel it always rendered and slots it +into whichever surface is up, so `ContextMenuController` still drives +`.active` and `.anchor` as though it were talking to a `wa-popup`, and +fourteen call sites changed one tag name each and nothing else. + +**It is a correctness fix, not a taste one, and the failure was +measured on the device rather than inferred.** Chrome 113 has no +Popover API, so `wa-popup` takes its own documented fallback and +positions with `strategy: "fixed"`; `.main-panel` carries +`contain: layout style paint`, and paint containment *clips* fixed +descendants. On the reference device the main panel spans 0-318 of a +439px viewport while the open menu spanned 191-401 — three of its seven +items cut off, with no way to reach them. `showModal()` is Chrome 37 +and uses the real top layer, so a dialog is immune by construction. + +Six things about it are load-bearing. + +**"Dialogs are fine" needed checking, because every other dialog in +this app is mounted in `index.html`** — outside `.main-panel` — so it +was not evidence about one opened from inside a view. A probe dialog +appended to `track-list`'s shadow root paints to y=439, over the mini +player and the tab bar, with the contained ancestor still in place. A +top-layer element's containing block is the viewport, contained +ancestor or not. + +**The sheet has to un-do the UA stylesheet.** A native `` +carries `max-width: calc(100% - 6px - 2em)` and `margin: auto`, which +drew a 354px panel floating in the middle of a 424px screen. +`max-width: none` plus explicit margins is what makes it a sheet. + +**The row sizing lives in `contextMenuStyles`, not in the component.** +The panel is the *host's* light DOM — it stays in the host's shadow +root, so only the host's stylesheet can reach it. `menu-surface` puts +`data-sheet` on the panel and that shared stylesheet does the rest, +which is how fourteen menus went from 29px rows to 48px ones in one +edit. + +**A dismissal has to travel back.** `wa-dialog` closes itself on +Escape, which would leave the controller believing the menu is open — +and the failure mode is not a stuck sheet but the *next* long-press +doing nothing, which reads as the gesture breaking. `menu-dismiss` is +that signal; the three surfaces that do not use `ContextMenuController` +bind it themselves. + +**The playlist submenu is a sheet too, and it had to be.** It is a +`placement="right-start"` flyout, and making the menu full-width moved +its anchor — measured at x −182 to 0, entirely off-screen, so "Add to +Playlist" led nowhere. It stacks as a second sheet over the first, +which is also why `menu-shown` does not re-assert focus while the +submenu is open. + +**And which call sites exist is swept, not remembered.** A thirteenth +menu written as a bare `` works perfectly in every tier here +and is clipped on the device, so `menu-surface.test.ts` reads the +source and fails on one outside a three-file allowlist — +`menu-surface` itself, `job-indicator` (in `.top-bar`, which no +ancestor contains — the contrast that proved the diagnosis on #62) and +`now-playing`'s cover preview (a hover affordance, which a touch device +never opens). **The sweep found two of the fourteen**; twelve were +converted by hand. + **And a menu opens from a finger, through the event it already has.** `utils/long-press.ts` is one document-capture listener installed once from `index.ts`: a touch that holds still for 500 ms dispatches a diff --git a/frontend/src/components/artists-view/artists-view.ts b/frontend/src/components/artists-view/artists-view.ts index 16659af..3c143fe 100644 --- a/frontend/src/components/artists-view/artists-view.ts +++ b/frontend/src/components/artists-view/artists-view.ts @@ -26,14 +26,15 @@ import { contextMenuStyles, isContextMenuKey, } from '@utils/context-menu-controller.js'; -import type { ContextMenuHost } from '@utils/context-menu-controller.js'; +import type { ContextMenuHost, MenuTarget } from '@utils/context-menu-controller.js'; import { FavoritesController } from '@store/controllers/favorites-controller'; import { ViewLifecycleMixin } from '@utils/view-lifecycle'; import { RovingGridController } from '@utils/roving-grid'; import '@awesome.me/webawesome/dist/components/icon/icon.js'; import '@awesome.me/webawesome/dist/components/popup/popup.js'; -import type WaPopup from '@awesome.me/webawesome/dist/components/popup/popup.js'; +import type { MenuSurface } from '../menu-surface/menu-surface'; +import '../menu-surface/menu-surface'; import '@awesome.me/webawesome/dist/components/dropdown-item/dropdown-item.js'; import '@components/playlist-picker/playlist-picker.js'; import { dict, list } from '@utils/binding'; @@ -131,17 +132,17 @@ export class ArtistsView private contextMenuArtistId: number | null = null; @query('#context-menu') - private contextMenuPopup!: WaPopup; + private contextMenuPopup!: MenuSurface; @query('#playlist-submenu') - private playlistSubmenuPopup!: WaPopup; + private playlistSubmenuPopup!: MenuSurface; - getContextMenuPopup(): WaPopup | undefined { + getContextMenuPopup(): MenuTarget | undefined { return this.contextMenuPopup; } getPlaylistSubmenuPopup(): - | WaPopup + | MenuTarget | undefined { return this.playlistSubmenuPopup; } @@ -1336,11 +1337,8 @@ export class ArtistsView private renderContextMenu() { return html` - @@ -1434,13 +1432,12 @@ export class ArtistsView ` : nothing} - + - @@ -1468,7 +1465,7 @@ export class ArtistsView ` : nothing} - + `; } diff --git a/frontend/src/components/cover-grid/cover-grid.ts b/frontend/src/components/cover-grid/cover-grid.ts index 8e4f3de..c4580c9 100644 --- a/frontend/src/components/cover-grid/cover-grid.ts +++ b/frontend/src/components/cover-grid/cover-grid.ts @@ -23,7 +23,8 @@ import { gridColumnsFor, gridSpacingFor } from '@utils/grid-spacing'; import { queueStore } from '@store/queue-store'; import type { QueueSource } from '@store/queue-store'; import '@awesome.me/webawesome/dist/components/popup/popup.js'; -import type WaPopup from '@awesome.me/webawesome/dist/components/popup/popup.js'; +import type { MenuSurface } from '../menu-surface/menu-surface'; +import '../menu-surface/menu-surface'; import '@awesome.me/webawesome/dist/components/dropdown-item/dropdown-item.js'; import '@awesome.me/webawesome/dist/components/icon/icon.js'; import '@components/playlist-picker/playlist-picker.js'; @@ -51,7 +52,7 @@ import { ContextMenuController, isContextMenuKey, } from '@utils/context-menu-controller.js'; -import type { ContextMenuHost } from '@utils/context-menu-controller.js'; +import type { ContextMenuHost, MenuTarget } from '@utils/context-menu-controller.js'; import { FavoritesController } from '@store/controllers/favorites-controller'; import { creditLink, exploreLinkStyles } from '../../utils/explore-link'; import { creditStore } from '@store/credit-store'; @@ -415,17 +416,17 @@ export class CoverGrid splitIndex = 0; @query('#context-menu') - private contextMenuPopup!: WaPopup; + private contextMenuPopup!: MenuSurface; @query('#playlist-submenu') - private playlistSubmenuPopup!: WaPopup; + private playlistSubmenuPopup!: MenuSurface; // ContextMenuHost interface. - getContextMenuPopup(): WaPopup | undefined { + getContextMenuPopup(): MenuTarget | undefined { return this.contextMenuPopup; } - getPlaylistSubmenuPopup(): WaPopup | undefined { + getPlaylistSubmenuPopup(): MenuTarget | undefined { return this.playlistSubmenuPopup; } @@ -2087,11 +2088,8 @@ export class CoverGrid const { ctxMenu } = this; return html` - ${ctxMenu.contextMenuOpen @@ -2204,13 +2202,12 @@ export class CoverGrid ` : nothing} - + - ${ctxMenu.playlistSubmenuOpen @@ -2232,7 +2229,7 @@ export class CoverGrid ` : nothing} - + `; diff --git a/frontend/src/components/explore-album-details/explore-album-details.ts b/frontend/src/components/explore-album-details/explore-album-details.ts index 5106123..71bc799 100644 --- a/frontend/src/components/explore-album-details/explore-album-details.ts +++ b/frontend/src/components/explore-album-details/explore-album-details.ts @@ -49,9 +49,11 @@ import { contextMenuStyles, isContextMenuKey, } from '@utils/context-menu-controller.js'; -import type { ContextMenuHost } from '@utils/context-menu-controller.js'; +import type { ContextMenuHost, MenuTarget } from '@utils/context-menu-controller.js'; import '@awesome.me/webawesome/dist/components/popup/popup.js'; import type WaPopup from '@awesome.me/webawesome/dist/components/popup/popup.js'; +import type { MenuSurface } from '../menu-surface/menu-surface'; +import '../menu-surface/menu-surface'; import '@awesome.me/webawesome/dist/components/dropdown-item/dropdown-item.js'; import { dictByName } from '@utils/binding'; import type { TrackDetails } from '@components/track-details/track-details.js'; @@ -327,7 +329,7 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { @state() private ctxMenuTrack: MBTrack | null = null; @query('#track-context-menu') - private contextMenuPopup!: WaPopup; + private contextMenuPopup!: MenuSurface; @query('#playlist-submenu') private playlistSubmenuPopup?: WaPopup; @@ -337,11 +339,11 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { // -- ContextMenuHost interface -- - getContextMenuPopup(): WaPopup | undefined { + getContextMenuPopup(): MenuTarget | undefined { return this.contextMenuPopup; } - getPlaylistSubmenuPopup(): WaPopup | undefined { + getPlaylistSubmenuPopup(): MenuTarget | undefined { return this.playlistSubmenuPopup; } @@ -3781,11 +3783,8 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { const track = this.ctxMenuTrack; return html` - ${this.ctxMenu.contextMenuOpen && track @@ -3846,13 +3845,12 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { ` : nothing} - + - ${this.ctxMenu.playlistSubmenuOpen @@ -3869,7 +3867,7 @@ export class ExploreAlbumDetails extends LitElement implements ContextMenuHost { ` : nothing} - + `; } } diff --git a/frontend/src/components/explore-artist-details/explore-artist-details.ts b/frontend/src/components/explore-artist-details/explore-artist-details.ts index c981310..5c42394 100644 --- a/frontend/src/components/explore-artist-details/explore-artist-details.ts +++ b/frontend/src/components/explore-artist-details/explore-artist-details.ts @@ -61,9 +61,11 @@ import { contextMenuStyles, isContextMenuKey, } from '@utils/context-menu-controller.js'; -import type { ContextMenuHost } from '@utils/context-menu-controller.js'; +import type { ContextMenuHost, MenuTarget } from '@utils/context-menu-controller.js'; import '@awesome.me/webawesome/dist/components/popup/popup.js'; import type WaPopup from '@awesome.me/webawesome/dist/components/popup/popup.js'; +import type { MenuSurface } from '../menu-surface/menu-surface'; +import '../menu-surface/menu-surface'; import '@awesome.me/webawesome/dist/components/dropdown-item/dropdown-item.js'; import { dict, dictByName } from '@utils/binding'; import type { TrackDetails } from '@components/track-details/track-details.js'; @@ -215,7 +217,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost @state() private ctxMenuTarget: ContextMenuTarget | null = null; @query('#context-menu') - private contextMenuPopup!: WaPopup; + private contextMenuPopup!: MenuSurface; @query('#playlist-submenu') private playlistSubmenuPopup?: WaPopup; @@ -233,11 +235,11 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost // -- ContextMenuHost interface -- - getContextMenuPopup(): WaPopup | undefined { + getContextMenuPopup(): MenuTarget | undefined { return this.contextMenuPopup; } - getPlaylistSubmenuPopup(): WaPopup | undefined { + getPlaylistSubmenuPopup(): MenuTarget | undefined { return this.playlistSubmenuPopup; } @@ -2621,11 +2623,8 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost const target = this.ctxMenuTarget; return html` - ${this.ctxMenu.contextMenuOpen && target @@ -2643,13 +2642,12 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost ` : nothing} - + - ${this.ctxMenu.playlistSubmenuOpen @@ -2666,7 +2664,7 @@ export class ExploreArtistDetails extends LitElement implements ContextMenuHost ` : nothing} - + `; } diff --git a/frontend/src/components/explore-view/explore-view.ts b/frontend/src/components/explore-view/explore-view.ts index e3d4c63..a7bc195 100644 --- a/frontend/src/components/explore-view/explore-view.ts +++ b/frontend/src/components/explore-view/explore-view.ts @@ -37,9 +37,11 @@ import { contextMenuStyles, isContextMenuKey, } from '@utils/context-menu-controller.js'; -import type { ContextMenuHost } from '@utils/context-menu-controller.js'; +import type { ContextMenuHost, MenuTarget } from '@utils/context-menu-controller.js'; import '@awesome.me/webawesome/dist/components/popup/popup.js'; import type WaPopup from '@awesome.me/webawesome/dist/components/popup/popup.js'; +import type { MenuSurface } from '../menu-surface/menu-surface'; +import '../menu-surface/menu-surface'; import '@awesome.me/webawesome/dist/components/dropdown-item/dropdown-item.js'; import { dict, dictByName } from '@utils/binding'; import { ICON_QUEUE } from '@utils/icon-language'; @@ -206,13 +208,13 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte @state() private ctxMenuTarget: ExploreMenuTarget | null = null; @litQuery('#explore-context-menu') - private contextMenuPopup!: WaPopup; + private contextMenuPopup!: MenuSurface; // -- ContextMenuHost interface -- // No playlist submenu — same reason as the album/artist detail // pages: every action here resolves its one file lazily. - getContextMenuPopup(): WaPopup | undefined { + getContextMenuPopup(): MenuTarget | undefined { return this.contextMenuPopup; } @@ -1341,11 +1343,8 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte const owned = Boolean(target?.localId); return html` - ${this.ctxMenu.contextMenuOpen && target @@ -1374,7 +1373,7 @@ export class ExploreView extends ViewLifecycleMixin(LitElement) implements Conte ` : nothing} - + `; } diff --git a/frontend/src/components/genres-view/genres-view.ts b/frontend/src/components/genres-view/genres-view.ts index 8c0a6f7..4cea184 100644 --- a/frontend/src/components/genres-view/genres-view.ts +++ b/frontend/src/components/genres-view/genres-view.ts @@ -24,14 +24,15 @@ import { contextMenuStyles, isContextMenuKey, } from '@utils/context-menu-controller.js'; -import type { ContextMenuHost } from '@utils/context-menu-controller.js'; +import type { ContextMenuHost, MenuTarget } from '@utils/context-menu-controller.js'; import { FavoritesController } from '@store/controllers/favorites-controller'; import { ViewLifecycleMixin } from '@utils/view-lifecycle'; import { RovingGridController } from '@utils/roving-grid'; import '@awesome.me/webawesome/dist/components/icon/icon.js'; import '@awesome.me/webawesome/dist/components/popup/popup.js'; -import type WaPopup from '@awesome.me/webawesome/dist/components/popup/popup.js'; +import type { MenuSurface } from '../menu-surface/menu-surface'; +import '../menu-surface/menu-surface'; import '@awesome.me/webawesome/dist/components/dropdown-item/dropdown-item.js'; import '@components/playlist-picker/playlist-picker.js'; import { dictByName } from '@utils/binding'; @@ -137,19 +138,19 @@ export class GenresView private contextMenuGenreName: string | null = null; @query('#context-menu') - private contextMenuPopup!: WaPopup; + private contextMenuPopup!: MenuSurface; @query('#playlist-submenu') - private playlistSubmenuPopup!: WaPopup; + private playlistSubmenuPopup!: MenuSurface; // ----- ContextMenuHost interface ----- - getContextMenuPopup(): WaPopup | undefined { + getContextMenuPopup(): MenuTarget | undefined { return this.contextMenuPopup; } getPlaylistSubmenuPopup(): - | WaPopup + | MenuTarget | undefined { return this.playlistSubmenuPopup; } @@ -1176,11 +1177,8 @@ export class GenresView private renderContextMenu() { return html` - @@ -1284,13 +1282,12 @@ export class GenresView ` : nothing} - + - @@ -1318,7 +1315,7 @@ export class GenresView ` : nothing} - + `; } diff --git a/frontend/src/components/menu-surface/menu-surface.ts b/frontend/src/components/menu-surface/menu-surface.ts new file mode 100644 index 0000000..94652cd --- /dev/null +++ b/frontend/src/components/menu-surface/menu-surface.ts @@ -0,0 +1,318 @@ +/** + * Where a context menu is drawn: a popup on a desktop, a bottom sheet + * on a phone (#60). + * + * Every context menu in this app is a `.context-menu-panel` inside a + * `` anchored to the touch point, driven by + * `ContextMenuController`. On the reference device that is structurally + * broken, and the failure was measured on the hardware rather than + * inferred: + * + * - Chrome 113 has **no Popover API** (`popover` is Chrome 114), so + * `wa-popup` takes its own documented fallback and positions with + * `strategy: "fixed"` instead of the top layer. Measured on the + * device: `HTMLElement.prototype.hasOwnProperty('popover')` is false + * and the popup's computed `position` is `fixed`. + * - `index.css` puts `contain: layout style paint` on `.main-panel`, + * the ancestor of every view. Paint containment **clips** fixed + * descendants. Measured: `.main-panel` computes `contain: content` + * and spans 0-318 of a 439px viewport, while the open menu spans + * 191-401 — so 83px of it, three of its seven items, is cut off. + * + * A `` fixes it by construction rather than by styling, because + * `showModal()` is Chrome 37 and uses the real top layer. **That was + * measured too, and it needed to be**: every other dialog in this app + * is mounted in `index.html`, *outside* `.main-panel`, so "dialogs are + * fine" was not evidence about a dialog opened from inside a view. A + * probe dialog appended to `track-list`'s shadow root paints to y=439, + * over the mini player and the tab bar, with the contained ancestor + * still there. + * + * Four things about this component are load-bearing. + * + * **It is one element with two presentations, not two components.** + * The host keeps rendering exactly the panel it rendered before and + * slots it into whichever surface is up, so the twelve call sites + * changed one tag name each and nothing else — no second item model, no + * second keyboard model, and `ContextMenuController` still drives + * `.active` and `.anchor` as if it were talking to a `wa-popup`. + * + * **Which surface exists is `matchMedia`, not a media query.** The + * decision is whether a `` is in the tree at all, which is + * `job-band` and `player-controls`' rule: a `display: none` surface is + * still in the shadow root and still something a positional or by-role + * query finds. + * + * **The sheet has to un-do the UA stylesheet to be full-bleed.** + * A native `` carries `max-width: calc(100% - 6px - 2em)` and + * `margin: auto`, which on the device produced a 354px panel floating + * in the middle of a 424px screen. `max-width: none` and explicit + * margins are what make it a sheet rather than a small centred box. + * The *positioning* needs no such care: a top-layer dialog's containing + * block is the viewport even with a paint-contained ancestor, which is + * why `bottom: 0` reaches y=439 and not the main panel's 318. + * + * **Dismissal has to travel back.** `wa-dialog` closes itself on + * Escape, which would otherwise leave the controller's + * `contextMenuOpen` true and the menu unopenable until something else + * cleared it. `menu-dismiss` is that signal, and the controller listens + * for it on the document beside the click and contextmenu listeners it + * already has. + */ +import { LitElement, css, html } from 'lit'; +import { customElement, property, query, state } from 'lit/decorators.js'; +import '@awesome.me/webawesome/dist/components/popup/popup.js'; +import '@awesome.me/webawesome/dist/components/dialog/dialog.js'; +import type WaPopup from '@awesome.me/webawesome/dist/components/popup/popup.js'; + +import { PHONE_QUERY } from '@utils/breakpoints'; +import { nameDialogsIn } from '@utils/name-dialog'; + +/** The event a surface dispatches when it closed itself. */ +export const MENU_DISMISS_EVENT = 'menu-dismiss'; + +/** + * The event a surface dispatches once it has finished showing. + * + * Only the sheet sends it, and only because `wa-dialog` moves focus to + * itself on the frame after `showModal()` -- see `MenuKeyboard.refocus` + * for why waiting longer is not the fix. + */ +export const MENU_SHOWN_EVENT = 'menu-shown'; + +/** + * A `wa-popup` anchor: a real element or a virtual one. + * + * `undefined` rather than `null` for "not set yet", because that is + * what `wa-popup`'s own property accepts — this surface hands the value + * straight through and must not widen it. + */ +type MenuAnchor = WaPopup['anchor'] | undefined; + +/** A `wa-dialog`, as much of it as this file needs. */ +type DialogEl = HTMLElement & { open: boolean }; + +@customElement('menu-surface') +export class MenuSurface extends LitElement { + /** Whether the menu is showing. Set by `ContextMenuController`. */ + @property({ type: Boolean }) active = false; + + /** + * Where the popup hangs from. Ignored in sheet mode, which is + * anchored to the bottom of the screen rather than to the touch + * point — that is the whole point of a sheet. + */ + @property({ attribute: false }) anchor: MenuAnchor = undefined; + + /** + * `wa-popup`'s placement, defaulted because all twelve call sites + * passed the same one. Kept as a property so a future menu that + * wants another does not have to reach past this component. + */ + @property() placement = 'bottom-start'; + + /** + * What to call the sheet, for a surface whose content is not a + * `.context-menu-panel` with an `aria-label` of its own -- the + * playlist submenu, whose content is a `playlist-picker`. + */ + @property() label = ''; + + @state() private sheet = false; + + @query('wa-popup') private popup?: WaPopup; + + @query('wa-dialog') private dialog?: DialogEl; + + private phoneQuery?: MediaQueryList; + + static override styles = css` + :host { + display: contents; + } + + wa-popup { + z-index: 200; + } + + /* The sheet. A native dialog's UA stylesheet centres it and + caps its width, which on the device drew a 354px box in the + middle of a 424px screen — so all four of these are undoing + that rather than decorating. */ + wa-dialog::part(dialog) { + margin: auto auto 0 auto; + max-width: none; + max-height: 85vh; + width: 100%; + border-radius: 12px 12px 0 0; + background: var(--yj-bg-elevated, #343a40); + padding: 0; + } + + /* **A long menu scrolls; it does not hang off the bottom.** + Measured on the device at 80vh: seven 48px rows plus the grip + came to 364px against a 351px dialog, so the last row's + bottom was at y=452 on a 439px screen -- the one row a + destructive action is most likely to be. The cap has to stay + (a sheet covering the whole screen is a page, not a sheet), + so the body is what gives. */ + wa-dialog::part(body) { + padding: 0; + overflow-y: auto; + } + + /* A sheet is dragged at with a thumb, so it says where its top + edge is. Decorative: the panel below it carries the actions. */ + .grip { + width: 36px; + height: 4px; + margin: 8px auto 4px; + border-radius: 2px; + background: var(--yj-text-tertiary, #888); + } + `; + + override connectedCallback(): void { + super.connectedCallback(); + + // Looked up here rather than at module load, so a test can + // install its own matchMedia before the element is created. + this.phoneQuery = window.matchMedia?.(PHONE_QUERY); + this.sheet = this.phoneQuery?.matches ?? false; + this.phoneQuery?.addEventListener('change', this.onPhoneChange); + } + + override disconnectedCallback(): void { + super.disconnectedCallback(); + this.phoneQuery?.removeEventListener('change', this.onPhoneChange); + } + + private onPhoneChange = (e: MediaQueryListEvent): void => { + this.sheet = e.matches; + }; + + /** + * Re-run the popup's positioning. + * + * Forwarded rather than dropped because `page-header` calls it when + * it opens the overflow menu: the popup is rendered before the + * button it anchors to has settled. A sheet has nothing to + * reposition -- it is anchored to the bottom of the screen -- so + * there it is deliberately a no-op rather than an error. + */ + reposition(): void { + this.popup?.reposition(); + } + + /** + * The panel the host slotted in. It is light DOM here and stays in + * the host's shadow root, which is what keeps the host's own + * `contextMenuStyles` applying to it in both presentations. + */ + private get panel(): HTMLElement | null { + return this.querySelector('.context-menu-panel'); + } + + override updated(): void { + const panel = this.panel; + + // The sheet's rows are bigger, and that rule lives in the one + // stylesheet every call site already includes rather than in + // twelve places. The attribute is how it knows. + if (panel) panel.toggleAttribute('data-sheet', this.sheet); + + if (this.sheet) { + this.syncSheet(panel); + + return; + } + + if (this.popup) { + if (this.anchor) this.popup.anchor = this.anchor; + + this.popup.active = this.active; + } + } + + private syncSheet(panel: HTMLElement | null): void { + const dialog = this.dialog; + + if (!dialog) return; + + // The dialog is named after the menu it contains, so no call + // site has to say the same thing twice: the panel already + // carries `role="menu"` and an `aria-label` naming what it acts + // on. `without-header` renders no heading, which is + // `name-dialog`'s documented `aria-label` path. + const label = panel?.getAttribute('aria-label') || this.label; + + if (label) dialog.setAttribute('label', label); + + nameDialogsIn(this.shadowRoot); + + if (dialog.open !== this.active) dialog.open = this.active; + } + + /** + * `wa-dialog` closed itself — Escape, or its own close button. + * The controller owns `contextMenuOpen`, so it has to hear about + * it or the menu is left open in state and shut on screen. + */ + private onDialogShown = (): void => { + if (!this.active) return; + + this.dispatchEvent( + new CustomEvent(MENU_SHOWN_EVENT, { + bubbles: true, + composed: true, + }), + ); + }; + + private onDialogHide = (): void => { + if (!this.active) return; + + this.dispatchEvent( + new CustomEvent(MENU_DISMISS_EVENT, { + bubbles: true, + composed: true, + }), + ); + }; + + override render() { + if (this.sheet) { + // **The anchor stays out of the sheet.** One call site -- + // `page-header`'s overflow menu -- slots its own trigger + // button as the thing the popup hangs from, and a sheet + // hangs from the bottom of the screen instead. Rendering + // that slot outside the dialog is what keeps the button on + // the page rather than inside the surface it opens. + return html` + + +
+ + + `; + } + + return html` + + + + + `; + } +} + +declare global { + interface HTMLElementTagNameMap { + 'menu-surface': MenuSurface; + } +} diff --git a/frontend/src/components/page-header/page-header.ts b/frontend/src/components/page-header/page-header.ts index d67fed1..48bba04 100644 --- a/frontend/src/components/page-header/page-header.ts +++ b/frontend/src/components/page-header/page-header.ts @@ -1,9 +1,9 @@ import { LitElement, html, css, nothing } from 'lit'; import { customElement, property, query, state } from 'lit/decorators.js'; import '@awesome.me/webawesome/dist/components/icon/icon.js'; -import '@awesome.me/webawesome/dist/components/popup/popup.js'; import '@awesome.me/webawesome/dist/components/dropdown-item/dropdown-item.js'; -import type WaPopup from '@awesome.me/webawesome/dist/components/popup/popup.js'; +import type { MenuSurface } from '../menu-surface/menu-surface'; +import '../menu-surface/menu-surface'; import { designTokens } from '../../styles/tokens.css'; import { @@ -183,8 +183,8 @@ export class PageHeader extends LitElement { @query('#page-header-overflow') private menuPanel?: HTMLElement; - @query('wa-popup') - private popup?: WaPopup; + @query('menu-surface') + private popup?: MenuSurface; private menuKeyboard = new MenuKeyboard(() => this.closeMenu()); @@ -418,7 +418,7 @@ export class PageHeader extends LitElement { outline-offset: -1px; } - wa-popup { + menu-surface { z-index: 200; } @@ -670,10 +670,17 @@ export class PageHeader extends LitElement { return html`
${this.actions.map((a) => this.renderActionButton(a))} - +
`; } diff --git a/frontend/src/components/playlist-details/playlist-details.ts b/frontend/src/components/playlist-details/playlist-details.ts index c90e7ed..9db18c4 100644 --- a/frontend/src/components/playlist-details/playlist-details.ts +++ b/frontend/src/components/playlist-details/playlist-details.ts @@ -7,7 +7,8 @@ import { } from 'lit/decorators.js'; import '@awesome.me/webawesome/dist/components/icon/icon.js'; import '@awesome.me/webawesome/dist/components/popup/popup.js'; -import type WaPopup from '@awesome.me/webawesome/dist/components/popup/popup.js'; +import type { MenuSurface } from '../menu-surface/menu-surface'; +import '../menu-surface/menu-surface'; import '@awesome.me/webawesome/dist/components/dropdown-item/dropdown-item.js'; import '@lit-labs/virtualizer'; import type { LitVirtualizer } from '@lit-labs/virtualizer'; @@ -35,7 +36,7 @@ import { contextMenuStyles, isContextMenuKey, } from '@utils/context-menu-controller.js'; -import type { ContextMenuHost } from '@utils/context-menu-controller.js'; +import type { ContextMenuHost, MenuTarget } from '@utils/context-menu-controller.js'; import { focusRovingRow, nextRovingIndex } from '@utils/roving-rows'; import { FavoritesController } from '@store/controllers/favorites-controller'; import { notificationStore } from '@store/notification-store'; @@ -145,10 +146,10 @@ export class PlaylistDetails private dragImageEl: HTMLElement | null = null; @query('#context-menu') - private contextMenuPopup!: WaPopup; + private contextMenuPopup!: MenuSurface; @query('#playlist-submenu') - private playlistSubmenuPopup!: WaPopup; + private playlistSubmenuPopup!: MenuSurface; @query('track-details') private trackDetailsDialog!: TrackDetails; @@ -163,11 +164,11 @@ export class PlaylistDetails // ContextMenuHost interface // ================================================================= - getContextMenuPopup(): WaPopup | undefined { + getContextMenuPopup(): MenuTarget | undefined { return this.contextMenuPopup; } - getPlaylistSubmenuPopup(): WaPopup | undefined { + getPlaylistSubmenuPopup(): MenuTarget | undefined { return this.playlistSubmenuPopup; } @@ -1617,11 +1618,8 @@ export class PlaylistDetails private renderContextMenu() { return html` - @@ -1783,13 +1781,12 @@ export class PlaylistDetails ` : nothing} - + - @@ -1816,7 +1813,7 @@ export class PlaylistDetails ` : nothing} - + `; } } diff --git a/frontend/src/components/playlist-view/playlist-view.ts b/frontend/src/components/playlist-view/playlist-view.ts index 0192d12..c3b56ab 100644 --- a/frontend/src/components/playlist-view/playlist-view.ts +++ b/frontend/src/components/playlist-view/playlist-view.ts @@ -2,7 +2,9 @@ import { LitElement, html, css, nothing } from 'lit'; import { customElement, state, query } from 'lit/decorators.js'; import '@awesome.me/webawesome/dist/components/icon/icon.js'; import '@awesome.me/webawesome/dist/components/popup/popup.js'; -import type WaPopup from '@awesome.me/webawesome/dist/components/popup/popup.js'; + +import type { MenuSurface } from '../menu-surface/menu-surface'; +import '../menu-surface/menu-surface'; import '@awesome.me/webawesome/dist/components/dropdown-item/dropdown-item.js'; import { @@ -140,7 +142,7 @@ export class PlaylistView extends ViewLifecycleMixin(LitElement) { private pendingDropPaths: string[] = []; @query('#playlist-context-menu') - private playlistContextMenuPopup!: WaPopup; + private playlistContextMenuPopup!: MenuSurface; @query('duplicate-tracks-dialog') private duplicateDialog!: DuplicateTracksDialog; @@ -1059,7 +1061,7 @@ export class PlaylistView extends ViewLifecycleMixin(LitElement) { ); } - private closePlaylistContextMenu() { + private closePlaylistContextMenu = () => { if (!this.playlistContextMenuOpen) return; this.menuKeyboard.close(); @@ -1072,7 +1074,7 @@ export class PlaylistView extends ViewLifecycleMixin(LitElement) { if (popup) { popup.active = false; } - } + }; private async onPlaylistContextAction( action: string, @@ -1500,13 +1502,11 @@ export class PlaylistView extends ViewLifecycleMixin(LitElement) { ` : this.renderPlaylistList()} - ${this.playlistContextMenuOpen ? html` @@ -1564,7 +1564,7 @@ export class PlaylistView extends ViewLifecycleMixin(LitElement) { ` : nothing} - + diff --git a/frontend/src/components/queue-panel/queue-panel.ts b/frontend/src/components/queue-panel/queue-panel.ts index 1621817..4dcd99e 100644 --- a/frontend/src/components/queue-panel/queue-panel.ts +++ b/frontend/src/components/queue-panel/queue-panel.ts @@ -9,7 +9,8 @@ import { } from 'lit/decorators.js'; import '@awesome.me/webawesome/dist/components/icon/icon.js'; import '@awesome.me/webawesome/dist/components/popup/popup.js'; -import type WaPopup from '@awesome.me/webawesome/dist/components/popup/popup.js'; +import type { MenuSurface } from '../menu-surface/menu-surface'; +import '../menu-surface/menu-surface'; import '@awesome.me/webawesome/dist/components/dropdown-item/dropdown-item.js'; import { QueueController } from '@store/controllers/queue-controller'; import { creditStore } from '@store/credit-store'; @@ -34,7 +35,7 @@ import { contextMenuStyles, isContextMenuKey, } from '@utils/context-menu-controller.js'; -import type { ContextMenuHost } from '@utils/context-menu-controller.js'; +import type { ContextMenuHost, MenuTarget } from '@utils/context-menu-controller.js'; import { focusRovingRow, nextRovingIndex } from '@utils/roving-rows'; import { FavoritesController } from '@store/controllers/favorites-controller'; import { @@ -140,13 +141,13 @@ export class QueuePanel private delegationAttached = false; @query('#add-to-playlist-popup') - private addToPlaylistPopup!: WaPopup; + private addToPlaylistPopup!: MenuSurface; @query('#context-menu') - private contextMenuPopup!: WaPopup; + private contextMenuPopup!: MenuSurface; @query('#playlist-submenu') - private playlistSubmenuPopup!: WaPopup; + private playlistSubmenuPopup!: MenuSurface; /** Unsubscribes the credit-arrival repaint. */ private creditsUnsub?: () => void; @@ -305,11 +306,11 @@ export class QueuePanel // ContextMenuHost interface // ================================================================= - getContextMenuPopup(): WaPopup | undefined { + getContextMenuPopup(): MenuTarget | undefined { return this.contextMenuPopup; } - getPlaylistSubmenuPopup(): WaPopup | undefined { + getPlaylistSubmenuPopup(): MenuTarget | undefined { return this.playlistSubmenuPopup; } @@ -1092,7 +1093,7 @@ export class QueuePanel } } - private closePlaylistPicker() { + private closePlaylistPicker = () => { if (!this.playlistPickerOpen) return; this.playlistPickerOpen = false; @@ -1102,7 +1103,7 @@ export class QueuePanel if (popup) { popup.active = false; } - } + }; private onPlaylistActionComplete = () => { this.closePlaylistPicker(); @@ -2042,9 +2043,11 @@ export class QueuePanel - ${this.playlistPickerOpen @@ -2060,7 +2063,7 @@ export class QueuePanel > ` : nothing} - +
- ${this.ctxMenu.contextMenuOpen @@ -2195,13 +2195,12 @@ export class QueuePanel ` : nothing} - + - ${this.ctxMenu.playlistSubmenuOpen && @@ -2224,7 +2223,7 @@ export class QueuePanel ` : nothing} - + `; diff --git a/frontend/src/components/smart-playlist-details/smart-playlist-details.ts b/frontend/src/components/smart-playlist-details/smart-playlist-details.ts index 83adde0..4b9b112 100644 --- a/frontend/src/components/smart-playlist-details/smart-playlist-details.ts +++ b/frontend/src/components/smart-playlist-details/smart-playlist-details.ts @@ -26,7 +26,7 @@ import { contextMenuStyles, isContextMenuKey, } from '@utils/context-menu-controller.js'; -import type { ContextMenuHost } from '@utils/context-menu-controller.js'; +import type { ContextMenuHost, MenuTarget } from '@utils/context-menu-controller.js'; import { focusRovingRow, nextRovingIndex } from '@utils/roving-rows'; import { FavoritesController } from '@store/controllers/favorites-controller'; import { @@ -40,7 +40,8 @@ import { } from '@utils/drag-image'; import '@awesome.me/webawesome/dist/components/icon/icon.js'; import '@awesome.me/webawesome/dist/components/popup/popup.js'; -import type WaPopup from '@awesome.me/webawesome/dist/components/popup/popup.js'; +import type { MenuSurface } from '../menu-surface/menu-surface'; +import '../menu-surface/menu-surface'; import '@awesome.me/webawesome/dist/components/dropdown-item/dropdown-item.js'; import '@lit-labs/virtualizer'; import type { LitVirtualizer } from '@lit-labs/virtualizer'; @@ -176,10 +177,10 @@ export class SmartPlaylistDetails private dragImageEl: HTMLElement | null = null; @query('#context-menu') - private contextMenuPopup!: WaPopup; + private contextMenuPopup!: MenuSurface; @query('#playlist-submenu') - private playlistSubmenuPopup!: WaPopup; + private playlistSubmenuPopup!: MenuSurface; @query('track-details') private trackDetailsDialog!: TrackDetails; @@ -188,11 +189,11 @@ export class SmartPlaylistDetails // ContextMenuHost interface // ================================================================= - getContextMenuPopup(): WaPopup | undefined { + getContextMenuPopup(): MenuTarget | undefined { return this.contextMenuPopup; } - getPlaylistSubmenuPopup(): WaPopup | undefined { + getPlaylistSubmenuPopup(): MenuTarget | undefined { return this.playlistSubmenuPopup; } @@ -1465,11 +1466,8 @@ export class SmartPlaylistDetails private renderContextMenu() { return html` - ${this.ctxMenu.contextMenuOpen @@ -1583,13 +1581,12 @@ export class SmartPlaylistDetails ` : nothing} - + - @@ -1616,7 +1613,7 @@ export class SmartPlaylistDetails ` : nothing} - + `; } } diff --git a/frontend/src/components/track-list/track-list.ts b/frontend/src/components/track-list/track-list.ts index d278389..db23e33 100644 --- a/frontend/src/components/track-list/track-list.ts +++ b/frontend/src/components/track-list/track-list.ts @@ -18,7 +18,7 @@ import { isContextMenuKey, } from '@utils/context-menu-controller.js'; -import type { ContextMenuHost } from '@utils/context-menu-controller.js'; +import type { ContextMenuHost, MenuTarget } from '@utils/context-menu-controller.js'; import { PlayerController } from '@store/controllers/player-controller'; import { SearchController } from '@store/controllers/search-controller'; import '@components/page-header/page-header'; @@ -63,7 +63,8 @@ import type { } from '@lit-labs/virtualizer'; import { flow } from '@lit-labs/virtualizer/layouts/flow.js'; import '@awesome.me/webawesome/dist/components/popup/popup.js'; -import type WaPopup from '@awesome.me/webawesome/dist/components/popup/popup.js'; +import type { MenuSurface } from '../menu-surface/menu-surface'; +import '../menu-surface/menu-surface'; import '@awesome.me/webawesome/dist/components/dropdown-item/dropdown-item.js'; import '@awesome.me/webawesome/dist/components/icon/icon.js'; import { describeError } from '@utils/describe-error'; @@ -231,18 +232,18 @@ export class TrackList private tracks: library.Track[] = []; @query('#context-menu') - private contextMenuPopup!: WaPopup; + private contextMenuPopup!: MenuSurface; @query('#playlist-submenu') - private playlistSubmenuPopup!: WaPopup; + private playlistSubmenuPopup!: MenuSurface; // -- ContextMenuHost interface -- - getContextMenuPopup(): WaPopup | undefined { + getContextMenuPopup(): MenuTarget | undefined { return this.contextMenuPopup; } - getPlaylistSubmenuPopup(): WaPopup | undefined { + getPlaylistSubmenuPopup(): MenuTarget | undefined { return this.playlistSubmenuPopup; } @@ -2323,11 +2324,8 @@ export class TrackList `} - ${this.ctxMenu.contextMenuOpen @@ -2413,13 +2411,12 @@ export class TrackList ` : nothing} - + - ${this.ctxMenu.playlistSubmenuOpen && this.selection.hasSelection @@ -2437,7 +2434,7 @@ export class TrackList ` : nothing} - + `; diff --git a/frontend/src/utils/context-menu-controller.ts b/frontend/src/utils/context-menu-controller.ts index 824caa4..53d51c5 100644 --- a/frontend/src/utils/context-menu-controller.ts +++ b/frontend/src/utils/context-menu-controller.ts @@ -5,7 +5,20 @@ import type { } from 'lit'; import type WaPopup from '@awesome.me/webawesome/dist/components/popup/popup.js'; +/** + * What this controller needs of a surface: something it can switch on + * and point at. Both `wa-popup` and `menu-surface` satisfy it. + */ +export type MenuTarget = HTMLElement & { + active: boolean; + anchor?: WaPopup['anchor']; +}; + import { registerViewAware } from './view-lifecycle'; +import { + MENU_DISMISS_EVENT, + MENU_SHOWN_EVENT, +} from '../components/menu-surface/menu-surface'; /** * Host interface for components using the ContextMenuController. @@ -17,10 +30,18 @@ export interface ContextMenuHost extends ReactiveControllerHost { updateComplete: Promise; shadowRoot: ShadowRoot | null; - /** Return the main context-menu popup element. */ - getContextMenuPopup(): WaPopup | undefined; - /** Return the playlist submenu popup element. */ - getPlaylistSubmenuPopup(): WaPopup | undefined; + /** + * Return the main context-menu surface. + * + * `MenuSurface` since #60, which is a `wa-popup` above 600px and a + * bottom sheet below it. The type is the narrow shape this + * controller drives rather than either element, so a host that + * still renders a bare `wa-popup` — the playlist submenu does — + * satisfies it unchanged. + */ + getContextMenuPopup(): MenuTarget | undefined; + /** Return the playlist submenu surface. */ + getPlaylistSubmenuPopup(): MenuTarget | undefined; /** * Called when the context menu is closed by an * outside click/contextmenu/mousedown. Components @@ -33,6 +54,15 @@ export interface ContextMenuHost /** Submenu close delay in milliseconds. */ const SUBMENU_CLOSE_DELAY = 150; +/** + * How long to keep trying to put focus on a menu's first item. + * + * Long enough to outlast `wa-dialog`'s show animation, which ends by + * focusing the dialog; short enough that a menu which genuinely has no + * items stops rather than spinning for the life of the page. + */ +const FOCUS_RETRY_BUDGET_MS = 500; + /** A menu item, focusable and clickable. Web Awesome sets `role` itself. */ type MenuItem = HTMLElement & { active?: boolean; disabled?: boolean }; @@ -73,6 +103,30 @@ export class MenuKeyboard { void this.focusFirstItem(panel); } + /** + * Take focus back, for a surface that finished showing after we + * had already placed it. + * + * `wa-dialog` focuses `[autofocus]` or *itself* on the animation + * frame after `showModal()`, and it cannot see our first menu item + * to prefer it: the panel is slotted through `menu-surface`, so the + * dialog's own `querySelector` stops at the ``. Retrying on a + * longer budget does not fix this either -- the first attempt + * *succeeds*, and the steal happens afterwards. Measured on the + * device: the sheet opened with focus on the `` and every + * arrow key went nowhere. + * + * So the surface says when it has settled and this re-asserts. It + * is a no-op for a menu that is closed or that already has focus. + */ + refocus(): void { + const panel = this.panel; + + if (!panel || panel.contains(deepActiveElement())) return; + + void this.focusFirstItem(panel); + } + /** * Focus the first item, once the items are items. * @@ -91,11 +145,24 @@ export class MenuKeyboard { await Promise.all(candidates.map((el) => el.updateComplete ?? null)); - // …and once the popup has positioned itself. `wa-popup` places the + // …and once the surface has shown itself. `wa-popup` places the // panel on an animation frame, and `focus()` on a not-yet-shown // element is a silent no-op — which looks identical to a menu // that opened and refused to take focus. - for (let attempt = 0; attempt < 3; attempt++) { + // + // **The budget is time, not frames, because #60 gave this a + // second kind of surface.** Three frames was enough for a + // popup; a `wa-dialog` runs a show *animation* and moves focus + // to the dialog itself when it finishes, which lands after + // those frames and takes the focus back. Measured on the + // device: the sheet opened with `document.activeElement` on the + // ``, so every arrow key went nowhere. Retrying to a + // deadline is `roving-grid`'s rule for the same reason — the + // thing being waited for is another component's animation, not + // a fixed number of paints. + const deadline = Date.now() + FOCUS_RETRY_BUDGET_MS; + + while (Date.now() < deadline) { // Bail if the menu closed while we waited. if (this.panel !== panel) return; @@ -259,6 +326,20 @@ export class ContextMenuController /** Bound close handler for document events. */ private closeHandler = () => this.close(); + /** + * A surface finished showing; see `MenuKeyboard.refocus`. + * + * **Not while the submenu is up.** Both surfaces send this, and the + * submenu's sheet opens *over* the main one -- so re-asserting + * focus on the main panel's first item would snatch it straight + * back out of the playlist picker the user just opened. + */ + private shownHandler = () => { + if (this.contextMenuOpen && !this.playlistSubmenuOpen) { + this.keyboard.refocus(); + } + }; + /** Bound mousedown handler for outside-click detection. */ private mousedownCloseHandler = ( e: MouseEvent, @@ -325,12 +406,28 @@ export class ContextMenuController 'mousedown', this.mousedownCloseHandler, ); + document.addEventListener( + MENU_DISMISS_EVENT, + this.closeHandler, + ); + document.addEventListener( + MENU_SHOWN_EVENT, + this.shownHandler, + ); } private detach(): void { if (!this.listening) return; this.listening = false; + document.removeEventListener( + MENU_DISMISS_EVENT, + this.closeHandler, + ); + document.removeEventListener( + MENU_SHOWN_EVENT, + this.shownHandler, + ); document.removeEventListener( 'click', this.closeHandler, @@ -550,6 +647,48 @@ export const contextMenuStyles = css` z-index: 200; } + /* --------------------------------------------------------------- + The sheet (#60). + + menu-surface puts data-sheet on the panel when it is drawn + as a bottom sheet, and these rules are here rather than in that + component because the panel is the *host's* light DOM: it lives + in the host's shadow root, so only the host's stylesheet can + reach it. This file is the one every call site already includes, + which is what makes twelve menus grow thumb-sized rows from one + edit. + + Measured on the device before the change: rows were 29px, against + the 44px floor plan 018 promises and the 48px this issue asks + for. --------------------------------------------------------- */ + .context-menu-panel[data-sheet] { + border: none; + border-radius: 0; + box-shadow: none; + min-width: 0; + padding: 4px 0 8px; + background-color: transparent; + } + + .context-menu-panel[data-sheet] wa-dropdown-item { + font-size: var(--yj-text-md, 0.9375rem); + min-height: 48px; + align-items: center; + } + + .context-menu-panel[data-sheet] wa-dropdown-item::part(base) { + min-height: 48px; + align-items: center; + } + + /* A submenu arrow means "a flyout opens to the right", which is not + what happens on a phone and is not a thing a thumb can aim at. + The row still works — it is the tap handler that opens the + playlist picker — so what goes is the arrow, not the item. */ + .context-menu-panel[data-sheet] .submenu-arrow { + display: none; + } + .context-menu-panel { background-color: var( --yj-bg-elevated, diff --git a/frontend/test/components/menu-surface.test.ts b/frontend/test/components/menu-surface.test.ts new file mode 100644 index 0000000..f657220 --- /dev/null +++ b/frontend/test/components/menu-surface.test.ts @@ -0,0 +1,222 @@ +/** + * Where a context menu is drawn (#60). + * + * **This file asserts the mechanism, not the symptom, and that is the + * whole point of it.** The defect is that on the reference device's + * Chrome 113 a `wa-popup` has no Popover API to promote it to the top + * layer, so it falls back to `position: fixed` and is then *clipped* by + * `.main-panel`'s `contain: paint`. No tier here can reproduce that: + * this runner's Chromium and CI's WebKit both have the Popover API, so + * the popup is top-layered and looks perfectly correct. A test that + * asserted "the menu is not clipped" would pass on the broken build. + * + * What is checkable everywhere is *which surface exists*. A native + * `` uses the real top layer, which Chrome 37 has, so "it is a + * dialog at phone width" is the property that makes the device + * behaviour follow. The measurements that needed the hardware are on + * the PR and in `.planning/NOTES.md`. + */ +import { describe, expect, it, beforeEach, afterEach } from 'vitest'; + +import '@components/menu-surface/menu-surface'; +import { MENU_DISMISS_EVENT } from '@components/menu-surface/menu-surface'; +import { fixture } from '@test/support/render'; + +/** Every source file, as text. */ +const SOURCES = import.meta.glob('../../src/**/*.ts', { + eager: true, + query: '?raw', + import: 'default', +}); + +/** + * The two files allowed to render a raw `wa-popup`. + * + * `menu-surface` *is* the popup, in its desktop presentation. + * `job-indicator` is the documented exception and the contrast that + * proved the diagnosis: it lives in `.top-bar`, no ancestor of which + * has containment, so even the fixed fallback lands correctly on the + * device -- measured on #62, unclipped at every width. + * + * `now-playing`'s cover preview is the third, and it is a different + * reason: it is not a menu. It opens on `mouseenter` over the album + * art, so a touch device never sees it at all, and a bottom sheet for + * a hover preview would be absurd. It is also in the bottom bar rather + * than the main panel, so nothing clips it either. + * + * **Both were found by this sweep, not by the conversion**, which is + * the argument for having it: twelve call sites were converted by hand + * and two more existed. + */ +const MAY_USE_POPUP = [ + 'menu-surface/menu-surface.ts', + 'jobs/job-indicator.ts', + 'now-playing/now-playing.ts', +]; + +/** + * Answer `matchMedia` for the phone query, on `transport-context`'s + * pattern: what is under test is the component's reaction to the + * answer, not whether this runner's window can get below 600px. + */ +const realMatchMedia = window.matchMedia; + +function pretendPhone(phone: boolean): void { + window.matchMedia = ((query: string) => ({ + matches: phone && query.includes('599'), + media: query, + addEventListener: () => {}, + removeEventListener: () => {}, + })) as unknown as typeof window.matchMedia; +} + +/** A surface with the panel a real call site slots into it. */ +async function surfaceWithPanel(): Promise { + const el = await fixture('menu-surface'); + + el.innerHTML = + ''; + + const surface = el as unknown as HTMLElement & { + active: boolean; + updateComplete: Promise; + }; + + surface.active = true; + await surface.updateComplete; + + return el; +} + +/** + * The sweep, in the spirit of `icon-language.test.ts` and + * `TestNoDirectRuntimeEmits`: the rule is about *every* call site, and + * checking one checks nothing. + * + * Twelve menus were converted by hand. A thirteenth written as a bare + * `` would work perfectly in every tier here and be clipped + * on the device, which is exactly the failure this whole change is + * about and exactly the one no runtime assertion can see. + */ +describe('every menu goes through the one surface', () => { + it('reads the sources at all', () => { + // A sweep over an empty glob passes, so this is asserted first. + expect(Object.keys(SOURCES).length).toBeGreaterThan(100); + }); + + it('leaves no raw wa-popup outside the two files allowed one', () => { + const offenders = Object.entries(SOURCES) + .filter(([path]) => !MAY_USE_POPUP.some((ok) => path.endsWith(ok))) + .filter(([, src]) => src.includes(' path.replace(/^.*\/src\//, 'src/')); + + expect( + offenders, + 'these render a popup directly; use so the phone gets a sheet', + ).toEqual([]); + }); +}); + +describe('menu-surface', () => { + afterEach(() => { + window.matchMedia = realMatchMedia; + }); + + describe('above the phone breakpoint', () => { + beforeEach(() => pretendPhone(false)); + + it('draws a popup, which is what the desktop has always had', async () => { + const el = await surfaceWithPanel(); + + expect(el.shadowRoot?.querySelector('wa-popup')).not.toBeNull(); + expect(el.shadowRoot?.querySelector('wa-dialog')).toBeNull(); + }); + + it('does not mark the panel as a sheet', async () => { + const el = await surfaceWithPanel(); + + expect( + el.querySelector('.context-menu-panel')?.hasAttribute('data-sheet'), + ).toBe(false); + }); + }); + + describe('at phone width', () => { + beforeEach(() => pretendPhone(true)); + + /** + * The load-bearing one. `wa-dialog` renders a *native* ``, + * and it is the native element -- not the wrapper -- that gets the + * top layer and therefore escapes the paint containment that clips + * the popup on the device. + */ + it('draws a native dialog, which is what escapes the clip', async () => { + const el = await surfaceWithPanel(); + + const wrapper = el.shadowRoot?.querySelector('wa-dialog'); + + expect(wrapper, 'no wa-dialog at phone width').not.toBeNull(); + expect(el.shadowRoot?.querySelector('wa-popup')).toBeNull(); + + await (wrapper as HTMLElement & { updateComplete: Promise }) + .updateComplete; + + expect( + wrapper?.shadowRoot?.querySelector('dialog'), + 'the wrapper is not backed by a native dialog', + ).not.toBeNull(); + }); + + /** + * The rows are sized by `contextMenuStyles`, which lives in the + * *host's* shadow root — so the only thing this component can do is + * say which mode it is in. That attribute is the contract between + * the two, and it is what twelve call sites get their thumb-sized + * rows from. + */ + it('marks the panel as a sheet, which is what sizes the rows', async () => { + const el = await surfaceWithPanel(); + + expect( + el.querySelector('.context-menu-panel')?.hasAttribute('data-sheet'), + ).toBe(true); + }); + + /** + * `wa-dialog` closes itself on Escape. Without this the controller + * would still believe the menu was open, and the *next* long-press + * would do nothing — which is the failure mode that looks like the + * gesture breaking rather than the dialog. + */ + it('reports a dismissal it did not initiate', async () => { + const el = await surfaceWithPanel(); + + let dismissed = 0; + + document.addEventListener(MENU_DISMISS_EVENT, () => { + dismissed += 1; + }); + + el.shadowRoot + ?.querySelector('wa-dialog') + ?.dispatchEvent(new CustomEvent('wa-hide', { bubbles: false })); + + expect(dismissed, 'no menu-dismiss reached the document').toBe(1); + }); + + /** + * A dialog with no accessible name is what `utils/name-dialog.ts` + * exists for; here the name is already written on the panel, so no + * call site says it twice. + */ + it('names the sheet after the menu it contains', async () => { + const el = await surfaceWithPanel(); + + const wrapper = el.shadowRoot?.querySelector('wa-dialog'); + + expect(wrapper?.getAttribute('label')).toBe('Track actions'); + }); + }); +});