timothy/ersatztv
1
0
Fork 0
Code Issues 54 Pull Requests 6 Actions Packages Projects Releases 9 Wiki Activity

FieldHelp's panel placement is verified by hand, not by a test #839

New Issue
Open
opened 2026-08-26 19:24:07 +02:00 by timothy · 1 comment
timothy commented 2026-08-26 19:24:07 +02:00
Owner
Copy Link
Copy Source

Split out of #734.

FieldHelp's panel (web/src/components/fieldHelp.tsx, docs/spa-conventions.md §15) must not be clipped, and no committed test can see whether it is — the behaviour is CSS and layout.

.ctv-card sets overflow: hidden, which clips a positioned descendant whatever its z-index. #734 first shipped an in-flow panel and one field's explainer showed 12px of a 92px paragraph — and not as an edge case: the Audio card's last row always carries a detail, so one field's help was destroyed in every state of that card. The fix portals the panel to document.body with position: fixed and flips it above when it would run off the bottom.

That is currently pinned only by a sentence in §15 asking the next person to re-measure by hand. That is the shape this repo has repeatedly found rots: a documented manual step is not a guard, and this failure is silent — the panel still appears, it is just unreadable.

jsdom cannot help: it has no layout, so offsetHeight is 0 and elementFromPoint is meaningless. The 14 unit tests in fieldHelp.test.tsx cover the state machine and the DOM contract and are mutation-proved, but they are blind to placement.

NOT in scope: the hover chain

An earlier draft of this issue asked for a mouse-walk test from trigger into panel. Do not build that, and do not re-add the ::before hover bridge to make it pass. The bridge was added and withdrawn inside #734: it held for a strictly vertical descent and failed for a diagonal one — the natural reach toward a panel below-right — leaving a safe sideways exit of 1.25px on an 18px icon. §15 and spa.field-progressive-disclosure now state the contract deliberately: hover is for reading the paragraph in place; the panel's interactive content is reached by pinning. Against main a hover-walk assertion fails by design (measured at the withdrawal commit: diagonal closes at step 4 of 12, vertical at step 5). Pinning already survives arbitrary pointer motion and is covered by a unit test.

Why it was not done in #734

web/e2e/ holds exactly one spec (boot-gate.spec.ts). It is test.describe.serial, and its first test consumes the one-shot setup-claim for the config dir scripts/e2e-ui.sh boots. A second spec file needs a logged-in session without re-claiming — storage-state reuse, or an ordering contract between spec files that does not exist yet. Establishing that convention is a larger change to a shared harness than the drift it prevents, and #734 was already seven commits and four adversarial review rounds deep.

