Comments cause parent selector output repetition #285

argyleink · 2022-03-03T23:28:37Z

Bug description

Source CSS

progress {
  --_track: hsl(228 100% 90%);
  --_track-size: min(10px, 1ex);
  --_progress: hsl(228 100% 50%);
  --_radius: 1e3px;
  --_indeterminate-track: linear-gradient(to right,
    var(--_track) 45%,
    var(--_progress) 0%,
    var(--_progress) 55%,
    var(--_track) 0%
  );
  --_indeterminate-track-size: 225% 100%;
  --_indeterminate-track-animation: progress-loading 2s infinite ease;
  
  /*  reset  */
  appearance: none;
  border: none;
  
  /*  custom style  */
  position: relative;
  height: var(--_track-size);
  border-radius: var(--_radius);
  overflow: hidden;

  @media (prefers-color-scheme: dark) {
    --_track: hsl(228 20% 30%);
    --_progress: hsl(228 100% 75%);
  }

  &:focus-visible {
    outline-color: var(--_progress);
  }
  
  /*  Safari/Chromium  */
  &[value]::-webkit-progress-bar {
    background-color: var(--_track);
  }
  
  &[value]::-webkit-progress-value {
    background-color: var(--_progress);
    transition: inline-size .25s ease-out;
  }
  
  /*  Firefox  */
  &[value]::-moz-progress-bar {
    background-color: var(--_progress);
  }
  
  /*  indeterminate  */
  &:indeterminate::after {
    content: "";
    inset: 0;
    position: absolute;
    background: var(--_indeterminate-track);
    background-size: var(--_indeterminate-track-size);
    background-position: right; 
    animation: var(--_indeterminate-track-animation);
  }
  
  /*  indeterminate Safari  */
  &:indeterminate::-webkit-progress-bar {
    background: var(--_indeterminate-track);
    background-size: var(--_indeterminate-track-size);
    background-position: right; 
    animation: var(--_indeterminate-track-animation);
  }
  
  /*  indeterminate Firefox  */
  &:indeterminate::-moz-progress-bar {
    background: var(--_indeterminate-track);
    background-size: var(--_indeterminate-track-size);
    background-position: right; 
    animation: var(--_indeterminate-track-animation);
  }
  
  /*  complete  */
  &:not([max])[value="1"]::before,
  &[max="100"][value="100"]::before {
    content: "✓";
    
    position: absolute;
    inset-block: 0;
    inset-inline: auto 0;
    display: flex;
    align-items: center;
    padding-inline-end: max(calc(var(--_track-size) / 4), 3px);

    color: white;
    font-size: calc(var(--_track-size) / 1.25);
  }
}

Expected CSS

progress {
  --_track: hsl(228 100% 90%);
  --_track-size: min(10px, 1ex);
  --_progress: hsl(228 100% 50%);
  --_radius: 1e3px;
  --_indeterminate-track: linear-gradient(to right,
    var(--_track) 45%,
    var(--_progress) 0%,
    var(--_progress) 55%,
    var(--_track) 0%
  );
  --_indeterminate-track-size: 225% 100%;
  --_indeterminate-track-animation: progress-loading 2s infinite ease;
  
  /*  reset  */
  -webkit-appearance: none;
     -moz-appearance: none;
          appearance: none;
  border: none;
  
  /*  custom style  */
  position: relative;
  height: var(--_track-size);
  border-radius: var(--_radius);
  overflow: hidden
}

@media (prefers-color-scheme: dark) {
progress {
    --_track: hsl(228 20% 30%);
    --_progress: hsl(228 100% 75%)
}}
progress:focus-visible {
    outline-color: var(--_progress);
  }
  /*  Safari/Chromium  */
progress[value]::-webkit-progress-bar {
    background-color: var(--_track);
  }
progress[value]::-webkit-progress-value {
    background-color: var(--_progress);
    -webkit-transition: inline-size .25s ease-out;
    transition: inline-size .25s ease-out;
  }
progress {
  
  /*  Firefox  */
}progress[value]::-moz-progress-bar {
    background-color: var(--_progress);
  }progress {
  
  /*  indeterminate  */
}progress:indeterminate::after {
    content: "";
    inset: 0;
    position: absolute;
    background: var(--_indeterminate-track);
    background-size: var(--_indeterminate-track-size);
    background-position: right; 
    -webkit-animation: var(--_indeterminate-track-animation); 
            animation: var(--_indeterminate-track-animation);
  }

  /*  indeterminate Safari  */
progress:indeterminate::-webkit-progress-bar {
    background: var(--_indeterminate-track);
    background-size: var(--_indeterminate-track-size);
    background-position: right; 
    -webkit-animation: var(--_indeterminate-track-animation); 
            animation: var(--_indeterminate-track-animation);
  }
  
  /*  indeterminate Firefox  */
progress:indeterminate::-moz-progress-bar {
    background: var(--_indeterminate-track);
    background-size: var(--_indeterminate-track-size);
    background-position: right; 
    animation: var(--_indeterminate-track-animation);
  }
  
  /*  complete  */
progress[value="1"]:not([max])::before,
  progress[max="100"][value="100"]::before {
    content: "✓";
    
    position: absolute;
    inset-block: 0;
    inset-inline: auto 0;
    display: flex;
    align-items: center;
    -webkit-padding-end: max(calc(var(--_track-size) / 4), 3px);
            padding-inline-end: max(calc(var(--_track-size) / 4), 3px);

    color: white;
    font-size: calc(var(--_track-size) / 1.25);
  }@-webkit-keyframes progress-loading {
  50% {
    background-position: left; 
  }
}@keyframes progress-loading {
  50% {
    background-position: left; 
  }
}

Actual CSS

progress {
  --_track: hsl(228 100% 90%);
  --_track-size: min(10px, 1ex);
  --_progress: hsl(228 100% 50%);
  --_radius: 1e3px;
  --_indeterminate-track: linear-gradient(to right,
    var(--_track) 45%,
    var(--_progress) 0%,
    var(--_progress) 55%,
    var(--_track) 0%
  );
  --_indeterminate-track-size: 225% 100%;
  --_indeterminate-track-animation: progress-loading 2s infinite ease;
  
  /*  reset  */
  -webkit-appearance: none;
     -moz-appearance: none;
          appearance: none;
  border: none;
  
  /*  custom style  */
  position: relative;
  height: var(--_track-size);
  border-radius: var(--_radius);
  overflow: hidden
}@media (prefers-color-scheme: dark) {progress {
    --_track: hsl(228 20% 30%);
    --_progress: hsl(228 100% 75%)
}
  }progress:focus-visible {
    outline-color: var(--_progress);
  }progress {
  
  /*  Safari/Chromium  */
}progress[value]::-webkit-progress-bar {
    background-color: var(--_track);
  }progress[value]::-webkit-progress-value {
    background-color: var(--_progress);
    -webkit-transition: inline-size .25s ease-out;
    transition: inline-size .25s ease-out;
  }progress {
  
  /*  Firefox  */
}progress[value]::-moz-progress-bar {
    background-color: var(--_progress);
  }progress {
  
  /*  indeterminate  */
}progress:indeterminate::after {
    content: "";
    inset: 0;
    position: absolute;
    background: var(--_indeterminate-track);
    background-size: var(--_indeterminate-track-size);
    background-position: right; 
    -webkit-animation: var(--_indeterminate-track-animation); 
            animation: var(--_indeterminate-track-animation);
  }progress {
  
  /*  indeterminate Safari  */
}progress:indeterminate::-webkit-progress-bar {
    background: var(--_indeterminate-track);
    background-size: var(--_indeterminate-track-size);
    background-position: right; 
    -webkit-animation: var(--_indeterminate-track-animation); 
            animation: var(--_indeterminate-track-animation);
  }progress {
  
  /*  indeterminate Firefox  */
}progress:indeterminate::-moz-progress-bar {
    background: var(--_indeterminate-track);
    background-size: var(--_indeterminate-track-size);
    background-position: right; 
    animation: var(--_indeterminate-track-animation);
  }progress {
  
  /*  complete  */
}progress[value="1"]:not([max])::before,
  progress[max="100"][value="100"]::before {
    content: "✓";
    
    position: absolute;
    inset-block: 0;
    inset-inline: auto 0;
    display: flex;
    align-items: center;
    -webkit-padding-end: max(calc(var(--_track-size) / 4), 3px);
            padding-inline-end: max(calc(var(--_track-size) / 4), 3px);

    color: white;
    font-size: calc(var(--_track-size) / 1.25);
  }@-webkit-keyframes progress-loading {
  50% {
    background-position: left; 
  }
}@keyframes progress-loading {
  50% {
    background-position: left; 
  }
}.button-group {
  display: flex;
  flex-flow: row wrap;
  gap: var(--size-3);
}label {
  display: flex;
  align-items: baseline;
  gap: 1ch;
}main {
  display: grid;
  gap: var(--size-3);
  padding: var(--size-3);
  max-inline-size: 80vw;
}body {
  display: grid;
  align-content: center;
  justify-content: center;
  place-content: center;
}

Does it happen with `npx @csstools/csstools-cli <plugin-name> minimal-example.css`?

No response

Debug output

No response

Extra config

No response

What plugin are you experiencing this issue on?

PostCSS Nesting

Plugin version

7.4.2

What OS are you experiencing this on?

No response

Node Version

17.1.0

Validations

Follow our Code of Conduct
Check that there isn't already an issue that request the same feature to avoid creating a duplicate.

Would you like to open a PR for this bug?

I'm willing to open a PR

The text was updated successfully, but these errors were encountered:

Antonio-Laguna · 2022-03-04T06:13:29Z

@argyleink thanks for reporting this!

I've narrowed it down to this minimum case:

.foo {
	/*  Baz  */
	&.bar {
		background-color: red;
	}
}

Which produces:

.foo {
	/*  Baz  */
}
.foo.bar {
		background-color: red;
	}

This is mostly due to cleanup-parent not cleaning up given it contains a comment node. My initial instinct was to move the comment along with the new declaration. @romainmenke what do you think about this?

romainmenke · 2022-03-04T07:26:13Z

Tricky one 🤔

Technically it is correct and some authors might even expect exactly the current behaviour.
The comment is not located within the nested rule but in the parent and the parent should not be modified by postcss-nesting.

The tricky bit is that we can't differentiate between a leading or a trailing comment :

.foo {
	/* #region: Base styles */
	color: red;
	width: 20px;
	/* #endregion: Base styles  */
	&.bar {
		/* #region: Specific styles */
		color: red;
		width: 20px;
		/* #endregion: Specific styles  */
	}
}

Would become :

.foo {
	/* #region: Base styles */
	color: red;
	width: 20px;
}

.foo.bar {
	/* #endregion: Base styles  */
	/* #region: Specific styles */
	color: red;
	width: 20px;
	/* #endregion: Specific styles  */
}

romainmenke · 2022-03-04T16:54:19Z

@argyleink Is this a blocking issue for you?
Currently I am leaning towards not fixing this.

it's technically correct
a CSS minifier would strip these

I can be convinced to fix this if a well defined heuristic can be found.

argyleink · 2022-03-04T17:01:11Z

not blocking, just noise. i'm commenting out my comments during dev so it's not noise in devtools..

can efforts be made to avoid producing empty selectors? i'm not worried about where the comments go, it's more the side effects and cruft produced by using comments inside nesting. some part of the plugin script is generating a new selector for the parent and then not putting anything into it, then inserting it into the stylesheet. could the selector be verified to contain rules before insertion? i get leaning onto minifiers, but it'd be cleaner if the plugin didnt produce them at all.

romainmenke · 2022-03-04T17:13:35Z

can efforts be made to avoid producing empty selectors?

This is done for truly empty selectors, but selectors with a comment node are not empty for PostCSS.

Some plugins also listen to comments to change behaviour half way through a file : https://github.com/postcss/autoprefixer#control-comments

some part of the plugin script is generating a new selector

The order is preserved as this is important.
For example :

.foo {
	color: red;

	&:where(:hover) {
		width:10px;
	}

	width: 20px;
}

width: 20px can not be moved up as then it wouldn't override width: 10px;.
This CSS is non-sensical, but there will be valid use cases that will break if we change the order.

To preserve the order we must insert duplicates of the parent selector.
If the parent is empty this is skipped. But a comment counts as "not empty".

Side note : We tried to enforce the spec here and disallowed declarations after nested rules. This backfired as there is too much sugary syntax that is a declaration in PostCSS first but then expanded to a rule later. We reverted this enforcement.

