stylex icon indicating copy to clipboard operation
stylex copied to clipboard

[babel-plugin] StyleX adds logical properties classes when it should not

Open zaydek opened this issue 1 year ago • 3 comments

Describe the issue

Related to https://github.com/facebook/stylex/issues/752.

This was very confusing for me to discover. I believe StyleX has a bug when replacing logical properties.

See the following code for example:

import * as stylex from "@stylexjs/stylex";
import { CSSPropertiesWithExtras } from "@stylexjs/stylex/lib/StyleXTypes";

const styles = stylex.create({
  idea: (args: { width?: CSSPropertiesWithExtras["width"] }) => ({
    width: args.width,
  }),
});

export function Idea() {
  return (
    <div {...stylex.props(styles.idea({ width: 64 }))}>
      <div {...stylex.props(styles.idea({}))}>Hello</div>
    </div>
  );
}

Output (this is correct)::

<div class="idea__styles.idea width-x1bl4301" style="--width: 64px;">
  <div class="idea__styles.idea">Hello</div>
</div>

Now see what happens when we use inline-size instead of width:

import * as stylex from "@stylexjs/stylex";
import { CSSPropertiesWithExtras } from "@stylexjs/stylex/lib/StyleXTypes";

const styles = stylex.create({
  idea: (args: { inlineSize?: CSSPropertiesWithExtras["inlineSize"] }) => ({
    inlineSize: args.inlineSize,
  }),
});

export function Idea() {
  return (
    <div {...stylex.props(styles.idea({ inlineSize: 64 }))}>
      <div {...stylex.props(styles.idea({}))}>Hello</div>
    </div>
  );
}

Output (I believe this is incorrect):

<div class="idea__styles.idea width-x1avlqir" style="--inlineSize: 64px;">
  <div class="idea__styles.idea width-x1avlqir">Hello</div>
                                ^^^^^^^^^^^^^^
</div>

When using logical properties, it looks like StyleX is adding the class when it should not, because in this case inlineSize is undefined and therefore the class should be omitted altogether. This was causing me a lot of problems because it took me a while to figure out it seems to be a side effect of logical properties and not all properties.

I'll try disabling styleResolution: 'legacy-expand-shorthands' and see if this problem goes away on its own.

Expected behavior

See above

Steps to reproduce

See above

Test case

No response

Additional comments

No response

zaydek avatar Dec 05 '24 08:12 zaydek

Follow up: It doesn't look like the Vite plugin I'm using vite-plugin-stylex touches styleResolution which would lead me to believe it is already set to the default setting 'application-order' as described by the docs. So this seems to be potentially an isolated error related to logical properties and not styleResolution, I think.

zaydek avatar Dec 05 '24 08:12 zaydek

This is a known issue. This was originally done to intentionally disallow vertical writing modes and to normalize style properties that are safe to do so for LTR and RTL "horizontal" layouts. I.e. Unless you're using vertical writing modes, inline-size and width are equivalent.

The rationale for this decision was to minimize the total size of the CSS created and maximize CSS rule re-usability.

Another reason was the fact that it is possible to polyfill horizonal logical styles but not vertical writing modes on browsers that don't support logical properties natively.


We will soon fix this problem and officially start supporting vertical writing modes. The support for logical properties in browsers is widespread and we don't need to worry about polyfills anymore.

nmn avatar Dec 05 '24 12:12 nmn

OK thanks. Sounds good.

zaydek avatar Dec 06 '24 02:12 zaydek