Changeset 246430 in webkit


Ignore:
Timestamp:
Jun 13, 2019 11:52:07 PM (5 years ago)
Author:
Devin Rousso
Message:

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.

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:
Location:
trunk
Files:
12 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r246429 r246430  
     12019-06-13  Devin Rousso  <drousso@apple.com>
     2
     3        Web Inspector: REGRESSION(r246178): extra spaces added in at-rules when formatting CSS
     4        https://bugs.webkit.org/show_bug.cgi?id=198806
     5
     6        Reviewed by Joseph Pecoraro.
     7
     8        * inspector/formatting/resources/css-tests/keyframes.css:
     9        * inspector/formatting/resources/css-tests/keyframes-expected.css:
     10        * inspector/formatting/resources/css-tests/media-query.css:
     11        * inspector/formatting/resources/css-tests/media-query-expected.css:
     12        * inspector/formatting/resources/css-tests/selectors.css:
     13        * inspector/formatting/resources/css-tests/selectors-expected.css:
     14        * inspector/formatting/resources/css-tests/wrapping.css:
     15        * inspector/formatting/resources/css-tests/wrapping-expected.css:
     16
    1172019-06-13  Antoine Quint  <graouts@apple.com>
    218

  • trunk/LayoutTests/inspector/formatting/resources/css-tests/keyframes-expected.css

    r246178 r246430  
    99}
    1010
     11@-webkit-keyframes spin {
     12    0% {
     13        -webkit-transform: rotate(-180deg) translate(0px, 0px);
     14    }
     15
     16    100% {
     17        -webkit-transform: rotate(180deg) translate(10px, 75px);
     18    }
     19}
     20

  • trunk/LayoutTests/inspector/formatting/resources/css-tests/keyframes.css

    r246178 r246430  
    11@-webkit-keyframes spin{0%{-webkit-transform:rotate(-180deg)translate(0px,0px);}100%{-webkit-transform:rotate(180deg)translate(10px,75px);}}
     2@-webkit-keyframes   spin   {   0%   {   -webkit-transform   :   rotate   (   -180deg   )   translate   (   0px   ,   0px   )   ;   }   100%   {   -webkit-transform   :   rotate   (   180deg   )   translate   (   10px   ,   75px   )   ;   }   }

  • trunk/LayoutTests/inspector/formatting/resources/css-tests/media-query-expected.css

    r246178 r246430  
    2121
    2222/* MEDIA QUERY */
    23 @media screen and (max-device-width:480px) {
     23@media screen and (max-device-width: 480px) {
     24    html {
     25        -webkit-text-size-adjust: none;
     26    }
     27}
     28
     29@media screen and (max-device-width: 480px) {
    2430    html {
    2531        -webkit-text-size-adjust: none;
    … …  
    3339}
    3440
     41@media not ((screen) and (print)), (print) {
     42    body {
     43        color: red
     44    }
     45}
     46
     47/* font-face */
     48@font-face {
     49    font-family: "MyWebFont";
     50    src: url("myfont.woff2") format("woff2"), url("myfont.woff") format("woff");
     51}
     52
     53/* page */
     54@page :first {
     55    margin: 1in;
     56}
     57
     58@page :first {
     59    margin: 1in;
     60}
     61
     62/* supports */
     63@supports (display: flex) {
     64    .module {
     65        display: flex;
     66    }
     67}
     68
     69@supports (display: flex) {
     70    .module {
     71        display: flex;
     72    }
     73}
     74
     75@supports (-webkit-backdrop-filter: blur(10px)) {
     76    .home header {
     77        -webkit-backdrop-filter: blur(10px);
     78    }
     79}
     80
     81@supports (-webkit-backdrop-filter: blur(10px)) {
     82    .home header {
     83        -webkit-backdrop-filter: blur(10px);
     84    }
     85}
     86

  • trunk/LayoutTests/inspector/formatting/resources/css-tests/media-query.css

    r246178 r246430  
    44/* MEDIA QUERY */
    55@media screen and(max-device-width:480px){html{-webkit-text-size-adjust:none;}}
     6@media   screen   and   (   max-device-width   :   480px   )   {   html   {   -webkit-text-size-adjust   :   none   ;   }   }
    67@media not((screen)and(print)),(print){body{color:red}}
     8@media   not   (   (   screen   )   and   (   print   )   )   ,   (   print   )   {   body   {   color   :   red   }   }
     9
     10/* font-face */
     11@font-face {
     12    font-family: "MyWebFont";
     13    src: url("myfont.woff2") format("woff2"),
     14         url("myfont.woff") format("woff");
     15}
     16
     17/* page */
     18@page :first{margin:1in;}
     19@page   :first   {   margin   :   1in   ;   }
     20
     21/* supports */
     22@supports(display:flex){.module{display:flex;}}
     23@supports   (   display   :   flex   )   {   .module   {   display   :   flex   ;   }   }
     24@supports(-webkit-backdrop-filter:blur(10px)){.home header{-webkit-backdrop-filter:blur(10px);}}
     25@supports   (   -webkit-backdrop-filter   :   blur   (   10px   )   )   {   .home   header   {   -webkit-backdrop-filter   :   blur   (   10px   )   ;   }   }

  • trunk/LayoutTests/inspector/formatting/resources/css-tests/selectors-expected.css

    r246178 r246430  
    44
    55/* COMPLEX SELECTOR */
     6div div > div#id.foo.bar:hover .something > .child ~ .sibling + .sibling:after {
     7    color: red;
     8}
     9
     10div div > div#id.foo.bar:hover .something > .child ~ .sibling + .sibling:after {
     11    color: red;
     12}
     13
    614div div > div#id.foo.bar:hover .something > .child ~ .sibling + .sibling::after {
    715    color: red;
    816}
    917
     18div div > div#id.foo.bar:hover .something > .child ~ .sibling + .sibling::after {
     19    color: red;
     20}
     21
     22div div > div#id.foo.bar:hover .something > .child ~ .sibling + :matches(.sibling):after {
     23    color: red;
     24}
     25
     26div div > div#id.foo.bar:hover .something > .child ~ .sibling + :matches(.sibling):after {
     27    color: red;
     28}
     29
     30div div > div#id.foo.bar:hover .something > .child ~ .sibling + :matches(.sibling)::after {
     31    color: red;
     32}
     33
     34div div > div#id.foo.bar:hover .something > .child ~ .sibling + :matches(.sibling)::after {
     35    color: red;
     36}
     37

  • trunk/LayoutTests/inspector/formatting/resources/css-tests/selectors.css

    r246178 r246430  
    33
    44/* COMPLEX SELECTOR */
     5div div>div#id.foo.bar:hover .something>.child~.sibling+.sibling:after{color:red;}
     6div   div   >   div#id.foo.bar:hover   .something   >   .child   ~   .sibling   +   .sibling:after   {   color   :   red   ;   }
    57div div>div#id.foo.bar:hover .something>.child~.sibling+.sibling::after{color:red;}
     8div   div   >   div#id.foo.bar:hover   .something   >   .child   ~   .sibling   +   .sibling::after   {   color   :   red   ;   }
     9div div>div#id.foo.bar:hover .something>.child~.sibling+:matches(.sibling):after{color:red;}
     10div   div   >   div#id.foo.bar:hover   .something   >   .child   ~   .sibling   +   :matches(.sibling):after   {   color   :   red   ;   }
     11div div>div#id.foo.bar:hover .something>.child~.sibling+:matches(.sibling)::after{color:red;}
     12div   div   >   div#id.foo.bar:hover   .something   >   .child   ~   .sibling   +   :matches(.sibling)::after   {   color   :   red   ;   }

  • trunk/LayoutTests/inspector/formatting/resources/css-tests/wrapping-expected.css

    r246178 r246430  
    88}
    99
     10/* NEWLINE-SEPARATED SELECTORS SHOULD COMBINE */
     11h1, h2, h3, h4, h5, h6 {
     12    font-size: 1em
     13}
     14

  • trunk/LayoutTests/inspector/formatting/resources/css-tests/wrapping.css

    r246178 r246430  
    22a.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}
    33a.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}
     4
     5/* NEWLINE-SEPARATED SELECTORS SHOULD COMBINE */
     6h1,
     7h2,
     8h3,
     9h4,
     10h5,
     11h6 {font-size:1em}

  • trunk/Source/WebInspectorUI/ChangeLog

    r246419 r246430  
     12019-06-13  Devin Rousso  <drousso@apple.com>
     2
     3        Web Inspector: REGRESSION(r246178): extra spaces added in at-rules when formatting CSS
     4        https://bugs.webkit.org/show_bug.cgi?id=198806
     5
     6        Reviewed by Joseph Pecoraro.
     7
     8        * UserInterface/Workers/Formatter/CSSFormatter.js:
     9        (CSSFormatter.prototype._format):
     10        Add more specific tests for at-rules, and add/remove whitespace depending on the type of
     11        at-rule (e.g. `@supports` vs `@media`), as well as where the scanner is in the parameters of
     12        the at at-rule (e.g. `@supports |` vs `@media (|`).
     13
     14        * UserInterface/Workers/Formatter/FormatterContentBuilder.js:
     15        (FormatterContentBuilder):
     16        (FormatterContentBuilder.prototype.get lastToken): Added.
     17        (FormatterContentBuilder.prototype.get currentLine):
     18        (FormatterContentBuilder.prototype.removeLastNewline):
     19        (FormatterContentBuilder.prototype.removeLastWhitespace):
     20        (FormatterContentBuilder.prototype._popFormattedContent):
     21        (FormatterContentBuilder.prototype._append):
     22        Update `lastTokenWasNewline` and `lastTokenWasWhitespace` when removing newlines/whitespace.
     23        Memoize the `currentLine` so it's less expensive to re-fetch.
     24
    1252019-06-13  Devin Rousso  <drousso@apple.com>
    226

  • trunk/Source/WebInspectorUI/UserInterface/Workers/Formatter/CSSFormatter.js

    r246178 r246430  
    7575
    7676        const removeSpaceBefore = new Set([`,`, `(`, `)`, `}`, `:`, `;`]);
    77         const removeSpaceAfter = new Set([`(`, `{`, `}`, `!`, `;`]);
     77        const removeSpaceAfter = new Set([`(`, `{`, `}`, `,`, `!`, `;`]);
     78
     79        const inAtRuleRegExp = /^\s*@[a-zA-Z][-a-zA-Z]+/;
     80        const inAtRuleBeforeParenthesisRegExp = /^\s*@[a-zA-Z][-a-zA-Z]+$/;
     81        const inAtRuleAfterParenthesisRegExp = /^\s*@[a-zA-Z][-a-zA-Z]+[^("':]*\([^"':]*:/;
     82        const inAtSupportsRuleRegExp = /^\s*@[a-zA-Z][-a-zA-Z]+[^"':]*:/;
     83
     84        const lineStartCouldBePropertyRegExp = /^\s+[-_a-zA-Z][-_a-zA-Z0-9]*/;
     85
     86        const lastTokenWasOpenParenthesisRegExp = /\(\s*$/;
     87
     88        let depth = 0;
    7889
    7990        for (let i = 0; i < this._sourceText.length; ++i) {
     91            let c = this._sourceText[i];
     92
     93            let testCurrentLine = (regExp) => regExp.test(this._builder.currentLine);
     94
    8095            let inSelector = () => {
    8196                let nextOpenBrace = this._sourceText.indexOf(`{`, i);
    … …  
    101116                }
    102117
    103                 if (!/^\s+[-_a-zA-Z][-_a-zA-Z0-9]*/.test(this._builder.currentLine))
    104                     return true;
    105 
    106                 return false;
    107             };
    108 
    109             let inMediaQuery = () => {
    110                 return /^\s*@media/.test(this._builder.currentLine);
     118                if (testCurrentLine(lineStartCouldBePropertyRegExp))
     119                    return false;
     120
     121                return true;
     122            };
     123
     124            let inProperty = () => {
     125                if (!depth)
     126                    return false;
     127                return !testCurrentLine(inAtRuleRegExp) && !inSelector();
    111128            };
    112129
    … …  
    118135                    this._builder.dedent();
    119136
    120                 if (!this._builder.lastNewlineAppendWasMultiple && newlineBefore.has(c))
     137                if (!this._builder.lastTokenWasNewline && newlineBefore.has(c))
    121138                    this._builder.appendNewline();
    122139
    123140                if (!this._builder.lastTokenWasWhitespace && addSpaceBefore.has(c)) {
    124                     if (c !== `(` || inMediaQuery())
     141                    let shouldAddSpaceBefore = () => {
     142                        if (c === `(`) {
     143                            if (testCurrentLine(inAtSupportsRuleRegExp))
     144                                return false;
     145                            if (!testCurrentLine(inAtRuleRegExp))
     146                                return false;
     147                        }
     148                        return true;
     149                    };
     150                    if (shouldAddSpaceBefore())
    125151                        this._builder.appendSpace();
    126152                }
    127153
    128                 if (this._builder.lastTokenWasWhitespace && removeSpaceBefore.has(c)) {
    129                     if ((c !== `:` || !inSelector()) && (c !== `(` || !inMediaQuery() || this._sourceText[i - 1] === `(`))
    130                         this._builder.removeLastWhitespace();
     154                while (this._builder.lastTokenWasWhitespace && removeSpaceBefore.has(c)) {
     155                    let shouldRemoveSpaceBefore = () => {
     156                        if (c === `:`) {
     157                            if (!testCurrentLine(this._builder.currentLine.includes(`(`) ? inAtRuleRegExp : inAtRuleBeforeParenthesisRegExp)) {
     158                                if (!inProperty())
     159                                    return false;
     160                            }
     161                        }
     162                        if (c === `(`) {
     163                            if (!testCurrentLine(lastTokenWasOpenParenthesisRegExp)) {
     164                                if (testCurrentLine(inAtRuleRegExp) && !testCurrentLine(inAtRuleAfterParenthesisRegExp))
     165                                    return false;
     166                            }
     167                        }
     168                        return true;
     169                    };
     170                    if (!shouldRemoveSpaceBefore())
     171                        break;
     172                    this._builder.removeLastWhitespace();
    131173                }
    132174            };
    133175
    134176            let formatAfter = () => {
    135                 if (this._builder.lastTokenWasWhitespace && removeSpaceAfter.has(c)) {
    136                     if (c !== `(` || inMediaQuery())
    137                         this._builder.removeLastWhitespace();
     177                while (this._builder.lastTokenWasWhitespace && removeSpaceAfter.has(c)) {
     178                    let shouldRemoveSpaceAfter = () => {
     179                        if (c === `(`) {
     180                            if (!testCurrentLine(lastTokenWasOpenParenthesisRegExp)) {
     181                                if (!testCurrentLine(inAtRuleRegExp)) {
     182                                    if (!inProperty())
     183                                        return false;
     184                                }
     185                            }
     186                        }
     187                        return true;
     188                    };
     189                    if (!shouldRemoveSpaceAfter())
     190                        break;
     191                    this._builder.removeLastWhitespace();
    138192                }
    139193
    140194                if (!this._builder.lastTokenWasWhitespace && addSpaceAfter.has(c)) {
    141                     if (c !== `:` || !inSelector())
     195                    let shouldAddSpaceAfter = () => {
     196                        if (c === `:`) {
     197                            if (!testCurrentLine(this._builder.currentLine.includes(`(`) ? inAtRuleRegExp : inAtRuleBeforeParenthesisRegExp)) {
     198                                if (!inProperty())
     199                                    return false;
     200                            }
     201                        }
     202                        if (c === `)`) {
     203                            if (!testCurrentLine(inAtRuleRegExp)) {
     204                                if (!inProperty())
     205                                    return false;
     206                            }
     207                        }
     208                        return true;
     209                    };
     210                    if (shouldAddSpaceAfter())
    142211                        this._builder.appendSpace();
    143212                }
    … …  
    153222                }
    154223            };
    155 
    156             let c = this._sourceText[i];
    157224
    158225            let specialSequenceEnd = null;
    … …  
    183250            if (/\s/.test(c)) {
    184251                if (c === `\n` && !this._builder.lastTokenWasNewline) {
    185                     this._builder.removeLastWhitespace();
    186                     this._builder.appendNewline();
    187                 } else if (!this._builder.lastTokenWasWhitespace && !removeSpaceAfter.has(this._sourceText[i - 1]))
     252                    while (this._builder.lastTokenWasWhitespace)
     253                        this._builder.removeLastWhitespace();
     254                    if (!removeSpaceAfter.has(this._builder.lastToken))
     255                        this._builder.appendNewline();
     256                    else
     257                        this._builder.appendSpace();
     258                } else if (!this._builder.lastTokenWasWhitespace && !removeSpaceAfter.has(this._builder.lastToken))
    188259                    this._builder.appendSpace();
    189260                continue;
    190261            }
     262
     263            if (c === `{`)
     264                ++depth;
     265            else if (c === `}`)
     266                --depth;
    191267
    192268            formatBefore();

  • trunk/Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js

    r246178 r246430  
    3434
    3535        this._startOfLine = true;
     36        this._currentLine = null;
    3637        this.lastTokenWasNewline = false;
    3738        this.lastTokenWasWhitespace = false;
    … …  
    7576    }
    7677
     78    get lastToken()
     79    {
     80        return this._formattedContent.lastValue;
     81    }
     82
    7783    get currentLine()
    7884    {
    79         return this._formattedContent.slice(this._formattedContent.lastIndexOf("\n") + 1).join("");
     85        if (!this._currentLine)
     86            this._currentLine = this._formattedContent.slice(this._formattedContent.lastIndexOf("\n") + 1).join("");
     87        return this._currentLine;
    8088    }
    8189
    … …  
    144152    {
    145153        console.assert(this.lastTokenWasNewline);
    146         console.assert(this._formattedContent.lastValue === "\n");
     154        console.assert(this.lastToken === "\n");
    147155        if (this.lastTokenWasNewline) {
    148156            this._popFormattedContent();
    149157            this._formattedLineEndings.pop();
    150158            this._startOfLine = false;
    151             this.lastTokenWasNewline = false;
    152             this.lastTokenWasWhitespace = false;
     159            this.lastTokenWasNewline = this.lastToken === "\n";
     160            this.lastTokenWasWhitespace = this.lastToken === " ";
    153161        }
    154162    }
    … …  
    157165    {
    158166        console.assert(this.lastTokenWasWhitespace);
    159         console.assert(this._formattedContent.lastValue === " ");
     167        console.assert(this.lastToken === " ");
    160168        if (this.lastTokenWasWhitespace) {
    161169            this._popFormattedContent();
    … …  
    163171            // because `appendSpace` takes care of not adding whitespace
    164172            // to the beginning of a line.
    165             this.lastTokenWasWhitespace = false;
     173            this.lastTokenWasNewline = this.lastToken === "\n";
     174            this.lastTokenWasWhitespace = this.lastToken === " ";
    166175        }
    167176    }
    … …  
    197206        let removed = this._formattedContent.pop();
    198207        this._formattedContentLength -= removed.length;
     208        this._currentLine = null;
    199209    }
    200210
    … …  
    203213        this._formattedContent.push(str);
    204214        this._formattedContentLength += str.length;
     215        this._currentLine = null;
    205216    }
    206217
Note: See TracChangeset for help on using the changeset viewer.