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.

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

Unsafe grouping #16

Closed
marvinroger opened this Issue Aug 22, 2014 · 27 comments

Comments

Projects
None yet
9 participants
@marvinroger

marvinroger commented Aug 22, 2014

Hi,

I recently saw a couple of interesting comments about unsafe media query grouping. Correct me if I am wrong, but I think node-css-mqpacker is affected by this problem - I am not able to test for now -. There must be a way to solve this, by ensuring that media queries are not conflicted.

This is highly problematic as now the package may break website render and therefore is not suitable for production.

@hail2u hail2u added the bug label Aug 22, 2014

@hail2u

This comment has been minimized.

Show comment
Hide comment
@hail2u

hail2u Aug 22, 2014

Owner

You're right. But I don't think it can be solved in CSS without HTML. This problem depends on HTML class attribute's value.

But, yes, it's not safe for production. I'll add section to README about this problem. Thanks!

Owner

hail2u commented Aug 22, 2014

You're right. But I don't think it can be solved in CSS without HTML. This problem depends on HTML class attribute's value.

But, yes, it's not safe for production. I'll add section to README about this problem. Thanks!

@marvinroger

This comment has been minimized.

Show comment
Hide comment
@marvinroger

marvinroger Aug 22, 2014

The issue goes deeper than that. Say:

  • HTML
<div id="square">I am green</div>
  • CSS
#square {
  width: 100px;
  height: 100px;

  text-align: center;
  line-height: 100px;
}

@media (min-width: 1px) { 
  #square { background-color: red; }
}
@media (min-width: 2px) { 
  #square { background-color: yellow; }
}
@media (min-width: 1px) { 
  #square { background-color: green; }
}

