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.

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

cssDeclarationSorter is not safe when shorthand after longhand property #535

Closed
ai opened this issue Jul 4, 2018 · 49 comments
Closed

cssDeclarationSorter is not safe when shorthand after longhand property #535

ai opened this issue Jul 4, 2018 · 49 comments
Labels
Milestone

Comments

@ai
Copy link
Member

ai commented Jul 4, 2018

Input:

.bounce {
	animation-name: bounce;
	animation: bounce2 1s ease;
	z-index: 1442;
}

Output:

._bounce_{animation:_bounce2_ 1s ease;animation-name:_bounce_;z-index:1442}

Expected output:

._bounce_{animation-name:_bounce_;animation:_bounce2_ 1s ease;z-index:1442}

/cc @evilebottnawi


Other bug (need add tests):

Input:

.class {
  border-radius: 10px;
  border-bottom-left-radius: 0;
}

Output:

.class{border-bottom-left-radius:0;border-radius:10px}

Other bug (need also add tests):

Input:

border-width: 2px;
border-style: solid;
border-top: 0;
border-left: 0;

Output:

border-left: 0;
border-style: solid;
border-top: 0;
border-width: 2px;
@alexander-akait
Copy link
Member

Looks bug, feel free to fix, don't have time right now 😞

@ai
Copy link
Member Author

ai commented Jul 4, 2018

I am not sure, that css-declaration-sorter is safe. Seems like it was designed to use manually by the developer. And could be dangerous to run it on legacy code.

@ai
Copy link
Member Author

ai commented Jul 4, 2018

Maybe we should disable it in default preset and keep only in advanced?

@alexander-akait
Copy link
Member

@ai in theory it is dangerous only in some cases where using idents, maybe we can handle this in safe way

@ai
Copy link
Member Author

ai commented Jul 4, 2018

@evilebottnawi it can be dangerous if the code is written wrong. Like in this example (shorthand override longhand). Yeap, we could make css-declaration-sorter very smart to predict it, but I am not sure, that it will be done soon.

What do you think if we will disable css-declaration-sorter in default preset and enable back when it will become smarter?

@alexander-akait
Copy link
Member

alexander-akait commented Jul 4, 2018

@ai feel free to do this and release patch version

@ai
Copy link
Member Author

ai commented Jul 4, 2018

I started to moving it, but can’t fix some tests #536

@Siilwyn
Copy link
Contributor

Siilwyn commented Jul 5, 2018

👋 chiming in, a straightforward fix would be to keep the animation-*, list-style-*, grid-*, counter-* in the same order as @evilebottnawi mentioned. Would it be an idea for me to shoot in a PR at cssnano? Think a custom order makes the most sense.

@ai
Copy link
Member Author

ai commented Jul 5, 2018

@Siilwyn yeap, but we need to keep order of any longhand/shorthand properties

@kingback
Copy link

kingback commented Aug 8, 2018

same problem when using -webkit-background-size

Input:

.a {
  background: url('xxx');
  -webkit-background-size: contain;
}

Output:

.a{-webkit-background-size:contain;background:url(xxx)}

And -webkit-background-size is overrided by background

Expected output:

.a{background:url(xxx);-webkit-background-size:contain;}

@Siilwyn
Copy link
Contributor

Siilwyn commented Aug 8, 2018

@kingback do you include prefixed properties in your source code?

Regarding the fixed order I think I'll have time to look into it tomorrow!

@kingback
Copy link

yes, some legacy code use prefixed properties to compatible with old mobile devices, maybe we can remove the vendor prefix before sort properties

@Siilwyn
Copy link
Contributor

Siilwyn commented Aug 14, 2018

Still a valid use-case, @kingback I would recommend to look at using autoprefixer. But I'll take a look at prefixed properties too.

@andyjansson
Copy link
Contributor

Given the amount of issues we have around cssDeclarationSorter it might be warranted to disable the plugin entirely.

@alexander-akait
Copy link
Member

@andyjansson i think we should fix it, don't disable, can we provide list of problems here?

@andyjansson
Copy link
Contributor

Well the issue is that it's entirely too naïve and doesn't respect precedence. I do not believe we can easily fix it without major rewrites, which is hard given that it is an external dependency.

@alexander-akait
Copy link
Member

@andyjansson we can move sort algorithm inside css-declaration-sorter in theory and don't use dependecy

@OlehDutchenko
Copy link

