makeStyles: expansion of shorthands does not work sometimes

[layershifter]

Intro

When using a library that generates Atomic CSS such as Fela or Styletron, one can run into an issue where mixed shorthand and longhand properties are applied in an unexpected way due to the rendering order of CSS classes.

This packages helps to prevent those issues by always expanding shorthand values so that no conflicts occur at all.

https://github.com/robinweser/inline-style-expand-shorthand

As makeStyles() also uses Atomic CSS we have the same problem and we are using the same package to expand shorthands:

import { expandProperty } from "inline-style-expand-shorthand";

const longhands = expandProperty("padding", "10px 15px 5px");

// longhands === output
const output = {
  paddingTop: "10px",
  paddingRight: "15px",
  paddingBottom: "5px",
  paddingLeft: "15px"
};

Problem

It was not a big problem in Fluent N* as we don't use there CSS variables, with makeStyles() we have a problem there.

CSS variables cannot be expanded

There are cases when CSS variables cannot be expanded, for example background:

// Input:
const input = { padding: "var(--foo)" };
// Output
const output = {
  paddingTop: "var(--foo)",
  paddingRight: "var(--foo)",
  paddingBottom: "var(--foo)",
  paddingLeft: "var(--foo)"
};

This behavior is questionable as -foo can have different values:

--foo: 5px; /* can be expanded */
--foo: 5px 10px; /* cannot be expanded */

https://codesandbox.io/s/dank-hooks-r7jmr?file=/src/App.js

Not all shorthands are expanded

A list of shorthands supported by inline-style-expand-shorthand is very limited.

Complete list of shorthands based on MDN data

For example, background is not supported (robinweser/inline-style-expand-shorthand#5):

// Input
const input = { background: "var(--foo)" };
// Output
const output = { background: "var(--foo)" };

https://codesandbox.io/s/silent-flower-txe4u?file=/src/App.js

Some shorthands are not properly expanded

For example, flex, see robinweser/inline-style-expand-shorthand#14.

[layershifter]

We briefly discussed this with @miroslavstastny, we see following options.

⬇⬇⬇

(not an option) 1️⃣ Use Atomic CSS & fix expansion

The problem it's probably impossible. There are many cases where cannot expand (CSS variables that using compound values) or it will be too complex.

.a {
  --foo: 5px 10px;
}

// ⚠ if we will expand this we will get "paddingLeft: "5px 10px"
const input = { padding: "var(--foo)" };

2️⃣ Do not use Atomic CSS

We need expand properties to be able merge properly atomic CSS rules. Without expansion our behavior will be be deterministic:

/* Order of insertion can be different */
.a {
  padding-left: 5px;
}
.b {
  padding: 10px;
}

<-- ⚠ Applied padding depends on order of class in CSS i.e. insertion/rendering
order -->
<div>
  <div class="a" id="foo">
    <div class="b" id="bar">
      <div class="a b"></div>
    </div>
  </div>
</div>

microsoft/griffel#129 describes different problem, but the result of non-deterministic behavior will be the same: padding in this example depends on rendering order of #foo & #bar.

Pros

• Without Atomic CSS result will be more stable
• Smaller bundles

Cons

• We will not be able to create CSS extraction tool (i.e. extract styles to .css files)
• More runtime work: we will need to make the same thing as Emotion does

Implementation

Currently, i.e. before

• All runtime work i.e. insertion happens in useStylesBefore()
• mergeClasses() only merges classes

const useStylesBefore = makeStyles({
  base: { padding: "5px" },
  override: { paddingLeft: "10px" }
});
const classesBefore = useStylesBefore();

// 💅 CSS
// .a { padding-left: 5px }
// .b { padding-right: 5px }
// .c { padding-top: 5px }
// .d { padding-bottom: 5px }
// .e { padding-left: 10px }

mergeClasses(classesBefore.base, classesBefore.override); // ➡ "b c d e"

Without Atomic CSS, i.e. after

• Runtime work i.e. insertion happens in useStylesBefore()
• mergeClasses() only merges CSS declarations and performs insertion
• Will require stylis in our runtime (even with build time optimizations)

const useStylesAfter = makeStyles({
  base: { padding: "5px" },
  override: { paddingLeft: "10px" }
});
const classesAfter = useStylesBefore();

// 💅 CSS
// .base { padding: 5px }
// .override { padding-left: 10px }

const buttonClasses = mergeClasses(classesAfter.base, classesAfter.override); // ➡ "base_override"
// 💅 CSS .base_override { padding: 5px; padding-left: 10px }

const buttonClasses = mergeClasses(buttonClasses, classesAfter.override); // ➡ "base_override_override"
// ⚠ Yeah, it gets duplicated as CSS is not merged, it's concatenated
// 💅 CSS .base_override { padding: 5px; padding-left: 10px; padding-left: 10px }

3️⃣ Use Atomic CSS, fix expansion as we can, have restrictions

A middle ground solution: keep Atomic CSS, but introduce some restrictions.

For example, we cannot expand background, following options are possible:

• ban it in typings and do not insert it to CSS i.e. force customers to use longhands
• do not expand background, but warn customers/throw once mergeClasses() or makeStyles() gets longhands (backgroundColor) and shorthands (background) in the same chain

As we know that our CSS variables are not compound, we can write expansion for shorthands that is more specific.

[bsunderhus]

This issue should be solve by #20573 RFC, Next steps is implementing the RFC