Fix Stripe Link email asterisk position when label font differs - #3518
vivi-the-going-merry[bot] wants to merge 3 commits into
Conversation
…iframe Measure the Email label from Stripe's own .Label appearance rules instead of the page's label styles, pass fontFamily to the .Label rule, and drop the asterisk's left padding. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Repository: Strategy11/formidable-forms/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| PHP | Oct 2, 2026 8:11p.m. | Review ↗ | |
| JavaScript | Oct 2, 2026 8:11p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
|
Still no before/after image: Stripe Link only renders with live Stripe keys, and this agent has none. The fix is unchanged since the first push (measures the label from the .Label appearance rule, adds fontFamily to .Label, drops the asterisk left padding). Leaving franky-review off. Either verify on a Stripe-connected site and add the label, or tell me to send it to Franky as-is. |
There was a problem hiding this comment.
Request Changes. The change reads correctly and nothing in it is broken, but it is unverified in a browser and it leaves the reported case (a theme font) unfixed for any font loaded as a web font.
- Not run live. Stripe Link needs a connected Stripe account, and I have none here. The PR can't show before/after either. So what I checked was source only: how
frm_stripe_vars.appearanceRules['.Label']is built and read (get_appearance_rules()andget_style_settings_for_form()), and the newgetEmailAsteriskOffset()against the old one. Nothing was rendered, and the asterisk's position next to Stripe's real label was not observed. - Blocking: the measurement still happens in the page, and the Stripe iframe can't see page fonts (inline comment on
frmstrp.js:783). - Non-blocking:
fontFamilyon.Labelalso restyles the Payment Element labels (inline comment on the controller). - Checked, not a finding:
frmstrp.min.jsis not regenerated, butnpm run minimizeruns in the release-prep commits (Prepare for 6.30,Build and minimize js files), not per fix..Labelis read unguarded elsewhere in the same file. The padding change is sound: the measured width already includes the left padding, so zeroing it on the asterisk is right. - CI: all the checks that ran are green. The rest are skipped for this bot-authored branch.
| position: 'absolute', | ||
| visibility: 'hidden', | ||
| whiteSpace: 'nowrap', | ||
| fontFamily: rules.fontFamily || 'inherit', |
There was a problem hiding this comment.
Blocking: this measures with the page's font, but the label Stripe draws can't use it. elements() is created with no fonts option, so inside the iframe a web font (Google Fonts, @font-face, anything loaded by the theme) falls back to a system font with different glyph widths. The span measured here uses the real web font, so for those sites the offset is still wrong, which is the bug in formidable-pro#6786. The fix only holds when $settings['font'] is a system stack.
The issue's first suggested fix was to pass fontFamily and load the font through fonts in elements(). This PR does the first half only.
Pick one:
- Pass
fonts: [ { cssSrc: <the style's font stylesheet URL> } ]when the style uses a web font, so the iframe label matches this probe. - Measure with a font the iframe is guaranteed to have, i.e. only send
fontFamilyto Stripe (and use it here) when it is a system stack, and otherwise send a generic family such assans-serifto Stripe and use the same value here.
Also: when rules.fontFamily is missing, 'inherit' takes the page body font, not Stripe's default, so that fallback has the same mismatch.
There was a problem hiding this comment.
Option 2. The .Label font stack sent to Stripe now drops families the page loads as web fonts (document.fonts), falling back to the rest of the stack or sans-serif; the probe in getEmailAsteriskOffset() reads the same filtered rules. A missing fontFamily now measures with system-ui, sans-serif instead of inherit. Option 1 is not possible in Lite: styles carry only a font-family string, no stylesheet URL.
| 'backgroundColor' => $settings['bg_color_active'], | ||
| ), | ||
| '.Label' => array( | ||
| 'fontFamily' => $settings['font'], |
There was a problem hiding this comment.
Non-blocking: .Label is shared by every Stripe element, so this also changes the font of the Payment Element's labels (card number, expiry and so on), not only the Link email label. That is probably what you want (it matches .Input), but the PR description says the change is scoped to the asterisk. Say so in the description, and look at a card form on a theme font before merging.
There was a problem hiding this comment.
Agreed, the description now says the Payment Element labels pick up the style font too. Not checked on a rendered card form (no Stripe keys here), so worth a look on a theme font before merging.
The Stripe iframe does not load page web fonts, so the .Label font stack now drops them before it is sent to Stripe and used for the asterisk probe. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Method: in-place push |
There was a problem hiding this comment.
Approve. The round-1 blocker is fixed: the probe and the iframe now use the same font stack, with page web fonts dropped from both.
- Checked in a real browser:
removeWebFonts()against a page with@font-facerules forOpen Sans(local src) andLato(a 404ing url).document.fontslists both even thoughLatonever loaded, so unloaded web fonts are still caught."Open Sans", Lato, Helvetica, sans-serifbecameHelvetica, sans-serif,'Lato'becamesans-serif, andArial, sans-serifwas kept. Quoted and unquoted names both match. - Not run live: Stripe Link needs a connected Stripe account, which I do not have, so the asterisk next to Stripe's real label was not observed and there is no before/after. Source-verified only for that part.
- Non-blocking, inline: the hard-coded English
Email, and the fallback when the style font is blank. frmstrp.min.jsis not regenerated; that happens in the release-prep commits, as noted last round.- CI: every check that ran is green, the rest are skipped for this bot-authored branch.
| function getEmailAsteriskOffset() { | ||
| const rules = getAppearanceRules()[ '.Label' ] || {}; | ||
| const label = document.createElement( 'span' ); | ||
| label.innerHTML = 'Email '; |
There was a problem hiding this comment.
Non-blocking: the probe text is the English word Email, but the Link authentication element renders a localized label (its locale defaults to the visitor's browser locale). On a site where Stripe draws E-Mail or Correo electrónico the offset is still off. This was already true before the PR. If you want to close it, measure the text Stripe actually renders, or note the limit in the docblock.
There was a problem hiding this comment.
Took the docblock option: getEmailAsteriskOffset() now says the probe text is English, so the offset can be off when Stripe renders a localized label. Not measuring the rendered text: it lives in the cross-origin iframe and Link has no API to read it.
| position: 'absolute', | ||
| visibility: 'hidden', | ||
| whiteSpace: 'nowrap', | ||
| fontFamily: rules.fontFamily || 'system-ui, sans-serif', |
There was a problem hiding this comment.
Non-blocking: when the style's Font field is blank, PHP now sends .Label.fontFamily = ''. Stripe then draws with its default stack, while this probe falls back to system-ui, sans-serif, which differs from that default on Windows and Linux, so the offset can be a few pixels off. The simplest fix is to skip the fontFamily key in get_appearance_rules() when $settings['font'] is empty, and keep one shared fallback constant in the JS.
There was a problem hiding this comment.
Done in 051aea5: get_appearance_rules() drops .Label.fontFamily when the style font is blank, so Stripe uses its default stack and the probe falls back to system-ui, sans-serif only in that case. Checked with a harness: blank font → no fontFamily on .Label (still on .Input, unchanged), Arial → kept; the old code sent an empty string. I did not add a second JS constant: the fallback is already in one place (getEmailAsteriskOffset()), and removeWebFonts() returns sans-serif for a different case (every family was a web font).
Stripe then draws with its default stack instead of an empty value. Note the English-only probe text in the offset docblock. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Method: in-place push |
There was a problem hiding this comment.
Approve. Both notes from my last review are dealt with at 051aea5.
- Blank style font:
get_appearance_rules()now drops.Label.fontFamilywhen the style font is empty, so Stripe gets its default stack instead of an empty value. I ran the branch in PHP for'',nulland'Arial, sans-serif': the first two omitfontFamily, the last keeps it.php -landnode --checkpass on both changed files. - English-only probe text: now stated in the
getEmailAsteriskOffset()docblock, so the limit is recorded. - The rest of the diff is unchanged since my last approval at
ad853ba, whereremoveWebFonts()was checked in a real browser.
Not run live: Stripe Link needs a connected Stripe account, which I do not have, so the asterisk next to Stripe's real label was not observed and there is no before/after. That part is source-verified only, so I'm requesting a human look on a Stripe-connected site.
CI: every check that ran is green (Psalm, PHP syntax 7 and 8.4, Typos, CodeRabbit, DeepSource, Scrutinizer); the rest are skipped for this bot-authored branch.
Relates to Strategy11/formidable-pro#6786
Broken: The required asterisk on the Stripe Link Email element is positioned from a hidden label measured with the page's label styles (theme font, letter-spacing, size, body inheritance). Stripe draws its label in the iframe from the
.Labelappearance rule only, and the iframe cannot load the page's web fonts, so the asterisk lands in a gap or overlaps when the two differ. The asterisk also kept the left padding already counted in the measured width.Changed:
get_appearance_rules():.Labelnow passesfontFamily(same as.Input), and leaves it out when the style's Font field is blank so Stripe uses its default stack. This also applies the style font to the Payment Element's labels, not only the Link email label.getAppearanceRules()/removeWebFonts(): the.Labelfont stack sent to Stripe drops families the page loads as web fonts (document.fonts), falling back to the rest of the stack orsans-serif.getEmailAsteriskOffset(): measures a plain span styled only from those rules (family, size, weight, padding; letter-spacing/text-transform reset), so it mirrors what Stripe renders. Drops the style-container lookup.padding-left: 0.Email, so the offset can be off when Stripe renders a localized label (noted in the docblock).Verified:
node --checkandphp -l; a PHP harness onget_appearance_rules()(blank font → no.Label.fontFamily,.Inputunchanged; red on the previous head); font-stack filtering run in a node harness with a fakedocument.fonts(web families dropped, system families kept, all-web stack becomessans-serif, source rules not mutated). Not rendered live: Stripe Link needs live Stripe keys, so there is no before/after image. A system font missing from the visitor's machine is the same in the page and the iframe.stripe/js/frmstrp.min.jsis not regenerated here.🤖 Generated with Claude Code