Fix 1p dialog horizontal overflow at the root + add layout test suite#177
Closed
djscruggs wants to merge 7 commits into
Closed
Fix 1p dialog horizontal overflow at the root + add layout test suite#177djscruggs wants to merge 7 commits into
djscruggs wants to merge 7 commits into
Conversation
Add `/test/wallet-chooser`, registered only outside production builds, that mounts the presentational `MediatorWizard` with fake state driven by URL query params (`hints`, `qr`, `type`). This exercises the first party dialog's layout and CSS across wallet counts, the cross-device QR section, and request types without any CHAPI registration, iframes, or popups — the slow, flaky parts of testing this dialog by hand. It backs the automated visual/layout suite added in following commits. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add a Playwright suite that drives the `/test/wallet-chooser` harness across wallet counts and the cross-device QR section on Chromium, WebKit, and Firefox, asserting structural invariants rather than pixels: the expected hints render, the popup window does not scroll, no horizontal scrollbar appears, the header stays pinned while the list scrolls, the panel fills the popup width, and the QR section/expander behave per wallet count. `npm run test:e2e` starts the dev server automatically and runs all three engines. A stricter "no horizontal overflow" assertion is included but skipped: it detects the overflow condition the 7.4.1 `overflow-x: hidden` only clips, and is meant to be enabled alongside the root-cause fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add `npm run gallery`, which screenshots the first party wallet chooser across wallet counts, themes, and engines (collapsed and expanded) via the harness route, then builds an `index.html` contact sheet for quick visual review. Output lands in the git-ignored `test/e2e/gallery/`. Document both the regression suite and the gallery in the README, including the Brave/mobile coverage caveats. Also git-ignore the local `scratchpad/` working directory. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The first party dialog headers and separators bleed their border edge-to-edge using a negative side-margin (`margin: -15px`) that cancels the panel's padding. Under the dialog's border-box flex layout that overhang was measured as horizontal overflow, which 7.4.1 only hid with `overflow-x: hidden`. Neutralize the side margins and recreate the full-bleed border with padding instead, so nothing overhangs, then remove the `overflow-x: hidden` workaround. Also target the wallet hint list by its own `.wrm-hint-list` class for the flex sizing rather than the generic `.wrm-flex-column-stretch`, which upstream also applies to a wrapper div around the integrated list; styling that class reached the wrong element. Enable the previously skipped "no horizontal overflow" test, which now passes on all three engines and guards this fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
djscruggs
changed the title
Add emulated iPhone 15 (`iphone`) and Pixel 7 (`android-pixel`) Playwright projects. On a phone the popup is clamped to the screen width and crosses the dialog's 430px "small screen" CSS breakpoint, so these exercise layout branches the 500px desktop projects do not; all invariants hold there too. Also capture an `-expanded-scrolled` gallery shot when the expanded QR code sits below the fold (many wallets on a short/phone viewport), so the gallery shows the QR is reachable by scrolling. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The horizontal-overflow assertion compared `scrollWidth` to `clientWidth` for every descendant. Inline elements (e.g. <span>, <strong>) report `clientWidth: 0` with a nonzero `scrollWidth`, which Firefox surfaces and Chromium does not — a cross-engine difference that is not overflow. Skip inline elements and any element with a zero content box, so the check only flags block/flex elements whose content genuinely exceeds their width. Verified it still catches a real overhang via the header margin. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The previous root-cause attempt removed the dialog header's negative side margins to kill the horizontal overflow, which also removed the header's full-bleed border: its bottom rule stopped at the panel padding and read like an inset separator instead of spanning the popup edge-to-edge. The header itself never overflowed — as a direct child of the padded panel its negative margins cancel the padding exactly. The 15px overflow came from the in-flow body dividers (the `.wrm-separator` rows and the empty `.wrm-modal-content-header` divider), which reuse the same negative side-margin but sit inside a wrapper 15px narrower than the panel on each side, so the margin pushed past its right edge. Restore the header's full-bleed margins and neutralize the side margins on the body dividers only; they read as inset rules with no overhang. This removes the real overflow at its source, so the temporary `overflow-x: hidden` workaround stays gone with the header appearance matching the deployed dialog. Add a layout test asserting the header border spans the full panel width, so a future change cannot silently shrink the bleed again (the geometric-overflow checks could not catch it). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Header border fix — before/after (chromium, light, 5 wallets / no QR). Before (the approach in this PR's After (
|
Contributor
Author
|
Closing in favor of #179. Per review feedback, splitting this work: #179 lands the tooling only (harness route, Playwright layout suite, screenshot gallery) with no style changes —
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the root cause of the first party dialog's horizontal scrollbar (the 7.4.1
overflow-x: hiddenwas a temporary clip, not a fix), and adds the automated visual/layout test suite that proves it — and would have caught this week's 7.4.x regressions.The fix (
f27243e)Root cause (as diagnosed in the alpha review): the dialog headers and separators bleed their border edge-to-edge using a negative side-margin (
margin: -15px) that cancels the panel's 15px padding. Under this dialog'sbox-sizing: border-boxflex layout, that 15px overhang on each side is measured as real horizontal overflow → scrollbar.overflow-x: hiddenonly clipped it..wrm-modal-content-headerand the cross-device/web-share.wrm-separators, and recreate the full-bleed border with padding instead — nothing overhangs.overflow-x: hidden..wrm-hint-listclass for the flex sizing instead of the generic.wrm-flex-column-stretch(which upstream also applies to a wrapper div around the integrated list, so the sizing/overflow reached the wrong element — the fragile block flagged in review).Full-bleed separators still render edge-to-edge; verified in the gallery.
The test suite
Dev-only harness route (
97abf4a) —/test/wallet-chooser?hints=N&qr=1mounts the presentationalMediatorWizardwith fake state. No CHAPI registration, wallets, iframes, popups, or certs. Excluded from production builds (NODE_ENV !== 'production').Geometric-invariant suite (
826bd2c,f27243e) —npm run test:e2e, on Chromium + WebKit + Firefox, starts the dev server automatically. Asserts structure not pixels: expected hints render, no horizontal overflow (the strict content-wider-than-box check, not just the scrollbar), no window scroll, header pinned while the list scrolls, panel fills the popup, expander/QR behave per wallet count. 109 pass, 0 skipped.Screenshot gallery (
3ed6b33) —npm run gallerycaptures every state × theme × engine (collapsed and expanded) and builds anindex.htmlcontact sheet, replacing the manual screenshot-and-Slack loop. Output in the git-ignoredtest/e2e/gallery/.Why the test suite matters here
The strict "no horizontal overflow" assertion failed on
mainbefore this fix — it detected the overflow the band-aid was hiding — and passes after it, on all three engines. The regression and its fix are now both guarded.Coverage notes
Deliberately deferred
Testing
npm run test:e2e(109 pass) andnpm run gallery(30 tests, 48 screenshots) green locally. Lint clean. CHANGELOG entry added (7.4.4). README documents both commands.🤖 Generated with Claude Code