If viewport is higher than 2px, the square must be green. (http://jsfiddle.net/9upL6joa/)
However, after packed up with latest version as of now (v1.0.0), the square gets yellow. (http://jsfiddle.net/rb9Lkt7a/)

With only a common ID selector, so the problem don't depend on HTML class.
I perfectly agree with the fact that the @media (min-width: 2px) is unnecessary, we can safely remove it here. But in a bigger codebase, this could happen, and a post-processor cannot expect the code it processes to be perfectly optimized.

I am pretty sure this can be resolved by keeping rules ordered (and therefore some same mediaqueries would not be merged), but the logic behind might be pretty hard to implement (I don't have any experience in post-processing).

marvinroger commented Aug 22, 2014

The issue goes deeper than that. Say:

  • HTML
<div id="square">I am green</div>
  • CSS
#square {
  width: 100px;
  height: 100px;

  text-align: center;
  line-height: 100px;
}

@media (min-width: 1px) { 
  #square { background-color: red; }
}
@media (min-width: 2px) { 
  #square { background-color: yellow; }
}
@media (min-width: 1px) { 
  #square { background-color: green; }
}

If viewport is higher than 2px, the square must be green. (http://jsfiddle.net/9upL6joa/)
However, after packed up with latest version as of now (v1.0.0), the square gets yellow. (http://jsfiddle.net/rb9Lkt7a/)

With only a common ID selector, so the problem don't depend on HTML class.
I perfectly agree with the fact that the @media (min-width: 2px) is unnecessary, we can safely remove it here. But in a bigger codebase, this could happen, and a post-processor cannot expect the code it processes to be perfectly optimized.

I am pretty sure this can be resolved by keeping rules ordered (and therefore some same mediaqueries would not be merged), but the logic behind might be pretty hard to implement (I don't have any experience in post-processing).

@hail2u

This comment has been minimized.

Show comment
Hide comment
@hail2u

hail2u Aug 23, 2014

Owner

I design this tool for a proper CSS codes. Buggy CSS like your second example should be fixed before post-processing.

Owner

hail2u commented Aug 23, 2014

I design this tool for a proper CSS codes. Buggy CSS like your second example should be fixed before post-processing.

@lunelson

This comment has been minimized.

Show comment
Hide comment
@lunelson

lunelson Sep 29, 2014

From what I can tell at the pleeease live demo, the grouping seems based on wherever a rule first appears; I would suggest an alternative, that media queries be combined based on their values: for min-width queries from lowest to highest, and for max-width queries the other way. This is a logic that one could work with I think, when writing the CSS

lunelson commented Sep 29, 2014

From what I can tell at the pleeease live demo, the grouping seems based on wherever a rule first appears; I would suggest an alternative, that media queries be combined based on their values: for min-width queries from lowest to highest, and for max-width queries the other way. This is a logic that one could work with I think, when writing the CSS

@oilvier

This comment has been minimized.

Show comment
Hide comment
@oilvier

oilvier Oct 9, 2014

+1 with @lunelson

My CSS file looks like this :

/* Grid system */
@import "grids.css";

/* Regular CSS */
.header {}
.footer {}
...

/* Responsive */
@import "tablet.css";
@media only screen and (max-width: 600px) {
    /* Specific rule for that specific breakpoint */
}
@import "phone.css";

In grids.css I have the following :

.grid-1 {width:100%;}
.grid-1-2 {width:50%;}
...

@media only screen and (max-width: 800px) {
    .tablet-grid-1 {width:100%;}
    .tablet-grid-1-2 {width:50%;}
    ...
}

@media only screen and (max-width: 400px) {
    .phone-grid-1 {width:100%;}
    .phone-grid-1-2 {width:50%;}
    ...
}

In tablet.css :

@media only screen and (max-width: 800px) {
    /* Specific style for tablets */
}

and in phone.css :

@media only screen and (max-width: 400px) {
    /* Specific style for phones */
}

Every rules are correctly rendered in my processed file, as the first media queries encountered in grids.css are the same that I use in tablet.css and phone.css.

The problem come from the additionnal media query @media only screen and (max-width: 600px) which is rendered at the end of the processed file because it's the last media query.
If I put something in that media query that I want to override in phone.css it won't work because this last media query is rendered after phone.css in the final file.

oilvier commented Oct 9, 2014

+1 with @lunelson

My CSS file looks like this :

/* Grid system */
@import "grids.css";

/* Regular CSS */
.header {}
.footer {}
...

/* Responsive */
@import "tablet.css";
@media only screen and (max-width: 600px) {
    /* Specific rule for that specific breakpoint */
}
@import "phone.css";

In grids.css I have the following :

.grid-1 {width:100%;}
.grid-1-2 {width:50%;}
...

@media only screen and (max-width: 800px) {
    .tablet-grid-1 {width:100%;}
    .tablet-grid-1-2 {width:50%;}
    ...
}

@media only screen and (max-width: 400px) {
    .phone-grid-1 {width:100%;}
    .phone-grid-1-2 {width:50%;}
    ...
}

In tablet.css :

@media only screen and (max-width: 800px) {
    /* Specific style for tablets */
}

and in phone.css :

@media only screen and (max-width: 400px) {
    /* Specific style for phones */
}

Every rules are correctly rendered in my processed file, as the first media queries encountered in grids.css are the same that I use in tablet.css and phone.css.

The problem come from the additionnal media query @media only screen and (max-width: 600px) which is rendered at the end of the processed file because it's the last media query.
If I put something in that media query that I want to override in phone.css it won't work because this last media query is rendered after phone.css in the final file.

@Maks3w

This comment has been minimized.

Show comment
Hide comment
@Maks3w

Maks3w Oct 24, 2014

Contributor

I've open the pull request #24 where it's suggested to follow a specific order of media queries for avoid this kind of collateral effects when grouping.

In fact the PR ensure if the CSS developer write the media queries with the proposed order then the post processed CSS file will maintain the same order.

I invite you to discuss this proposal in the PR comments

Contributor

Maks3w commented Oct 24, 2014

I've open the pull request #24 where it's suggested to follow a specific order of media queries for avoid this kind of collateral effects when grouping.

In fact the PR ensure if the CSS developer write the media queries with the proposed order then the post processed CSS file will maintain the same order.

I invite you to discuss this proposal in the PR comments

@Volker-E

This comment has been minimized.

Show comment
Hide comment
@Volker-E

Volker-E Feb 25, 2015

Same problem here, like the one of @oilvier - but I guess, it should be packed into another issue.
The media queries should at least be output in ascending order of min-width values IMHO.

Volker-E commented Feb 25, 2015

Same problem here, like the one of @oilvier - but I guess, it should be packed into another issue.
The media queries should at least be output in ascending order of min-width values IMHO.

@lunelson

This comment has been minimized.

Show comment
Hide comment
@lunelson

lunelson Feb 25, 2015

In the meantime you can hack the ordering by outputting the queries you are going to use, somewhere at the top of your stylesheet, with dummy rules if necessary, e.g.

@media screen and (min-width: 20em) { body:before { content: ''; } }
@media screen and (min-width: 30em) { body:before { content: ''; } }
@media screen and (min-width: 40em) { body:before { content: ''; } }
@media screen and (min-width: 50em) { body:before { content: ''; } }

... and the no matter in what order you use them later, they should appear in the mqpacker output in that order

lunelson commented Feb 25, 2015

In the meantime you can hack the ordering by outputting the queries you are going to use, somewhere at the top of your stylesheet, with dummy rules if necessary, e.g.

@media screen and (min-width: 20em) { body:before { content: ''; } }
@media screen and (min-width: 30em) { body:before { content: ''; } }
@media screen and (min-width: 40em) { body:before { content: ''; } }
@media screen and (min-width: 50em) { body:before { content: ''; } }

... and the no matter in what order you use them later, they should appear in the mqpacker output in that order

@Volker-E

This comment has been minimized.

Show comment
Hide comment
@Volker-E

Volker-E Feb 25, 2015

@lunelson Exactly what I did too. :) But it's a hack, which contradicts the first and foremost usecase of css-mqpacker by inserting extra (unnecessary) rules in output

Volker-E commented Feb 25, 2015

@lunelson Exactly what I did too. :) But it's a hack, which contradicts the first and foremost usecase of css-mqpacker by inserting extra (unnecessary) rules in output

@lunelson

This comment has been minimized.

Show comment
Hide comment
@lunelson

lunelson Feb 25, 2015

True, but if mqpacker offers this as a feature it should be togglable somehow, and it's still not clear what mqpacker should do if you write both max-width and min-width queries, or if you write queries that use both

lunelson commented Feb 25, 2015

True, but if mqpacker offers this as a feature it should be togglable somehow, and it's still not clear what mqpacker should do if you write both max-width and min-width queries, or if you write queries that use both

@Maks3w

This comment has been minimized.

Show comment
Hide comment
@Maks3w

Maks3w Feb 25, 2015

Contributor

I suggest don't think in the large final result of all the possible things. Let's start the way step by step and improve the things when needed

Contributor

Maks3w commented Feb 25, 2015

I suggest don't think in the large final result of all the possible things. Let's start the way step by step and improve the things when needed

@hail2u

This comment has been minimized.

Show comment
Hide comment
@hail2u

hail2u Feb 25, 2015

Owner

@Volker-E Comments also work mentioned in README.md:

@media screen and (min-width: 20em) { /*! Wider > 20em */ }
@media screen and (min-width: 30em) { /*! Wider > 30em */ }
@media screen and (min-width: 40em) { /*! Wider > 40em */ }
@media screen and (min-width: 50em) { /*! Wider > 50em */ }

It's still hack, but there is no extra CSS rules in output.


I can understand what @Maks3w and almost all users need. Defining queries order first is a compromise, but this always works as expected. There is no magic here, like *.d.ts in TypeScript.

Owner

hail2u commented Feb 25, 2015

@Volker-E Comments also work mentioned in README.md:

@media screen and (min-width: 20em) { /*! Wider > 20em */ }
@media screen and (min-width: 30em) { /*! Wider > 30em */ }
@media screen and (min-width: 40em) { /*! Wider > 40em */ }
@media screen and (min-width: 50em) { /*! Wider > 50em */ }

It's still hack, but there is no extra CSS rules in output.


I can understand what @Maks3w and almost all users need. Defining queries order first is a compromise, but this always works as expected. There is no magic here, like *.d.ts in TypeScript.

@Maks3w

This comment has been minimized.

Show comment
Hide comment
@Maks3w

Maks3w Feb 25, 2015

Contributor

Create a base structure in the first processed file is an interesting idea. Just for me lack in the need of cover all possible values which are infinite.

For example. The main idea behind a opiniated order is allow the develop of independent UI components. This is, the final file does not know what media queries exists, just join and sort following many rules and the UI component must follow the same rules.

Contributor

Maks3w commented Feb 25, 2015

Create a base structure in the first processed file is an interesting idea. Just for me lack in the need of cover all possible values which are infinite.

For example. The main idea behind a opiniated order is allow the develop of independent UI components. This is, the final file does not know what media queries exists, just join and sort following many rules and the UI component must follow the same rules.

@Maks3w

This comment has been minimized.

Show comment
Hide comment
@Maks3w

Maks3w Feb 25, 2015

Contributor

Could be this implemented as an opt-out function as is now?

May in the future we could develop some kind of customization in the sort algorithm

Contributor

Maks3w commented Feb 25, 2015

Could be this implemented as an opt-out function as is now?

May in the future we could develop some kind of customization in the sort algorithm

@hail2u

This comment has been minimized.

Show comment
Hide comment
@hail2u

hail2u Feb 25, 2015

Owner

Processing independent components (including 3rd-party library) altogether with this kind of tools (including CSS minifier) is not a good idea, I think. They must be processed individually or untouched, just concatenate.

Could be this implemented as an opt-out function as is now?

Sorting only min-width?

Owner

hail2u commented Feb 25, 2015

Processing independent components (including 3rd-party library) altogether with this kind of tools (including CSS minifier) is not a good idea, I think. They must be processed individually or untouched, just concatenate.

Could be this implemented as an opt-out function as is now?

Sorting only min-width?

@Maks3w

This comment has been minimized.

Show comment
Hide comment
@Maks3w

Maks3w Feb 25, 2015

Contributor

@hail2u There is no reason for don't apply a postCSS rule to a 3rd party library. This is part of the optimization pipeline like run unCSS. Of course the developer must not apply this filter if is unsafe.
For example. I've a project with TWBS3 packed along my custom CSS but other project from other client I can't do it without break the UI.

@hail2u With the current sort algorithm. If later someone has a better proposal then we could change. As I said, start the ball rolling and keep improving with new ideas.

Contributor

Maks3w commented Feb 25, 2015

@hail2u There is no reason for don't apply a postCSS rule to a 3rd party library. This is part of the optimization pipeline like run unCSS. Of course the developer must not apply this filter if is unsafe.
For example. I've a project with TWBS3 packed along my custom CSS but other project from other client I can't do it without break the UI.

@hail2u With the current sort algorithm. If later someone has a better proposal then we could change. As I said, start the ball rolling and keep improving with new ideas.

hail2u added a commit that referenced this issue Feb 25, 2015

Sort `min-width` queries
This will fix part of issue #16.

@hail2u hail2u referenced this issue Feb 25, 2015

Merged

[WIP] Sort `min-width` queries #28

17 of 17 tasks complete
@hail2u

This comment has been minimized.

Show comment
Hide comment
@hail2u

hail2u Feb 25, 2015

Owner

Order of CSS rulesets is very important. UnCSS doesn't change order, but CSS MQPacker does. It seems always unsafe to me.

Anyway, I have started to implement on #28.

Owner

hail2u commented Feb 25, 2015

Order of CSS rulesets is very important. UnCSS doesn't change order, but CSS MQPacker does. It seems always unsafe to me.

Anyway, I have started to implement on #28.

@Maks3w

This comment has been minimized.

Show comment
Hide comment
@Maks3w

Maks3w Feb 25, 2015

Contributor

Yes mqpacker is unsafe. But your fraids can go away if you use visual regression testing tools.

Time ago I started to use phantomCSS and I free to apply aggresive optimizations without risks

Contributor

Maks3w commented Feb 25, 2015

Yes mqpacker is unsafe. But your fraids can go away if you use visual regression testing tools.

Time ago I started to use phantomCSS and I free to apply aggresive optimizations without risks

@Volker-E

This comment has been minimized.

Show comment
Hide comment
@Volker-E

Volker-E Feb 26, 2015

Yes, mqpacker is in rare cases unsafe, but without enough knowledge writing CSS is unsafe to a certain degree. With every abstraction tool helping to target handling of complexity in software development unsafety/uncertainty raises.
I think we shouldn't believe that devs using mqpacker aren't aware of the potential pitfalls. I guess, most devs arrive here because of the cumbersome output of lots of same media queries by CSS preprocessors.

I started my first comment above, because it wasn't clear enough for me after reading through Readme, that mqpacker simply sorts the media queries in order of appearance in the original CSS file.
Next logical step for me was the above mentioned hack of putting a random selector with my personal order at top of my Stylesheet.

I do believe, that taking min-width (and maybe min-device-width as second) in account at reordering media queries is a decent way to save from most worrying media query cascades.
At least as an option.

Volker-E commented Feb 26, 2015

Yes, mqpacker is in rare cases unsafe, but without enough knowledge writing CSS is unsafe to a certain degree. With every abstraction tool helping to target handling of complexity in software development unsafety/uncertainty raises.
I think we shouldn't believe that devs using mqpacker aren't aware of the potential pitfalls. I guess, most devs arrive here because of the cumbersome output of lots of same media queries by CSS preprocessors.

I started my first comment above, because it wasn't clear enough for me after reading through Readme, that mqpacker simply sorts the media queries in order of appearance in the original CSS file.
Next logical step for me was the above mentioned hack of putting a random selector with my personal order at top of my Stylesheet.

I do believe, that taking min-width (and maybe min-device-width as second) in account at reordering media queries is a decent way to save from most worrying media query cascades.
At least as an option.

hail2u added a commit that referenced this issue Mar 5, 2015

Add `sort` option
This will fix a part of issue #16.

zoxon added a commit to zoxon/gulp-front that referenced this issue Sep 7, 2016

@kyleshevlin

This comment has been minimized.

Show comment
Hide comment
@kyleshevlin

kyleshevlin Oct 20, 2016

I don't know that this is a solvable issue without there being a config step to indicate order of media queries, and thus, what people are calling a hack above, really seems like a simple, clever solution to the problem to me. I used to do this with sprockets-media_query_combiner in Rails and did it so often that a _query_order.scss partial was just a standard part of our builds.

kyleshevlin commented Oct 20, 2016

I don't know that this is a solvable issue without there being a config step to indicate order of media queries, and thus, what people are calling a hack above, really seems like a simple, clever solution to the problem to me. I used to do this with sprockets-media_query_combiner in Rails and did it so often that a _query_order.scss partial was just a standard part of our builds.

@nathanjessen

This comment has been minimized.

Show comment
Hide comment
@nathanjessen

nathanjessen Jan 20, 2017

Cool plugin but the lack of smarter sorting is keeping me from using the plugin especially on larger teams. The sort option should be default and it helps with a lot of the bugs but doesn't cover all cases. If the plugin can't handle simple min/max width queries, then I can't even imagine how many issues would be present with some of the other features used in media queries.

nathanjessen commented Jan 20, 2017

Cool plugin but the lack of smarter sorting is keeping me from using the plugin especially on larger teams. The sort option should be default and it helps with a lot of the bugs but doesn't cover all cases. If the plugin can't handle simple min/max width queries, then I can't even imagine how many issues would be present with some of the other features used in media queries.

@hail2u

This comment has been minimized.

Show comment
Hide comment
@hail2u

hail2u Jan 21, 2017

Owner

You should clarify the reason “The sort option should be default”. Why don’t you configure this option?

Owner

hail2u commented Jan 21, 2017

You should clarify the reason “The sort option should be default”. Why don’t you configure this option?

@nathanjessen

This comment has been minimized.

Show comment
Hide comment
@nathanjessen

nathanjessen Jan 21, 2017

I retract my statement regarding the sort being default. However, the plugin is only truly safe to use on extremely well written CSS that already sorts media queries correctly and follows all CSS best practices which is probably less than 0.5% of projects. So I'd say it's dangerous to have the plugin out there without a big fat warning at the top of the readme reminding developers that the plugin works by altering the order of CSS rules and it may have undesired affects to the output.

nathanjessen commented Jan 21, 2017

I retract my statement regarding the sort being default. However, the plugin is only truly safe to use on extremely well written CSS that already sorts media queries correctly and follows all CSS best practices which is probably less than 0.5% of projects. So I'd say it's dangerous to have the plugin out there without a big fat warning at the top of the readme reminding developers that the plugin works by altering the order of CSS rules and it may have undesired affects to the output.

@hail2u

This comment has been minimized.

Show comment
Hide comment
@hail2u

hail2u Jan 21, 2017

Owner

Do I need to include a sentence: “This plugin only works less than 0.5% of projects”? It’s crazy.

Owner

hail2u commented Jan 21, 2017

Do I need to include a sentence: “This plugin only works less than 0.5% of projects”? It’s crazy.

@yowainwright

This comment has been minimized.

Show comment
Hide comment
@yowainwright

yowainwright Jan 24, 2017

I don't think this is an issue. The sort option works great. Thanks @hail2u for making this AWESOME PLUGIN that saves me many KBs. 💪

yowainwright commented Jan 24, 2017

I don't think this is an issue. The sort option works great. Thanks @hail2u for making this AWESOME PLUGIN that saves me many KBs. 💪

@kyleshevlin

This comment has been minimized.

Show comment
Hide comment
@kyleshevlin

kyleshevlin Jan 24, 2017

I feel like this conversation isn't productive at this point. I want to help it stay productive, thus, here is my argument and reasons regarding this issue:

I don't think the sort option should be defaulted. One of the features/bugs of CSS is that code further down the file overwrites code further up. This is a known quantity that front end developers have to account for. Your codebase should already be prepared for this fact. Because this is something the developer has to be prepared for, utilizing someone else's sorting order could result in unexpected results simply from the sort order being different than what you expected/would have chosen.

Making sorting an opt-in option enforces the developer to make a choice between creating their own standard via a query_order hack like I suggest above or adopting the standards of project. Since you already should have your own standards, it is more likely the case that developers would not need to opt into this.

It is unhelpful to use unsubstantiated, hyperbolic stats to make your point. I don't disagree that there could be a warning, and off the top of my head, I don't know if the documentation has one or describes how the packer sorts queries. If it's not documented that this works on a first-come, first-served basis, then that should be changed.

kyleshevlin commented Jan 24, 2017

I feel like this conversation isn't productive at this point. I want to help it stay productive, thus, here is my argument and reasons regarding this issue:

I don't think the sort option should be defaulted. One of the features/bugs of CSS is that code further down the file overwrites code further up. This is a known quantity that front end developers have to account for. Your codebase should already be prepared for this fact. Because this is something the developer has to be prepared for, utilizing someone else's sorting order could result in unexpected results simply from the sort order being different than what you expected/would have chosen.

Making sorting an opt-in option enforces the developer to make a choice between creating their own standard via a query_order hack like I suggest above or adopting the standards of project. Since you already should have your own standards, it is more likely the case that developers would not need to opt into this.

It is unhelpful to use unsubstantiated, hyperbolic stats to make your point. I don't disagree that there could be a warning, and off the top of my head, I don't know if the documentation has one or describes how the packer sorts queries. If it's not documented that this works on a first-come, first-served basis, then that should be changed.

@yowainwright

This comment has been minimized.

Show comment
Hide comment
@yowainwright

yowainwright Jan 24, 2017

Agreed. The issue name Unsafe Grouping does not correspond with the issue's conversation.

MQ Packer does just what it says—packs media queries. ⚡️

If an app's style cascade is working then MQ Packer will work. If it doesn't, minify the css and call it a day. Chances are the few KBs that would be saved (after Gzip) with MQ Packer don't add up to much when compared with a style cascade that needs TLC. Check out stylelint. Maybe that could offer more insight? Or, better yet, PR the always working 🦄 fix!

yowainwright commented Jan 24, 2017

Agreed. The issue name Unsafe Grouping does not correspond with the issue's conversation.

MQ Packer does just what it says—packs media queries. ⚡️

If an app's style cascade is working then MQ Packer will work. If it doesn't, minify the css and call it a day. Chances are the few KBs that would be saved (after Gzip) with MQ Packer don't add up to much when compared with a style cascade that needs TLC. Check out stylelint. Maybe that could offer more insight? Or, better yet, PR the always working 🦄 fix!

@hail2u hail2u closed this in #44 Jan 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment