CSS rule ordering can break mixed usage of shorthand and non shorthand properties

[esamattis]

I'm having hard time reproducing this but it seems that this does not produce predictable results.

Consider following base styles:

const noPadding = css({padding: 0});
const leftPadding = css({paddingLeft: 100});

When combined like:

const combined = css(leftPadding, noPadding);

I would expect that following css would be generated:

.css-12345 {
    padding-left: 100px;
    padding: 0px;
}

but it seems that sometimes this is generated:

.css-12345 {
    padding: 0px;
    padding-left: 100px;
}

Which will results in different layout. I suspect that this is because glamor relies on object key ordering which is not guaranteed in Javascript really. But glamor has the ordering information from the css(leftPadding, noPadding) call so it should be possible enforce the order with glamor.

Has this been already thought out in glamor or do I just have bug in my code? :)

[otbe]

I think the problem arises from mixed usage of the "shorthand" and "extended" version of padding. We had several bugs related to this fact with margin, padding and so on. glamor merges two rules by object assignment for simple properties, so only two identical keys will be merged properly.

Maybe glamor should be more optioniated about what key is merged cc @donaldpipowitch, @ChristopherBiscardi.

In the meantime you can use padding-left, padding-right, padding-top, padding-bottom. This had solved the issue for us :)

[esamattis]

You are absolutely right and I'm right now using that exact workaround.

But when using the the css(leftPadding, noPadding) form this issue could be mitigated if the css would be generated using that ordering information.

But of course when doing something like this:

css({paddingLeft: 100, padding: 0});

Things get a lot trickier. Maybe glamor could normalize the shorthand rules? But that would be quite a lot work since there are quite a few of those in css...

Radium seems to just warn when using those: https://github.com/FormidableLabs/radium/blob/v0.18.1/src/plugins/check-props-plugin.js

[donaldpipowitch]

Agree with this. This is a big gotcha which needs to be addressed.

• We could try to disallow shorthands...
• We could try to normalize shorthands...
• We could try to be smart about the ordering...
• ...other solutions?

I probably would prefer something which would behave like normal CSS (e.g. allowing shorthands and don't normalize them).

[esamattis]

I'm not sure about normalizing since it makes debugging a harder because in that case what you see in the devtools would be different from what was written.

Personally I would prefer that glamor would just disallow mixed short and long forms in a single definition like css({paddingLeft: 100, padding: 0}) but allow them when combining multiple ones because in that case it is possible to do the right thing.

This is important for react-simple because it would allow style overriding nicely.

const BorderedButton = simple("button", {
  paddingLeft: 100,
  border: "1px solid black",
});

// Keep the border but remove padding
const NoPaddingButton = simple(BorderedButton, {
    padding: 0,
});

[otbe]

Mh...maybe a two step solution?

1. for glamor 2.x warn if it notice mixed usage of shorthand/extended version (like radium)
2. for glamor 3 solve this problem somehow

I guess solving the problem by keeping track of the order is very tricky, because key ordering in JS objects cannot be treated as stable as you mentioned already. I think we have to change the data structure to accomplish this :/

[esamattis]

Mh...maybe a two step solution?

I like this. React does this too. First warn and then break after couple versions if really needed.

I guess solving the problem by keeping track of the order is very tricky, because key ordering in JS objects cannot be treated as stable as you mentioned already. I think we have to change the data structure to accomplish this :/

I haven't looked into glamor internals too much yet but I guess it means that instead of passing one object to the css generator an array of objects must be passed.

[otbe]

I haven't looked into glamor internals too much yet but I guess it means that instead of passing one object to the css generator an array of objects must be passed.

Exactly :)

[donaldpipowitch]

I guess solving the problem by keeping track of the order is very tricky, because key ordering in JS objects cannot be treated as stable as you mentioned already.

