Attachment 356891 Details for Bug 192396 – Patch

[patch] Patch

patch.txt (text/plain), 20.62 KB, created by Nikita Vasilyev on 2018-12-08 16:00:26 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Nikita Vasilyev

Created: 2018-12-08 16:00:26 PST

Size: 20.62 KB

patch

obsolete

>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 565ab81a36f..05d01cf7734 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,19 @@
>+2018-12-08  Nikita Vasilyev  <nvasilyev@apple.com>
>+
>+        Web Inspector: Styles: toggling selected properties may cause data corruption
>+        https://bugs.webkit.org/show_bug.cgi?id=192396
>+        <rdar://problem/46478383>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * inspector/css/add-css-property-expected.txt: Added.
>+        * inspector/css/add-css-property.html: Added.
>+        Test adding new properties.
>+
>+        * inspector/css/modify-css-property-expected.txt:
>+        * inspector/css/modify-css-property.html:
>+        Test commenting out and uncommenting CSS properties.
>+
> 2018-12-06  Wenson Hsieh  <wenson_hsieh@apple.com>
> 
>         [iOS] WKWebView should match UITextView behavior when editing text with an RTL keyboard
>diff --git a/LayoutTests/inspector/css/add-css-property-expected.txt b/LayoutTests/inspector/css/add-css-property-expected.txt
>new file mode 100644
>index 00000000000..e3947d4b86c
>--- /dev/null
>+++ b/LayoutTests/inspector/css/add-css-property-expected.txt
>@@ -0,0 +1,17 @@
>+Testing that CSSStyleDeclaration updates after inserting new CSS properties.
>+
>+
>+== Running test suite: AddCSSProperty
>+-- Running test case: AddCSSProperty.AppendAfterMissingSemicolon
>+PASS: `color: green` property should be appended.
>+
>+-- Running test case: AddCSSProperty.InsertBeforeAndAfter
>+PASS: `color: green` property should be inserted before the first property.
>+PASS: `display: block` property should be appended at the end.
>+
>+-- Running test case: AddCSSProperty.AppendAfterCommentedOutProperty
>+PASS: `display: inline` property should be appended.
>+
>+-- Running test case: AddCSSProperty.AppendAfterCommentedOutPropertyWithoutSemicolon
>+PASS: `display: inline` property should be appended.
>+
>diff --git a/LayoutTests/inspector/css/add-css-property.html b/LayoutTests/inspector/css/add-css-property.html
>new file mode 100644
>index 00000000000..0a1ccc3df80
>--- /dev/null
>+++ b/LayoutTests/inspector/css/add-css-property.html
>@@ -0,0 +1,162 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+    <meta charset="UTF-8">
>+    <script src="../../http/tests/inspector/resources/inspector-test.js"></script>
>+    <script>
>+        function test() {
>+            let nodeStyles = null;
>+            let suite = InspectorTest.createAsyncSuite("AddCSSProperty");
>+
>+            suite.addTestCase({
>+                name: "AddCSSProperty.AppendAfterMissingSemicolon",
>+                test(resolve, reject) {
>+                    let getMatchedStyleDeclaration = () => {
>+                        for (let rule of nodeStyles.matchedRules) {
>+                            if (rule.selectorText === ".rule-a")
>+                                return rule.style;
>+                        }
>+
>+                        InspectorTest.fail("No declaration found.");
>+                        reject();
>+                    };
>+
>+                    let styleDeclaration = getMatchedStyleDeclaration();
>+                    styleDeclaration.locked = true;
>+                    let newProperty = styleDeclaration.newBlankProperty(1);
>+                    newProperty.name = "color";
>+                    newProperty.rawValue = "green";
>+
>+                    InspectorTest.expectEqual(styleDeclaration.text, `font-size: 14px;color: green;`, "`color: green` property should be appended.");
>+
>+                    resolve();
>+                }
>+            });
>+
>+            suite.addTestCase({
>+                name: "AddCSSProperty.InsertBeforeAndAfter",
>+                test(resolve, reject) {
>+                    let getMatchedStyleDeclaration = () => {
>+                        for (let rule of nodeStyles.matchedRules) {
>+                            if (rule.selectorText === ".rule-b")
>+                                return rule.style;
>+                        }
>+
>+                        InspectorTest.fail("No declaration found.");
>+                        reject();
>+                    };
>+
>+                    let styleDeclaration = getMatchedStyleDeclaration();
>+                    styleDeclaration.locked = true;
>+                    let newProperty = styleDeclaration.newBlankProperty(0);
>+                    newProperty.name = "color";
>+                    newProperty.rawValue = "green";
>+
>+                    let expected = `color: green;\n    outline: 2px solid brown;\n`;
>+                    InspectorTest.expectEqual(styleDeclaration.text, expected, "`color: green` property should be inserted before the first property.");
>+
>+                    styleDeclaration = getMatchedStyleDeclaration();
>+                    newProperty = styleDeclaration.newBlankProperty(2);
>+                    newProperty.name = "display";
>+                    newProperty.rawValue = "block";
>+
>+                    expected = `color: green;\n    outline: 2px solid brown;display: block;\n`;
>+                    InspectorTest.expectEqual(styleDeclaration.text, expected, "`display: block` property should be appended at the end.");
>+
>+                    resolve();
>+                }
>+            });
>+
>+            suite.addTestCase({
>+                name: "AddCSSProperty.AppendAfterCommentedOutProperty",
>+                test(resolve, reject) {
>+                    let getMatchedStyleDeclaration = () => {
>+                        for (let rule of nodeStyles.matchedRules) {
>+                            if (rule.selectorText === ".rule-c")
>+                                return rule.style;
>+                        }
>+
>+                        InspectorTest.fail("No declaration found.");
>+                        reject();
>+                    };
>+
>+                    let styleDeclaration = getMatchedStyleDeclaration();
>+                    styleDeclaration.locked = true;
>+                    let newProperty = styleDeclaration.newBlankProperty(2);
>+                    newProperty.name = "display";
>+                    newProperty.rawValue = "inline";
>+
>+                    const expected = `\n    font-size: 14px;\n    /* color: red; */display: inline;\n`;
>+                    InspectorTest.expectEqual(styleDeclaration.text, expected, "`display: inline` property should be appended.");
>+
>+                    resolve();
>+                }
>+            });
>+
>+            suite.addTestCase({
>+                name: "AddCSSProperty.AppendAfterCommentedOutPropertyWithoutSemicolon",
>+                test(resolve, reject) {
>+                    let getMatchedStyleDeclaration = () => {
>+                        for (let rule of nodeStyles.matchedRules) {
>+                            if (rule.selectorText === ".rule-d")
>+                                return rule.style;
>+                        }
>+
>+                        InspectorTest.fail("No declaration found.");
>+                        reject();
>+                    };
>+
>+                    let styleDeclaration = getMatchedStyleDeclaration();
>+                    styleDeclaration.locked = true;
>+                    let newProperty = styleDeclaration.newBlankProperty(2);
>+                    newProperty.name = "display";
>+                    newProperty.rawValue = "inline";
>+
>+                    const expected = `\n    font-size: 14px;\n    /* color: red */display: inline;\n`;
>+                    InspectorTest.expectEqual(styleDeclaration.text, expected, "`display: inline` property should be appended.");
>+
>+                    resolve();
>+                }
>+            });
>+
>+            WI.domManager.requestDocument((documentNode) => {
>+                WI.domManager.querySelector(documentNode.id, "#x", (contentNodeId) => {
>+                    if (contentNodeId) {
>+                        let domNode = WI.domManager.nodeForId(contentNodeId);
>+                        nodeStyles = WI.cssManager.stylesForNode(domNode);
>+
>+                        if (nodeStyles.needsRefresh) {
>+                            nodeStyles.singleFireEventListener(WI.DOMNodeStyles.Event.Refreshed, (event) => {
>+                                suite.runTestCasesAndFinish()
>+                            });
>+                        } else
>+                            suite.runTestCasesAndFinish();
>+                    } else {
>+                        InspectorTest.fail("DOM node not found.");
>+                        InspectorTest.completeTest();
>+                    }
>+                });
>+            });
>+        }
>+    </script>
>+</head>
>+<body onload="runTest()">
>+<p>Testing that CSSStyleDeclaration updates after inserting new CSS properties.</p>
>+
>+<style>
>+.rule-a {font-size: 14px}
>+.rule-b {
>+    outline: 2px solid brown;
>+}
>+.rule-c {
>+    font-size: 14px;
>+    /* color: red; */
>+}
>+.rule-d {
>+    font-size: 14px;
>+    /* color: red */
>+}
>+</style>
>+<div id="x" class="rule-a rule-b rule-c rule-d" style="width: 100px"></div>
>+</body>
>+</html>
>diff --git a/LayoutTests/inspector/css/modify-css-property-expected.txt b/LayoutTests/inspector/css/modify-css-property-expected.txt
>index c8aa3d9cbfc..07b4ae0b3fa 100644
>--- a/LayoutTests/inspector/css/modify-css-property-expected.txt
>+++ b/LayoutTests/inspector/css/modify-css-property-expected.txt
>@@ -16,3 +16,18 @@ PASS: Style declaration text should stay "width: 64px".
> PASS: "width" property value should update to "200px".
> PASS: Inline style declaration text should update when not locked.
> 
>+-- Running test case: ModifyCSSProperty.CommentOutAndUncommentProperty
>+PASS: Commented out property should be disabled.
>+PASS: Style declaration text should update immediately with uncommented property.
>+PASS: Uncommented property should be enabled.
>+PASS: Style declaration text should update immediately with commented out property.
>+PASS: Commented out property should be disabled.
>+
>+-- Running test case: ModifyCSSProperty.CommentOutAndUncommentProperty2
>+PASS: Commented out property should be disabled.
>+PASS: Style declaration text should update immediately with uncommented property.
>+PASS: Uncommented property should be enabled.
>+PASS: Commented out property should be disabled.
>+PASS: Style declaration text should update immediately with commented out property.
>+PASS: Uncommented property should be enabled.
>+
>diff --git a/LayoutTests/inspector/css/modify-css-property.html b/LayoutTests/inspector/css/modify-css-property.html
>index c35b9f03157..92723b141df 100644
>--- a/LayoutTests/inspector/css/modify-css-property.html
>+++ b/LayoutTests/inspector/css/modify-css-property.html
>@@ -23,6 +23,8 @@ function test() {
>                     if (rule.selectorText === ".rule-b")
>                         return rule.style;
>                 }
>+                InspectorTest.fail("No declaration found.");
>+                reject();
>             };
> 
>             let getProperty = (propertyName) => {
>@@ -31,6 +33,8 @@ function test() {
>                     if (property.name === propertyName)
>                         return property;
>                 }
>+                InspectorTest.fail("No property found.");
>+                reject();
>             };
> 
>             let styleDeclaration = getMatchedStyleDeclaration();
>@@ -54,6 +58,8 @@ function test() {
>                     if (rule.selectorText === ".rule-a")
>                         return rule.style;
>                 }
>+                InspectorTest.fail("No declaration found.");
>+                reject();
>             };
> 
>             let getProperty = (propertyName) => {
>@@ -62,6 +68,8 @@ function test() {
>                     if (property.name === propertyName)
>                         return property;
>                 }
>+                InspectorTest.fail("No property found.");
>+                reject();
>             };
> 
>             let styleDeclaration = getMatchedStyleDeclaration();
>@@ -93,6 +101,8 @@ function test() {
>                     if (styleDeclaration.type === styleDeclaration.constructor.Type.Inline)
>                         return styleDeclaration;
>                 }
>+                InspectorTest.fail("No declaration found.");
>+                reject();
>             };
> 
>             let getProperty = (propertyName) => {
>@@ -101,6 +111,8 @@ function test() {
>                     if (property.name === propertyName)
>                         return property;
>                 }
>+                InspectorTest.fail("No property found.");
>+                reject();
>             };
> 
>             let styleDeclaration = getInlineStyleDeclaration();
>@@ -127,6 +139,107 @@ function test() {
>         }
>     });
> 
>+    suite.addTestCase({
>+        name: "ModifyCSSProperty.CommentOutAndUncommentProperty",
>+        test(resolve, reject) {
>+            let getMatchedStyleDeclaration = () => {
>+                for (let rule of nodeStyles.matchedRules) {
>+                    if (rule.selectorText === ".rule-c")
>+                        return rule.style;
>+                }
>+                InspectorTest.fail("No declaration found.");
>+                reject();
>+            };
>+
>+            let getProperty = (propertyName) => {
>+                let styleDeclaration = getMatchedStyleDeclaration();
>+                for (let property of styleDeclaration.allProperties) {
>+                    if (property.name === propertyName)
>+                        return property;
>+                }
>+                InspectorTest.fail("No property found.");
>+                reject();
>+            };
>+
>+            let styleDeclaration = getMatchedStyleDeclaration();
>+            styleDeclaration.locked = true;
>+
>+            InspectorTest.expectThat(!getProperty("padding-right").enabled, `Commented out property should be disabled.`);
>+
>+            let disabled = false;
>+            getProperty("padding-right").commentOut(disabled);
>+
>+            let expectedStyleText = `
>+        /* padding-left: 2em; */
>+        padding-right: 0px;
>+    `;
>+            InspectorTest.expectEqual(styleDeclaration.text, expectedStyleText, `Style declaration text should update immediately with uncommented property.`);
>+
>+            InspectorTest.expectThat(getProperty("padding-right").enabled, `Uncommented property should be enabled.`);
>+
>+            disabled = true;
>+            getProperty("padding-right").commentOut(disabled);
>+
>+            expectedStyleText = `
>+        /* padding-left: 2em; */
>+        /* padding-right: 0px; */
>+    `;
>+            InspectorTest.expectEqual(styleDeclaration.text, expectedStyleText, `Style declaration text should update immediately with commented out property.`);
>+
>+            InspectorTest.expectThat(!getProperty("padding-right").enabled, `Commented out property should be disabled.`);
>+
>+            resolve();
>+        }
>+    });
>+
>+    suite.addTestCase({
>+        name: "ModifyCSSProperty.CommentOutAndUncommentProperty2",
>+        test(resolve, reject) {
>+            let getMatchedStyleDeclaration = () => {
>+                for (let rule of nodeStyles.matchedRules) {
>+                    if (rule.selectorText === ".rule-d")
>+                        return rule.style;
>+                }
>+                InspectorTest.fail("No declaration found.");
>+                reject();
>+            };
>+
>+            let getProperty = (propertyName) => {
>+                let styleDeclaration = getMatchedStyleDeclaration();
>+                for (let property of styleDeclaration.allProperties) {
>+                    if (property.name === propertyName)
>+                        return property;
>+                }
>+                InspectorTest.fail("No property found.");
>+                reject();
>+            };
>+
>+            let styleDeclaration = getMatchedStyleDeclaration();
>+            styleDeclaration.locked = true;
>+
>+            InspectorTest.expectThat(!getProperty("font-size").enabled, `Commented out property should be disabled.`);
>+
>+            let disabled = false;
>+            getProperty("font-size").commentOut(disabled);
>+
>+            let expectedStyleText = `font-size: 13px;/*border: 2px solid brown*/`;
>+            InspectorTest.expectEqual(styleDeclaration.text, expectedStyleText, `Style declaration text should update immediately with uncommented property.`);
>+
>+            InspectorTest.expectThat(getProperty("font-size").enabled, `Uncommented property should be enabled.`);
>+            InspectorTest.expectThat(!getProperty("border").enabled, `Commented out property should be disabled.`);
>+
>+            disabled = false;
>+            getProperty("border").commentOut(disabled);
>+
>+            expectedStyleText = `font-size: 13px;border: 2px solid brown`;
>+            InspectorTest.expectEqual(styleDeclaration.text, expectedStyleText, `Style declaration text should update immediately with commented out property.`);
>+
>+            InspectorTest.expectThat(getProperty("border").enabled, `Uncommented property should be enabled.`);
>+
>+            resolve();
>+        }
>+    });
>+
>     WI.domManager.requestDocument((documentNode) => {
>         WI.domManager.querySelector(documentNode.id, "#x", (contentNodeId) => {
>             if (contentNodeId) {
>@@ -160,7 +273,12 @@ function test() {
>         margin-top: 1em;
>     }
>     .rule-b {font-size: 12px; color: antiquewhite}
>+    .rule-c {
>+        /* padding-left: 2em; */
>+        /* padding-right: 0px; */
>+    }
>+    .rule-d {/*font-size: 13px;*//*border: 2px solid brown*/}
>     </style>
>-    <div id="x" class="test-node rule-a rule-b" style="width: 100px"></div>
>+    <div id="x" class="test-node rule-a rule-b rule-c rule-d" style="width: 100px"></div>
> </body>
> </html>
>diff --git a/Source/WebInspectorUI/ChangeLog b/Source/WebInspectorUI/ChangeLog
>index 698900f5b78..100f29b0f71 100644
>--- a/Source/WebInspectorUI/ChangeLog
>+++ b/Source/WebInspectorUI/ChangeLog
>@@ -1,3 +1,35 @@
>+2018-12-08  Nikita Vasilyev  <nvasilyev@apple.com>
>+
>+        Web Inspector: Styles: toggling selected properties may cause data corruption
>+        https://bugs.webkit.org/show_bug.cgi?id=192396
>+        <rdar://problem/46478383>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Uncommenting a property after a commented out property used to insert an unnecessary semicolon,
>+        and not updating ranges of the following properties.
>+
>+        For example:
>+
>+            /* color: red; */
>+            /* font-size: 12px */
>+
>+        Uncommenting `font-size` would result in something like this:
>+
>+            /* color: red; */; font-size: 12px
>+                             ^
>+                             unnecessary semicolon
>+
>+        Now the semicolon doesn't get inserted and the white space is preserved better:
>+
>+            /* color: red; */
>+            font-size: 12px
>+
>+        * UserInterface/Models/CSSProperty.js:
>+        (WI.CSSProperty.prototype._updateOwnerStyleText):
>+        (WI.CSSProperty.prototype._appendSemicolonIfNeeded): Removed.
>+        (WI.CSSProperty.prototype._prependSemicolonIfNeeded): Added.
>+
> 2018-12-06  Matt Baker  <mattbaker@apple.com>
> 
>         Web Inspector: REGRESSION(r238602): Elements: collapsing a DOM node with the left arrow doesn't work
>diff --git a/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js b/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js
>index d6e4ad71535..4ba36efc8c3 100644
>--- a/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js
>+++ b/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js
>@@ -358,6 +358,8 @@ WI.CSSProperty = class CSSProperty extends WI.Object
>             return;
>         }
> 
>+        this._prependSemicolonIfNeeded();
>+
>         let styleText = this._ownerStyle.text || "";
> 
>         // _styleSheetTextRange is the position of the property within the stylesheet.
>@@ -375,7 +377,7 @@ WI.CSSProperty = class CSSProperty extends WI.Object
>             console.info(`${prefix}%c${oldText}%c${newText}%c${postfix}`, `background: hsl(356, 100%, 90%); color: black`, `background: hsl(100, 100%, 91%); color: black`, `background: transparent`);
>         }
> 
>-        let newStyleText = this._appendSemicolonIfNeeded(styleText.slice(0, range.startOffset)) + newText + styleText.slice(range.endOffset);
>+        let newStyleText = styleText.slice(0, range.startOffset) + newText + styleText.slice(range.endOffset);
> 
>         let lineDelta = newText.lineCount - oldText.lineCount;
>         let columnDelta = newText.lastLine.length - oldText.lastLine.length;
>@@ -387,12 +389,19 @@ WI.CSSProperty = class CSSProperty extends WI.Object
>         this._ownerStyle.shiftPropertiesAfter(this, lineDelta, columnDelta, propertyWasRemoved);
>     }
> 
>-    _appendSemicolonIfNeeded(styleText)
>+    _prependSemicolonIfNeeded()
>     {
>-        if (/[^;\s]\s*$/.test(styleText))
>-            return styleText.trimRight() + "; ";
>+        for (let i = this.index - 1; i >= 0; --i) {
>+            let property = this._ownerStyle.allProperties[i];
>+            if (!property.enabled)
>+                continue;
>+
>+            let match = property.text.match(/[^;\s](\s*)$/);
>+            if (match)
>+                property.text = property.text.trimRight() + ";" + match[1];
> 
>-        return styleText;
>+            break;
>+        }
>     }
> };
>

Flags:

hi: review+
hi: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 192396: 356761 | 356785 | 356790 | 356850 | 356890 | 356891 | 357397