Skip to content

fix(text-field): floated label position + visual regression suite + CI split#530

Merged
kelsos merged 3 commits intorotki:mainfrom
kelsos:fix/text-field-floated-label-and-visual-suite
Apr 30, 2026
Merged

fix(text-field): floated label position + visual regression suite + CI split#530
kelsos merged 3 commits intorotki:mainfrom
kelsos:fix/text-field-floated-label-and-visual-suite

Conversation

@kelsos
Copy link
Copy Markdown
Member

@kelsos kelsos commented Apr 30, 2026

Summary

Three logically distinct changes, one PR:

  1. fix(text-field): correct floated label position with prepend / dense (e21f3bb) — fixes long-standing CSS positioning bugs surfaced visually during a manual review of RuiTextField.
  2. test(visual): add Playwright visual regression suite for RuiTextField (5090f59) — pixel-level guard against the class of bug we just fixed (CSS-only regressions that DOM tests can't catch).
  3. ci: split into parallel jobs for faster feedback (bd0d187) — wall-time on PRs drops by exposing the test halves in parallel.

What's broken (and now fixed)

In the floated state, RuiTextField's label was misaligned across the cross-product of {default, filled, outlined} × {normal, dense} × {with prepend}. Concretely:

  • Outlined + dense + prepend: label sat below the fieldset border, over the prepend icon.
  • Default + dense: label overlapped the input text in the compact row.
  • All variants + prepend: floated label landed under the icon's trailing padding instead of aligned with the input column (composable was measuring contentRect.width + left and silently dropping padding-right).
  • Outlined + dense + active: leading-[2.5] from the dense compound was overriding the active slot's leading-tight, inflating the label line-box from 15px → 30px.

Root causes — three in text-field-styles.ts, one in use-prepend-append-width.ts — see commit e21f3bb for details.

Visual regression infrastructure

New suite under apps/example/e2e/visual/ driven by Playwright toHaveScreenshot. Snapshot portability is the hard part of pixel testing — solved by pinning Playwright's official Docker image (mcr.microsoft.com/playwright:v1.59.1-noble) for both contributor runs and CI. Without the pin, snapshots captured locally and on CI diverge by a handful of antialiasing pixels and every run fails.

apps/example/
  e2e/visual/                            # specs + per-spec snapshots
    _setup.ts                            # disable animations, wait for fonts
    text-field.visual.spec.ts            # 3×2×2 matrix × 3 states = 36 cells
  src/
    pages/visual/text-field.vue          # /visual/text-field route
    views/visual/
      VisualGrid.vue                     # generic matrix-cell renderer
      TextFieldFixture.vue               # feeds VisualGrid with the matrix
  scripts/visual-docker.sh               # docker run wrapper

Local workflow:

  • pnpm test:visual — verify against committed baselines
  • pnpm test:visual:update — regenerate baselines

Both wrap scripts/visual-docker.sh which bind-mounts the repo, anonymous-volumes each node_modules/, and runs corepack + pnpm install + build + playwright inside the container.

testIgnore: '**/visual/**' (gated on PLAYWRIGHT_VISUAL=1) keeps the existing behavioural pnpm test:e2e run unchanged — visual specs only execute when the dedicated scripts opt in.

CI split

Replace the single serial ci job with seven jobs running fully in parallel:

commit-lint │ validate │ unit ──┐                  ┌── e2e ×2 shards
                                ├─→ coverage      │
                       storybook ┘                  └── visual
  • validate — install + lint + example-app typecheck. (Library typecheck is implicit in build:types in every other job, no need to repeat here.)
  • unit — vitest unit tests, uploads coverage as artefact.
  • storybook — split off so the storybook build (~30-60s) and its Playwright run parallelise with vitest instead of stacking serially. Uploads coverage artefact.
  • coverageneeds: [unit, storybook]. Downloads both blobs, merges, forwards to codecov.
  • e2e — sharded ×2 via Playwright's --shard flag with a matrix strategy. fail-fast: false so one shard's failure doesn't cancel the other.
  • visual — Playwright Docker image as container:.

Total compute rises by ~4-5 min per run because each parallel job re-runs pnpm install and the library build. That's free on OSS runners and the wall-time cut shows up in every PR review cycle.

Test plan

  • pnpm test:visual — 36/36 pass, captured in pinned container
  • pnpm typecheck — passes
  • pnpm run lint — passes (84 pre-existing warnings, 0 errors)
  • Existing behavioural pnpm test:e2e — passes, doesn't pick up visual specs
  • All 36 snapshots reviewed by hand for visual correctness across the matrix
  • CI passes after push (will surface here)

kelsos added 3 commits April 30, 2026 12:04
The floating-label state had three pre-existing positioning bugs that
together produced visibly wrong layouts whenever a prepend icon was
present or `dense` was used:

- The active label slot dropped `--prepend-w` / `--append-w` from its
  padding, so the floated label drifted away from the input column for
  default and filled. Restored both vars in the active rule, and added
  a dedicated outlined+active override that keeps the floated label
  anchored to the field's left edge per MD3 (outlined floats over the
  border, not over the input column).

- `outlined+dense+active` inherited `leading-[2.5]` from the dense
  compound and silently overrode the active slot's `leading-tight`,
  inflating the label line-box from ~15px to ~30px and pushing the
  floated label below the fieldset border. Re-asserted `!leading-tight`
  for that compound.

