Attachment 372026 Details for Bug 198806 – Patch

[patch] Patch

bug-198806-20190613000646.patch (text/plain), 20.30 KB, created by Devin Rousso on 2019-06-13 00:06:47 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Devin Rousso

Created: 2019-06-13 00:06:47 PDT

Size: 20.30 KB

patch

obsolete

>diff --git a/Source/WebInspectorUI/ChangeLog b/Source/WebInspectorUI/ChangeLog
>index eaecae15bb51b34f2800dc6b54be72133ad7360e..7686d52ff68c3b96c24b944381389ee396c0d50d 100644
>--- a/Source/WebInspectorUI/ChangeLog
>+++ b/Source/WebInspectorUI/ChangeLog
>@@ -1,3 +1,19 @@
>+2019-06-13  Devin Rousso  <drousso@apple.com>
>+
>+        Web Inspector: REGRESSION(r246178): extra space inserted before pseudo-selector after a closing parenthesis
>+        https://bugs.webkit.org/show_bug.cgi?id=198806
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * UserInterface/Workers/Formatter/CSSFormatter.js:
>+        (CSSFormatter.prototype._format):
>+
>+        * UserInterface/Workers/Formatter/FormatterContentBuilder.js:
>+        (FormatterContentBuilder.prototype.get lastToken): Added.
>+        (FormatterContentBuilder.prototype.removeLastNewline):
>+        (FormatterContentBuilder.prototype.removeLastWhitespace):
>+        Update `lastTokenWasNewline` and `lastTokenWasWhitespace` when removing newlines/whitespace.
>+
> 2019-06-10  Devin Rousso  <drousso@apple.com>
> 
>         Web Inspector: Timelines: imported recordings do not have JavaScript call trees
>diff --git a/Source/WebInspectorUI/UserInterface/Workers/Formatter/CSSFormatter.js b/Source/WebInspectorUI/UserInterface/Workers/Formatter/CSSFormatter.js
>index 3847f8a5b08f74ba552464472debdb63fe48e224..f09f6b4613807069262d96e0b76a2132ea0db653 100644
>--- a/Source/WebInspectorUI/UserInterface/Workers/Formatter/CSSFormatter.js
>+++ b/Source/WebInspectorUI/UserInterface/Workers/Formatter/CSSFormatter.js
>@@ -74,10 +74,19 @@ CSSFormatter = class CSSFormatter
>         const addSpaceAfter = new Set([`,`, `+`, `*`, `~`, `>`, `)`, `:`]);
> 
>         const removeSpaceBefore = new Set([`,`, `(`, `)`, `}`, `:`, `;`]);
>-        const removeSpaceAfter = new Set([`(`, `{`, `}`, `!`, `;`]);
>+        const removeSpaceAfter = new Set([`(`, `{`, `}`, `,`, `!`, `;`]);
>+
>+        let depth = 0;
> 
>         for (let i = 0; i < this._sourceText.length; ++i) {
>-            let inSelector = () => {
>+            let c = this._sourceText[i];
>+
>+            let inAtRule = (suffix = "") => {
>+                let re = new RegExp(`^\\s*@[a-zA-Z][-a-zA-Z]+${suffix}`);
>+                return re.test(this._builder.currentLine);
>+            };
>+
>+            let inSelector = (suffix = "") => {
>                 let nextOpenBrace = this._sourceText.indexOf(`{`, i);
>                 if (nextOpenBrace !== -1) {
>                     let nextQuote = Infinity;
>@@ -100,14 +109,16 @@ CSSFormatter = class CSSFormatter
>                     }
>                 }
> 
>-                if (!/^\s+[-_a-zA-Z][-_a-zA-Z0-9]*/.test(this._builder.currentLine))
>-                    return true;
>+                if (/^\s+[-_a-zA-Z][-_a-zA-Z0-9]*/.test(this._builder.currentLine))
>+                    return false;
> 
>-                return false;
>+                return true;
>             };
> 
>-            let inMediaQuery = () => {
>-                return /^\s*@media/.test(this._builder.currentLine);
>+            let inProperty = () => {
>+                if (!depth)
>+                    return false;
>+                return !inAtRule() && !inSelector();
>             };
> 
>             let formatBefore = () => {
>@@ -117,28 +128,58 @@ 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 (inAtRule("[^\"':]*:")) // when an @supports property value has a function
>+                                return false;
>+                            if (!inAtRule())
>+                                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 === `:` && !inAtRule(this._builder.currentLine.includes(`(`) ? "" : "$") && !inProperty())
>+                            return false;
>+                        if (c === `(` && !/\(\s*$/.test(this._builder.currentLine) && inAtRule() && !inAtRule("[^(^\"':]*\\([^\"':]*:"))
>+                            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 === `(` && !/\(\s*$/.test(this._builder.currentLine) && !inAtRule() && !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 === `:` && !inAtRule(this._builder.currentLine.includes(`(`) ? "" : "$") && !inProperty())
>+                            return false;
>+                        if (c === `)` && !inAtRule() && !inProperty())
>+                            return false;
>+                        return true;
>+                    };
>+                    if (shouldAddSpaceAfter())
>                         this._builder.appendSpace();
>                 }
> 
>@@ -153,8 +194,6 @@ CSSFormatter = class CSSFormatter
>                 }
>             };
> 
>-            let c = this._sourceText[i];
>-
>             let specialSequenceEnd = null;
>             if (quoteTypes.has(c))
>                 specialSequenceEnd = c;
>@@ -182,13 +221,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();
>diff --git a/Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js b/Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js
>index a2de66a1327e8a5d669fc8272df82bae5d2a5778..572af5cb6c3558e017e38c77ccd22d7bbe9768ba 100644
>--- a/Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js
>+++ b/Source/WebInspectorUI/UserInterface/Workers/Formatter/FormatterContentBuilder.js
>@@ -74,6 +74,11 @@ FormatterContentBuilder = class FormatterContentBuilder
>         };
>     }
> 
>+    get lastToken()
>+    {
>+        return this._formattedContent.lastValue;
>+    }
>+
>     get currentLine()
>     {
>         return this._formattedContent.slice(this._formattedContent.lastIndexOf("\n") + 1).join("");
>@@ -143,26 +148,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 === " ";
>         }
>     }
> 
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index df1ae1d9b645facf32e372d15b208fd64767bdf1..bb854d548b37041db59689c3b3eb9c6b6e02db20 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,19 @@
>+2019-06-13  Devin Rousso  <drousso@apple.com>
>+
>+        Web Inspector: REGRESSION(r246178): extra space inserted before pseudo-selector after a closing parenthesis
>+        https://bugs.webkit.org/show_bug.cgi?id=198806
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * 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-12  Antti Koivisto  <antti@apple.com>
> 
>         (Async scrolling) Handle 'position:fixed' inside 'position:sticky' correctly.
>diff --git a/LayoutTests/inspector/formatting/resources/css-tests/keyframes-expected.css b/LayoutTests/inspector/formatting/resources/css-tests/keyframes-expected.css
>index e7106720447fdd05d3ea98264fca41c921b74e2e..ca38fa083ebc4ed33db3aa0033ca7a4f5e83752d 100644
>--- a/LayoutTests/inspector/formatting/resources/css-tests/keyframes-expected.css
>+++ b/LayoutTests/inspector/formatting/resources/css-tests/keyframes-expected.css
>@@ -8,3 +8,13 @@
>     }
> }
> 
>+@-webkit-keyframes spin {
>+    0% {
>+        -webkit-transform: rotate(-180deg) translate(0px, 0px);
>+    }
>+
>+    100% {
>+        -webkit-transform: rotate(180deg) translate(10px, 75px);
>+    }
>+}
>+
>diff --git a/LayoutTests/inspector/formatting/resources/css-tests/keyframes.css b/LayoutTests/inspector/formatting/resources/css-tests/keyframes.css
>index bd88b56cc49697b78200bb99dd0f4297236f4483..669aab55feb0b69989c0e9fb31270192444dbd29 100644
>--- a/LayoutTests/inspector/formatting/resources/css-tests/keyframes.css
>+++ b/LayoutTests/inspector/formatting/resources/css-tests/keyframes.css
>@@ -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   )   ;   }   }
>diff --git a/LayoutTests/inspector/formatting/resources/css-tests/media-query-expected.css b/LayoutTests/inspector/formatting/resources/css-tests/media-query-expected.css
>index 02ebe939e1ce860301e50fed93af0605a12a6e7a..d49c1236442fc881d2afc27fb7b28c878f5afdfa 100644
>--- a/LayoutTests/inspector/formatting/resources/css-tests/media-query-expected.css
>+++ b/LayoutTests/inspector/formatting/resources/css-tests/media-query-expected.css
>@@ -20,7 +20,13 @@
> }
> 
> /* 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;
>     }
>@@ -32,3 +38,49 @@
>     }
> }
> 
>+@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);
>+    }
>+}
>+
>diff --git a/LayoutTests/inspector/formatting/resources/css-tests/media-query.css b/LayoutTests/inspector/formatting/resources/css-tests/media-query.css
>index b86bcffc09a2e879abc977ca2a438d5430dd085f..a740d3deadf66a70495e4348ecac4a1f0b78d4de 100644
>--- a/LayoutTests/inspector/formatting/resources/css-tests/media-query.css
>+++ b/LayoutTests/inspector/formatting/resources/css-tests/media-query.css
>@@ -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   )   ;   }   }
>diff --git a/LayoutTests/inspector/formatting/resources/css-tests/selectors-expected.css b/LayoutTests/inspector/formatting/resources/css-tests/selectors-expected.css
>index 04201811fc680317f7f6a9d10bad528c0612159b..f3b745e3e1c65a7af9abd06a8fe171d0fb847092 100644
>--- a/LayoutTests/inspector/formatting/resources/css-tests/selectors-expected.css
>+++ b/LayoutTests/inspector/formatting/resources/css-tests/selectors-expected.css
>@@ -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;
>+}
>+
>diff --git a/LayoutTests/inspector/formatting/resources/css-tests/selectors.css b/LayoutTests/inspector/formatting/resources/css-tests/selectors.css
>index 5a3ef7c4b47aad4a629686c5297fbe28ba5a2981..252a91bcd2a15ebc5ee91dc2566bac4968e3a5e7 100644
>--- a/LayoutTests/inspector/formatting/resources/css-tests/selectors.css
>+++ b/LayoutTests/inspector/formatting/resources/css-tests/selectors.css
>@@ -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   ;   }
>diff --git a/LayoutTests/inspector/formatting/resources/css-tests/wrapping-expected.css b/LayoutTests/inspector/formatting/resources/css-tests/wrapping-expected.css
>index 795d5dc9eb3ab8cf54ba0e502bd2dcb04db2c8a3..ccdf7133ddc4cafb60eb3f587832ca8800090f84 100644
>--- a/LayoutTests/inspector/formatting/resources/css-tests/wrapping-expected.css
>+++ b/LayoutTests/inspector/formatting/resources/css-tests/wrapping-expected.css
>@@ -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
>+}
>+
>diff --git a/LayoutTests/inspector/formatting/resources/css-tests/wrapping.css b/LayoutTests/inspector/formatting/resources/css-tests/wrapping.css
>index be6bf9d2aa84b2f1549fb990d69717857308ee1d..d601db3a1cb271299e69abfd755e9578a0004980 100644
>--- a/LayoutTests/inspector/formatting/resources/css-tests/wrapping.css
>+++ b/LayoutTests/inspector/formatting/resources/css-tests/wrapping.css
>@@ -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}

Actions: View | Formatted Diff | Diff

Attachments on bug 198806: 372026 | 372092 | 372106