Skip to content
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.

Sign up for GitHub

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

Jump to bottom

mergeRules when combined with mergeLonghand can create the wrong rules #701

Open
adamwathan opened this issue Jan 30, 2019 · 9 comments · May be fixed by #886
Open

mergeRules when combined with mergeLonghand can create the wrong rules #701

adamwathan opened this issue Jan 30, 2019 · 9 comments · May be fixed by #886
Labels
bug
Projects
merge-rules bugs
Milestone
backlog

Comments

@adamwathan
Copy link

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
Copy link
Member

alexander-akait commented Jan 30, 2019 •
edited

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
Copy link
Member

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

@adamwathan
Copy link
Author

adamwathan commented Jan 30, 2019 •
edited

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
Copy link
Member

alexander-akait commented Jan 30, 2019 •
edited

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

@adamwathan
Copy link
Author

Added expected behavior to the comment 👍

@alexander-akait alexander-akait added this to the 4.1 milestone Jan 31, 2019
@tmorehouse

This comment has been minimized.

Sign in to view
@tmorehouse tmorehouse mentioned this issue Feb 16, 2019
Closed
@alexander-akait
Copy link
Member

@tmorehouse not related to this issue

mizunashi-mana added a commit to mizunashi-mana/blog that referenced this issue Apr 21, 2019
@mizunashi-mana
Disable mergeRules on cssnano
a233641 
See cssnano/cssnano#701
@anikethsaha
Copy link
Member

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 !

@anikethsaha anikethsaha linked a pull request Mar 3, 2020 that will close this issue
Open
@Splaktar Splaktar mentioned this issue Mar 6, 2020
Closed
@ludofischer ludofischer modified the milestones: 4.1, backlog Mar 9, 2021
@ludofischer ludofischer mentioned this issue Apr 2, 2021
Open
@ludofischer ludofischer removed this from To do in merge-longhand bugs Apr 28, 2021
@ludofischer ludofischer added this to To do in merge-rules bugs Apr 28, 2021
@ludofischer ludofischer mentioned this issue Nov 4, 2022
Merged
@ludofischer
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
merge-rules bugs
To do
Milestone
backlog
Development

Successfully merging a pull request may close this issue.

fix: merge-rule before longhand in default preset anikethsaha/cssnano
5 participants
@ludofischer @tmorehouse @adamwathan @alexander-akait @anikethsaha