Fix clipped "Allow Wallet" permission dialog.#191
Open
djscruggs wants to merge 3 commits into
Open
Conversation
The first party permission popup opened at a fixed height of 230px, plus a 30px URL bar allowance applied only when the user agent looked like Firefox. Both parts of that guess were wrong: Chrome and Safari also show a URL bar in popups (and no user agent string predicts the real chrome height), and the greeting itself grows with the wallet's icon and name -- a site with a web app manifest renders a 48px icon above a name and origin line. The dialog is laid out to exactly fill the popup with one internally scrolling region, so content that did not fit was clipped mid-line rather than made scrollable, cutting off the wallet row. Apply the chrome allowance on every browser, raise the base height to 260, and have the popup measure its own content once loaded and resize to fit, replacing both guesses with a measurement. The sizing decision is a pure function (getWindowHeightAdjustment) clamped to the available screen height, a minimum usable height, and a maximum single adjustment; fitWindowToContent is the DOM shell around it and is a no-op where resizing is not permitted. Measured against a real cross-origin wallet, the greeting was clipped by 40px before this change and fits exactly after it. Also let the dev-only wallet chooser harness render an origin manifest, so it reproduces the taller greeting that the no-manifest default hid.
djscruggs
marked this pull request as ready for review
July 24, 2026 22:44
Contributor
Author
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
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.





Problem
The first party "Allow Wallet" permission popup opened at a fixed height of
230px, plus a 30px URL bar allowance applied only when the user agent looked
like Firefox. The wallet row was cut off mid-line:
popups, and no user agent string predicts the real chrome height — the
browser subtracts its own chrome from the height passed to
window.openbyan amount that varies with browser, platform, and user settings.
renders a 48px icon above a name line and an origin line. The no-manifest
case (a small default glyph and a bare origin) fits; the real one does not.
The dialog is laid out to exactly fill the popup with one internally scrolling
region, so content that did not fit was clipped rather than made
scrollable. This is why the symptom is a half-visible wallet row with the
Block/Allow footer still in view — the document never overflows and the footer
is pinned outside the scroll region, so neither a document-level scroll check
nor a footer-position check detects it.
Fix
260 (
DEFAULT_ALLOW_WALLET_POPUP_HEIGHT= 290).replacing both guesses with a measurement. Over-allowing height now costs
nothing, since a popup with more viewport than it needs shrinks.
The sizing decision is a pure function,
getWindowHeightAdjustment, clamped tothe available screen height, a 160px minimum usable height, and a 400px maximum
single adjustment.
fitWindowToContentis the DOM shell around it: it measuresthe scrolling region (not just the document, which never overflows here) and is
a no-op wherever resizing is not permitted, so a popup that cannot resize still
works — it just scrolls.
Resizing runs after
nextTick()and after images settle, because an icon thathas not loaded yet contributes no height.
Verification
Measured against a real cross-origin CHAPI wallet driving the actual popup,
bodyOverflow= pixels of greeting clipped inside the scrolling body:mainIn a real
window.openpopup,fitWindowToContentgrows a 200px popup by33px, a 230px popup by 3px, and leaves a 290px popup unchanged — overflow 0 in
all three.
Tests: 11 unit tests for the sizing logic (
npm run test:unit, Node'sbuilt-in runner — no new dependency) and 12 geometric-invariant e2e tests for
the dialog, passing on Chromium, Firefox, and WebKit. Confirmed red before the
fix.
Note that Playwright pins the viewport, so
resizeByis inert under it bydesign: the e2e tests assert the CSS/content invariant at a given height, and
the resize arithmetic is covered by the unit tests.
Notes for review
urlBarHeightis shared withDEFAULT_HINT_CHOOSER_POPUP_HEIGHTandDEFAULT_CROSS_DEVICE_HINT_CHOOSER_POPUP_HEIGHT, so making it unconditionalalso gives those popups 30px more room on Chrome and Safari. That is the same
correction, and safe now that oversized popups shrink to fit — but it is a
wider blast radius than the allow-wallet dialog alone and worth a look.
/test/wallet-chooserpassedmanifest: null, so it never rendered the icon and name that cause this bug.This PR adds manifest support (
name=,manifest=0). The existing galleryscreenshots were taken under that blind spot.
.github/workflows/main.yml, Node 24) and does notrun either test suite, so the new tests do not gate this PR. Lint passes. I
ran unit + e2e locally.
wallet-chooser.spec.js→"has no horizontal overflow" fails 5× on stock
mainas well (verified bystashing). It is the documented KNOWN FAILING test awaiting the
vue-web-request-mediatorextraction.left as
dd);package.json'sversionis untouched. The onepackage.jsonchange is adding thetest:unitscript.