facebook / stylex

StyleX is the styling system for ambitious user interfaces.
https://stylexjs.com
MIT License
8.41k stars 310 forks source link

Overlapping styles with different order conditions break "last style wins" #460

Closed marklawlor closed 9 months ago

marklawlor commented 9 months ago

Describe the issue

If you have two styles with overlapping conditions, but in different order, the "last style wins" rule is broken and the styling reverts back to CSS specificity order.

I know this seems like a "just don't this" issue. But this occurred in a more complex scenario and it wasn't immediately obvious what was happening.

Expected behavior

"last style wins" should still apply

Steps to reproduce

import { css, html } from 'react-strict-dom';

export function Test() {
  return (
    <>
      <html.div style={[styles.red, styles.blue]}>I should be blue</html.div>
      <html.div style={[styles.blue, styles.red]}>I should be red</html.div>
    </>
  );
}

const styles = css.create({
  red: {
    ':active': {
      ':hover': {
        color: 'red'
      }
    }
  },
  blue: {
    ':hover': {
      ':active': {
        color: 'blue'
      }
    }
  }
});

Test case

No response

Additional comments

Suggested fix: I think this could be solved by a simple priority system for conditions. Either sorted alphabetically or using a bespoke order.

nmn commented 9 months ago

This is not a supported pattern in StyleX and the StyleX ESLint rule would warn you when using this.

Instead, you must treat all pseudo classes and media queries as "conditions" within the value of a CSS property:

const styles = css.create({
  red: {
    color: {
      default: null,
      ':active': {
        default: null,
        ':hover': 'red',
      }
    }
  },
  blue: {
    color: {
      default: null,
      ':hover': {
        default: null,
        ':active': 'blue',
      }
    }
  },
});

Only the last applied style will win in this case.


NOTE: Since you're using react-strict-dom, know that the project is very early and there are still gaps in the React-Native implementation. If you see any RN-specific issues, report them there. Any web-specific issues should be reported here.

marklawlor commented 9 months ago

Thanks for the quick response

That appears to have fixed the minimal reproducible example. Here is a modified example where the issue still occurs.

// app.js
import { css, html } from 'react-strict-dom';
import { theme } from './theme.stylex';

export function Test() {
  return (
    <>
      <html.div style={[styles.red, theme.blue]}>test</html.div>
      <html.div style={[theme.blue, styles.red]}>test</html.div>
    </>
  );
}

const styles = css.create({
  red: {
    color: {
      default: null,
      ':active': {
        default: null,
        ':hover': 'red',
      }
    }
  },
});
// theme.stylex.js
import { css, html } from 'react-strict-dom';

export const theme = css.create({
  blue: {
    color: {
      default: null,
      ':hover': {
        default: null,
        ':active': 'blue',
      }
    }
  },
});

Yep, just testing some projects out with react-strict-dom :+1: I believe this is a web-specific issue

marklawlor commented 9 months ago

StyleX ESLint rule would warn you when using this

It would be nice if this wasn't the case.

Utility Classes Atomic utility class names like Tailwind CSS and Tachyons rely on conventions and lint rules to ensure that conflicting class names are not applied on the same element.

Like the docs say, the problem with Tailwind CSS is that it requires a linter to avoid conflicting class footguns. It would be nice if StyleX wasn't hindered by this as well.

nmn commented 9 months ago

@marklawlor We will make this is a compile error in the future.

Also, in your new repro, what is the behaviour that you're seeing? Also web or RN?


It would be nice if StyleX wasn't hindered by this as well.

We have currently chosed to mix styles in a way that feels like merging objects. If you think of it that way, styles should merge in the way you expect. So far, other being more strict to disallow the unsupported pattern, no other solution to this problem seems viable.

marklawlor commented 9 months ago

I believe is a web only issue.

The generated HTML and CSS. The result is that both elements are red when the :active:hover condition is met.

<div class="html-div dom__styles.block xe8uvvx x1ghz6dp x1717udv test__styles.red xa2ikkt theme__theme.blue x1nkz3uh">
  I should be blue
</div>
<div class="html-div dom__styles.block xe8uvvx x1ghz6dp x1717udv theme__theme.blue x1nkz3uh test__styles.red xa2ikkt">
  I should be red
</div>
.x1nkz3uh:active:hover:not(#\#):not(#\#):not(#\#) {
    color: blue;
}

.xa2ikkt:active:hover:not(#\#):not(#\#):not(#\#) {
    color: red;
}
marklawlor commented 9 months ago

Actually, it appears I had a stale Metro cache :man_facepalming: This may have been an issue with Metro Web :sweat_smile: