fixed problems with colon character #363

FezVrasta · 2013-11-18T21:57:12Z

This pull should fixes problems with the colon character when used on pseudo-classes and other new CSS3 cases.

Before this code:

a:hover {
    color:black;
}
::selection {
    color:black;
}

was converted in:

a: hover {
    color: black;
}
: : selection {
    color: black;
}

Notice the extra white-spaces added after each colon character.
My fix should have fixed this problem.

This pull should fixes problems with the colon character when used on pseudo-classes and other new CSS3 cases.

evocateur · 2013-11-18T22:30:28Z

js/lib/beautify-css.js

+          var test1 = source_text.substr(pos, source_text.indexOf(';') + 1);
+          var test2 = source_text.substr(pos, source_text.indexOf('{') + 1);
+
+          if(test1.length > test2.length) {


Please match the style of the file: 4 spaces per tab, space between if keyword and opening paren.

bitwiseman · 2013-11-18T22:30:44Z

Great, please at least add tests for your fixes (and related inputs). If possible, please translate to python css beautifier as well.

FezVrasta · 2013-11-18T22:38:33Z

I don't know how to add tests and I don't know python. I'll fix the formatting problems.

With my fix there is an edge case bug that I'm trying to fix on Stackoverflow

bitwiseman · 2013-11-19T22:22:04Z

If you can't do the python, that is fine. I can do the translation.

However, we're not taking significant changes in this area without tests to demonstrate proper functioning and protect from future regressions. Here is an example of someone adding tests for css-beautify changes. It is easy and straight-forward.

8d27ea1

FezVrasta · 2013-11-19T22:46:38Z

I must be stupid but I can't understand how the file you have linked me could act as test...

bitwiseman · 2013-11-20T01:40:09Z

Add lines to that file (just like in the commit I linked) that follow the pattern:

btc("input_css", "expected_output_css");

btc() is a helper function that runs the input_css through the beautifier and then checks that the resulting text matches expected_output_css.

FezVrasta · 2013-11-20T10:05:32Z

I hope it's ok how I did.

evocateur · 2013-11-20T18:13:04Z

After writing the tests, you need to run them with npm test. (If you haven't already, run npm install . in the root of your clone before running the tests.)

As written, the tests are failing with this output:

---- css_beautify input -------
#foo : hover{color:purple}
---- css_beautify expected ----
#foo:hover {
  color: purple
}

---- css_beautify output ------
#foo: hover {
  color: purple
}


---- css_beautify input -------
#foo:hover {
  color: purple
}

---- css_beautify expected ----
#foo:hover {
  color: purple
}

---- css_beautify output ------
#foo: hover {
  color: purple
}


