css / csso

CSS minifier with structural optimizations
https://css.github.io/csso/csso.html
MIT License
3.76k stars 187 forks source link

overflow-x and overflow-y properties overridden by combining of rules #267

Open dancoates opened 8 years ago

dancoates commented 8 years ago

Just ran into this issue. When given this input:

.item {
    overflow: visible;
    overflow-x: hidden;
}

.item2 {
    overflow: visible;
}

csso will produce this output:

.item{overflow-x:hidden}.item,.item2{overflow:auto}

Where the second rule overrides the first one and produces a different result to the unoptimized css.

lahmatiy commented 8 years ago

Yep, that's a most annoying structural optimisation problem for now. It's not overflow specific but common problem.There are already several similar issues here (most earliest is #143). I'm going to fix it soon. I suggest to switch off structural optimisations as temporary solution. Pass restructuring: false into settings or --restructure-off flag if you're using a CLI.

dancoates commented 8 years ago

Ah ok. I can imagine that it must be a nightmare to consider all cases around the inconsistencies of css property naming.

I did try a few other cases like setting margin: 0; then margin-top: 20px; in place of overflow: visible; and overflow-x: hidden; and couldn't reproduce, but I guess they may have been saved by other optimizations rewriting the css before reaching to this one.

Feel free to close this as a dupe :)

dancoates commented 8 years ago

For what it is worth, cssnano seems to have the exact same issue: https://github.com/ben-eb/cssnano/issues/113

lahmatiy commented 8 years ago

Yes, it' s hard sometimes. But in this case an old algorithm problem that doesn't take in account declarations order on block merging. As you mention other optimisators has the same or related issues. margin and some other properties (like 'padding', 'border') are collapsing if possible. That's why you cann't to reproduce issue with margin. I keep those issues open to use their examples for test cases when problem will be fixed.

apipkin commented 8 years ago

I ran into a similar problem with this and a display:flex fallback to a display:table-cell. Because the optimizer took:

.list {
    display: table-cell;
    display: flex;
}
.list--master {
    display: flex
}

and merged them into

.list, .list--master {
    display: flex;
}
.list {
    display: table-cell;
}

instead of preserving the order.

Keeping an eye on order with similar property prefixes when restructuring may be a good way to go. This would prevent property fallbacks from becoming the norm. (Probably way easier said than done...)

lahmatiy commented 8 years ago

Yep, preserving declaration order is planned for next release. It's in development now.

nitriques commented 8 years ago

I had the same problem with

cursor: pointer;
cursor: hand;

The latter won. At least, I do not have to support this anymore and could remove hand ;)