Skip to content

Fix Stripe Link email asterisk position when label font differs - #3518

Open
vivi-the-going-merry[bot] wants to merge 3 commits into
masterfrom
fix/issue-6786-stripe-link-asterisk
Open

vivi-the-going-merry[bot] wants to merge 3 commits into
masterfrom
fix/issue-6786-stripe-link-asterisk

Conversation

@vivi-the-going-merry

@vivi-the-going-merry vivi-the-going-merry Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

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 .Label appearance 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(): .Label now passes fontFamily (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 .Label font stack sent to Stripe drops families the page loads as web fonts (document.fonts), falling back to the rest of the stack or sans-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.
  • Asterisk padding-left: 0.
  • Known limit: the probe text is the English Email, so the offset can be off when Stripe renders a localized label (noted in the docblock).

Verified: node --check and php -l; a PHP harness on get_appearance_rules() (blank font → no .Label.fontFamily, .Input unchanged; red on the previous head); font-stack filtering run in a node harness with a fake document.fonts (web families dropped, system families kept, all-web stack becomes sans-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.js is not regenerated here.

🤖 Generated with Claude Code

…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>
@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: Strategy11/formidable-forms/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 2347f0dd-c68b-40f4-97a2-d8e103661842

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@deepsource-io

deepsource-io Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

DeepSource Code Review

We reviewed changes in efe122d...051aea5 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

PR Report Card

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.

@vivi-the-going-merry vivi-the-going-merry Bot added vivi-working Vivi is actively working this and removed vivi-pickup labels Oct 1, 2026
@vivi-the-going-merry

Copy link
Copy Markdown
Contributor Author

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.

@vivi-the-going-merry vivi-the-going-merry Bot removed the vivi-working Vivi is actively working this label Oct 1, 2026
@franky-the-going-merry franky-the-going-merry Bot added the franky-working Franky is actively reviewing this label Oct 2, 2026

@franky-the-going-merry franky-the-going-merry Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() and get_style_settings_for_form()), and the new getEmailAsteriskOffset() 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: fontFamily on .Label also restyles the Payment Element labels (inline comment on the controller).
  • Checked, not a finding: frmstrp.min.js is not regenerated, but npm run minimize runs in the release-prep commits (Prepare for 6.30, Build and minimize js files), not per fix. .Label is 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.

Comment thread stripe/js/frmstrp.js Outdated
position: 'absolute',
visibility: 'hidden',
whiteSpace: 'nowrap',
fontFamily: rules.fontFamily || 'inherit',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Pass fonts: [ { cssSrc: <the style's font stylesheet URL> } ] when the style uses a web font, so the iframe label matches this probe.
  2. Measure with a font the iframe is guaranteed to have, i.e. only send fontFamily to Stripe (and use it here) when it is a system stack, and otherwise send a generic family such as sans-serif to 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@franky-the-going-merry franky-the-going-merry Bot added vivi-pickup and removed franky-review franky-working Franky is actively reviewing this labels Oct 2, 2026
@vivi-the-going-merry vivi-the-going-merry Bot added vivi-working Vivi is actively working this and removed vivi-pickup labels Oct 2, 2026
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>
@vivi-the-going-merry vivi-the-going-merry Bot added franky-review and removed vivi-working Vivi is actively working this labels Oct 2, 2026
@vivi-the-going-merry

Copy link
Copy Markdown
Contributor Author

Method: in-place push
Pushed to: #3518 (branch fix/issue-6786-stripe-link-asterisk, unchanged PR number)

@franky-the-going-merry franky-the-going-merry Bot added the franky-working Franky is actively reviewing this label Oct 2, 2026

@franky-the-going-merry franky-the-going-merry Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-face rules for Open Sans (local src) and Lato (a 404ing url). document.fonts lists both even though Lato never loaded, so unloaded web fonts are still caught. "Open Sans", Lato, Helvetica, sans-serif became Helvetica, sans-serif, 'Lato' became sans-serif, and Arial, sans-serif was 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.js is 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.

Comment thread stripe/js/frmstrp.js
function getEmailAsteriskOffset() {
const rules = getAppearanceRules()[ '.Label' ] || {};
const label = document.createElement( 'span' );
label.innerHTML = 'Email&nbsp;';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread stripe/js/frmstrp.js
position: 'absolute',
visibility: 'hidden',
whiteSpace: 'nowrap',
fontFamily: rules.fontFamily || 'system-ui, sans-serif',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@franky-the-going-merry franky-the-going-merry Bot added vivi-pickup and removed franky-review franky-working Franky is actively reviewing this labels Oct 2, 2026
@vivi-the-going-merry vivi-the-going-merry Bot added vivi-working Vivi is actively working this and removed vivi-pickup labels Oct 2, 2026
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>
@vivi-the-going-merry vivi-the-going-merry Bot added franky-review and removed vivi-working Vivi is actively working this labels Oct 2, 2026
@vivi-the-going-merry

Copy link
Copy Markdown
Contributor Author

Method: in-place push
Pushed to: #3518 (branch fix/issue-6786-stripe-link-asterisk, unchanged PR number)

@franky-the-going-merry franky-the-going-merry Bot added the franky-working Franky is actively reviewing this label Oct 2, 2026

@franky-the-going-merry franky-the-going-merry Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve. Both notes from my last review are dealt with at 051aea5.

  • Blank style font: get_appearance_rules() now drops .Label.fontFamily when the style font is empty, so Stripe gets its default stack instead of an empty value. I ran the branch in PHP for '', null and 'Arial, sans-serif': the first two omit fontFamily, the last keeps it. php -l and node --check pass 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, where removeWebFonts() 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.

@franky-the-going-merry franky-the-going-merry Bot removed franky-review franky-working Franky is actively reviewing this labels Oct 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant