-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(core): render plain Link anchors inline so an ancestor Text clamp can truncate them - #6043
fix(core): render plain Link anchors inline so an ancestor Text clamp can truncate them #6043parinith-web wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Hi @parinith-web!
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!
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.
aed06ab to
04ece17
Compare
@cixzhang
cixzhang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Existing block and self-clamped Links lose keyboard focus visibility in this change. Please preserve their focusable box while keeping only unclamped plain anchors inline.
[Reviewed by Robohands]
Fixes #6021
Summary
Builds on @ManoharPaturi's fix in #6038, which correctly identified and fixed the root cause: Link's plain anchor was
display: inline-flex, whichestablishes its own formatting context that an ancestor-webkit-line-clamp(from<Text maxLines>) cannot reach into — so<Text maxLines={2}><Link>...</Link></Text>silently failed to truncate.His fix changes the plain anchor to
display: inlinewhile preservinginline-flexwhere it's actually load-bearing (external-linkicon + gap, and the button-rendered form), via a dedicated
flexLayoutstyle.This PR adds one follow-up fix on top of that: it addresses the Copilotreview comment left on #6038 about the ne
Link.test.tsxhelper,displayDeclarationsFor, which matched an element's classList against rawselector text usingString.includes- asubstring check on hashed atomicclass names (StyleX-generated, e.g.
x1a2b3c). A shorter class name can bea substring of anunrelated longer one, so the check could both false-positive and false-negative depending on what else StyleX emitted for that test run.
Change
Rewrote
displayDeclarationsForto walk the real CSSOM instead of rawtext: parses eachCSSStyleRule.selectorTextinto class tokens, compares those tokens against the element's actualclassListby exact match, and reads the declaration viarule.style.getPropertyValue('display')rather than a body-text regex. Also descends into grouping rules (e.g.@media) so nested declarations aren't missed.Testing
vitest run packages/core/src/Link packages/core/src/Text— all passing (Link: 39, useLinkComponent: 11, useLinkify: 13, Text: 32, plus TextArea/TextInput which depend on Text — 252 total)tsc --noEmit(core) — clean