Skip to content

Commit bc86c6d

Browse files
skovhuscursoragent
andauthored
fix: avoid pseudo-selector duplicate for conditional borderColor + :focus override (#391)
* fix: avoid pseudo-selector duplicate when conditional and pseudo overrides target the same property Two related bugs caused 'the same pseudo selector or at-rule cannot be used more than once' for components like: styled.textarea` border: ${themedBorder('bgBorder')}; border-color: ${(p) => p.$hasError ? p.theme.color.redBase : undefined}; &:focus { border-color: ${(p) => p.$hasError ? p.theme.color.redBase : p.theme.color.controlPrimary}; } `; 1. `(p) => p.$x ? value : undefined` (or `null`/`false`/`""`) now resolves to a single positive variant bucket via `splitVariantsResolved*` instead of falling through to a dynamic style function, matching the semantics of `p.$x && value` and avoiding pseudo-map collisions. 2. When seeding `default` of a new pseudo map from an inherited base value, the inherited value's own `default` is now unwrapped instead of being nested, preventing malformed shapes like `{ default: { default: A, ":focus": B }, ":focus": C }`. Co-authored-by: Kenneth Skovhus <skovhus@users.noreply.github.com> * fix: drop unsafe negative-only variant lowering for `prop ? undefined : value` The negative-only branch added in the previous commit emitted a single `splitVariantsResolvedValue` variant with a `!prop` `when` key, but `handleSplitVariantsResolvedValue` treats negated entries as the unconditional default and applies them directly to the base `styleObj`. That silently dropped the `!prop` gate and produced always-on styles. Restrict `buildOneSidedVariantResult` to the positive-only direction (`prop ? value : undefined/null/false/""`) — which was the originally reported pattern — and let the inverse fall through to the dynamic style-function fallback. Add a regression test that fails if the unconditional-leak ever reappears. Addresses Codex review on PR #391. Co-authored-by: Kenneth Skovhus <skovhus@users.noreply.github.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
1 parent e9a907e commit bc86c6d

5 files changed

Lines changed: 203 additions & 5 deletions

File tree

src/__tests__/transform.test.ts

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2233,9 +2233,9 @@ export const App = () => <Box>Hello</Box>;
22332233
});
22342234

22352235
describe("conditional value handling", () => {
2236-
it("should bail when a boolean literal is used as a CSS value in conditional expression", () => {
2236+
it("emits a positive-only variant when alternate is `false` (omit-declaration sentinel)", () => {
22372237
// In styled-components, falsy interpolations like `false` mean "omit this declaration".
2238-
// We should bail rather than producing invalid CSS like `cursor: "false"`.
2238+
// We model this as a single positive variant bucket — equivalent to `$disabled && "not-allowed"`.
22392239
const source = `
22402240
import styled from "styled-components";
22412241
@@ -2252,7 +2252,38 @@ export const App = () => <Button $disabled>Click</Button>;
22522252
{ adapter: fixtureAdapter },
22532253
);
22542254

2255-
expect(result.code).toBeNull();
2255+
expect(result.code).not.toBeNull();
2256+
expect(result.code).toContain('cursor: "not-allowed"');
2257+
expect(result.code).not.toContain('cursor: "false"');
2258+
expect(result.code).not.toContain("cursor: false");
2259+
});
2260+
2261+
it("does not unconditionally apply the alternate when consequent is `undefined`", () => {
2262+
// `prop ? undefined : value` must NOT be lowered to a single `!prop`
2263+
// variant via splitVariantsResolved* — that path treats `!`-prefixed
2264+
// `when` strings as the unconditional default and would silently drop
2265+
// the gate. Falling back to a dynamic style function is the safe choice.
2266+
const source = `
2267+
import styled from "styled-components";
2268+
2269+
const Button = styled.button<{ $disabled?: boolean }>\`
2270+
cursor: \${(p) => (p.$disabled ? undefined : "pointer")};
2271+
\`;
2272+
2273+
export const App = () => <Button>Click</Button>;
2274+
`;
2275+
2276+
const result = transformWithWarnings(
2277+
{ source, path: "negative-only-variant.tsx" },
2278+
{ jscodeshift: j, j, stats: () => {}, report: () => {} },
2279+
{ adapter: fixtureAdapter },
2280+
);
2281+
2282+
expect(result.code).not.toBeNull();
2283+
// Guard against the regression where the value bleeds into an unconditional
2284+
// base style rule (which would set cursor: "pointer" for ALL <Button> uses,
2285+
// including when $disabled is true).
2286+
expect(result.code).not.toMatch(/cursor:\s*"pointer"\s*[,}]/);
22562287
});
22572288

22582289
it("should bail when true is used as a CSS value in conditional expression", () => {

src/internal/builtin-handlers/conditionals.ts

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -452,6 +452,37 @@ export function tryResolveConditionalValue(
452452
return branchToExpr(value);
453453
};
454454

455+
// Convert `prop ? value : undefined/null/false/""` into a positive-only
456+
// `splitVariantsResolved*` result (one variant bucket gated on `prop`).
457+
// Returns null when both branches resolve, or when the alternate isn't an
458+
// empty CSS sentinel — the caller continues with two-side handling or falls
459+
// back to a dynamic style function.
460+
//
461+
// We intentionally do NOT handle the inverse `prop ? undefined : value`
462+
// here: `splitVariantsResolved*` treats variants whose `when` starts with
463+
// `!` as the unconditional default (applied directly to `styleObj`), so
464+
// emitting a single negated variant would silently drop the `!prop` gate
465+
// and apply the value unconditionally. Inverse forms continue to be lowered
466+
// by the dynamic style-function fallback, which is correct (just less
467+
// optimal).
468+
const buildOneSidedVariantResult = (args: {
469+
cons: Branch;
470+
alt: Branch;
471+
alternate: unknown;
472+
truthyWhen: string;
473+
}): HandlerResult | null => {
474+
const { cons, alt, alternate, truthyWhen } = args;
475+
if (!cons || alt || !isEmptyCssBranch(alternate)) {
476+
return null;
477+
}
478+
const variants = [
479+
{ nameHint: "truthy" as const, when: truthyWhen, expr: cons.expr, imports: cons.imports },
480+
];
481+
return cons.usage === "props"
482+
? { type: "splitVariantsResolvedStyles", variants }
483+
: { type: "splitVariantsResolvedValue", variants };
484+
};
485+
455486
// Helper: resolve a 4-branch compound ternary once both the outer prop and inner prop
456487
// have been identified. Returns null if leaf branches can't all be resolved as "create".
457488
const tryBuildDualBranchResult = (outerProp: string, innerProp: string): HandlerResult | null => {
@@ -739,6 +770,21 @@ export function tryResolveConditionalValue(
739770
}
740771
}
741772

773+
// Positive-only variant for `prop ? value : undefined/null/false/""` —
774+
// styled-components treats falsy interpolations as "omit this declaration",
775+
// so model it as a single-side variant bucket rather than emitting a
776+
// dynamic style function (which would clash with pseudo overrides on the
777+
// same property elsewhere in the rule).
778+
const oneSided = buildOneSidedVariantResult({
779+
cons,
780+
alt,
781+
alternate,
782+
truthyWhen: outerProp,
783+
});
784+
if (oneSided) {
785+
return oneSided;
786+
}
787+
742788
if (!cons || !alt) {
743789
return buildRuntimeCallResult();
744790
}
@@ -792,6 +838,15 @@ export function tryResolveConditionalValue(
792838
: { type: "splitVariantsResolvedValue", variants };
793839
}
794840
}
841+
const oneSided = buildOneSidedVariantResult({
842+
cons,
843+
alt,
844+
alternate,
845+
truthyWhen: destructuredProp,
846+
});
847+
if (oneSided) {
848+
return oneSided;
849+
}
795850
if (!cons || !alt) {
796851
return buildRuntimeCallResult();
797852
}
@@ -841,6 +896,15 @@ export function tryResolveConditionalValue(
841896
}
842897
}
843898

899+
const oneSided = buildOneSidedVariantResult({
900+
cons,
901+
alt,
902+
alternate,
903+
truthyWhen: resolvedProp,
904+
});
905+
if (oneSided) {
906+
return oneSided;
907+
}
844908
if (!cons || !alt) {
845909
return buildRuntimeCallResult();
846910
}

src/internal/lower-rules/interpolated-variant-resolvers.ts

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,9 @@ export function handleSplitVariantsResolvedValue(ctx: SplitVariantsContext): boo
267267
registerImports(parsed.imports, resolverImports);
268268

269269
// Get or create a condition map (pseudo/media) for a property, preserving the default value.
270+
// When the inherited base value is itself a pseudo/media map (e.g., styleObj already has
271+
// `{ default: A, ":focus": B }` from an earlier rule), flatten its `default` so we don't
272+
// produce a malformed nested map like `{ default: { default: A, ":focus": B } }`.
270273
const getOrCreateConditionMap = (prop: string): Record<string, unknown> => {
271274
const existing = target[prop];
272275
const map =
@@ -275,7 +278,7 @@ export function handleSplitVariantsResolvedValue(ctx: SplitVariantsContext): boo
275278
: ({} as Record<string, unknown>);
276279
if (!("default" in map)) {
277280
const baseValue = existing ?? styleObj[prop];
278-
map.default = baseValue ?? null;
281+
map.default = unwrapConditionMapDefault(baseValue);
279282
}
280283
target[prop] = map;
281284
return map;
@@ -539,9 +542,10 @@ export function handleSplitMultiPropVariantsResolvedValue(ctx: SplitVariantsCont
539542
: ({} as Record<string, unknown>);
540543
// Set default from target first, then fall back to base styleObj.
541544
// Only use null if neither has a value (for properties like outlineStyle that need explicit null).
545+
// unwrapConditionMapDefault prevents nesting an inherited pseudo map under `default`.
542546
if (!("default" in map)) {
543547
const baseValue = existing ?? styleObj[stylexPropMulti];
544-
map.default = baseValue ?? null;
548+
map.default = unwrapConditionMapDefault(baseValue);
545549
}
546550
for (const ps of pseudos) {
547551
map[ps] = parsed.exprAst as any;
@@ -744,3 +748,30 @@ export function handleDualBranchCompoundVariantsResolvedValue(ctx: SplitVariants
744748
decl.needsWrapperComponent = true;
745749
return true;
746750
}
751+
752+
// --- Non-exported helpers ---
753+
754+
/**
755+
* When seeding `default` of a new pseudo/media map from an inherited base value,
756+
* collapse a base value that is itself a pseudo/media map down to its `default`
757+
* entry. Without this, layering a new pseudo override (e.g. variant bucket)
758+
* would produce a malformed nested map like
759+
* `{ default: { default: A, ":focus": B }, ":focus": C }`, which StyleX rejects
760+
* with "the same pseudo selector or at-rule cannot be used more than once".
761+
*
762+
* Returns `null` (the StyleX "no override" sentinel) when no usable default is
763+
* present.
764+
*/
765+
function unwrapConditionMapDefault(value: unknown): unknown {
766+
if (!value || typeof value !== "object" || Array.isArray(value) || isAstNode(value)) {
767+
return value ?? null;
768+
}
769+
const map = value as Record<string, unknown>;
770+
if ("default" in map) {
771+
return map.default ?? null;
772+
}
773+
// Object without a `default` key — treat as no inherited base value.
774+
// (Bare pseudo-only maps like { ":focus": X } never appear here in practice
775+
// because callers always seed `default` first.)
776+
return null;
777+
}
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
// Repro: Same pseudo selector or at-rule cannot be used more than once
2+
// Pattern: dynamic borderColor override at default scope (ternary), then borderColor again under :focus
3+
import styled from "styled-components";
4+
import { themedBorder } from "./lib/helpers";
5+
6+
const JsonTextarea = styled.textarea<{ $hasError?: boolean }>`
7+
border: ${themedBorder("bgBorderFaint")};
8+
border-color: ${(props) => (props.$hasError ? props.theme.color.greenBase : undefined)};
9+
border-radius: 6px;
10+
11+
&:focus {
12+
outline: none;
13+
border-color: ${(props) =>
14+
props.$hasError ? props.theme.color.greenBase : props.theme.color.controlPrimary};
15+
}
16+
`;
17+
18+
export const App = () => (
19+
<div style={{ display: "flex", flexDirection: "column", gap: 8, padding: 16 }}>
20+
<JsonTextarea defaultValue="default" />
21+
<JsonTextarea $hasError defaultValue="error" />
22+
</div>
23+
);
Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
import * as React from "react";
2+
import * as stylex from "@stylexjs/stylex";
3+
import { pixelVars, $colors } from "./tokens.stylex";
4+
5+
type JsonTextareaProps = { hasError?: boolean } & React.ComponentProps<"textarea">;
6+
7+
function JsonTextarea(props: JsonTextareaProps) {
8+
const { children, hasError, ...rest } = props;
9+
return (
10+
<textarea
11+
{...rest}
12+
sx={[styles.jsonTextarea, styles.jsonTextareaBorder, hasError && styles.jsonTextareaHasError]}
13+
>
14+
{children}
15+
</textarea>
16+
);
17+
}
18+
19+
export const App = () => (
20+
<div style={{ display: "flex", flexDirection: "column", gap: 8, padding: 16 }}>
21+
<JsonTextarea defaultValue="default" />
22+
<JsonTextarea hasError defaultValue="error" />
23+
</div>
24+
);
25+
26+
const styles = stylex.create({
27+
jsonTextarea: {
28+
borderRadius: 6,
29+
borderColor: {
30+
default: null,
31+
":focus": $colors.controlPrimary,
32+
},
33+
outline: {
34+
default: null,
35+
":focus": "none",
36+
},
37+
},
38+
jsonTextareaBorder: {
39+
borderWidth: pixelVars.thin,
40+
borderStyle: "solid",
41+
borderColor: $colors.bgBorderFaint,
42+
},
43+
jsonTextareaHasError: {
44+
borderColor: {
45+
default: $colors.greenBase,
46+
":focus": $colors.greenBase,
47+
},
48+
},
49+
});

0 commit comments

Comments
 (0)