Skip to content

Conversation

aklkv
Copy link
Collaborator

@aklkv aklkv commented Aug 17, 2025

📝 Summary of CSS Issues & Recommendations

🚨 What Happened

  • While migrating Embroider’s CSS minification from csso to Lightning CSS, builds started failing.
  • Error reported:
    SyntaxError: Invalid state
    data: { type: 'SelectorError', value: { type: 'InvalidState' } }
    
  • Investigation showed the generated CSS contained selectors like:
    .hds-form-file-input:disabled::file-selector-button::before { … }
  • This is invalid CSS — spec forbids chaining two pseudo-elements (::file-selector-button::before).

🔍 Root Cause

  • In SCSS, the hds-button-state-disabled() mixin includes:
    &::before {
      border-color: transparent;
    }
  • When applied inside a &::file-selector-button block, it compiles to ::file-selector-button::before.
  • Browsers silently drop these rules, so the issue wasn’t obvious until a strict parser (Lightning CSS) enforced the spec.

⚡ Impact

  • Browsers: Invalid rules are ignored, so styles may not apply as intended (e.g., border-color: transparent never hits).
  • Build tooling: Strict parsers/minifiers (Lightning CSS, future PostCSS upgrades, etc.) fail builds instead of ignoring bad selectors.
  • Developer experience: Downstream consumers (e.g. Embroider, Cloud UI) cannot adopt modern tooling unless this is fixed upstream.

✅ Recommendations

1. Fix the mixin

  • Option A: Add a flag to disable pseudo-element emission.

    @mixin hds-button-state-disabled($include-before: true) {
      color: var(--token-color-foreground-disabled);
      background-color: var(--token-color-surface-faint);
      border-color: var(--token-color-border-primary);
      box-shadow: none;
      cursor: not-allowed;
    
      @if $include-before {
        &::before { border-color: transparent; }
      }
    }

    Use with $include-before: false for ::file-selector-button.

  • Option B: Split into two mixins (*-base and *-decorators) and include only what’s valid in each context.

2. Add validation in the build pipeline

Since the library uses rollup-plugin-scss, add strict CSS validation before publishing:

  • Lightning CSS parse check
    After SCSS compiles, run:

    transform({
      code: Buffer.from(css, 'utf8'),
      filename: filePath,
      minify: false,        // parse-only
      errorRecovery: false, // fail on first invalid selector
    });

    This fails fast if invalid CSS is generated.

  • Stylelint rule for chained pseudo-elements
    Custom rule: flag any ::<something>::<something> in selectors.
    Catches misuse at the SCSS source before it compiles.

3. CI integration

  • Run validation (Lightning CSS or Stylelint) in CI for every SCSS change.
  • Fail the build if invalid selectors appear in compiled CSS.
  • This prevents regressions and ensures library consumers don’t hit the same problem.

📌 Takeaway

  • The design system SCSS mixin is generating invalid CSS when reused with pseudo-elements.
  • Browsers mask the issue, but modern tooling correctly enforces the spec and fails.
  • Fixing the mixin + adding early validation (Rollup + Stylelint/Lightning CSS parse) will make the library spec-compliant, robust, and unblock consumers moving to modern build pipelines.

🤔 Why clean-css and csso didn’t fail (but Lightning CSS does)

Older/legacy-focused minifiers like clean-css (and historically csso) primarily optimize for byte reduction and throughput, not strict standards enforcement. In practice this means:

  • Lenient parsing philosophy – If something looks odd or unknown, they typically don’t stop the build. They either pass it through verbatim or quietly drop it, assuming browsers will ignore invalid bits anyway.
  • Back-compat with messy CSS – The ecosystem used to tolerate “quirky” outputs from preprocessors (Sass/Less) and templating pipelines. Strict parsing would have broken lots of builds, so forgiveness was a feature.
  • No heavy transforms that require full correctness – Since they aren’t doing modern syntax lowering/transpilation, they don’t need a fully spec-valid AST the way a modern transformer does.
  • Practical goal: keep shipping – The cost of failing production builds on a questionable selector was considered higher than letting it through, because browsers already ignore invalid selectors.

By contrast, Lightning CSS is designed for spec compliance and safe transforms (e.g., prefixing, modern syntax → widely supported output). To guarantee correctness it must trust the AST, so it throws on invalid CSS, like a chained pseudo-element selector (::file-selector-button::before).

Quick comparison

Tool Default posture What happens on invalid selector?
clean-css Lenient minifier Usually passes through or drops silently
csso Mostly lenient minifier Similar behavior; doesn’t typically hard-fail
Lightning CSS Spec‑strict transformer/minifier Errors (or warns with errorRecovery: true)

Implication for us: With Lightning CSS (and generally modern pipelines), invalid CSS should be fixed at the source. If we relied on lenient tools before, they may have masked issues that were already ineffective in browsers.

📸 Screenshots

Screenshot 2025-08-16 at 6 24 33 PM

🔗 External links

Jira ticket: HDS-XXX
Figma file: [if it applies]


👀 Component checklist

💬 Please consider using conventional comments when reviewing this PR.

📋 PCI review checklist
  • If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
  • If applicable, I've worked with GRC to document the impact of any changes to security controls.
    Examples of changes to controls include access controls, encryption, logging, etc.
  • If applicable, I've worked with GRC to ensure compliance due to a significant change to the in-scope PCI environment.
    Examples include changes to operating systems, ports, protocols, services, cryptography-related components, PII processing code, etc.

@aklkv aklkv self-assigned this Aug 17, 2025
Copy link

vercel bot commented Aug 17, 2025

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

Project Deployment Preview Updated (UTC)
hds-showcase Ready Ready Preview Sep 4, 2025 9:14am
hds-website Ready Ready Preview Sep 4, 2025 9:14am

@aklkv aklkv marked this pull request as ready for review August 20, 2025 07:37
@aklkv aklkv requested a review from a team as a code owner August 20, 2025 07:37
@aklkv
Copy link
Collaborator Author

aklkv commented Aug 25, 2025

This PR is failing correctly as there is obvious error in CSS so it means my safeguard works correctly. There are several ways to fix the issue but I will leave it ip to @hashicorp/hds-engineering to decide how to correctly address it

@didoo
Copy link
Contributor

didoo commented Aug 26, 2025

For context: it may be that the &::before { border-color: transparent; } declaration is not needed at all (something to double check, I just did a quick check in some of the instances of the showcase)

@aklkv
Copy link
Collaborator Author

aklkv commented Sep 4, 2025

@didoo let me know if you stil want this safe guard at all or any other options would works as outlined above

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.

3 participants