Maybe we can treat it as stable and put a disclaimer in the readme? It looks like all JS engines currently in usage do keep the key order, if no numeric key is used (which isn't the case for CSS properties anyway 👍 ). It looks like even React relies on this behaviour in Keyed Fragments (see the last sentence).

See also this SO answer which is very interesting:

ES2015 has an order in certain cases.

• integer-like keys in ascending order
• normal keys in insertion order
• Symbols in insertion order

[ChristopherBiscardi]

Maybe glamor should be more optioniated about what key is merged

Maybe, changing the data structure probably makes more sense though. I've looked into using Maps in the API, which would solve the problem but as far as I looked doesn't provide a super nice DX (objects are real nice but maybe the css template string is the winner long term). Maybe a babel plugin could rewrite objects to Maps or something (presumably the AST will maintain ordering). I'm at the point there where I have to write the implementation here to get any more information and will likely do so soonish (Maybe a css.Map([['key1', 'value1'], ['key2', 'value2']]) would work for example.)

Aphrodite merged a similar PR to offer a Map based API in addition to the object one which yields the following:

const styles = StyleSheet.create({
    ordered: new Map([
        ["margin", 0],
        ["marginLeft", 15],
    ]),
});

Also prior art here is Aphrodite's object key ordering docs, which say basically the same stuff we're saying here.

---

@otbe @donaldpipowitch Is this something we want to look into for v3? If so I'll bump the priority up in my queue to work on.

---

It looks like all JS engines currently in usage do keep the property order

I'm pretty sure Chrome doesn't guarantee that, but could be wrong.

[donaldpipowitch]

Is this something we want to look into for v3?

I'd say yes. However I'd like to convert our code base to TypeScript first and start every new development with TypeScript as a base for easier refactoring. (But that's just my opinion.) Maybe I could start with that this week.

[donaldpipowitch]

Fat finger. Sorry for closing the issue...

[esamattis]

Maybe we can treat it as stable and put a disclaimer in the readme?

I've actually seen the order change in glamor with my apps in just Chrome. Lastly today. I just cannot extract it out as a standalone example. Not sure what really causes it. So I would not treat it stable.

Maybe a babel plugin could rewrite objects to Maps or something (presumably the AST will maintain ordering)

As a third party lib author building on top glamor I don't see this very appealing because babel plugins tend to stop working when the code gets wrapped. They require very specific way of writing code to work.

[esamattis]

Hmm, I think the issue for me comes from code like this:

css(styleObject1, styleObject2, styleObject3, styleObject4)

but this is the fixable case.

[otbe]

Mh while thinking about this issue I may prefer warning for shorthand/extended in one object and decomposing while merging of multiple objects is a better idea then struggling around with order or an entire new order stable API :/

[ChristopherBiscardi]

@epeli We already have a separated case in tests I believe for shorthand/longhand bug, which manifests in Chrome.

As a third party lib author building on top glamor I don't see this very appealing because babel plugins tend to stop working when the code gets wrapped.

yeah, that's fair. We'd just have to expose the new API I think anyway.

@otbe I'm not sure that's good enough for this. If you look at the test added at #162 and the corresponding issue #158 you'll see the font shorthand drop below fontSize even though it's being merged in a new object.

[otbe]

@ChristopherBiscardi the problem in your example is that font: inherit can't be decomposed properly (and glamor does not decompose at all ATM). In case of margin, padding etc it should work like:

css({
   padding: 0,
   paddingLeft: '100px'
}) 
// --> console.warn

css({ padding: 0 }, 
    { paddingLeft: '100px' }) 
// equals after decomposing shorthand
css({paddingRight: 0, paddingLeft: 0, paddingTop: 0, paddingBottom: 0}, 
    {paddingLeft: '100px'}) 

// {paddingRight: 0, paddingLeft: '100px', paddingTop: 0, paddingBottom: 0}

My point is not to depend on any order instead use object merging (its just simpler :))
Maybe we should implement something like console.warn for mixed shorthand/extended and wait for user feedback!?

[ChristopherBiscardi]

Yeah we should definitely be warning about this regardless :)

Simplest solution seems like warning for any of the following shorthand properties in development, explaining that they can't be combined with other properties easily.