People, you are killing cssnano! (((

@andyjansson
Copy link
Contributor

@dutchenkoOleg That's an unwarranted response. What gives?

@Siilwyn
Copy link
Contributor

Siilwyn commented Aug 27, 2018

Yes I'm certain I can make this work, just need to put time and thoughts into the problem. Doesn't really matter it is external or not right? I'm willing to change parts on my end. :)

@OlehDutchenko
Copy link

@andyjansson #535 (comment) and next one

@andyjansson
Copy link
Contributor

@dutchenkoOleg and how are we "killing cssnano"? Like @ai says, css-declaration-sorter is unsafe in its current form, leading to broken CSS. We are discussing whether disabling this plugin until a resolution can be found is an appropriate course of action.

If you have any concerns, go ahead and voice them, but we can do without the outburts and hyperboles.

@OlehDutchenko
Copy link

I think you needed to turn off this plug-in immediately, when you realized that this is unsafe, rather than discuss it "...to be or not to be..."

@kingback
Copy link

agree with @andyjansson, not only this issue, there're issues like postcss-calc/issues/48 makes me feel like it's unsafe to use cssnano, I think we should support disable plugins, like:

{
  preset: ['default', {
    cssDeclarationSorter: false, // disable plugin
    postcssCalc: false, // disable plugin
    discardComments: { // custom plugin config
      removeAll: true,
    }
  }]
}

So we can have a choice to deside which plugin we don't want to use.

@andyjansson
Copy link
Contributor

I think we should support disable plugins

We've always supported that. https://cssnano.co/guides/presets#excluding-transforms

there're issues like postcss-calc/issues/48 makes me feel like it's unsafe to use cssnano

The issue here is MoOx/reduce-css-calc. It's had a fix for the longest time, but the package has been poorly maintained.

@ai do you have access to this repo? If not, other options are to fork the code, or move the logic to postcss-calc.

@andyjansson
Copy link
Contributor

border-style is a shorthand.

That depends on what you compare it with. It's a shorthand of border-<TRBL>-style, but it's a longhand of border.

Whether it's a shorthand or not isn't really the key issue here; precedence is. The order of declaration makes a huge semantic difference, regardless of whether shorthand or longhand declarations are involved.

border-width: 2px;
border-top-width: 0;
border-top-width: 0;
border-width: 2px;

The above two snippets, although being identical in what properties they contain, mean two very different things due to the order they're declared in.

@alexander-akait alexander-akait changed the title cssDeclarationSorter and animation cssDeclarationSorter is not safe when shorthand after longhand property Sep 20, 2018
@ai
Copy link
Member Author

ai commented Sep 24, 2018

Another week ended. Did we released any solution or we are still in political powerless?

It is a critical issue. We had a quick option and long option. Why we prefer to choose the worst option to do nothing?

/cc @evilebottnawi

@Siilwyn
Copy link
Contributor

Siilwyn commented Sep 24, 2018

@ai the quick option is already merged at fa8c500, just no version bump yet.

Meanwhile I'm waiting for feedback on my PR which is ready to be merged too, maybe you could take a look?

@alexander-akait
Copy link
Member

@ai I apologize for the current situation, but I really have a lot of work and I will not have enough time 😞 So I gave github/npm access to you (@ai) and @andyjansson so feel free to release, i can find time only tomorrow.

@alexander-akait
Copy link
Member

alexander-akait commented Sep 24, 2018

@Siilwyn you solution is not good. Why?

It is bad to have list of properties in file https://github.com/cssnano/cssnano/pull/580/files#diff-bc4bbe1119837b527465f18f074304ff (each new property require adding to file).

A lot of changes not related to PR. Example:
From:

body{font-size:100%;line-height:1.5}

To

body{line-height:1.5;font-size:100%}

Why it is happens? It is not alphabetic order. We need maximum alphabetic order for better gzip.

Here issue about new option Siilwyn/css-declaration-sorter#64 to fix this bug. Main idea order all property in alphabetic mode except shorthard property after longhand (maybe exists edge case what we should handle in other way)

@dkenul
Copy link

dkenul commented Sep 24, 2018

Would it be possible to at least update the optimisations docs:
https://cssnano.co/optimisations/

to indicate that this plugin is unsafe and have it off by default? Recently updated to a newer version of cssnano in a legacy project and this threw me for a loop.

I'll make a PR for that if anyone wants. This is not safe, especially when dealing with a CSS pre-processor.

@ai
Copy link
Member Author

ai commented Sep 24, 2018

I will try to release it right now

@alexander-akait
Copy link
Member

alexander-akait commented Sep 24, 2018

@dkenul

I'll make a PR for that if anyone wants. This is not safe, especially when dealing with a CSS pre-processor.

Please provide example.

@dkenul
Copy link

dkenul commented Sep 24, 2018

@evilebottnawi I realize now I may have not worded that the best. It's not anything intrinsic to the pre-processor, but more so that it makes it harder to reason about things like mixins:

mixins.less

.mixin-a {
  background-color: red;
} 

someotherfile.less

.selector-a {
  .mixin-a;
  background: blue;
}

Output:

.selector-a {
  background: blue;
  background-color: red;
}

Since there is a higher level of abstraction, it's a bit easier to shoot yourself in the foot.

@alexander-akait
Copy link
Member

@dkenul don't use cssnano with a CSS pre-processor. It is always unsafe. Use cssnano only on pure css code

@Siilwyn
Copy link
Contributor

Siilwyn commented Sep 24, 2018

@evilebottnawi I know that, it's not ideal but it is still much better than disabling the plugin! That is why I proposed this as the solution for now: #535 (comment)

Please reconsider, I would hate it if all these hours I spend on a solution I proposed and worked on to be wasted. :(

@dkenul
Copy link

dkenul commented Sep 24, 2018

@evilebottnawi can you elaborate? This specific issue can occur with or without a pre-processor.

@ai
Copy link
Member Author

ai commented Sep 24, 2018

I released 4.1.1 without css sorter

@alexander-akait
Copy link
Member

@dkenul please create new issue with example what you have in css, what you actually have after minification and what you expected, thanks!

@jordrake
Copy link
Contributor

What about the alternative where we maintain a collection of shorthand and their longhand expansions? It will still require vigilance to keep up to date but should be less change overall. Similar to https://github.com/stylelint/stylelint/blob/master/lib/reference/shorthandData.js

Perhaps also expose it as an option too to allow people to customise it such that new additions can be added outside of releases.

@alexander-akait
Copy link
Member

@jordrake sounds good

@Siilwyn
Copy link
Contributor

Siilwyn commented Oct 12, 2018

@jordrake I already keep a collection of properties up-to-date at css-declaration-sorter by downloading them from the MDN docs, so that should be not much extra effort to do.

@alexander-akait alexander-akait added this to the 4.2 milestone Jan 31, 2019
@alexander-akait
Copy link
Member

Fixed #855

@ludofischer ludofischer modified the milestones: 4.2, backlog, 5.0.0 Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants