mergeRules when combined with mergeLonghand can create the wrong rules

[adamwathan]

Apologies for not being able to reduce this to the absolute minimum reproduction, but given this CSS:

.has-errors .input {
  background-color: #fcebea;
  border-color: #cc1f1a;
}

.has-errors .checkbox-label {
  background-color: #fcebea;
  border-color: #cc1f1a;
}

.has-errors .checkbox-inline {
  background-color: #fcebea;
  border-color: #cc1f1a;
}

.error-banner .field-errors.filled {
  width: 100%;
  padding: 1.5rem 1.5rem 1rem;
  background-color: #fcebea;
  border-bottom-width: 1px;
  border-style: solid;
  border-color: #cc1f1a;
}

.error-banner .field-errors.filled   .field-error {
  width: 100%;
  font-size: .875rem;
  color: #22292f;
  line-height: 1.5;
  margin-bottom: .5rem;
}

When minified with mergeRules and mergeLonghand both enabled, the output is:

.error-banner .field-errors.filled,
.has-errors .checkbox-inline,
.has-errors .checkbox-label,
.has-errors .input {
    background-color: #fcebea;
    border-color: #cc1f1a
}

.error-banner .field-errors.filled {
    width: 100%;
    padding: 1.5rem 1.5rem 1rem;
    border-bottom: 1px;
    border-style: solid
}

.error-banner .field-errors.filled .field-error {
    width: 100%;
    font-size: .875rem;
    color: #22292f;
    line-height: 1.5;
    margin-bottom: .5rem
}

The important changes here are that the border-color declarations were grouped and the border-color declaration from the .error-banner .field-errors.filled selector was removed, and that what was originally border-bottom-width: 1px is simplified to border-bottom: 1px.

But because the border-color declaration was hoisted to the group at the top, the border color no longer takes effect for the .error-banner .field-errors.filled selector because border-bottom: 1px sets a default color of black which overrides it.

This sort of hand-crafted diff tries to make the important changes clear:

+ .error-banner .field-errors.filled,
  .has-errors .checkbox-inline,
  .has-errors .checkbox-label,
  .has-errors .input {
      background-color: #fcebea;
      border-color: #cc1f1a
  }
  
  .error-banner .field-errors.filled {
      width: 100%;
      padding: 1.5rem 1.5rem 1rem;
-     border-bottom-width: 1px;
+     border-bottom: 1px;
      border-style: solid
-     border-color: #cc1f1a
  }
  
  .error-banner .field-errors.filled .field-error {
      width: 100%;
      font-size: .875rem;
      color: #22292f;
      line-height: 1.5;
      margin-bottom: .5rem
  }

In this case the expected behavior is that because border-color was hoisted out, border-bottom-width cannot be simplified to border-bottom, or alternatively, the border-color declaration should not be removed from that selector at all.

Tricky stuff to get right for sure, I don't envy anyone trying to build a tool like this but am extremely grateful for your hard work 👍

[alexander-akait]

Oh, can we reduce example? I think problem in postcss-merge-longhand, he runs before postcss-merge-rules and create invalid border values so we have invalid merged rules

[alexander-akait]

To be honestly, i don't have enough time and funds :disappointed: to works a lot of time on this so feel free to send a PR

[adamwathan]

Cool thanks I will see if I can get it working and PR something! 👍

For others, here's the minimum reproduction:

.foo {
  border-color: #cc1f1a;
}

.bar {
  border-bottom-width: 1px;
  border-style: solid;
  border-color: #cc1f1a;
}

Compiles to:

.bar,
.foo {
    border-color: #cc1f1a
}

.bar {
    border-bottom: 1px;
    border-style: solid
}

Expected:

.foo {
  border-color: #cc1f1a;
}

.bar {
  border-bottom: 1px;
  border-style: solid;
  border-color: #cc1f1a;
}

This is related but possibly a different issue, but this CSS:

.foo {
  border-color: #cc1f1a;
}

.bar {
  border-bottom: 1px;
  border-color: #cc1f1a;
}

...compiles to:

.bar,
.foo {
    border-color: #cc1f1a
}

.bar {
    border-bottom: 1px
}

...which is also wrong, since it's important that the border-color declaration comes after border-bottom for the color to take affect.

Expected result is that it just doesn't change:

.foo {
  border-color: #cc1f1a;
}

.bar {
  border-bottom: 1px;
  border-color: #cc1f1a;
}

[alexander-akait]

@adamwathan Can you provide what you expected too for other developers?

[adamwathan]

Added expected behavior to the comment 👍

[tmorehouse]

@evilebottnawi I am seeing a similar issue (which may be related) when using the default preset:

The following CSS:

  background-position: center right calc(0.375em + 0.1875rem);

is being converted to:

  background-position: 100% right calc(0.375em + 0.1875rem)

which does not produce the same result visually.

But, by changing the order of the original css to the following:

  background-position: right calc(0.375em + 0.1875rem) center;

cssnano leaves it alone and doesn't mangle it.

I've opened a new issue https://github.com/cssnano/cssnano/issues/712

[alexander-akait]

@tmorehouse not related to this issue

[anikethsaha]

Input

.foo {
  border-color: #cc1f1a;
}

.bar {
  border-bottom-width: 1px;
  border-style: solid;
  border-color: #cc1f1a;
}

With postcss-merge-longhand only

.foo {
  border-color: #cc1f1a;
}

.bar {
  border-bottom: 1px;
  border-color: #cc1f1a;
  border-style: solid;
}

With postcss-merge-rules only

.foo {
  border-color: #cc1f1a;
}

.bar {
  border-bottom-width: 1px;
  border-style: solid;
  border-color: #cc1f1a;
}

With postcss-merge-longhand before postcss-merge-rules

.foo,.bar {
  border-color: #cc1f1a;
}

.bar {
  border-bottom: 1px;
  border-style: solid;
}

With postcss-merge-rules before postcss-merge-longhand

.foo {
  border-color: #cc1f1a;
}

.bar {
  border-bottom: 1px;
  border-color: #cc1f1a;
  border-style: solid;
}

Here, both plugins are working fine as independently but not together. The fix as expected is to have the merging of the rule before merging the longhand!

cc @evilebottnawi !

[ludofischer]

After running a quick test, I think #1450 might solve this