Web Inspector: REGRESSION(r246178): extra spaces added in at-rules when formatting CSS
authordrousso@apple.com <drousso@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 14 Jun 2019 06:52:07 +0000 (06:52 +0000)
committerdrousso@apple.com <drousso@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 14 Jun 2019 06:52:07 +0000 (06:52 +0000)
https://bugs.webkit.org/show_bug.cgi?id=198806

Reviewed by Joseph Pecoraro.

Source/WebInspectorUI:

* UserInterface/Workers/Formatter/CSSFormatter.js:
(CSSFormatter.prototype._format):
Add more specific tests for at-rules, and add/remove whitespace depending on the type of
at-rule (e.g. `@supports` vs `@media`), as well as where the scanner is in the parameters of
the at at-rule (e.g. `@supports |` vs `@media (|`).

* UserInterface/Workers/Formatter/FormatterContentBuilder.js:
(FormatterContentBuilder):
(FormatterContentBuilder.prototype.get lastToken): Added.
(FormatterContentBuilder.prototype.get currentLine):
(FormatterContentBuilder.prototype.removeLastNewline):
(FormatterContentBuilder.prototype.removeLastWhitespace):
(FormatterContentBuilder.prototype._popFormattedContent):
(FormatterContentBuilder.prototype._append):
Update `lastTokenWasNewline` and `lastTokenWasWhitespace` when removing newlines/whitespace.
Memoize the `currentLine` so it's less expensive to re-fetch.

LayoutTests:

* inspector/formatting/resources/css-tests/keyframes.css:
* inspector/formatting/resources/css-tests/keyframes-expected.css:
* inspector/formatting/resources/css-tests/media-query.css:
* inspector/formatting/resources/css-tests/media-query-expected.css:
* inspector/formatting/resources/css-tests/selectors.css:
* inspector/formatting/resources/css-tests/selectors-expected.css:
* inspector/formatting/resources/css-tests/wrapping.css:
* inspector/formatting/resources/css-tests/wrapping-expected.css:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@246430 268f45cc-cd09-0410-ab3c-d52691b4dbfc

12 files changed:
LayoutTests/ChangeLog
LayoutTests/inspector/formatting/resources/css-tests/keyframes-expected.css
LayoutTests/inspector/formatting/resources/css-tests/keyframes.css
LayoutTests/inspector/formatting/resources/css-tests/media-query-expected.css
LayoutTests/inspector/formatting/resources/css-tests/media-query.css
LayoutTests/inspector/formatting/resources/css-tests/selectors-expected.css
LayoutTests/inspector/formatting/resources/css-tests/selectors.css
LayoutTests/inspector/formatting/resources/css-tests/wrapping-expected.css
LayoutTests/inspector/formatting/resources/css-tests/wrapping.css
Source/WebInspectorUI/ChangeLog
Source/WebInspectorUI/UserInterface/Workers/Formatter/CSSFormatter.js
Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js

index a3cf515..fcb7e12 100644 (file)
@@ -1,3 +1,19 @@
+2019-06-13  Devin Rousso  <drousso@apple.com>
+
+        Web Inspector: REGRESSION(r246178): extra spaces added in at-rules when formatting CSS
+        https://bugs.webkit.org/show_bug.cgi?id=198806
+
+        Reviewed by Joseph Pecoraro.
+
+        * inspector/formatting/resources/css-tests/keyframes.css:
+        * inspector/formatting/resources/css-tests/keyframes-expected.css:
+        * inspector/formatting/resources/css-tests/media-query.css:
+        * inspector/formatting/resources/css-tests/media-query-expected.css:
+        * inspector/formatting/resources/css-tests/selectors.css:
+        * inspector/formatting/resources/css-tests/selectors-expected.css:
+        * inspector/formatting/resources/css-tests/wrapping.css:
+        * inspector/formatting/resources/css-tests/wrapping-expected.css:
+
 2019-06-13  Antoine Quint  <graouts@apple.com>
 
         REGRESSION (r246103) [ Mojave+ WK1 ] Layout Test scrollbars/scrollbar-iframe-click-does-not-blur-content.html is timing out