---- css_beautify input -------
: : selection {
color: #ff0000;}
---- css_beautify expected ----
::selection {
  color: #ff0000;
}
---- css_beautify output ------
::selection {
  color: #ff0000;
}


---- css_beautify input -------
::selection {
  color: #ff0000;
}
---- css_beautify expected ----
::selection {
  color: #ff0000;
}
---- css_beautify output ------
::selection {
  color: #ff0000;
}


4 tests failed.

You'll notice there are extra line breaks after all of the output, and that the pseudo-class selector is still mangled.

Also, there are still stylistic anomalies in beautify-css.js. You can run js-beautify on it in-place to resolve these discrepancies:

npm -g install js-beautify
js-beautify -jr js/lib/beautify-css.js

FezVrasta · 2013-11-20T18:22:20Z

I've not git on my PC, don't know if I can use npm on Windows even.

By the way I've noticed that the problem is if the file starts with something that doesn't have colon in it... I'll try to fix this problem.

evocateur · 2013-11-20T18:23:05Z

js/test/beautify-tests.js

+
+        // colon character
+        btc("#foo : hover{color:purple}","#foo:hover {\n  color: purple\n}\n");
+        btc(": : selection {\ncolor: #ff0000;}", "::selection {\n  color: #ff0000;\n}");


These inputs are somewhat nonsensical, as they don't represent valid CSS syntax. It would be clearer if you just asserted properly formatted input (and then omit the second parameter, because it defaults to the first).

btc("#foo:hover {\n color: purple\n}\n"); btc("::selection {\n color: #ff0000;\n}\n");

Note that the second test has an added \n, which fixes one breaking test. The #foo:hover case is still breaking.

I thought that the first parameter was the not formatted input and the second one the formatted one. Isn't it?

If the input and output are expected to be identical, all the test methods support simply passing one argument.

My input and output was not identical.

Like I said initially, they should be. Who in their right mind writes invalid CSS syntax and then expects it to magically work?

I get what you mean, but if the beautifier can even remove extra spaces and make the syntax valid I can't see the problem. I didn't added extra code to do it, it's just a side effect of my patch.

Yes, yes, I want to take a part in this bikeshed as well!

The tests by themselves are ok — we have weird corner cases and some broken-code-formatting checks in the jsbeautifier code as well. The strong uneasiness I myself feel is that the tests need to clearly cover the proper input first, but currently there are no tests that make sure that the beautifier formats a clean, proper css code as well.

I.e something like this would make the tests much more structured and clear:

// selector support btc("#foo :hover {...}", "#foo :hover {...}"); ... // handle some garbage as well, just because we can btc(" : : selection", "::selection")

...and then we might have a separate discussion WHY would we ever want to test the garbage cases and what do we achieve by that.

Fair enough. Although, for the purposes of this bugfix, #foo :hover is not valid CSS syntax. Pseudo-classes must always be attached to other selectors (some pseudo-elements (doubled colons), however, can be distinct).

FezVrasta · 2013-11-20T21:08:33Z

I've just written a different patch for this problem, I can't find bugs with this method. The tests should be the same.

… tags that allows custom text

evocateur · 2013-11-20T21:44:26Z

And yet, things are still failing: https://travis-ci.org/einars/js-beautify/jobs/14276649

evocateur · 2013-11-20T21:47:57Z

js/lib/beautify-css.js

@@ -233,7 +233,19 @@
                insideRule = false;
            } else if (ch === ":") {
                eatWhitespace();
-                output.push(ch, " ");
+
+                var text_after_pos = source_text.replace(new RegExp("('|\").*('|\")", "gm"), "").substr(pos - 1) + ";",


Running a global replace with a non-reusable regexp on the source content every time a colon is encountered is dramatically bad practice.

FezVrasta · 2013-11-20T21:50:43Z

This one is a problem of the test, the correct output is the one provided as css_beautify output. Notice the space between the colon and the not.

---- css_beautify input -------
a: not("foobar\";{}omg"){
content: 'example\';{} text';
content: "example\";{} text";}
---- css_beautify expected ----
a: not("foobar\";{}omg") {
    content: 'example';{} text';
    content: "example\";{} text";
}
---- css_beautify output ------
a:not("foobar\";{}omg") {
  content: 'example\';{} text';
  content: "example\";{} text";
}

The problem with the document starting with double dots is a problem that I will fix asap.
The other error is a typo of my test, there is a single quote between two single quotes, and this is a probem because makes the CSS not valid.

About bad practice etc:

honestly I think that keep the beautifier as-is, with lot of bugs and cases not supported is even worse than have some hack inside the code. But this is your choice. Personally I will use my patch for my projects.

There's not a function because would be useless to have a function that is called only one time, just because it's very specific as I've said before.

FezVrasta · 2013-11-20T21:59:32Z

About the regex, maybe it could be performed only one time as first action of the beautifier. This would make everything faster and nicer.

…g with colon

bitwiseman · 2013-11-21T01:00:34Z

Regex over the entire input once first? Um, no.

evocateur · 2013-11-21T01:42:07Z

Interestingly, as long as one does not attempt to fix illegal syntax, I was able to accomplish this with far less confusing code:

diff --git a/js/lib/beautify-css.js b/js/lib/beautify-css.js
index 8830659..4e23dc6 100644
--- a/js/lib/beautify-css.js
+++ b/js/lib/beautify-css.js
@@ -204,6 +204,7 @@
         /*_____________________--------------------_____________________*/

         var insideRule = false;
+        var blockLevel = 0;
         while (true) {
             var isAfterSpace = skipWhitespace();

@@ -226,27 +227,29 @@
                 } else {
                     indent();
                     print["{"](ch);
+                    blockLevel += 1;
                 }
             } else if (ch === '}') {
                 outdent();
                 print["}"](ch);
                 insideRule = false;
+                blockLevel -= 1;
             } else if (ch === ":") {
                 eatWhitespace();
-
-                var text_after_pos = source_text.replace(new RegExp("('|\").*('|\")", "gm"), "").substr(pos - 1) + ";",
-                    semicolon = text_after_pos.substr(0, text_after_pos.indexOf(';')).length,
-                    closed_brace = text_after_pos.substr(0, text_after_pos.indexOf('}')).length,
-                    open_brace = text_after_pos.substr(0, text_after_pos.indexOf('{')).length,
-                    test1 = (semicolon > closed_brace) ? closed_brace : semicolon;
-
-                if (test1 > open_brace && open_brace !== 0) {
-                    output.push(ch);
-                } else {
+                if (blockLevel) {
+                    // 'property: value' delimiter
                     output.push(ch, " ");
+                    insideRule = true;
+                } else {
+                    if (peek() === ":") {
+                        // pseudo-element
+                        next();
+                        output.push("::");
+                    } else {
+                        // pseudo-class
+                        output.push(ch);
+                    }
                 }
-
-                insideRule = true;
             } else if (ch === '"' || ch === '\'') {
                 output.push(eatString(ch));
             } else if (ch === ';') {
diff --git a/js/test/beautify-tests.js b/js/test/beautify-tests.js
index 69c9678..6de0f0b 100755
--- a/js/test/beautify-tests.js
+++ b/js/test/beautify-tests.js
@@ -1760,14 +1760,21 @@ function run_beautifier_tests(test_obj, Urlencoded, js_beautify, html_beautify,

         btc("#bla, #foo{color:green}", "#bla, #foo {\n  color: green\n}\n");
         btc("@media print {.tab{}}", "@media print {\n  .tab {}\n}\n");
+        btc("@media print {.tab,.bat{}}", "@media print {\n  .tab, .bat {}\n}\n");
+        // btc("@media screen {.tab,.bat:hover {color:red}}", "@media screen {\n  .tab, .bat:hover {\n    color: red\n  }\n}\n");
         btc("#bla, #foo{color:black}", "#bla, #foo {\n  color: black\n}\n");
-        
+
         // colon character
-        btc("#foo : hover{color:purple}","#foo:hover {\n  color: purple\n}\n");
-        btc(": : selection {\ncolor: #ff0000;}", "::selection {\n  color: #ff0000;\n}");
-        
+        btc("#foo:hover {\n  color: purple\n}\n");
+        btc("#foo *:hover {\n  color: purple\n}\n");
+        btc("::selection {\n  color: #ff0000;\n}\n");
+
+        // btc("#foo : hover{color:purple}","#foo:hover {\n  color: purple\n}\n");
+        // btc(": : selection {\ncolor: #ff0000;}", "::selection {\n  color: #ff0000;\n}");
+
         // particular edge case with braces and semicolons inside tags that allows custom text
-        btc("a: not(\"foobar\\\";{}omg\"){\ncontent: 'example\\';{} text';\ncontent: \"example\\\";{} text\";}", "a: not(\"foobar\\\";{}omg\") {\n    content: 'example\\';{} text';\n    content: \"example\\\";{} text\";\n}");
+        btc("a:not(\"foobar\\\";{}omg\"){\ncontent: 'example\\';{} text';\ncontent: \"example\\\";{} text\";}",
+            "a:not(\"foobar\\\";{}omg\") {\n  content: 'example\\';{} text';\n  content: \"example\\\";{} text\";\n}\n");

         return sanitytest;
     }

With one huge caveat

If you uncomment the commented @media test I added, you'll find that nested blocks fail with my simplified code. Fairly certain they fail with the existing patch, as well.

evocateur · 2013-11-21T01:49:57Z

Anyway, some really useful links on this subject:

https://developer.mozilla.org/en-US/docs/Web/CSS/Syntax
https://developer.mozilla.org/en-US/docs/Web/CSS/At-rule
https://developer.mozilla.org/en-US/docs/Web/CSS/Pseudo-classes
https://developer.mozilla.org/en-US/docs/Web/CSS/Pseudo-elements

Problems like these almost convince me we should be using a parser like https://github.com/visionmedia/css

FezVrasta · 2013-11-22T10:31:16Z

With my patch it doesn't fail, but it adds "tabs" instead of spaces, I really don't know why.

---- css_beautify input -------
@media screen {.tab,.bat:hover {color:red}}
---- css_beautify expected ----
@media screen {
  .tab, .bat:hover {
    color: red
  }
}
---- css_beautify output ------
@media screen {
    .tab,
    .bat:hover {
        color: red
    }
}

evocateur · 2013-11-23T01:05:02Z

Since your tests never passed, I fixed it with a much simpler algorithm that doesn't allow malformed syntax, but does support nesting blocks. Thanks for raising the issue.

fixed problems with colon character

14743e1

This pull should fixes problems with the colon character when used on pseudo-classes and other new CSS3 cases.

evocateur reviewed Nov 18, 2013
View reviewed changes

fixed formatting troubles

43c06e9

added tests for colon character

306f321

evocateur reviewed Nov 20, 2013
View reviewed changes

new method to fix the colon bug

792c615

FezVrasta added 3 commits November 20, 2013 22:21

fixed edge case with content and not elements

7b529dc

added test for particular edge case with braces and semicolons inside…

eab7b74

… tags that allows custom text

removed commented console.log

b457034

evocateur reviewed Nov 20, 2013
View reviewed changes

fixed typo

2eb49a2

FezVrasta added 3 commits November 20, 2013 23:36

moved regex at top of the script and fixed bug with documents startin…

398843b

…g with colon

Update beautify-css.js

e0ca5ad

fixed extra space in test

c80476f

This was referenced Nov 20, 2013

fixed classes bug brackets-beautify/brackets-beautify#30

Merged

Problem with "content" #364

Closed

Update beautify-tests.js

10bee4d

evocateur closed this in f416b0b Nov 23, 2013

drewhamlett mentioned this pull request Nov 28, 2013

Use a better beauitifer brackets-beautify/brackets-beautify#34

Closed

bitwiseman added this to the v1.5.0 milestone Apr 5, 2014

bitwiseman mentioned this pull request Apr 5, 2014

Port css colon character fix to python #446

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed problems with colon character #363

fixed problems with colon character #363

FezVrasta commented Nov 18, 2013

evocateur Nov 18, 2013

bitwiseman commented Nov 18, 2013

FezVrasta commented Nov 18, 2013

bitwiseman commented Nov 19, 2013

FezVrasta commented Nov 19, 2013

bitwiseman commented Nov 20, 2013

FezVrasta commented Nov 20, 2013

evocateur commented Nov 20, 2013

FezVrasta commented Nov 20, 2013

evocateur Nov 20, 2013

FezVrasta Nov 20, 2013

evocateur Nov 20, 2013

FezVrasta Nov 20, 2013

evocateur Nov 20, 2013

FezVrasta Nov 20, 2013

einars Nov 20, 2013

evocateur Nov 21, 2013

FezVrasta commented Nov 20, 2013

evocateur commented Nov 20, 2013

evocateur Nov 20, 2013

FezVrasta commented Nov 20, 2013

FezVrasta commented Nov 20, 2013

bitwiseman commented Nov 21, 2013

evocateur commented Nov 21, 2013

evocateur commented Nov 21, 2013

FezVrasta commented Nov 22, 2013

evocateur commented Nov 23, 2013

fixed problems with colon character #363

fixed problems with colon character #363

Conversation

FezVrasta commented Nov 18, 2013

Choose a reason for hiding this comment

bitwiseman commented Nov 18, 2013

FezVrasta commented Nov 18, 2013

bitwiseman commented Nov 19, 2013

FezVrasta commented Nov 19, 2013

bitwiseman commented Nov 20, 2013

FezVrasta commented Nov 20, 2013

evocateur commented Nov 20, 2013

FezVrasta commented Nov 20, 2013

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

Choose a reason for hiding this comment

FezVrasta commented Nov 20, 2013

evocateur commented Nov 20, 2013

Choose a reason for hiding this comment

FezVrasta commented Nov 20, 2013

FezVrasta commented Nov 20, 2013

bitwiseman commented Nov 21, 2013

evocateur commented Nov 21, 2013

With one huge caveat

evocateur commented Nov 21, 2013

FezVrasta commented Nov 22, 2013

evocateur commented Nov 23, 2013