Skip to content

Navigation Menu

Sign in
Sign up

fix(core): render plain Link anchors inline so an ancestor Text clamp can truncate them - #6038

Open
ManoharPaturi wants to merge 4 commits into
facebook:main from
ManoharPaturi:fix/link-anchor-clamp
Open

fix(core): render plain Link anchors inline so an ancestor Text clamp can truncate them #6038
ManoharPaturi wants to merge 4 commits into
facebook:main from
ManoharPaturi:fix/link-anchor-clamp

Conversation

@ManoharPaturi

@ManoharPaturi ManoharPaturi commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Body:

Fixes #6021

Summary

A plain <Text maxLines={2}><Link>...</Link></Text> silently did not truncate:
the anchor was display: inline-flex, and an inline-flex box establishes its
own formatting context that an ancestor -webkit-line-clamp cannot reach.

This changes the plain anchor to display: inline, so it participates in the
surrounding line boxes and an ancestor clamp truncates it normally. The flex
layout is not lost where it is load-bearing — it moves to a dedicated style
applied only when needed:

  • external links (isExternalLink + anchor form) still lay out as
    inline-flex with alignItems: center and the icon gap, exactly as before
  • the button-rendered form keeps its previous inline-flex layout

An external link inside a clamped Text still cannot be truncated by the
ancestor (the icon needs the flex layout), so this also documents the
composition on both sides — Link guidance (do: plain links clamp; don't:
expect it for external links — pass maxLines to the Link) in all three doc
variants (EN/zh/dense) and matching Text guidance (EN/zh + dense).

Tests

Three new cases in Link.test.tsx assert the emitted display declarations
per form (via the injected-CSS helper pattern used by forced-colors tests):

  • plain anchor → inline, and not inline-flex
  • external-link anchor → keeps inline-flex
  • button-rendered form → keeps inline-flex

Testing

  • vitest run packages/core/src/Link — 63 passed
  • vitest run packages/core/src/Text — 189 passed
  • vitest run internal/eslint-plugin-astryx — 640 passed (docblock format)
  • tsc --noEmit (core) clean, eslint clean, check:changesets and
    check:sync pass

Copilot AI lite review requested due to automatic review settings September 4, 2026 18:26

vercel Bot commented Sep 4, 2026 •
edited
Loading

Copy link
Copy Markdown

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

Project Deployment Actions Updated
astryx Ready Ready Preview Sep 5, 2026 4:42am UTC

Request Review

meta-cla Bot commented Sep 4, 2026

Copy link
Copy Markdown

Hi @ManoharPaturi!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@github-actions github-actions Bot added community Authored by a community contributor (not on the eng/design team) needs:code-review High-risk change (new package/component/API) — needs human code review before merge needs:design-review Affects visuals — Design should review labels Sep 4, 2026

Copilot AI 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.

🟡 Changes recommended

The newly added CSS-introspection test helper matches selectors via substring checks, which can produce incorrect/flaky assertions and should be made selector-token-accurate.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Updates Link’s default anchor layout so it can be truncated by an ancestor <Text maxLines> clamp, while preserving inline-flex where it’s required (external-link icon and button-rendered form). Also adds documentation and tests to prevent regressions around the clamp behavior described in #6021.

Changes:

  • Render plain <a> links as display: inline (instead of inline-flex) to allow ancestor -webkit-line-clamp truncation.
  • Introduce a dedicated flexLayout style applied only to external-link icon and button-rendered forms.
  • Add docs + tests covering the truncation behavior and the external-link limitation.
File summaries
File Description
packages/core/src/Text/Text.doc.mjs Documents that external-link Link children won’t be truncated by an ancestor Text maxLines.
packages/core/src/Link/Link.tsx Switches base anchor display to inline and scopes inline-flex to icon/button cases via flexLayout.
packages/core/src/Link/Link.test.tsx Adds tests asserting the emitted display mode for plain/external/button Link forms.
packages/core/src/Link/Link.doc.mjs Adds Link guidance for when ancestor Text maxLines truncation will/won’t work.
.changeset/link-anchor-clamp.md Publishes the behavioral change as a patch changeset for @astryxdesign/core.
Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread packages/core/src/Link/Link.test.tsx Outdated
Comment on lines +429 to +438
for (const chunk of getAllInjectedCss().split('}')) {
const sel = chunk.split('{')[0] ?? '';
const body = chunk.split('{')[1] ?? '';
if (classes.some(c => sel.includes(c)) && body.includes('display')) {
const match = body.match(/display:\s*([^;]+);/);
if (match) {
declarations.push(match[1].trim());
}
}
}
@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Sep 4, 2026
parinith-web added a commit to parinith-web/astryx that referenced this pull request Sep 4, 2026 
The displayDeclarationsFor helper added in facebook#6038 matched an element's
classList against raw selector text via String.includes, which is a
substring check on hashed atomic class names (e.g. StyleX's x1a2b3c
style classes). A shorter class name can be a substring of an
unrelated longer one, so the check can both false-positive and
false-negative depending on what else StyleX has generated for the
test run.
Rewrite it to walk the real CSSOM: parse each CSSStyleRule's
selectorText into class tokens, compare those tokens against the
element's classList by exact match, and read the declaration via
rule.style.getPropertyValue('display') instead of a body-text regex.
Also descends into grouping rules (e.g. @media) so nested
declarations aren't missed.
Flagged by Copilot review on facebook#6038:
facebook#6038 (review)
All 63 Link tests and 189 Text tests still pass; tsc --noEmit is clean.

Copy link
Copy Markdown

Hey @ManoharPaturi, nice fix — the base/flexLayout split is exactly the right shape for this.

I took a stab at the one thing Copilot flagged (the displayDeclarationsFor helper in Link.test.tsx matching classes via String.includes on raw selector text, which can mis-match on hashed atomic class names). Reworked it to walk the real CSSOM instead — parses selectorText into class tokens and compares those exactly against the element's classList, reading the value via rule.style.getPropertyValue('display') rather than a regex over the rule body. Also descends into grouping rules like @media so nothing nested gets missed.

Ran it against your branch locally: all 63 Link tests, 189 Text tests, and 640 eslint-plugin-astryx tests still pass, and tsc --noEmit is clean.

Opened it as a PR on top of your branch here: #6043 — happy to fold it directly into this PR instead if you'd rather take the diff yourself, just say the word. Wanted to flag it here first rather than just have a second PR show up with no context.
ManoharPaturi reacted with heart emoji

@cixzhang cixzhang 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.

Existing block Links lose keyboard focus visibility in this change. In Chromium, <Link display="block"> changes from a 124px inline-flex focus box to an inline anchor split around a 260px block child; the 2px focus outline computes but paints nothing. Please preserve the old focusable box for block/own-clamped Link variants while keeping only unclamped plain anchors inline.

[Reviewed by Robohands]

ManoharPaturi reacted with heart emoji
// can truncate it. An inline-flex box establishes its own formatting
// context, which an ancestor -webkit-line-clamp cannot reach —
// <Text maxLines={2}><Link>...</Link></Text> silently did nothing.
display: 'inline',

@cixzhang cixzhang Sep 4, 2026

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.

display="block" now splits this inline anchor around its block child, so its keyboard focus outline disappears.

ManoharPaturi reacted with heart emoji 
The displayDeclarationsFor helper added in facebook#6038 matched an element's
classList against raw selector text via String.includes, which is a
substring check on hashed atomic class names (e.g. StyleX's x1a2b3c
style classes). A shorter class name can be a substring of an
unrelated longer one, so the check can both false-positive and
false-negative depending on what else StyleX has generated for the
test run.
Rewrite it to walk the real CSSOM: parse each CSSStyleRule's
selectorText into class tokens, compare those tokens against the
element's classList by exact match, and read the declaration via
rule.style.getPropertyValue('display') instead of a body-text regex.
Also descends into grouping rules (e.g. @media) so nested
declarations aren't missed.
Flagged by Copilot review on facebook#6038:
facebook#6038 (review)
All 63 Link tests and 189 Text tests still pass; tsc --noEmit is clean.

