csso icon indicating copy to clipboard operation
csso copied to clipboard

Strange restructurization of properties

Open underoot opened this issue 7 years ago • 4 comments

I've this stylesheet:

.block__elem1 {
	width: 100%;
	height: 100%;
	display: flex;

        padding: 20px;
}

.block__elem2 {
	width: 20rem;
        color: #fff;
}

.block__elem2 {
	width: 100%;
        height: 100%;
	display: flex;
}

After converting it through CSSO with restructure enabled I have:

.block__elem1,.block__elem2 {
    width: 100%;
    height: 100%;
    display: flex
}

.block__elem1 {
    padding: 20px
}

.block__elem2 {
    width: 20rem;
    color: #fff
}

But expected:

.block__elem1,.block__elem2 {
    width: 100%;
    height: 100%;
    display: flex
}

.block__elem1 {
    padding: 20px
}

.block__elem2 {
    color: #fff
}

underoot avatar May 16 '17 16:05 underoot

You're right. The reason of this behaviour a rem unit used for block_elem2. rem unit (and some others) has a different support across browsers, therefore it doesn't mix with other unit. After first step (initialMergeRuleset) we get just two rules:

.block__elem1 {
    width: 100%;
    height: 100%;
    display: flex;
    padding: 20px
}

.block__elem2 {
    width: 20rem;
    color: #fff;
    width: 100%;
    height: 100%;
    display: flex
}

Since width property for block__elem2 has two values with different support it doesn't merge. On last step (restructRuleset) rules are merging and we get the result you've provided. It isn't just a non-optimal minification, but also wrong transformation that's break original CSS. So it's a bug actually.

There are few related issues. I'm going to fix it but can't give a promise when. Sorry.

Anyway, your example would be useful. Thank you.

lahmatiy avatar May 16 '17 16:05 lahmatiy

Thank you for response and explanation!

underoot avatar May 16 '17 16:05 underoot

Hi,

I can confirm the bug with rem.

p {
  margin-top: 1rem;
}

* {
  margin: 0;
  padding: 0;
}

p {
  margin: 0;
} 

compiles to:

*, p {
  margin: 0;
}

p {
  margin-top: 1rem;
}

* {
  padding: 0;
}

This is a wrong behaviour, as rules order is modified, margin-top for p will be 1rem instead of 0.

If we replace rem with px:

p {
  margin-top: 1px;
}

* {
  margin: 0;
  padding: 0;
}

p {
  margin: 0;
} 

it compiles to:

*, p {
  margin: 0;
}

* {
  padding: 0;
}

I hope this will help to find a fix

jonathan-vallet avatar Jul 05 '17 12:07 jonathan-vallet

@jonathan-vallet Your case a little different. After initial merge we get

p {
  margin-top: 1rem;
  margin: 0;
}

* {
  margin: 0;
  padding: 0;
}

And CSSO should merge margin's declarations, but didn't because of rem unit is special. But in this case it doesn't the case and merge is safe. I believe I can fix it asap (top-right-bottom-left optimisation step is easy to fix that in general).

lahmatiy avatar Jul 05 '17 13:07 lahmatiy