-
Notifications
You must be signed in to change notification settings - Fork 323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Property merging works incorrectly if the properties are not declared consecutively #210
Comments
Seems to be broken in |
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.
|
Thanks, |
This will very likely be solved in clean-css 2.1 where a more advanced overriding of CSS will be available.
|
The assumption doesn't seem to be a safe one, because it breaks the existing stylesheets. In your example, if the latter |
That's why it is important to follow the conventions and group redefined properties. You can always switch it off with |
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. |
good idea |
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. |
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:
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
into
At this moment, it can successfully do this between the same kind of properties (eg. compare different Once the new optimizer is merged, we're going to be able to tackle these kind of issues very easily. Some background information: |
@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. |
@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. |
@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 :) |
@Venemo What branch please? |
@tomByrer The default branch (master) - where you can see my face in the commit logs :) |
@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 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. |
I've managed to slip this into the new optimizer. Opening pull request now. :) |
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. |
@Venemo is it fixed? It seems to still give the wrong output. |
@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. |
@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.
This is wrong because there is no such property as |
Ok, so we need to fix it in master to close this issue. Let me take a look at it. Good catch with |
@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 I can do this next weekend (march 8th-9th) but don't have any time for it until then. |
@Venemo If it only performs as good as the current one and all tests pass, then 👍 Take your time though, no rush! |
Using `--skip-aggressive-merging` / `noAggressiveMerging` switch skips property merging based on order. Will be fixed in #290.
In case of any issues in 2.2 please use |
@GoalSmashers What was wrong with it? |
See your own comment: #210 (comment) |
Ah, right. Thanks :) |
When
background
properties are declared one after another, the minified result is correct:However, if they are separated by another property declaration, the result is broken:
Notice that the fallback
background
declaration is missing.cleancss -v
2.0.7
The text was updated successfully, but these errors were encountered: