Make css-declaration-sorter safer #580

Siilwyn · 2018-08-11T18:22:29Z

Don't change the order of short- & longhand properties with the exception of margin,
padding & border since these are handled by the mergeLonghand optimisation.

Closes #535

zanettin · 2018-08-30T13:01:01Z

any estimates, when this get merged? 👼

alexander-akait · 2018-08-30T13:18:05Z

/cc @Siilwyn what is status?

Siilwyn · 2018-09-02T11:44:01Z

Still in progress, mostly doing local stuff. Will try to push more often. Though I'm also thinking of implementing it at the module instead of here.

zanettin · 2018-09-11T13:37:56Z

thanks for your work on this. the "new" sort logic blocks us from upgrading to postcss v7 because the working version of cssnano (v3.10) relys on a fn which is no longer available on postcss v7.

Closes #535

coveralls · 2018-09-16T11:54:42Z

Coverage increased (+0.0005%) to 98.559% when pulling 3025267 on Siilwyn:safer-sorting into cd94901 on cssnano:master.

Siilwyn · 2018-09-16T12:10:18Z

@andyjansson ready for review! :)

andyjansson · 2018-09-16T13:19:40Z

packages/cssnano-preset-advanced/src/__tests__/css-declaration-sorter.js

+    'sorts both short- and longhand properties',
+    processCSS,
+    'h1{animation-name:a;animation:b}',
+    'h1{animation:b;animation-name:a}',


This is wrong. The code needs to respect precedence. You can't just sort based on the relationship between the properties (what's a longhand or a shorthand of what), but you need to respect the order in which they are declared.

andyjansson · 2018-09-16T13:30:35Z

packages/cssnano-preset-default/src/index.js

@@ -49,6 +51,9 @@ const defaultOpts = {
    normalizeCharset: {
        add: false,
    },
+    cssDeclarationSorter: {
+        customOrder: path.join(__dirname, 'safe-order.json'),


The declarations cannot be sorted based on a sort order defined in a json file. They need to respect the order of declaration in the CSS, else the precedence will be different and the CSS will render differently in the browser.

Note that I'm leaving out the declarations that need to be kept in their original order, so this will work. Do you have an example that will fail?

andyjansson · 2018-09-16T14:43:41Z

packages/cssnano-preset-default/src/__tests__/css-declaration-sorter.js

+    processCSS,
+    'h1{animation-name:a;z-index:0;animation:b;color:0;}',
+    'h1{animation-name:a;animation:b;color:0;z-index:0}'
+);


We need a lot more tests here.

Could you provide a bit more descriptive feedback? Like what should be tested & what kind of tests?

Note that the module itself has tests too: https://github.com/Siilwyn/css-declaration-sorter/blob/master/tests/test.js

We need a range of tests to ensure that the module functions as expected, and to detect any future regressions. I expect tests covering cases where declarations should be rearranged, cases where they shouldn't, and cases with declarations of mixed precedence.

The first test covers "cases where they shouldn't", the second has mixed precedence and declarations that should be rearranged right? I can split up the 2nd test to clarify the difference, that might be better.

Sorry but I still don't get what you're after. Both tests cover the cases you mentioned earlier.

I'm after more tests so we can have confidence in the feature. We need tests to capture its function and any edge cases that might lurk about. Two test cases aren't nearly enough for that.

@Siilwyn when can you add more tests?

\cc @evilebottnawi

Yep need more tests, example with border/padding/margin, also all tests from related issues and prefixes

Once all other comments and questions are resolved I'll add tests for the border/padding/margin. 👍

"all tests from related issues and prefixes", there is a test for the prefixing, any related issues I need to be aware of?

andyjansson · 2018-09-16T14:48:11Z

packages/cssnano-preset-default/src/__tests__/integrations/basscss5.css

@@ -18,13 +18,13 @@ th{font-weight:700;text-align:left}
 td,th{line-height:inherit;padding:.25rem 1rem}
 th{vertical-align:bottom}
 td{vertical-align:top}
-body{font-size:100%;line-height:1.5}
+body{line-height:1.5;font-size:100%}


Some strange reordering of declarations in all integration tests. Shouldn't these be grouped alphabetically?

No, since font-size could conflict with font it is not being moved. (Check my comment on the issue too: "For now keep all properties that have a shorthand in the same order")

What do you mean? There' s no font declaration here.

It doesn't look at a selector by selector basis, the way this PR makes it safer is by not sorting properties that could conflict at all.

I really don't know what you mean, and there's no tests to infer meaning from either. Can you expand on what you're saying?

Ah sorry I missed this reply.

I'll add an extra test, what I mean is that any property that has a conflicting property will not be sorted.

I don't understand what you mean by conflicting. Conflicting with what? The functionality, as it stands, is completely opaque and non-intuitive. The tests aren't communicating its behaviour, and that means we can't have any confidence in its result.

Clarified this behaviour with a test: move properties that are not in safe-order.json to the start and sort remaining. Properties that are not 'safe', in other words: properties that have a shorthand/longhand are moved to the start. What I meant with conflicting is for a value that can be changed by multiple properties such as the font-size in this example because it can also be changed with font. These properties 'conflict', maybe it's a bad word to describe this, I'm not a native speaker. :)