If we had been able to add this enforcement it would be trivial to count all comments of this kind as belonging to the nested selector.

more reading on this : csstools/postcss-nesting#95 (comment)

i get leaning onto minifiers, but it'd be cleaner if the plugin didnt produce them at all.

If we can define a good heuristic we can do this.
But currently I see no way to differentiate trailing and leading comments as illustrated in my example above :/

argyleink · 2022-03-04T17:23:31Z

We tried to enforce the spec here and disallowed declarations after nested rules. This backfired as there is too much sugary syntax that is a declaration in PostCSS first but then expanded to a rule later. We reverted this enforcement.

yeah, i expect this to be a gotcha once folks switch from scss/postcss nesting to built-in. totally coo if you kept it 👍🏻

To preserve the order we must insert duplicates of the parent selector.
If the parent is empty this is skipped. But a comment counts as "not empty".

proposal: if a selector is only containing a comment, replace the selector and comment with just the comment?

progress {
  
  /*  indeterminate Firefox  */
}progress:indeterminate::-moz-progress-bar {
    background: var(--_indeterminate-track);
    background-size: var(--_indeterminate-track-size);
    background-position: right; 
    animation: var(--_indeterminate-track-animation);
  }progress {
  
  /*  complete  */
}

becomes

 /*  indeterminate Firefox  */progress:indeterminate::-moz-progress-bar {
    background: var(--_indeterminate-track);
    background-size: var(--_indeterminate-track-size);
    background-position: right; 
    animation: var(--_indeterminate-track-animation);
  }/*  complete  */

hopefully this keeps the order but targets just the problem area (selectors with only comments). that list of empty selectors in devtools looks buggy, this would be hard to explain to someone new who is just trying to diligently comment their code, yet the output bloats.

thanks for looking at this, def not as simple to solve as i'd anticipated!

romainmenke · 2022-03-04T17:25:48Z

if a selector is only containing a comment, replace the selector and comment with just the comment?

I like this!
Not going to comment on how hard it's going to be, don't want to jinx it :D

Antonio-Laguna · 2022-03-04T21:18:59Z

Thanks for all of this and I'm glad it was something that could get fixed!

@argyleink this got just released as 10.1.3!

Antonio-Laguna added bug Something isn't working plugins/postcss-nesting labels Mar 4, 2022

romainmenke added a commit that referenced this issue Mar 4, 2022

postcss-nesting : fix #285

85b9ace

romainmenke closed this as completed in ef97463 Mar 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments cause parent selector output repetition #285

Comments cause parent selector output repetition #285

argyleink commented Mar 3, 2022 •

edited by Antonio-Laguna

Loading

Antonio-Laguna commented Mar 4, 2022 •

edited

Loading

romainmenke commented Mar 4, 2022

romainmenke commented Mar 4, 2022

argyleink commented Mar 4, 2022

romainmenke commented Mar 4, 2022 •

edited

Loading

argyleink commented Mar 4, 2022

romainmenke commented Mar 4, 2022

Antonio-Laguna commented Mar 4, 2022

Comments cause parent selector output repetition #285

Comments cause parent selector output repetition #285

Comments

argyleink commented Mar 3, 2022 • edited by Antonio-Laguna Loading

Bug description

Source CSS

Expected CSS

Actual CSS

Does it happen with npx @csstools/csstools-cli <plugin-name> minimal-example.css?

Debug output

Extra config

What plugin are you experiencing this issue on?

Plugin version

What OS are you experiencing this on?

Node Version

Validations

Would you like to open a PR for this bug?

Antonio-Laguna commented Mar 4, 2022 • edited Loading

romainmenke commented Mar 4, 2022

romainmenke commented Mar 4, 2022

argyleink commented Mar 4, 2022

romainmenke commented Mar 4, 2022 • edited Loading

argyleink commented Mar 4, 2022

romainmenke commented Mar 4, 2022

Antonio-Laguna commented Mar 4, 2022

argyleink commented Mar 3, 2022 •

edited by Antonio-Laguna

Loading

Does it happen with `npx @csstools/csstools-cli <plugin-name> minimal-example.css`?

Antonio-Laguna commented Mar 4, 2022 •

edited

Loading

romainmenke commented Mar 4, 2022 •

edited

Loading