Sketch

  • Decide how a second spec acquires a session (storageState written by the boot-gate spec, or a fixture that claims/logs in idempotently) and write that down in docs/e2e-local.md → "UI-E2E harness" — that is the reusable part.
  • Then a field-help.spec.ts that, on the FFmpeg profile editor with the conditional rows switched on (Normalize loudness = loudnorm; the Audio card's last row is the case that actually broke), hovers every rendered trigger and asserts the panel is fully visible — hit-test down the panel with elementFromPoint, as #734 did — and that it stays inside the viewport.
  • Derive the trigger population from the DOM, not from a hand-written list, per testing.guard-derives-population-from-source. A hardcoded list would have missed Target loudness, which is the row that was broken.

Done-when

  • A second web/e2e/ spec can obtain a session without re-claiming setup, and the mechanism is documented
  • The panel-visibility assertion derives its trigger population from the DOM, with the conditional rows switched on
  • It fails against the pre-portal behaviour (mutation proof, executed — testing.mutation-claims-are-executed)
  • §15's "re-measure by hand" sentence is replaced by a pointer to the spec
  • No hover-walk assertion was added and the ::before bridge was not reintroduced
  • Adversarial review passed
Split out of #734. `FieldHelp`'s panel (`web/src/components/fieldHelp.tsx`, `docs/spa-conventions.md` §15) must not be clipped, and no committed test can see whether it is — the behaviour is CSS and layout. `.ctv-card` sets `overflow: hidden`, which clips a positioned descendant whatever its `z-index`. #734 first shipped an in-flow panel and one field's explainer showed **12px of a 92px paragraph** — and not as an edge case: the Audio card's last row always carries a `detail`, so one field's help was destroyed in every state of that card. The fix portals the panel to `document.body` with `position: fixed` and flips it above when it would run off the bottom. That is currently pinned only by **a sentence in §15 asking the next person to re-measure by hand**. That is the shape this repo has repeatedly found rots: a documented manual step is not a guard, and this failure is silent — the panel still appears, it is just unreadable. jsdom cannot help: it has no layout, so `offsetHeight` is 0 and `elementFromPoint` is meaningless. The 14 unit tests in `fieldHelp.test.tsx` cover the state machine and the DOM contract and are mutation-proved, but they are blind to placement. ## NOT in scope: the hover chain An earlier draft of this issue asked for a mouse-walk test from trigger into panel. **Do not build that, and do not re-add the `::before` hover bridge to make it pass.** The bridge was added and withdrawn inside #734: it held for a strictly vertical descent and failed for a diagonal one — the natural reach toward a panel below-right — leaving a safe sideways exit of 1.25px on an 18px icon. §15 and `spa.field-progressive-disclosure` now state the contract deliberately: **hover is for reading the paragraph in place; the panel's interactive content is reached by pinning.** Against `main` a hover-walk assertion fails *by design* (measured at the withdrawal commit: diagonal closes at step 4 of 12, vertical at step 5). Pinning already survives arbitrary pointer motion and is covered by a unit test. ## Why it was not done in #734 `web/e2e/` holds exactly one spec (`boot-gate.spec.ts`). It is `test.describe.serial`, and its first test consumes the **one-shot** setup-claim for the config dir `scripts/e2e-ui.sh` boots. A second spec file needs a logged-in session without re-claiming — storage-state reuse, or an ordering contract between spec files that does not exist yet. Establishing that convention is a larger change to a shared harness than the drift it prevents, and #734 was already seven commits and four adversarial review rounds deep. ## Sketch - Decide how a second spec acquires a session (`storageState` written by the boot-gate spec, or a fixture that claims/logs in idempotently) and write that down in `docs/e2e-local.md` → "UI-E2E harness" — that is the reusable part. - Then a `field-help.spec.ts` that, on the FFmpeg profile editor **with the conditional rows switched on** (Normalize loudness = loudnorm; the Audio card's last row is the case that actually broke), hovers every rendered trigger and asserts the panel is fully visible — hit-test down the panel with `elementFromPoint`, as #734 did — and that it stays inside the viewport. - Derive the trigger population from the DOM, not from a hand-written list, per `testing.guard-derives-population-from-source`. A hardcoded list would have missed Target loudness, which is the row that was broken. ## Done-when - [ ] A second `web/e2e/` spec can obtain a session without re-claiming setup, and the mechanism is documented - [ ] The panel-visibility assertion derives its trigger population from the DOM, with the conditional rows switched on - [ ] It fails against the pre-portal behaviour (mutation proof, executed — `testing.mutation-claims-are-executed`) - [ ] §15's "re-measure by hand" sentence is replaced by a pointer to the spec - [ ] No hover-walk assertion was added and the `::before` bridge was not reintroduced - [ ] Adversarial review passed
timothy added the frontendpriority: low labels 2026-08-26 19:24:34 +02:00
timothy commented 2026-08-26 19:29:59 +02:00
Author
Owner
Copy Link
Copy Source

Body corrected. As originally filed this issue predated 88a7b98ed, which withdrew the ::before hover bridge inside #734 — so the body still described the bridge as shipped and its Done-when still required a trigger→panel mouse-walk assertion. Against main that assertion fails by design, and the obvious way to make it pass is to re-add the mechanism §15 explicitly says not to re-add.

Scope is now placement only (unclipped + auto-flip), with an explicit out-of-scope section and a Done-when box asserting the bridge was not reintroduced. Raised by the fourth cold review of #734 (finding S1): a follow-up issue is a durable artifact, and this one was carrying a false claim that #734 itself created.

Body corrected. As originally filed this issue predated `88a7b98ed`, which **withdrew** the `::before` hover bridge inside #734 — so the body still described the bridge as shipped and its Done-when still required a trigger→panel mouse-walk assertion. Against `main` that assertion fails *by design*, and the obvious way to make it pass is to re-add the mechanism §15 explicitly says not to re-add. Scope is now placement only (unclipped + auto-flip), with an explicit out-of-scope section and a Done-when box asserting the bridge was **not** reintroduced. Raised by the fourth cold review of #734 (finding S1): a follow-up issue is a durable artifact, and this one was carrying a false claim that #734 itself created.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
ad-hoc
One-off / ad-hoc work not tracked by a dedicated issue
 api
REST API / HTTP endpoints
 bug
Something isn't working
 ci-cd
Build, test, deploy pipeline
 content
Channel content / schedules / playlists
 dependencies
Dependency updates (Renovate)
 enhancement
New feature or improvement
 frontend
ChicoryTV React SPA frontend
 in-progress
Claimed by an active session — do not pick up
 jellyfin
Jellyfin tuner / IPTV integration
 parked
Excluded from automatic queue pickup; work only when explicitly selected
 priority: high
priority: low
priority: medium
review
Adversarial review finding
 security
Security / vulnerability fix
No labels frontend priority: low
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate timothy (Timothy)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: timothy/ersatztv#839