zanettin · 2018-09-16T17:13:25Z

thanks @Siilwyn for all your work on this ❤️ just one tiny input about sorting (not sure if you already covered this case), but in the current version of cssnano, all vendor prefixed properties are pushed to the beginning which breaks some code of us:

working code:

background: red;
 -webkit-background-clip: text;

broken after sorting:

 -webkit-background-clip: text;
background: red;

would be very nice, if you could make sure that this works as well ❤️ and again - thank you very much!!!

Siilwyn · 2018-09-16T18:27:47Z

Thank you for your reply @zanettin your reply is much appreciated. :)

This is covered by this PR, sharp! I'll add a test to clarify this.

andyjansson · 2018-09-17T16:03:15Z

packages/cssnano-preset-advanced/src/index.js

@@ -9,6 +9,9 @@ const defaultOpts = {
    autoprefixer: {
        add: false,
    },
+    cssDeclarationSorter: {
+        customOrder: null,


Explain your reasoning behind having a different sorting order to the advanced preset and the consequences it'll have.

The advanced order can make unsafe changes to save more bytes, so it orders everything.

That's not really what the advanced preset is about, though. See the website on 'advanced transforms' for more info.

From the advanced transform page: "Certain optimizations are not suitable for all use cases; unlike those that we bundle by default, advanced transforms all carry a certain amount of risk.". This exactly falls into this category. When using legacy code with double properties that override each other sorting everything comes with a certain amount of risk.

The risk with the advanced preset comes from mixing sources. When working within the same stylesheet, it should be safe. The advanced preset is not "bug mode".

It does not only come from mixing sources, to be specific quoting the same page:

This transform changes CSS semantically; it may remove styles based on certain characteristics of the CSS, or it might update values to make the CSS smaller overall. Depending on exactly what the transform does, this may be undesirable because it might change the appearance of your website in certain use cases.

Is what this optimization falls under, I think reducing the size here further is worth the tiny risk of breaking legacy code which other optimizations in this preset also do.

Thoughts @ai @evilebottnawi ?

Certain optimizations are not suitable for all use cases [...] we've documented the assumptions that it must make about your CSS in order for the transform to be safe. [...] these transforms fall into either or both of categories.

Assumes concatenation
[...] assumes that the CSS passed through cssnano is all that is needed for a website to run [...]
may be problematic if the styles you are writing in some way interact with third party styles, or you are using multiple stylesheets instead of concatenating them

Changes semantics

may remove styles based on certain characteristics of the CSS, or it might update values to make the CSS smaller overall. [...] this may be undesirable [...]
A good example is autoprefixer

What you are going is against the spirit of the preset. It has clear qualifiers on what it takes to use the advanced preset safely. It's not supposed to be a roll of the dice on whether it works or not. Please respect that.

It's not a "roll of the dice" though, it is not safe when the CSS contains conflicting properties in one selector. I don't see how this is different from e.g. autoprefixer that removes outdated properties that is unsafe for CSS that contains outdated properties.

it is not safe when the CSS contains conflicting properties in one selector.

Yeah, we can't have that. It's a super-common thing in CSS to have properties that affect the same space. Bringing this into the advanced preset would destroy any use case it has, rendering it useless.

So being not super-common it would be useful... Is it super-common? For one, this issue took a while to surface which I think partly indicates this is uncommon. Secondly as far as I know CSS selectors normally do not contain properties that override each other, it's usually a mistake, e.g:

animation: bla; animation-name: bla;

The animation property also contains the name so instead of putting animation-name after it normally I would just change the name in the animation shorthand value.

I understand your concern though, this at least should be clarified in the docs. It comes down to if the benefit outweighs the risk, if it is common as you said then I see how it is not worth it. For me seeing CSS like this is pretty new, however I've also only been a front-end dev for 5 years and mainly worked with newish codebases.

alexander-akait · 2018-09-17T16:11:32Z

packages/cssnano/src/__tests__/fixtures.js

@@ -76,7 +76,7 @@ test(
    h2, h1 {
        font-weight: 400;
    }`,
-    `h1,h2{color:red;font-weight:400}`
+    `h1,h2{font-weight:400;color:red}`


Why it is happens?

Clarified this behaviour with a test: move properties that are not in safe-order.json to the start and sort remaining in bfe1257. Properties that are not 'safe', in other words properties that have a shorthand/longhand are moved to the start.

alexander-akait

The main idea of this plugin is to maximize the alphabetical order to better gzip, here i find many strange cases where we broke alphabetical order without reasons. Maybe we need use better logic here.

alexander-akait · 2018-09-17T16:13:04Z

packages/cssnano-preset-advanced/README.md

+
+```js
+{
+	customOrder: null


Don't think what we need this option, no one will constantly keep their list, it's too time consuming and takes a lot of time and effort, let's remove it from here

What do you mean? It's there so that the advanced preset does full sorting resulting in more optimization. Note that this is generated documentation too, no way to hide it.

alexander-akait · 2018-09-17T16:13:48Z

packages/cssnano-preset-default/package.json

@@ -4,7 +4,7 @@
  "main": "dist/index.js",
  "description": "Safe defaults for cssnano which require minimal configuration.",
  "scripts": {
-    "prepublish": "cross-env BABEL_ENV=publish babel src --out-dir dist --ignore /__tests__/"
+    "prepublish": "cross-env BABEL_ENV=publish babel src --out-dir dist --ignore /__tests__/ --copy-files"


Why it is here?

So that the custom order will get copied, babel normally ignores JSON files.

alexander-akait · 2018-09-17T16:14:13Z

packages/cssnano-preset-default/src/__tests__/css-declaration-sorter.js

+    processCSS,
+    'h1{animation-name:a;z-index:0;animation:b;color:0;}',
+    'h1{animation-name:a;animation:b;color:0;z-index:0}'
+);


Yep need more tests, example with border/padding/margin, also all tests from related issues and prefixes

Siilwyn · 2018-09-17T19:41:02Z

@evilebottnawi please read my last replies on the related issue, this PR uses simple ordering for now. As you see in some cases it can't order alphabetically, this is intended.

ai · 2018-09-18T20:03:06Z

The main idea of this plugin is to maximize the alphabetical order to better gzip, here i find many strange cases where we broke alphabetical order without reasons. Maybe we need use better logic here.

Unfortunately, it doesn’t work :(. We have an issue with this sorter right now in our current project (temporarily fixed with !important).

To be honest, this issue is really a high priority and I recommend to fix it ASAP.

ai · 2018-09-18T20:10:30Z

Maybe we should disable the rule, release a fix and then continue to discuss?

CSS minifier must be safe. People should trust it. So it is very important to fix safety problems ASAP.

andyjansson · 2018-09-19T00:57:44Z

@ai merging #599 would disable css-declaration-sorter.

@evilebottnawi @ai should we proceed with this course of action?

ai · 2018-09-19T05:20:22Z

@andyjansson sorry, what do you mean about course of actions? (I am very bad in language ileomes)

andyjansson · 2018-09-19T06:12:19Z

@ai whether we should proceed with disabling css-declaration-sorter

ai · 2018-09-19T06:50:24Z

I think we should disable this plugin at least for default preset

Siilwyn · 2018-09-19T07:57:06Z

@ai do you still have this issue with the changes made in this PR?

If not, we can merge it with safe sorting in default and advanced, and continue the discussion about the advanced preset after since you want to fix it ASAP. 🐎

@andyjansson thoughts?

ai · 2018-09-19T15:59:26Z

@Siilwyn yeap, accepting this PR is an option too

@andyjansson please, could we release it faster. It is a few months until the issue got reported. It is critical. We need to fix it ASAP. Disable plugin or accept this PR.

Siilwyn · 2018-09-20T08:46:06Z

Hey it seems it got disabled looking at master, would like to know what needs to change for this PR to be accepted. It's been open for so long it's weighing down on me. :/

andyjansson · 2018-09-20T15:51:07Z

Read the changes we've requested. You've marked the threads as "fixed" although they haven't been.

Siilwyn · 2018-09-22T10:05:09Z

@andyjansson it is unclear for me when conversations are resolved, I have unresolved and replied to all of them. Could you go through them to resolve or give feedback? That would be great.

Edit: I've removed border properties because of #592.

Siilwyn · 2018-10-02T17:14:10Z

So @evilebottnawi if I understand it correctly this PR will not be merged because it is not optimal? Even though the current state of disabling the plugin is even worse in terms of optimizations.

Or does it depend on Siilwyn/css-declaration-sorter#64? We can discuss it there too if you want.

Siilwyn · 2018-10-29T10:54:29Z

☝️ would love a reply.

Will close and delete this PR if there is no chance of getting merged.

alexander-akait · 2018-10-29T10:55:58Z

@Siilwyn sorry, don't have time right now

alexander-akait requested a review from andyjansson August 12, 2018 19:40

Retain longhand and shorthand properties order

d08d1d1

Closes #535

Only show basename of given custom order path

52c0ece

Siilwyn changed the title ~~WIP: make css-declaration-sorter safer~~ Make css-declaration-sorter safer Sep 16, 2018

Siilwyn mentioned this pull request Sep 16, 2018

Meta improvements #601

Merged

andyjansson reviewed Sep 16, 2018

View reviewed changes

Test if prefixed properties keep order

98d6f4a

andyjansson reviewed Sep 17, 2018

View reviewed changes

alexander-akait requested changes Sep 17, 2018

View reviewed changes

Siilwyn added 3 commits September 18, 2018 21:12

Clean up tests formatting

e5c6a2d

Add requested margin, padding and border tests

26bcae8

Test sorting both ways

85e76f0

Use default safe sorting for advanced

3025267

Add tests to clarify internal behavior

bfe1257

Remove border from safe properties

3b6062d

Siilwyn mentioned this pull request Oct 2, 2018

Allow order option to be function Siilwyn/css-declaration-sorter#64

Closed

Siilwyn closed this Dec 12, 2019

Siilwyn mentioned this pull request Dec 12, 2019

Upgrade css declaration sorter #855

Merged

Make css-declaration-sorter safer #580

Make css-declaration-sorter safer #580

Conversation

Siilwyn commented Aug 11, 2018 • edited

zanettin commented Aug 30, 2018

alexander-akait commented Aug 30, 2018

Siilwyn commented Sep 2, 2018

zanettin commented Sep 11, 2018

coveralls commented Sep 16, 2018 • edited

Siilwyn commented Sep 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siilwyn Sep 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siilwyn Sep 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait Sep 17, 2018 • edited

Choose a reason for hiding this comment

Siilwyn Sep 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siilwyn Sep 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanettin commented Sep 16, 2018

Siilwyn commented Sep 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siilwyn Sep 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siilwyn Sep 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siilwyn Sep 22, 2018 • edited

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Siilwyn Sep 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait Sep 17, 2018 • edited

Choose a reason for hiding this comment

Siilwyn commented Sep 17, 2018 • edited

ai commented Sep 18, 2018

ai commented Sep 18, 2018

andyjansson commented Sep 19, 2018

ai commented Sep 19, 2018

andyjansson commented Sep 19, 2018

ai commented Sep 19, 2018

Siilwyn commented Sep 19, 2018 • edited

ai commented Sep 19, 2018

Siilwyn commented Sep 20, 2018 • edited

andyjansson commented Sep 20, 2018

Siilwyn commented Sep 22, 2018 • edited

Siilwyn commented Oct 2, 2018

Siilwyn commented Oct 29, 2018 • edited

alexander-akait commented Oct 29, 2018

Siilwyn commented Aug 11, 2018 •

edited

coveralls commented Sep 16, 2018 •

edited

Siilwyn Sep 16, 2018 •

edited

Siilwyn Sep 16, 2018 •

edited

alexander-akait Sep 17, 2018 •

edited

Siilwyn Sep 17, 2018 •

edited

Siilwyn Sep 16, 2018 •

edited

Siilwyn Sep 18, 2018 •

edited

Siilwyn Sep 18, 2018 •

edited

Siilwyn Sep 22, 2018 •

edited

Siilwyn Sep 17, 2018 •

edited

alexander-akait Sep 17, 2018 •

edited

Siilwyn commented Sep 17, 2018 •

edited

Siilwyn commented Sep 19, 2018 •

edited

Siilwyn commented Sep 20, 2018 •

edited

Siilwyn commented Sep 22, 2018 •

edited

Siilwyn commented Oct 29, 2018 •

edited