Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Bug]: Carbon CSS has syntax errors #17824

Open
2 tasks done
wkeese opened this issue Oct 21, 2024 · 7 comments
Open
2 tasks done

[Bug]: Carbon CSS has syntax errors #17824

wkeese opened this issue Oct 21, 2024 · 7 comments
Labels
package: styles @carbon/styles severity: 3 https://ibm.biz/carbon-severity type: bug 🐛

Comments

@wkeese
Copy link
Contributor

wkeese commented Oct 21, 2024

Package

@carbon/styles

Browser

Chrome

Package version

1.67

React version

18

Description

Carbon's CSS -- node_modules/@carbon/styles/css/styles.css and also node_modules/@carbon/ibm-products/css/index-full-carbon.css -- has syntax errors.

  1. Mathematical calculations outside of calc():
  --temp-1lh: (
    var(--cds-body-compact-01-line-height, 1.28572) * 1em
  );
  --temp-expressive-1lh: (
    var(--cds-body-compact-02-line-height, 1.375) * 1em
  );
  --temp-1lh: (
    var(--cds-body-01-line-height, 1.42857) * 1em
  );
  1. In @carbon/ibm-products' CSS files, references to SCSS variables:
--c4p--page-header--navigation-buffer-top: $spacing-06;
--c4p--page-header--navigation-buffer-top: $spacing-06;

Reproduction/example

N/A

Steps to reproduce

Load node_modules/@carbon/styles/css/styles.css and also node_modules/@carbon/ibm-products/css/index-full-carbon.css in your IDE. For me, I loaded in Webstorm. The problems should also be detectable with stylelint but that prints thousands of errors.

I also wrote some Node code to test loading that CSS, but it doesn't seem to mind or flag the errors. I'll leave the code here anyway for reference:

const fs = require("node:fs");
const jsdom = require("jsdom");
const { JSDOM } = jsdom;

const carbonCss = fs.readFileSync('node_modules/@carbon/ibm-products/css/index-full-carbon.css', 'utf8');

// JSDOM can't [yet] handle CSS with @charset or with has().
// has() is supposed to be working though, see https://stackoverflow.com/questions/71490461/jsdom-and-pseudoclass-has.
const adjustedCarbonCSS = carbonCss.replace(/(@charset.*?\n)|(:has\(.*?\))/g, "");

const { window } = new JSDOM(`
<html>
  <head>
    <style>${adjustedCarbonCSS}</style>
  </head>
  <body class="cds--list-box__menu">
    <p>Hello world</p>
  </body>
</html>
`);

console.log(window.getComputedStyle(window.document.body).display);

Suggested Severity

None

Application/PAL

IKC

Code of Conduct
@sstrubberg sstrubberg moved this to 🕵️‍♀️ Triage in Design System Dec 10, 2024
@tay1orjones
Copy link
Member

Thanks for reporting - looks like this is part of the button mixin and related to the contextual layout tokens

carbon/packages/styles/scss/components/button/_mixins.scss

Lines 26 to 42 in 66c14c2

--temp-1lh: (
#{custom-property.get-var(
'body-compact-01-line-height',
map.get($body-compact-01, line-height)
)} * 1em
);
--temp-expressive-1lh: (
#{custom-property.get-var(
'body-compact-02-line-height',
map.get($body-compact-02, line-height)
)} * 1em
);
// -1px to compensate for border width
--temp-padding-block-max: calc(
(#{custom-property.get-var('layout-size-height-lg')} - var(--temp-1lh)) / 2 -
#{convert.to-rem(1px)}
);

@tay1orjones tay1orjones added severity: 3 https://ibm.biz/carbon-severity package: styles @carbon/styles and removed status: needs triage 🕵️‍♀️ labels Dec 10, 2024
@tay1orjones tay1orjones moved this from 🕵️‍♀️ Triage to ⏱ Backlog in Design System Dec 10, 2024
@wkeese
Copy link
Contributor Author

wkeese commented Dec 10, 2024

Thanks, I agree that's for (1).

For (2) it seems related to https://github.com/carbon-design-system/ibm-products/blob/5b5e681f7c52c0950419a9081df497cb2033791d/packages/ibm-products-styles/src/components/PageHeader/_page-header.scss#L124 but should I file it separately under https://github.com/carbon-design-system/ibm-products/issues?

@janhassel
Copy link
Member

janhassel commented Dec 11, 2024 •
edited
Loading

None of these are technically syntax errors themselves. CSS custom properties are typeless by default and can contain anything you want. That's why (2) is occurring: sass shouldn't replace $spacing-06 as it is a valid value for a custom property.

Issues can only stem from where/how you use it. For (1) I can elaborate on the original background: These custom properties are only parts of calculations which aren't intended to be used by themselves but only within other calculations. Example:

calc((layout.size('height') - var(--temp-1lh)) / 2 - convert.to-rem(1px)),

Given that CSS supports nested calcs (calc(2rem * calc(50px + 1em))) it's probably safe to wrap all occurrences in calc() just to be sure. What do you think, @tay1orjones?

For (2), it needs to be interpolated explicitly: --some-custom-property: #{$spacing-06};, so probably best to open a ticket (or PR) in the respective repo.

@wkeese
Copy link
Contributor Author

wkeese commented Dec 12, 2024 •
edited
Loading

Woops, you are right.

My real issue was that this syntax was breaking JSDOM, precluding me from properly testing accessibility in a Node environment. Or at least, that's what it seemed like, but (as shown above) I had trouble reducing to a simple test case.

I filed carbon-design-system/ibm-products#6575 about the issue in @carbon/ibm-products.

But as you said, neither of these things are technically syntax errors. So feel free to close the ticket if you want. But adding calc() wrappers for those --temp CSS variables might make the code clearer.

Maybe at some point, I will come back to the Node problem, and see if adding the calc() wrapper works around it.

@wkeese
Copy link
Contributor Author

wkeese commented Dec 12, 2024

PS: Since those CSS variables are global -- or at least, they are visible to everything in the Carbon CSS rollup file -- shouldn't they have more unique names, like --button-temp-1lh?

@janhassel
Copy link
Member

I only see them scoped in the .cds--btn (and another --temp-1lh in .cds--contained-list-item__content) selectors.

Image

Can you share the bundle?

@wkeese
Copy link
Contributor Author

wkeese commented Dec 13, 2024

Oh you're right, I forgot that variables can be scoped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
package: styles @carbon/styles severity: 3 https://ibm.biz/carbon-severity type: bug 🐛
Projects
Design System
Status: ⏱ Backlog
Milestone
No milestone
Development

No branches or pull requests
3 participants
@wkeese @tay1orjones @janhassel