diff --git a/CLAUDE.md b/CLAUDE.md index 87510e7..b884b6d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1462,6 +1462,32 @@ vary) wins, ours being told from theirs by **identity** rather than that ends the gesture is swallowed, keyed on the gesture rather than on a time window so the first tap on the menu it opened is not eaten too. +**A control revealed by `:hover` is gated on the device having hover, +and which way round depends on whether it is the only route to its +action.** The gate itself is not optional: a touch long-press +synthesises a hover state in the WebView, so every one of these flashed +into view during the 500 ms hold above — a control appearing because +the user was reaching for a different one. Where the action is reachable +another way the control is **absent** on a touch device (the home card's +play button, #68; the queue row's remove, which the row's bottom-sheet +menu carries since #60), and that is `display: none` outside +`(hover: hover) and (pointer: fine)` rather than `opacity: 0` or +`visibility: hidden`, both of which leave a button holding its hit area +and its place in the accessibility tree. Where the control is the +**only** route it is instead always visible under +`@media not all and (hover: hover)` — `track-details`'s cover-art +overlay and remove, `shortcut-capture`'s reset (#137) — because hiding +it takes the action away entirely. + +One thing to know before checking either: **no tier here can render as a +touch device.** CDP's `Emulation.setEmulatedMedia` does not reach the +component tier's iframe, and the e2e projects are Desktop Chrome and +Desktop Safari, neither of which has touch. So +`hover-affordance.test.ts` asserts the *parsed stylesheet* — which rule +sits inside which media query — and says so; the regression it exists +for is someone hoisting a rule out of its query as a tidy-up, which +nothing on a desktop renders differently. + Three lists had no focused row to open a menu *from* — the queue panel and both playlist detail views — and gained a roving tab stop through `utils/roving-rows.ts`. **`track-list` deliberately does not use it**: diff --git a/frontend/src/components/config-page/shortcut-capture.ts b/frontend/src/components/config-page/shortcut-capture.ts index f840bab..96774fe 100644 --- a/frontend/src/components/config-page/shortcut-capture.ts +++ b/frontend/src/components/config-page/shortcut-capture.ts @@ -79,6 +79,17 @@ export class ShortcutCapture extends LitElement { .reset-btn:hover { color: var(--yj-accent-text, #ffd43b); } + /* + * Reset is the only way to put a rebound shortcut back, so where + * the device has no hover it is always visible rather than an + * invisible button holding its hit area. The inverse of #68's + * rule, which applies where the hover control is redundant. + */ + @media not all and (hover: hover) { + .reset-btn { + opacity: 1; + } + } `; private handleClick = () => { diff --git a/frontend/src/components/queue-panel/queue-panel.ts b/frontend/src/components/queue-panel/queue-panel.ts index 4dcd99e..f33a603 100644 --- a/frontend/src/components/queue-panel/queue-panel.ts +++ b/frontend/src/components/queue-panel/queue-panel.ts @@ -639,23 +639,42 @@ export class QueuePanel text-overflow: ellipsis; } + /* + * The per-row remove is a hover affordance, and on a device + * without hover it is redundant rather than missing: the row's + * context menu is a bottom sheet since #60 and carries "Remove + * from Queue", so the action is one long-press away. An + * always-visible X would instead spend part of a 424px row on + * something already reachable. #68's treatment, for #68's reason. + * + * display:none outside the query rather than visibility:hidden: + * a hidden button still occupies its hit area and is still in + * the accessibility tree, so a phone would keep a target for a + * control it can never see. + */ .remove-button { - background: none; - border: none; - color: var(--yj-text-tertiary, #888); - cursor: pointer; - padding: 4px; - display: flex; - align-items: center; - visibility: hidden; + display: none; } - .track-item:hover .remove-button { - visibility: visible; - } + @media (hover: hover) and (pointer: fine) { + .remove-button { + background: none; + border: none; + color: var(--yj-text-tertiary, #888); + cursor: pointer; + padding: 4px; + display: flex; + align-items: center; + visibility: hidden; + } - .remove-button:hover { - color: var(--yj-error-text, #ff8787); + .track-item:hover .remove-button { + visibility: visible; + } + + .remove-button:hover { + color: var(--yj-error-text, #ff8787); + } } .list-area.drag-over { diff --git a/frontend/src/components/track-details/track-details.ts b/frontend/src/components/track-details/track-details.ts index 23cf502..992a326 100644 --- a/frontend/src/components/track-details/track-details.ts +++ b/frontend/src/components/track-details/track-details.ts @@ -529,6 +529,21 @@ export class TrackDetails extends LitElement { background: var(--yj-error, #e03131); } + /* + * Both are the *only* route to changing or removing a track's + * cover art, so where the device has no hover they are always + * visible rather than hidden — the inverse of #68's rule, which + * applies where the hover control is redundant. Revealed by + * opacity, so what is on screen is what the desktop reveal shows + * and nothing about the layout moves. + */ + @media not all and (hover: hover) { + .cover-art-overlay, + .cover-art-remove { + opacity: 1; + } + } + /* Error message */ .error-message { flex: 1; diff --git a/frontend/test/components/hover-affordance.test.ts b/frontend/test/components/hover-affordance.test.ts index 78d671c..75cde75 100644 --- a/frontend/test/components/hover-affordance.test.ts +++ b/frontend/test/components/hover-affordance.test.ts @@ -1,5 +1,6 @@ /** - * A hover affordance is gated on the device having hover. + * A hover affordance is gated on the device having hover — in whichever + * direction keeps the action reachable. * * The home page's cover cards reveal a play button on :hover. A touch * long-press synthesises a hover state in the WebView, so on a phone @@ -7,6 +8,15 @@ * utils/long-press.ts is measuring for a context menu — a control * appearing because the user was reaching for a different one. * + * #137 is the same sweep with the opposite answer for two of its three + * cases. Where the revealed control is the *only* route to its action, + * hiding it removes the action, so it is always visible where there is + * no hover: `track-details`'s cover-art overlay and remove, and + * `shortcut-capture`'s reset. The queue's per-row remove is the third, + * and is the redundant kind — since #60 the row's context menu is a + * bottom sheet carrying "Remove from Queue" — so it takes #68's + * treatment here. + * * This is asserted against the *parsed stylesheet* rather than by * emulating a touch device, and that is a limitation worth stating * rather than hiding. CDP's Emulation.setEmulatedMedia does not reach @@ -24,6 +34,9 @@ import { describe, expect, it } from 'vitest'; import '@components/home-view/home-view'; +import '@components/queue-panel/queue-panel'; +import '@components/track-details/track-details'; +import '@components/config-page/shortcut-capture'; import { fixture } from '@test/support/render'; /** Every rule in the element's own adopted stylesheets, flattened. */ @@ -83,3 +96,91 @@ describe('the home card play button', () => { expect(unconditional.some((r) => /display:\s*none/.test(r.text))).toBe(true); }); }); + +describe("the queue row's remove button", () => { + it('is absent where the device has no hover, the menu carrying the action', async () => { + const el = await fixture('queue-panel', {}); + const rules = rulesOf(el); + + expect(rules.length).toBeGreaterThan(0); + + // visibility:hidden alone would leave an invisible button holding + // its hit area on a phone, which is the trap #68's commit names. + const unconditional = rules.filter( + (r) => r.condition === null && r.text.startsWith('.remove-button'), + ); + + expect(unconditional.length).toBeGreaterThan(0); + expect(unconditional.some((r) => /display:\s*none/.test(r.text))).toBe(true); + + const reveals = rules.filter( + (r) => + r.text.includes('.remove-button') && /visibility:\s*visible/.test(r.text), + ); + + expect(reveals.length).toBeGreaterThan(0); + + for (const rule of reveals) { + expect(rule.condition).toMatch(/hover:\s*hover/); + expect(rule.condition).toMatch(/pointer:\s*fine/); + } + }); +}); + +/** + * The two affordances that are the only route to their action. + * + * Asserted as "there is a rule showing it, and its condition is a + * *negated* hover query" — the same stylesheet reading as above, for + * the same reason: this tier's iframe cannot be emulated as a touch + * device, and the regression worth catching is someone folding the rule + * away as redundant on the desktop it does nothing on. + */ +describe('an affordance with no other route', () => { + const cases: Array<[string, string, string[]]> = [ + ['track-details', 'track-details', ['.cover-art-overlay', '.cover-art-remove']], + ['shortcut-capture', 'shortcut-capture', ['.reset-btn']], + ]; + + for (const [name, tag, selectors] of cases) { + it(`${name} shows it where the device has no hover`, async () => { + const el = await fixture(tag, {}); + const rules = rulesOf(el); + + expect(rules.length).toBeGreaterThan(0); + + for (const selector of selectors) { + const shown = rules.filter( + (r) => + r.condition !== null && + r.text.includes(selector) && + /opacity:\s*1/.test(r.text), + ); + + const touch = shown.filter((r) => /not[\s\S]*hover:\s*hover/.test(r.condition!)); + + expect(touch.length).toBeGreaterThan(0); + } + }); + } + + // The one half this tier can measure rather than read: the query is + // negated, so on the hover-capable browser running these tests the + // control must still be revealed by hover and by nothing else. A rule + // written without the `not` would show it here, permanently, on every + // desktop. + it('leaves the desktop reveal alone, where the device does have hover', async () => { + expect(matchMedia('(hover: hover)').matches).toBe(true); + + const el = await fixture('shortcut-capture', { + action: 'player.next', + label: 'Next Track', + currentKey: 'X', + defaultKey: 'N', + }); + const btn = el.shadowRoot?.querySelector('.reset-btn'); + + expect(btn).not.toBeNull(); + expect(getComputedStyle(btn!).opacity).toBe('0'); + }); +});