index e710672..ca38fa0 100644 (file)
@@ -8,3 +8,13 @@
     }
 }
 
+@-webkit-keyframes spin {
+    0% {
+        -webkit-transform: rotate(-180deg) translate(0px, 0px);
+    }
+
+    100% {
+        -webkit-transform: rotate(180deg) translate(10px, 75px);
+    }
+}
+
index bd88b56..669aab5 100644 (file)
@@ -1 +1,2 @@
 @-webkit-keyframes spin{0%{-webkit-transform:rotate(-180deg)translate(0px,0px);}100%{-webkit-transform:rotate(180deg)translate(10px,75px);}}
+@-webkit-keyframes   spin   {   0%   {   -webkit-transform   :   rotate   (   -180deg   )   translate   (   0px   ,   0px   )   ;   }   100%   {   -webkit-transform   :   rotate   (   180deg   )   translate   (   10px   ,   75px   )   ;   }   }
index 02ebe93..d49c123 100644 (file)
 }
 
 /* MEDIA QUERY */
-@media screen and (max-device-width:480px) {
+@media screen and (max-device-width: 480px) {
+    html {
+        -webkit-text-size-adjust: none;
+    }
+}
+
+@media screen and (max-device-width: 480px) {
     html {
         -webkit-text-size-adjust: none;
     }
     }
 }
 
+@media not ((screen) and (print)), (print) {
+    body {
+        color: red
+    }
+}
+
+/* font-face */
+@font-face {
+    font-family: "MyWebFont";
+    src: url("myfont.woff2") format("woff2"), url("myfont.woff") format("woff");
+}
+
+/* page */
+@page :first {
+    margin: 1in;
+}
+
+@page :first {
+    margin: 1in;
+}
+
+/* supports */
+@supports (display: flex) {
+    .module {
+        display: flex;
+    }
+}
+
+@supports (display: flex) {
+    .module {
+        display: flex;
+    }
+}
+
+@supports (-webkit-backdrop-filter: blur(10px)) {
+    .home header {
+        -webkit-backdrop-filter: blur(10px);
+    }
+}
+
+@supports (-webkit-backdrop-filter: blur(10px)) {
+    .home header {
+        -webkit-backdrop-filter: blur(10px);
+    }
+}
+
index b86bcff..a740d3d 100644 (file)
@@ -3,4 +3,23 @@
 
 /* MEDIA QUERY */
 @media screen and(max-device-width:480px){html{-webkit-text-size-adjust:none;}}
+@media   screen   and   (   max-device-width   :   480px   )   {   html   {   -webkit-text-size-adjust   :   none   ;   }   }
 @media not((screen)and(print)),(print){body{color:red}}
+@media   not   (   (   screen   )   and   (   print   )   )   ,   (   print   )   {   body   {   color   :   red   }   }
+
+/* font-face */
+@font-face {
+    font-family: "MyWebFont";
+    src: url("myfont.woff2") format("woff2"),
+         url("myfont.woff") format("woff");
+}
+
+/* page */
+@page :first{margin:1in;}
+@page   :first   {   margin   :   1in   ;   }
+
+/* supports */
+@supports(display:flex){.module{display:flex;}}
+@supports   (   display   :   flex   )   {   .module   {   display   :   flex   ;   }   }
+@supports(-webkit-backdrop-filter:blur(10px)){.home header{-webkit-backdrop-filter:blur(10px);}}
+@supports   (   -webkit-backdrop-filter   :   blur   (   10px   )   )   {   .home   header   {   -webkit-backdrop-filter   :   blur   (   10px   )   ;   }   }
index 0420181..f3b745e 100644 (file)
@@ -3,7 +3,35 @@ a {
 }
 
 /* COMPLEX SELECTOR */