- For `default+dense+active`, the labelText kept stretching to the full
  inputWrapper height (~32px) and dragged its baseline into the input
  text. Translated only the labelText span (not the label container) so
  the underline pseudo stays anchored at the bottom of the input row.

`use-prepend-append-width` was measuring `contentRect.width + left`,
which silently dropped padding-right. Default's `pr-3`-only prepend
therefore reported 12px short of its real outer width and the floated
label landed under the icon's trailing padding instead of aligned with
the input text. Switched to `offsetWidth` so the value reflects the
full border-box.
Set up a pixel-level regression suite that catches CSS-only regressions
the existing behavioral specs miss (label position, dense overlap,
prepend alignment — the class of bugs surfaced and fixed in the
preceding commit).

Layout chosen so additional component fixtures slot in cleanly:

  apps/example/
    e2e/visual/                            # specs + per-spec snapshots
      _setup.ts                            # disable animations, wait for fonts
      text-field.visual.spec.ts            # 3x2x2 matrix x 3 states
    src/
      pages/visual/text-field.vue          # /visual/text-field route
      views/visual/
        VisualGrid.vue                     # generic matrix-cell renderer
        TextFieldFixture.vue               # feeds VisualGrid with the matrix

Snapshot portability — the single hardest part of pixel testing — is
solved by pinning Playwright's official Docker image
(`mcr.microsoft.com/playwright:v1.59.1-noble`) for both contributor
runs and CI. Any other approach (host Chromium, Ubuntu CI runner) gives
divergent antialiasing across machines and every snapshot fails. The
image's bundled browser binary is kept in sync with the
`@playwright/test` version pinned in `pnpm-workspace.yaml`.

Local workflow:
  - `pnpm test:visual`         verifies against committed baselines
  - `pnpm test:visual:update`  regenerates baselines

Both wrap `apps/example/scripts/visual-docker.sh`, which bind-mounts the
repo, anonymous-volumes each `node_modules/` (so a contributor's macOS
arm64 install isn't seen by the linux x64 container), and runs
corepack + pnpm install + build + playwright inside.

CI side: a new `visual` job runs the same image as a `container:` and
parallelises with the existing `ci` job — failures surface via the
existing playwright-report artefact upload pattern, with side-by-side
expected/actual/diff PNGs viewable in the report.

`testIgnore: '**/visual/**'` (gated on `PLAYWRIGHT_VISUAL=1`) keeps the
behavioral `pnpm test:e2e` run unchanged — visual specs only execute
when the dedicated scripts opt in.

Initial 36 snapshots (3 variants x 2 densities x 2 prepend modes x 3
states) capture the post-fix layout from the preceding commit.
Replace the single serial `ci` job with seven jobs that run fully in
parallel — wall time on PRs drops from ~max(commit-lint, ci, visual)
to ~max of the longest test job.

Layout:

  commit-lint │ validate │ unit ──┐                  ┌── e2e ×2 shards
                                  ├─→ coverage      │
                         storybook ┘                  └── visual

Job responsibilities:

- `validate` — install + lint + example-app typecheck. Library
  typecheck is implicit in `build:types` (run as part of `build:prod`
  in every other job), so it's not duplicated here.
- `unit` — install + build:prod + vitest unit tests. Uploads
  blob-format coverage as `vitest-report-unit` artefact.
- `storybook` — install + build:prod + build:storybook + storybook
  play-function tests via Playwright. Uploads `vitest-report-storybook`
  artefact. Splitting the storybook run off from `unit` parallelises
  the storybook build (~30-60s) and its Playwright run with the vitest
  pass instead of stacking them serially.
- `coverage` — `needs: [unit, storybook]`. Downloads both blobs,
  merges via vitest's `--merge-reports`, forwards to codecov.
- `e2e` — sharded ×2 via Playwright's `--shard` flag with a matrix
  strategy. `fail-fast: false` so a failure on one shard doesn't
  cancel the other; both shards' reports upload separately.
- `visual` — unchanged from the previous infra commit; runs in the
  pinned Playwright Docker image.

Total compute rises by ~4-5 min per run because each parallel job
re-runs `pnpm install` and the library build. That's free on OSS
runners and the wall-time cut shows up in every PR review cycle.
Walking through the timing: a shared `build` job uploading dist/ would
*slow* wall-time down (tests would have to wait for the build instead
of running in parallel with each job's own build) — the duplication
is a net win at this scale.

Both `unit`, `storybook` and `e2e` cache `~/.cache/ms-playwright` keyed
on `pnpm-lock.yaml` so the ~150MB chromium download only re-runs when
`@playwright/test` changes — saves ~15-30s per job after the first run
on a branch.

`actions/cache` pinned to v5.0.5; `actions/download-artifact` pinned to
v7 to match the upload action's major version.
@kelsos kelsos requested a review from a team as a code owner April 30, 2026 12:04
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.83%. Comparing base (58d8ba6) to head (bd0d187).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #530      +/-   ##
==========================================
- Coverage   84.83%   84.83%   -0.01%     
==========================================
  Files         145      145              
  Lines        5269     5267       -2     
  Branches     1577     1577              
==========================================
- Hits         4470     4468       -2     
  Misses        799      799              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kelsos kelsos merged commit bd0d187 into rotki:main Apr 30, 2026
11 checks passed
@kelsos kelsos deleted the fix/text-field-floated-label-and-visual-suite branch April 30, 2026 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants