Merge pull request 'Android: a seek bar you can actually hit, and the phone rule that never applied' (#193) from 187-seek-bar-hit-area into main
This commit was merged in pull request #193.
This commit is contained in:
@@ -45,18 +45,6 @@ export class SeekBar extends LitElement {
|
||||
private showRemaining: boolean = true;
|
||||
|
||||
static override styles = [designTokens, waSliderLabel, css`
|
||||
/* 12px below the phone breakpoint. The bottom bar's seek bar is
|
||||
display:none there (016 B2 phase 1), so the only instance a
|
||||
viewport media query can reach at that width is the full-screen
|
||||
now-playing view's -- which is exactly the one a thumb uses.
|
||||
The track size lives on wa-slider inside this shadow root, so a
|
||||
custom property set by the host would not reach it. */
|
||||
@media (max-width: 599px) {
|
||||
wa-slider {
|
||||
--track-size: 12px;
|
||||
}
|
||||
}
|
||||
|
||||
wa-slider {
|
||||
--track-size: 6px;
|
||||
flex: 1;
|
||||
@@ -80,6 +68,57 @@ export class SeekBar extends LitElement {
|
||||
background: var(--yj-bg-base, black);
|
||||
}
|
||||
|
||||
/* The phone's seek bar, and this block is last on purpose.
|
||||
|
||||
A media query adds no specificity, so this lived above the plain
|
||||
"wa-slider" rule and lost to it at every width: the 12px track it
|
||||
asks for had never once applied, and the bar measured 261x6 on
|
||||
the device while the source said 12. That is index.css's rule
|
||||
("the phone section is last on purpose") met inside a component's
|
||||
own stylesheet, and nothing renders differently in any tier here
|
||||
to say so.
|
||||
|
||||
The bottom bar's seek bar is display:none below this width (016
|
||||
B2 phase 1), so the only instance a viewport media query can
|
||||
reach is the full-screen now-playing view's -- which is exactly
|
||||
the one a thumb uses. The desktop bar keeps its 6px, where a
|
||||
mouse is precise and the thickness is right.
|
||||
|
||||
The painted track and the thing you can hit are allowed to
|
||||
differ, and a slider is the clearest case where they should: 12px
|
||||
is a progress bar you can see, and 44px is the app's touch floor
|
||||
(#56). A 44px-*thick* bar would be wrong-looking and would cost
|
||||
the album art the vertical space #51 spent an issue recovering.
|
||||
|
||||
Two things about how the target is built.
|
||||
|
||||
The padding goes on ::part(slider) rather than on the host,
|
||||
because that inner div is what carries the gesture -- it has the
|
||||
listener and the touch-action: none, and it is exactly the host's
|
||||
size, so padding the host would grow a box that does not take the
|
||||
press.
|
||||
|
||||
The padding is asymmetric and the margins cancel it, so the row
|
||||
does not grow by the difference. Both halves are measured: the
|
||||
seek row is 19px (its clocks, not the track, decide that) and the
|
||||
play button's top edge is 8px below it, so the target takes the
|
||||
space *above*, where .art is a non-interactive div. Growing the
|
||||
row instead cost the art 25px of 143. Verified on the device at
|
||||
424x439: hit area 44px, painted track 12px, row still 19px, art
|
||||
still 143px, 8px of clearance left under the play button, a press
|
||||
26px above the track seeks, and a hit test on the play button's
|
||||
top edge still reaches the play button. */
|
||||
@media (max-width: 599px) {
|
||||
wa-slider {
|
||||
--track-size: 12px;
|
||||
}
|
||||
|
||||
wa-slider::part(slider) {
|
||||
padding-block: 28px 4px;
|
||||
margin-block: -28px -4px;
|
||||
}
|
||||
}
|
||||
|
||||
#seek-bar-container {
|
||||
display: flex;
|
||||
justify-content: space-between;
|
||||
|
||||
@@ -0,0 +1,211 @@
|
||||
/**
|
||||
* The seek bar's painted track and the thing you can hit are allowed to
|
||||
* differ, and a slider is the clearest case where they should.
|
||||
*
|
||||
* On `now-playing-view` — the screen that exists so a phone has
|
||||
* somewhere to seek from — the slider measured 261x6 on the reference
|
||||
* device (#187). Six pixels is the whole of the drag target on the
|
||||
* app's primary seeking affordance, against a 44px floor the app set
|
||||
* for itself in #56 and holds to in the queue panel.
|
||||
*
|
||||
* Two separate faults, and the first is why the second was not obvious.
|
||||
*
|
||||
* **The phone rule had never applied.** `seek-bar`'s stylesheet asked
|
||||
* for a 12px track below 599px and then set 6px in a plain `wa-slider`
|
||||
* rule *written after it*. A media query adds no specificity, so the
|
||||
* plain rule won at every width — which is `index.css`'s documented
|
||||
* rule ("the phone section is last on purpose") reproduced inside a
|
||||
* component's own stylesheet. The source said 12 and the device said 6.
|
||||
*
|
||||
* **And 12px would still be under the floor**, so the target is built
|
||||
* around the track rather than by thickening it: padding on the part
|
||||
* that carries the gesture, with margins cancelling it so the row does
|
||||
* not grow.
|
||||
*
|
||||
* This is asserted against the *parsed stylesheet*, on
|
||||
* `hover-affordance.test.ts`'s precedent and with the same limitation
|
||||
* stated rather than hidden: no tier here renders at a phone width with
|
||||
* a real `wa-slider` laid out, so what can be checked is the shape the
|
||||
* browser built from the css`` literal. The pixel measurements that
|
||||
* chose these numbers were taken on the device and are recorded on
|
||||
* #187 and in the stylesheet's own comment — a number measured on a
|
||||
* phone is not a number CI can assert.
|
||||
*
|
||||
* Which is the regression worth catching anyway. Both failures are
|
||||
* invisible on a desktop: hoisting the block back above the plain rule
|
||||
* renders identically at every width CI runs at, and it is exactly what
|
||||
* a tidy-up does.
|
||||
*/
|
||||
import { describe, expect, it } from 'vitest';
|
||||
|
||||
import '@components/audio-player/seekbar/seek-bar';
|
||||
import { fixture } from '@test/support/render';
|
||||
|
||||
/** The app's touch floor, from #56. */
|
||||
const TOUCH_FLOOR = 44;
|
||||
|
||||
/** The width below which the phone's rules apply. */
|
||||
const PHONE_QUERY = /max-width:\s*599px/;
|
||||
|
||||
type Rule = { text: string; condition: string | null };
|
||||
|
||||
/**
|
||||
* Every rule in the element's own adopted stylesheets, flattened **in
|
||||
* order**, which is the whole point here: the fault being guarded is a
|
||||
* rule sitting in the wrong place, not a rule being absent.
|
||||
*/
|
||||
function rulesOf(host: Element): Rule[] {
|
||||
const sheets = host.shadowRoot?.adoptedStyleSheets ?? [];
|
||||
const out: Rule[] = [];
|
||||
|
||||
for (const sheet of sheets) {
|
||||
for (const rule of Array.from(sheet.cssRules)) {
|
||||
if (rule instanceof CSSMediaRule) {
|
||||
for (const inner of Array.from(rule.cssRules)) {
|
||||
out.push({ text: inner.cssText, condition: rule.conditionText });
|
||||
}
|
||||
|
||||
continue;
|
||||
}
|
||||
|
||||
out.push({ text: rule.cssText, condition: null });
|
||||
}
|
||||
}
|
||||
|
||||
return out;
|
||||
}
|
||||
|
||||
/**
|
||||
* The two px numbers of a `*-block` declaration, as [start, end].
|
||||
*
|
||||
* A symmetric pair is **serialised back as one value** — `padding-block:
|
||||
* 16px 16px` reads as `padding-block: 16px` — so a naive pair-reader
|
||||
* fails on the shorthand rather than on the thing it is checking, and
|
||||
* says the wrong thing about why. That is not hypothetical: it is what
|
||||
* the symmetric-padding reversion did while this test was being
|
||||
* proved.
|
||||
*/
|
||||
function blockPair(text: string, property: string): [number, number] | null {
|
||||
const declaration = new RegExp(`${property}:\\s*([^;]+)`).exec(text)?.[1];
|
||||
|
||||
if (declaration === undefined) {
|
||||
return null;
|
||||
}
|
||||
|
||||
const values = [...declaration.matchAll(/(-?[\d.]+)px/g)].map((m) =>
|
||||
Number(m[1]),
|
||||
);
|
||||
|
||||
const [start, end] = values;
|
||||
|
||||
if (start === undefined) {
|
||||
return null;
|
||||
}
|
||||
|
||||
return [start, end ?? start];
|
||||
}
|
||||
|
||||
describe("the seek bar's phone rules", () => {
|
||||
it('are last, so they are not silently overridden', async () => {
|
||||
const el = await fixture('seek-bar', {});
|
||||
const rules = rulesOf(el);
|
||||
|
||||
// A sweep that read nothing passes vacuously — the same first
|
||||
// assertion icon-language.test.ts makes, for the same reason.
|
||||
expect(rules.length).toBeGreaterThan(0);
|
||||
|
||||
const declaresTrackSize = (r: Rule) => /--track-size:/.test(r.text);
|
||||
|
||||
const lastUnconditional = rules.findLastIndex(
|
||||
(r) => r.condition === null && declaresTrackSize(r),
|
||||
);
|
||||
const phoneOverride = rules.findLastIndex(
|
||||
(r) => r.condition !== null && PHONE_QUERY.test(r.condition)
|
||||
&& declaresTrackSize(r),
|
||||
);
|
||||
|
||||
expect(lastUnconditional).toBeGreaterThanOrEqual(0);
|
||||
expect(phoneOverride).toBeGreaterThanOrEqual(0);
|
||||
|
||||
// A media query adds no specificity. Written first, it loses.
|
||||
expect(phoneOverride).toBeGreaterThan(lastUnconditional);
|
||||
});
|
||||
|
||||
it('give the slider a pointer target of at least the touch floor', async () => {
|
||||
const el = await fixture('seek-bar', {});
|
||||
const rules = rulesOf(el);
|
||||
|
||||
const track = rules.find(
|
||||
(r) => r.condition !== null && PHONE_QUERY.test(r.condition)
|
||||
&& /--track-size:/.test(r.text),
|
||||
);
|
||||
const target = rules.find(
|
||||
(r) => r.condition !== null && PHONE_QUERY.test(r.condition)
|
||||
&& r.text.includes('::part(slider)'),
|
||||
);
|
||||
|
||||
expect(track).toBeDefined();
|
||||
expect(target).toBeDefined();
|
||||
|
||||
const trackSize = Number(
|
||||
/--track-size:\s*(-?[\d.]+)px/.exec(track!.text)?.[1],
|
||||
);
|
||||
const padding = blockPair(target!.text, 'padding-block');
|
||||
|
||||
expect(padding).not.toBeNull();
|
||||
|
||||
// The padding is on ::part(slider) rather than on the host because
|
||||
// that inner div is what carries the gesture: it has the listener
|
||||
// and the touch-action, and it is exactly the host's size, so
|
||||
// padding the host grows a box that does not take the press.
|
||||
const hitArea = trackSize + padding![0] + padding![1];
|
||||
|
||||
expect(hitArea).toBeGreaterThanOrEqual(TOUCH_FLOOR);
|
||||
});
|
||||
|
||||
it('do not grow the row they sit in', async () => {
|
||||
const el = await fixture('seek-bar', {});
|
||||
|
||||
const target = rulesOf(el).find(
|
||||
(r) => r.condition !== null && PHONE_QUERY.test(r.condition)
|
||||
&& r.text.includes('::part(slider)'),
|
||||
);
|
||||
|
||||
expect(target).toBeDefined();
|
||||
|
||||
const padding = blockPair(target!.text, 'padding-block');
|
||||
const margin = blockPair(target!.text, 'margin-block');
|
||||
|
||||
expect(padding).not.toBeNull();
|
||||
expect(margin).not.toBeNull();
|
||||
|
||||
// now-playing-view's vertical budget is fixed and #51 measured
|
||||
// every pixel of it: letting the row grow by the difference cost
|
||||
// the album art 25px of 143 when it was tried on the device.
|
||||
expect(margin![0]).toBe(-padding![0]);
|
||||
expect(margin![1]).toBe(-padding![1]);
|
||||
});
|
||||
|
||||
it('take the space above, because what is below is the transport', async () => {
|
||||
const el = await fixture('seek-bar', {});
|
||||
|
||||
const target = rulesOf(el).find(
|
||||
(r) => r.condition !== null && PHONE_QUERY.test(r.condition)
|
||||
&& r.text.includes('::part(slider)'),
|
||||
);
|
||||
|
||||
expect(target).toBeDefined();
|
||||
|
||||
const pair = blockPair(target!.text, 'padding-block');
|
||||
|
||||
expect(pair).not.toBeNull();
|
||||
|
||||
const [above, below] = pair!;
|
||||
|
||||
// Measured at 424x439: the seek row is 19px and the play button's
|
||||
// top edge is 8px below it, while `.art` above is a non-interactive
|
||||
// div. A symmetric target would reach into the play button — the
|
||||
// most important control on the screen — so the growth is upward.
|
||||
expect(above).toBeGreaterThan(below);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user