+div div > div#id.foo.bar:hover .something > .child ~ .sibling + .sibling:after {
+    color: red;
+}
+
+div div > div#id.foo.bar:hover .something > .child ~ .sibling + .sibling:after {
+    color: red;
+}
+
 div div > div#id.foo.bar:hover .something > .child ~ .sibling + .sibling::after {
     color: red;
 }
 
+div div > div#id.foo.bar:hover .something > .child ~ .sibling + .sibling::after {
+    color: red;
+}
+
+div div > div#id.foo.bar:hover .something > .child ~ .sibling + :matches(.sibling):after {
+    color: red;
+}
+
+div div > div#id.foo.bar:hover .something > .child ~ .sibling + :matches(.sibling):after {
+    color: red;
+}
+
+div div > div#id.foo.bar:hover .something > .child ~ .sibling + :matches(.sibling)::after {
+    color: red;
+}
+
+div div > div#id.foo.bar:hover .something > .child ~ .sibling + :matches(.sibling)::after {
+    color: red;
+}
+
index 5a3ef7c..252a91b 100644 (file)
@@ -2,4 +2,11 @@
 a{}
 
 /* COMPLEX SELECTOR */
+div div>div#id.foo.bar:hover .something>.child~.sibling+.sibling:after{color:red;}
+div   div   >   div#id.foo.bar:hover   .something   >   .child   ~   .sibling   +   .sibling:after   {   color   :   red   ;   }
 div div>div#id.foo.bar:hover .something>.child~.sibling+.sibling::after{color:red;}
+div   div   >   div#id.foo.bar:hover   .something   >   .child   ~   .sibling   +   .sibling::after   {   color   :   red   ;   }
+div div>div#id.foo.bar:hover .something>.child~.sibling+:matches(.sibling):after{color:red;}
+div   div   >   div#id.foo.bar:hover   .something   >   .child   ~   .sibling   +   :matches(.sibling):after   {   color   :   red   ;   }
+div div>div#id.foo.bar:hover .something>.child~.sibling+:matches(.sibling)::after{color:red;}
+div   div   >   div#id.foo.bar:hover   .something   >   .child   ~   .sibling   +   :matches(.sibling)::after   {   color   :   red   ;   }
index 795d5dc..ccdf713 100644 (file)
@@ -7,3 +7,8 @@ a.browsewebappss:hover, a.businessstores:hover, a.buyiphones:hover, a.buynows:ho
     color: red
 }
 
+/* NEWLINE-SEPARATED SELECTORS SHOULD COMBINE */
+h1, h2, h3, h4, h5, h6 {
+    font-size: 1em
+}
+
index be6bf9d..d601db3 100644 (file)
@@ -1,3 +1,11 @@
 /* LONG LISTS SHOULDN'T WRAP */
 a.browsewebappss,a.businessstores,a.buyiphones,a.buynows,a.buynows-arrow,a.comingsoons,p::before,a.descargarahoras,a.downloadituness,a.downloadnows,a.finds,a.freetrials,a.getstarteds,a.gos,a.howtoapplys,a.howtobuys,a.joinnows,a.learnmores,a.nikebuynows,a.notifymes,a.ordernows,a.preordernows,a.preorders,a.reserves,a.startyoursearchs,a.submits,a.tryamacs,a.upgradenows {color:red}
 a.browsewebappss:hover,a.businessstores:hover,a.buyiphones:hover,a.buynows:hover,a.buynows-arrow:hover,a.comingsoons:hover,p::before,a.descargarahoras:hover,a.downloadituness:hover,a.downloadnows:hover,a.finds:hover,a.freetrials:hover,a.getstarteds:hover,a.gos:hover,a.howtoapplys:hover,a.howtobuys:hover,a.joinnows:hover,a.learnmores:hover,a.nikebuynows:hover,a.notifymes:hover,a.ordernows:hover,a.preordernows:hover,a.preorders:hover,a.reserves:hover,a.startyoursearchs:hover,a.submits:hover,a.tryamacs:hover,a.upgradenows:hover {color:red}
+
+/* NEWLINE-SEPARATED SELECTORS SHOULD COMBINE */
+h1,
+h2,
+h3,
+h4,
+h5,
+h6 {font-size:1em}
index 615d5c6..926b90c 100644 (file)
@@ -1,5 +1,29 @@
 2019-06-13  Devin Rousso  <drousso@apple.com>
 
+        Web Inspector: REGRESSION(r246178): extra spaces added in at-rules when formatting CSS
+        https://bugs.webkit.org/show_bug.cgi?id=198806
+
+        Reviewed by Joseph Pecoraro.
+
+        * UserInterface/Workers/Formatter/CSSFormatter.js:
+        (CSSFormatter.prototype._format):
+        Add more specific tests for at-rules, and add/remove whitespace depending on the type of
+        at-rule (e.g. `@supports` vs `@media`), as well as where the scanner is in the parameters of
+        the at at-rule (e.g. `@supports |` vs `@media (|`).
+
+        * UserInterface/Workers/Formatter/FormatterContentBuilder.js:
+        (FormatterContentBuilder):
+        (FormatterContentBuilder.prototype.get lastToken): Added.
+        (FormatterContentBuilder.prototype.get currentLine):
+        (FormatterContentBuilder.prototype.removeLastNewline):
+        (FormatterContentBuilder.prototype.removeLastWhitespace):
+        (FormatterContentBuilder.prototype._popFormattedContent):
+        (FormatterContentBuilder.prototype._append):
+        Update `lastTokenWasNewline` and `lastTokenWasWhitespace` when removing newlines/whitespace.
+        Memoize the `currentLine` so it's less expensive to re-fetch.
+
+2019-06-13  Devin Rousso  <drousso@apple.com>
+
         Web Inspector: Settings: indent type and size settings aren't respected everywhere
         https://bugs.webkit.org/show_bug.cgi?id=198804
 
index 3847f8a..c11bc56 100644 (file)
@@ -74,9 +74,24 @@ CSSFormatter = class CSSFormatter
         const addSpaceAfter = new Set([`,`, `+`, `*`, `~`, `>`, `)`, `:`]);
 
         const removeSpaceBefore = new Set([`,`, `(`, `)`, `}`, `:`, `;`]);