Copy link
Copy Markdown
Contributor Author

@parinith-web thank you for this — the CSSOM-token matching is a real improvement over my substring approach (exact class comparison + proper descent into grouping rules), and I appreciate you flagging it here first rather than surprising us with a stacked PR. Folded your commit into this branch as 1abf92c (kept your authorship). #6043 can be closed whenever convenient — all 63 Link tests green on the updated branch.

@cixzhang cixzhang 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.

Thanks for tightening the test helper. The earlier keyboard-focus regression is still present in Chromium: both <Link display="block"> and <Link maxLines={2}> compute a 2px outline but paint no visible focus box because the anchor remains inline around a block-level Text child. Please preserve a visible root focus box for those forms while keeping only the ancestor-clamped plain anchor inline.

[Reviewed by Robohands]

ManoharPaturi reacted with heart emoji

Copy link
Copy Markdown
Contributor Author

Thanks @cixzhang — fixed in 2a36cb1. The inline anchor is now reserved for the one composition this PR exists for: the unclamped plain Link under an ancestor <Text maxLines>. Any Link that establishes its own box keeps the inline-flex root and its visible focus outline:

  • display="block" → inline-flex root
  • own maxLines > 0 (self-clamping) → inline-flex root
  • external-link icon form → inline-flex root (unchanged)
  • button-rendered form → inline-flex root (unchanged)

So <Link display="block"> and <Link maxLines={2}> compute and paint the 2px focus box exactly as before this PR, while <Text maxLines={2}><Link>plain</Link></Text> — with no props on the Link — stays inline and truncable by the ancestor clamp. (One subtlety caught by the new tests: maxLines destructures with a default of 0, so the own-clamp condition is maxLines > 0, not a null check.)

Two new test cases assert the block and self-clamped forms keep inline-flex, on top of the existing plain-inline case. 254/254 Link + Text tests, core typecheck, and the full check:repo battery green on the updated branch.

@cixzhang cixzhang 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.

Thanks, the ancestor clamp and both requested focus cases now work in Chromium. One compatibility path remains: existing Links with a block-level child (for example, HStack plus an icon) still compute a 2px outline but paint no keyboard focus ring. Please keep those composed Links visibly focused too while leaving text-only ancestor-clamped Links inline.

[Reviewed by Robohands]

// root: an inline anchor around a block-level child computes a focus
// outline that paints nothing in Chromium, losing keyboard focus
// visibility on those forms.
const needsRootBox =

@cixzhang cixzhang Sep 5, 2026

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.

needsRootBox misses existing block-level children, so those composed Links still paint no focus ring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Lite
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@cixzhang cixzhang cixzhang requested changes
@imdreamrunner imdreamrunner Awaiting requested review from imdreamrunner imdreamrunner is a code owner
@josephfarina josephfarina Awaiting requested review from josephfarina josephfarina is a code owner
@cvkxx cvkxx Awaiting requested review from cvkxx
@ernestt ernestt Awaiting requested review from ernestt
@kentonquatman kentonquatman Awaiting requested review from kentonquatman
@rubyycheung rubyycheung Awaiting requested review from rubyycheung

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Meta Open Source bot. community Authored by a community contributor (not on the eng/design team) needs:code-review High-risk change (new package/component/API) — needs human code review before merge needs:design-review Affects visuals — Design should review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Link's inline-flex anchor silently defeats an ancestor Text maxLines clamp

AltStyle によって変換されたページ (->オリジナル) /