Property merging works incorrectly if the properties are not declared consecutively

[asapach]

When background properties are declared one after another, the minified result is correct:

.btn-primary {
  border-color: #005A84;
  background: #0083BF;
  background: linear-gradient(to bottom,  #0083BF 0%,#005A84 100%);
}

.btn-primary{border-color:#005A84;background:#0083BF;background:linear-gradient(to bottom,#0083BF 0,#005A84 100%)}

However, if they are separated by another property declaration, the result is broken:

.btn-primary {
  background: #0083BF;
  border-color: #005A84;
  background: linear-gradient(to bottom,  #0083BF 0%,#005A84 100%);
}

.btn-primary{border-color:#005A84;background:linear-gradient(to bottom,#0083BF 0,#005A84 100%)}

Notice that the fallback background declaration is missing.

cleancss -v 2.0.7

[asapach]

Seems to be broken in 2.0 and works correctly in 1.1 releases: tested against 1.1.7 and got expected results.

[GoalSmashers]

You can also disable advanced processing in 2.0 to make it work correctly.

Thank you for the head ups. We'll do our best to fix it with 2.0.8 ASAP.

On 23 Jan 2014, at 16:36, Aliaksei Sapach notifications@github.com wrote:

Seems to be broken in 2.0 and works correctly in 1.1 releases: tested against 1.1.7 and got expected results.

—
Reply to this email directly or view it on GitHub.

[asapach]

Thanks, noAdvanced does indeed provide a valid workaround.

[GoalSmashers]

This will very likely be solved in clean-css 2.1 where a more advanced overriding of CSS will be available.
Currently the only assumption we make is that consecutive declarations of the same property are made on purpose but ones with mixed order are simply redeclarations. Take such one for example:

a { background: #fff; }
...
a { color:red; background: linear-gradient(...) }

[asapach]

The assumption doesn't seem to be a safe one, because it breaks the existing stylesheets. In your example, if the latter background declaration would replace the former one, it would break older browsers that don't support linear-gradient.

[GoalSmashers]

That's why it is important to follow the conventions and group redefined properties. You can always switch it off with --skip-advanced / noAdvanced. But improvements are coming.

[asapach]

I'd rather prefer the tool warn me about my bad css, than assume that all developers follow the same conventions. IMHO, advanced processing should be either off by default, or it should be very strict and throw a bunch of warnings when the conventions are not being followed.

[tomByrer]

throw a bunch of warnings when the conventions are not being followed

good idea

[GoalSmashers]

Warnings were not really in the timeframe for 2.0 release. Let's see if we can do without them in 2.1 or whether should we add them.

[Venemo]

Hi @asapach :)

I'm working on a new, shiny property optimizer. Once it gets merged, things will get better. For example, it leaves intact the following kind of structure:

.first{background:#fff;background:rgba(1,2,3.4)}
.second{background:url(aaa);background:linear-gradient(bbb)}

The philosophy is the following: the opimizer knows about how understandable each value is for browsers; and less understandable values do not override more understandable values. This is the same way that browsers work. For example, it's capable of optimizing

.something{background:#fff;background:rgba(1,2,3.4);background-image:url(aaa);background-image:linear-gradient(bbb)}

into

.something{background:#fff url(aaa);background:rgba(1,2,3.4) linear-gradient(bbb)}

At this moment, it can successfully do this between the same kind of properties (eg. compare different background-color values with each other), however it can't yet do this accross different components of shorthands. So it can't yet tell that a particular background-image value (such as linear-gradient) is less understandable than a particular background-color value.

Once the new optimizer is merged, we're going to be able to tackle these kind of issues very easily.

Some background information:
http://stackoverflow.com/questions/21723729/whats-the-standard-way-to-deal-with-unsupported-css-expressions

[asapach]

@Venemo, that's a very ambitious goal. You will effectively have to replicate all bugs and features of all [major] browsers to determine which declarations are valid. And when a new browser version comes out and introduces a new feature, your optimizer might break. That's a slippery slope.

[Venemo]

@asapach Yes, I didn't say it's easy, but it's not too difficult :) Additionally to what I said, values that the optimizer doesn't understand are always considered less understandable, so it won't break if new standards are introduced.

[Venemo]

@asapach If you feel like taking a look at how it is now, take a look at my fork here: https://github.com/Venemo/clean-css - the usage / API is unchanged. I would welcome some early feedback :)

[tomByrer]

@Venemo What branch please?

[Venemo]

@tomByrer The default branch (master) - where you can see my face in the commit logs :)

[Venemo]

@tomByrer For the moment, I deleted my fork and re-forked because GitHub started messing up after the rebase. So you can't see my patch online at the moment.

@asapach Good news: I added a special-case hack to make things like background:#xxx;background:whatever() work as expected. Better support for it is coming soon. (The hack won't work for more complex cases.)

For now, we'd like to focus on getting the new optimizer landed in upstream master, and then it'll be a lot easier to add improvements like this.

[Venemo]

I've managed to slip this into the new optimizer. Opening pull request now. :)

[Venemo]

Some details:

The optimizer had already known the concept of understandability. Of course it doesn't know what kind of things each browser supports, but it has a good knowledge on whether something can override something else or not - this is the same mechanism that I implemented for #168. I added additional checks to make sure that this applies to whole shorthands as well.

[GoalSmashers]

@Venemo is it fixed? It seems to still give the wrong output.

[Venemo]

@GoalSmashers The new optimizer supports this case, yes. If I comment out the old optimizer entirely, the result is correct. I haven't looked into the old optimizer enough to tell why it does this.

[Venemo]

@GoalSmashers maybe it's a good idea to remove properties from the old optimizer that are already supported in the new one?

By the way, while I was looking at it, I found an unrelated issue in the old optimizer.

    'list-style-image': ['list'],
    'list-style-position': ['list'],
    'list-style-type': ['list'],

This is wrong because there is no such property as list. I think it should be list-style instead.

[GoalSmashers]

Ok, so we need to fix it in master to close this issue. Let me take a look at it.

Good catch with list-style. No idea how it came through...

[Venemo]

@GoalSmashers The easiest way to do it is to replace the old property optimizer entirely. This "only" requires the implementation of the same logic within processable - since the logic of the old optimizer was quite simple, its behaviour shouldn't be too hard to replicate.

I can do this next weekend (march 8th-9th) but don't have any time for it until then.

[GoalSmashers]

@Venemo If it only performs as good as the current one and all tests pass, then 👍 Take your time though, no rush!

[GoalSmashers]

In case of any issues in 2.2 please use --skip-aggressive-merging switch to turn off merging based on order. Correct merging for all properties will be implemented in #290 and clean-css 2.4.

[GoalSmashers]

@Venemo Fixed list-style-* in 831a1fe.

[Venemo]

@GoalSmashers What was wrong with it?

[GoalSmashers]

See your own comment: #210 (comment)

[Venemo]

Ah, right. Thanks :)