-        const removeSpaceAfter = new Set([`(`, `{`, `}`, `!`, `;`]);
+        const removeSpaceAfter = new Set([`(`, `{`, `}`, `,`, `!`, `;`]);
+
+        const inAtRuleRegExp = /^\s*@[a-zA-Z][-a-zA-Z]+/;
+        const inAtRuleBeforeParenthesisRegExp = /^\s*@[a-zA-Z][-a-zA-Z]+$/;
+        const inAtRuleAfterParenthesisRegExp = /^\s*@[a-zA-Z][-a-zA-Z]+[^("':]*\([^"':]*:/;
+        const inAtSupportsRuleRegExp = /^\s*@[a-zA-Z][-a-zA-Z]+[^"':]*:/;
+
+        const lineStartCouldBePropertyRegExp = /^\s+[-_a-zA-Z][-_a-zA-Z0-9]*/;
+
+        const lastTokenWasOpenParenthesisRegExp = /\(\s*$/;
+
+        let depth = 0;
 
         for (let i = 0; i < this._sourceText.length; ++i) {
+            let c = this._sourceText[i];
+
+            let testCurrentLine = (regExp) => regExp.test(this._builder.currentLine);
+
             let inSelector = () => {
                 let nextOpenBrace = this._sourceText.indexOf(`{`, i);
                 if (nextOpenBrace !== -1) {
@@ -100,14 +115,16 @@ CSSFormatter = class CSSFormatter
                     }
                 }
 
-                if (!/^\s+[-_a-zA-Z][-_a-zA-Z0-9]*/.test(this._builder.currentLine))
-                    return true;
+                if (testCurrentLine(lineStartCouldBePropertyRegExp))
+                    return false;
 
-                return false;
+                return true;
             };
 
-            let inMediaQuery = () => {
-                return /^\s*@media/.test(this._builder.currentLine);
+            let inProperty = () => {
+                if (!depth)
+                    return false;
+                return !testCurrentLine(inAtRuleRegExp) && !inSelector();
             };
 
             let formatBefore = () => {
@@ -117,28 +134,80 @@ CSSFormatter = class CSSFormatter
                 if (dedentBefore.has(c))
                     this._builder.dedent();
 
-                if (!this._builder.lastNewlineAppendWasMultiple && newlineBefore.has(c))
+                if (!this._builder.lastTokenWasNewline && newlineBefore.has(c))
                     this._builder.appendNewline();
 
                 if (!this._builder.lastTokenWasWhitespace && addSpaceBefore.has(c)) {
-                    if (c !== `(` || inMediaQuery())
+                    let shouldAddSpaceBefore = () => {
+                        if (c === `(`) {
+                            if (testCurrentLine(inAtSupportsRuleRegExp))
+                                return false;
+                            if (!testCurrentLine(inAtRuleRegExp))
+                                return false;
+                        }
+                        return true;
+                    };
+                    if (shouldAddSpaceBefore())
                         this._builder.appendSpace();
                 }
 
-                if (this._builder.lastTokenWasWhitespace && removeSpaceBefore.has(c)) {
-                    if ((c !== `:` || !inSelector()) && (c !== `(` || !inMediaQuery() || this._sourceText[i - 1] === `(`))
-                        this._builder.removeLastWhitespace();
+                while (this._builder.lastTokenWasWhitespace && removeSpaceBefore.has(c)) {
+                    let shouldRemoveSpaceBefore = () => {
+                        if (c === `:`) {
+                            if (!testCurrentLine(this._builder.currentLine.includes(`(`) ? inAtRuleRegExp : inAtRuleBeforeParenthesisRegExp)) {
+                                if (!inProperty())
+                                    return false;
+                            }
+                        }
+                        if (c === `(`) {
+                            if (!testCurrentLine(lastTokenWasOpenParenthesisRegExp)) {
+                                if (testCurrentLine(inAtRuleRegExp) && !testCurrentLine(inAtRuleAfterParenthesisRegExp))
+                                    return false;
+                            }
+                        }
+                        return true;
+                    };
+                    if (!shouldRemoveSpaceBefore())
+                        break;
+                    this._builder.removeLastWhitespace();
                 }
             };
 
             let formatAfter = () => {
-                if (this._builder.lastTokenWasWhitespace && removeSpaceAfter.has(c)) {
-                    if (c !== `(` || inMediaQuery())
-                        this._builder.removeLastWhitespace();
+                while (this._builder.lastTokenWasWhitespace && removeSpaceAfter.has(c)) {
+                    let shouldRemoveSpaceAfter = () => {
+                        if (c === `(`) {
+                            if (!testCurrentLine(lastTokenWasOpenParenthesisRegExp)) {
+                                if (!testCurrentLine(inAtRuleRegExp)) {
+                                    if (!inProperty())
+                                        return false;
+                                }
+                            }
+                        }
+                        return true;
+                    };
+                    if (!shouldRemoveSpaceAfter())
+                        break;
+                    this._builder.removeLastWhitespace();
                 }
 
                 if (!this._builder.lastTokenWasWhitespace && addSpaceAfter.has(c)) {
-                    if (c !== `:` || !inSelector())
+                    let shouldAddSpaceAfter = () => {
+                        if (c === `:`) {
+                            if (!testCurrentLine(this._builder.currentLine.includes(`(`) ? inAtRuleRegExp : inAtRuleBeforeParenthesisRegExp)) {
+                                if (!inProperty())
+                                    return false;
+                            }
+                        }
+                        if (c === `)`) {
+                            if (!testCurrentLine(inAtRuleRegExp)) {
+                                if (!inProperty())
+                                    return false;
+                            }
+                        }
+                        return true;
+                    };
+                    if (shouldAddSpaceAfter())
                         this._builder.appendSpace();
                 }
 
@@ -153,8 +222,6 @@ CSSFormatter = class CSSFormatter
                 }
             };
 
-            let c = this._sourceText[i];
-
             let specialSequenceEnd = null;
             if (quoteTypes.has(c))
                 specialSequenceEnd = c;
@@ -182,13 +249,22 @@ CSSFormatter = class CSSFormatter
 
             if (/\s/.test(c)) {
                 if (c === `\n` && !this._builder.lastTokenWasNewline) {
-                    this._builder.removeLastWhitespace();
-                    this._builder.appendNewline();
-                } else if (!this._builder.lastTokenWasWhitespace && !removeSpaceAfter.has(this._sourceText[i - 1]))
+                    while (this._builder.lastTokenWasWhitespace)
+                        this._builder.removeLastWhitespace();
+                    if (!removeSpaceAfter.has(this._builder.lastToken))
+                        this._builder.appendNewline();
+                    else
+                        this._builder.appendSpace();
+                } else if (!this._builder.lastTokenWasWhitespace && !removeSpaceAfter.has(this._builder.lastToken))
                     this._builder.appendSpace();
                 continue;
             }
 
+            if (c === `{`)
+                ++depth;
+            else if (c === `}`)
+                --depth;
+
             formatBefore();
             this._builder.appendToken(c, i);
             formatAfter();
index a2de66a..8d142e5 100644 (file)
@@ -33,6 +33,7 @@ FormatterContentBuilder = class FormatterContentBuilder
         this._formattedContentLength = 0;
 
         this._startOfLine = true;
+        this._currentLine = null;
         this.lastTokenWasNewline = false;
         this.lastTokenWasWhitespace = false;
         this.lastNewlineAppendWasMultiple = false;
@@ -74,9 +75,16 @@ FormatterContentBuilder = class FormatterContentBuilder
         };
     }
 
+    get lastToken()
+    {
+        return this._formattedContent.lastValue;
+    }
+
     get currentLine()
     {
-        return this._formattedContent.slice(this._formattedContent.lastIndexOf("\n") + 1).join("");
+        if (!this._currentLine)
+            this._currentLine = this._formattedContent.slice(this._formattedContent.lastIndexOf("\n") + 1).join("");
+        return this._currentLine;
     }
 
     setOriginalContent(originalContent)
@@ -143,26 +151,27 @@ FormatterContentBuilder = class FormatterContentBuilder
     removeLastNewline()
     {
         console.assert(this.lastTokenWasNewline);
-        console.assert(this._formattedContent.lastValue === "\n");
+        console.assert(this.lastToken === "\n");
         if (this.lastTokenWasNewline) {
             this._popFormattedContent();
             this._formattedLineEndings.pop();
             this._startOfLine = false;
-            this.lastTokenWasNewline = false;
-            this.lastTokenWasWhitespace = false;
+            this.lastTokenWasNewline = this.lastToken === "\n";
+            this.lastTokenWasWhitespace = this.lastToken === " ";
         }
     }
 
     removeLastWhitespace()
     {
         console.assert(this.lastTokenWasWhitespace);
-        console.assert(this._formattedContent.lastValue === " ");
+        console.assert(this.lastToken === " ");
         if (this.lastTokenWasWhitespace) {
             this._popFormattedContent();
             // No need to worry about `_startOfLine` and `lastTokenWasNewline`
             // because `appendSpace` takes care of not adding whitespace
             // to the beginning of a line.
-            this.lastTokenWasWhitespace = false;
+            this.lastTokenWasNewline = this.lastToken === "\n";
+            this.lastTokenWasWhitespace = this.lastToken === " ";
         }
     }
 
@@ -196,12 +205,14 @@ FormatterContentBuilder = class FormatterContentBuilder
     {
         let removed = this._formattedContent.pop();
         this._formattedContentLength -= removed.length;
+        this._currentLine = null;
     }
 
     _append(str)
     {
         this._formattedContent.push(str);
         this._formattedContentLength += str.length;
+        this._currentLine = null;
     }
 
     _appendIndent()