background, font, margin, border, border-top, border-right, border-bottom, border-left, border-width, border-color, border-style, transition, animation, transform, padding, list-style, border-radius

[paulmolluzzo]

This is also affects border-top-color (and related directions). Example here: https://codesandbox.io/s/qY3G75453

[paulmolluzzo]

Update: The example I listed above (https://codesandbox.io/s/qY3G75453) doesn't break in Safari or FF.

FF:

Safari:

Chrome:

Have we checked if there's a correlation between server- and client-side rendering affecting this issue? Have we checked that the other issues were only broken in Chrome?

---

Edit: There is a correlation with SSR and client rendering: SSR and rehydration will also break Safari and FF. Looking at the rendered CSS and the web inspectors it comes back to the same ordering issue.

[threepointone]

Don't think glamor should handle this frankly. It's a quirk with css, best if one doesn't use the shorthand at all.

[esamattis]

There's also problem with media queries.

This is brittle:

var style = css({
  width: 200,
  "@media(max-width: 500px)": {
    width: 100,
  }
});

if for some reason the media query css is generated before the plain width style it does not have any affect.

Although most browsers do preserve the object key order this is not a theoretical issue. When doing something like:

var style = css({
  width: 200,
  height: 100,
  ...someOtherStyles 
});

It can and will mess up the ordering at some point.

It would be nice if there where an api which would guarantee the generated css order. Like:

var style = css(
 {width: 200},
 {
  "@media(max-width: 500px)":  {width: 100}
 },
  myOtherStyles
);

[emmatown]

Edit: css already accepts multiple args as styles.

css accepts an array so you can just do this

var style = css([
 {width: 200},
 {
  "@media(max-width: 500px)":  {width: 100}
 },
  myOtherStyles
]);

[esamattis]

@mitchellhamilton I known but that does not guarantee the order. Key 7 is special case in V8 which can be used to test the ordering bugs.

This

const box = css([
    {width: 100, height: 100},
    {backgroundColor: "red"},
    {7: "foo"},
]);

will generate following style string:

.css-8u8zqa,[data-css-8u8zqa]{7:foo;width:100px;height:100px;background-color:red;}

[threepointone]

'Plain' styles will always be generated before media queries according to our algorithm.

[esamattis]

Oh, cool.

But that does not end there: What about the order of multiple media queries?

[threepointone]

I don't understand the point of using 7 as a key, it's not a valid selector or prop name

[esamattis]

I was just using it as proof that the array does not guarantee the order even thou it looks like it would.

[threepointone]

There're clearly trade offs with using an object to denote a sequence of rules. Indeed there are plenty of tradeoffs to this library itself. There might not be good solutions to most of them. Lemme know if you find an actual bug, and if there's no good workaround.

[esamattis]

I'll try to isolate one... It's quite hard actually. Couple of times I've hit really weird bugs in real world apps with media queries which I've solved just by refactoring the way I've merged the style objects.

[esamattis]

I think I got it. There's an issue when a media query is being overridden by the same media query.

Example: https://codesandbox.io/s/qjG2XvykR

In real world code I've hit this with glamorous when I have wanted to extend a component and change it's media query behaviour.

Example: https://codesandbox.io/s/VPAvP6mGO

It think it comes down to the fact that when a key is being overridden in an object it's position stays the same.

For example here foo is the first key even thou it's the last one set.

> Object.keys(Object.assign({foo: 1}, {bar: 2}, {foo: 3}))
[ 'foo', 'bar' ]

So far I've tested this only with V8.

There are obvious workarounds (like adding extra spaces in the media queries) for this bug but that's not really the issue. The issue is that it's really hard pin point when it happens. I've wasted hours on this.

Fixing this would also make the infamous shorthand properties less brittle in glamorous. Ie. this would work as expected https://codesandbox.io/s/jzgxvZov

[threepointone]

Noted. can you make a fresh issue specifically for this? It's not related to the shorthand properties issue, so don't want to reopen this. Thanks!

[esamattis]

Here #307