Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merging CSS rules with same properties fails in spec test #819

Open
schlessera opened this issue Jun 2, 2020 · 1 comment
Open

Merging CSS rules with same properties fails in spec test #819

schlessera opened this issue Jun 2, 2020 · 1 comment

Comments

@schlessera
Copy link
Collaborator

schlessera commented Jun 2, 2020

In the following spec test, you can see that there's two CSS rules that should have been merged together but weren't:

<style amp-custom>#i-amp-0{width:100vw}@media (min-width: 320px){#i-amp-0{width:320px}}#i-amp-1{width:100vw}@media (min-width:{#i-amp-1{width:320px)}}</style>

A single rule #i-amp-0,#i-amp-1{width:100vw} should have been generated instead of the two separate rules #i-amp-0{width:100vw} & #i-amp-1{width:100vw}.

This currently causes a failure in the PHP library tests here: https://travis-ci.org/github/ampproject/amp-wp/jobs/693721158#L950-L951

@sebastianbenz
Copy link
Collaborator

I think I intentionally didn't merge these as the order in which the rules appear matters.

For example:

<amp-img id="img1" sizes="(max-width: 100px) 100px, 100vw"></amp-img>
<amp-img id="img2" sizes="(max-width: 100px) 100px, (max-width: 200px) 200px, 120vw"></amp-img>

The correct order is:

#i-amp-1{width:100vw}
@media (max-width: 100px){ 
  #i-amp-1{width: 100px}
}

#i-amp-2{width:100vw}
/* max-width 200 needs to come before max-width 100 */
@media (max-width: 200px){ 
  #i-amp-2{width: 200px}
}
@media (max-width: 100px){ 
  #i-amp-2{width: 200px}
}

Merging rules would break the behavior by overwriting the implicit ordering semantics:

#i-amp-1{width:100vw}
@media (max-width: 100px){ 
  /* this will never trigger for #i-amp-2 */
  #i-amp-1, #i-amp-2{width: 100px}
}

#i-amp-2{width:100vw}
@media (max-width: 200px){ 
  #i-amp-2{width: 200px}
}

Merging the default rules could work as these always need to come first. Not sure though if that's worth the effort and would be better handled via a CSS compressor.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

2 participants