Skip to content

[style-value-parser] Build fix: mirror horizontal radii when border-radius has no "/" - #1907

Merged
mellyeliu merged 2 commits into
meta:mainfrom
henryqdineen:hqd-fix-border-radius-vertical-defaults
Oct 10, 2026
Merged

mellyeliu merged 2 commits into
meta:mainfrom
henryqdineen:hqd-fix-border-radius-vertical-defaults

Conversation

@henryqdineen

@henryqdineen henryqdineen commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator

What changed / motivation ?

main is currently red: packages/style-value-parser fails border-radius.test.js › symmetric radii serialize without a /, because roundTrip('10px 20px') returns 10px 20px / 10px. I noticed it while rebasing #1700 onto main, which now has a failing tests / packages build even though that PR only touches babel-plugin.

Why it broke main: Without a /, the parser passes only the four horizontal radii to BorderRadiusShorthand, and the constructor filled in the missing vertical radii from verticalTopLeft rather than from their horizontal counterparts. So 10px 20px was stored as horizontal 10 20 10 20 / vertical 10 10 10 10. That internal state has always been wrong, but toString hid it: it built the vertical string (sStr) from the horizontal values, so both halves always matched and the / was dropped. #1759 fixed sStr to read the vertical values (and #1742 made the same change), which exposed the bad constructor defaults. #1759's own new round-trip test fails on its own commit, but the tests workflow never ran on that PR, so it merged without anyone seeing it fail.

Fix: When no vertical radii are passed, each vertical radius now mirrors its horizontal counterpart, per the CSS spec. When some vertical radii are passed, the existing CSS fill-in rules still apply to the rest (topRight falls back to topLeft, bottomRight to topLeft, bottomLeft to topRight).

Linked PR/Issues

Follow-up to #1759 and #1742. Unblocks #1700.

Additional Context

The existing Valid: border-radius: … parse tests compare parse(...) to new BorderRadiusShorthand(...), so both sides go through the same constructor defaults and couldn't catch this. The round-trip test from #1759 does catch it and now passes. npm run test:packages, flow check, and Prettier all pass locally.

This also fixes a Prettier failure on main: an assertion added in #1742 to border-radius.test.js wasn't formatted.

Pre-flight checklist

🤖 Generated with Claude Code

…o `/`

Without a `/`, BorderRadiusShorthand defaulted the missing vertical radii
from verticalTopLeft, so `10px 20px` serialized as `10px 20px / 10px`.
This was masked until meta#1742/meta#1759 made toString read the vertical values.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@vercel

vercel Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
stylex Ignored Ignored Sep 24, 2026 2:10pm UTC

Request Review

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 24, 2026
@henryqdineen henryqdineen changed the title [style-value-parser] Fix main: mirror horizontal radii when border-radius has no "/" [style-value-parser] Build fix: mirror horizontal radii when border-radius has no "/" Sep 24, 2026

@imdreamrunner imdreamrunner left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this is the right fix. In CSS, when border-radius has no /, each corner's vertical radius equals its horizontal one. On main, the constructor sets all four vertical radii to top-left, so 10px 20px was stored with vertical radii 10px 10px 10px 10px. Nobody noticed until #1759 started writing out the vertical radii. Since then, 10px 20px comes back as 10px 20px / 10px, which is why main's packages job is red.

@henryqdineen

Copy link
Copy Markdown
Collaborator Author

Thanks @imdreamrunner. Do you have merge permissions?

@mellyeliu
mellyeliu merged commit 5a419da into meta:main Oct 10, 2026
8 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants