Attachment 357397 Details for Bug 192396 – Patch

[patch] Patch

patch.txt (text/plain), 22.06 KB, created by Nikita Vasilyev on 2018-12-15 01:21:20 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Nikita Vasilyev

Created: 2018-12-15 01:21:20 PST

Size: 22.06 KB

patch

obsolete

>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 565ab81a36f..4b486277f45 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,19 @@
>+2018-12-15  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 Devin Rousso.
>+
>+        * 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..28c39df157c
>--- /dev/null
>+++ b/LayoutTests/inspector/css/add-css-property-expected.txt
>@@ -0,0 +1,20 @@
>+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.InsertBetween
>+PASS: `font-family: fantasy` property should be inserted after the first property.
>+
>+-- 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..7a91fbd0fa2
>--- /dev/null
>+++ b/LayoutTests/inspector/css/add-css-property.html
>@@ -0,0 +1,157 @@
>+<!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");
>+
>+    function getMatchedStyle(selector, resolve) {
>+        for (let rule of nodeStyles.matchedRules) {
>+            if (rule.selectorText === selector)
>+                return rule.style;
>+        }
>+
>+        InspectorTest.fail(`No style found for selector "${selector}"`);
>+        resolve();
>+    }
>+
>+    suite.addTestCase({
>+        name: "AddCSSProperty.AppendAfterMissingSemicolon",
>+        test(resolve, reject) {
>+            let styleDeclaration = getMatchedStyle(".rule-a", resolve);
>+            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.");
>+
>+            styleDeclaration.locked = false;
>+            resolve();
>+        }
>+    });
>+
>+    suite.addTestCase({
>+        name: "AddCSSProperty.InsertBeforeAndAfter",
>+        test(resolve, reject) {
>+            let styleDeclaration = getMatchedStyle(".rule-b", resolve);
>+            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 = getMatchedStyle(".rule-b", resolve);
>+            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.");
>+
>+            styleDeclaration.locked = false;
>+            resolve();
>+        }
>+    });
>+
>+    suite.addTestCase({
>+        name: "AddCSSProperty.InsertBetween",
>+        test(resolve, reject) {
>+            let styleDeclaration = getMatchedStyle(".rule-c", resolve);
>+            styleDeclaration.locked = true;
>+            let newProperty = styleDeclaration.newBlankProperty(1);
>+            newProperty.name = "font-family";
>+            newProperty.rawValue = "fantasy";
>+
>+            let expected = `\n    background-color: peachpuff;font-family: fantasy;\n    color: burlywood\n`;
>+            InspectorTest.expectEqual(styleDeclaration.text, expected, "`font-family: fantasy` property should be inserted after the first property.");
>+
>+            styleDeclaration.locked = false;
>+            resolve();
>+        }
>+    });
>+
>+    suite.addTestCase({
>+        name: "AddCSSProperty.AppendAfterCommentedOutProperty",
>+        test(resolve, reject) {
>+            let styleDeclaration = getMatchedStyle(".rule-d", resolve);
>+            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.");
>+
>+            styleDeclaration.locked = false;
>+            resolve();
>+        }
>+    });
>+
>+    suite.addTestCase({
>+        name: "AddCSSProperty.AppendAfterCommentedOutPropertyWithoutSemicolon",
>+        test(resolve, reject) {
>+            let styleDeclaration = getMatchedStyle(".rule-e", resolve);
>+            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.");
>+
>+            styleDeclaration.locked = false;
>+            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 {
>+    background-color: peachpuff;
>+    color: burlywood
>+}
>+.rule-d {
>+    font-size: 14px;
>+    /* color: red; */
>+}
>+.rule-e {
>+    font-size: 14px;
>+    /* color: red */
>+}
>+</style>
>+<div id="x" class="rule-a rule-b rule-c rule-d rule-e" 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..c5374e54c9e 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.CommentOutAndUncommentPropertyWithNewlines
>+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.CommentOutAndUncommentPropertyWithoutNewlines
>+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..52266588482 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.");
>+                resolve();
>             };
> 
>             let getProperty = (propertyName) => {
>@@ -31,6 +33,8 @@ function test() {
>                     if (property.name === propertyName)
>                         return property;
>                 }
>+                InspectorTest.fail("No property found.");
>+                resolve();
>             };
> 
>             let styleDeclaration = getMatchedStyleDeclaration();
>@@ -54,6 +58,8 @@ function test() {
>                     if (rule.selectorText === ".rule-a")
>                         return rule.style;
>                 }
>+                InspectorTest.fail("No declaration found.");
>+                resolve();
>             };
> 
>             let getProperty = (propertyName) => {
>@@ -62,6 +68,8 @@ function test() {
>                     if (property.name === propertyName)
>                         return property;
>                 }
>+                InspectorTest.fail("No property found.");
>+                resolve();
>             };
> 
>             let styleDeclaration = getMatchedStyleDeclaration();
>@@ -93,6 +101,8 @@ function test() {
>                     if (styleDeclaration.type === styleDeclaration.constructor.Type.Inline)
>                         return styleDeclaration;
>                 }
>+                InspectorTest.fail("No declaration found.");
>+                resolve();
>             };
> 
>             let getProperty = (propertyName) => {
>@@ -101,6 +111,8 @@ function test() {
>                     if (property.name === propertyName)
>                         return property;
>                 }
>+                InspectorTest.fail("No property found.");
>+                resolve();
>             };
> 
>             let styleDeclaration = getInlineStyleDeclaration();
>@@ -127,6 +139,101 @@ function test() {
>         }
>     });
> 
>+    suite.addTestCase({
>+        name: "ModifyCSSProperty.CommentOutAndUncommentPropertyWithNewlines",
>+        test(resolve, reject) {
>+            let getMatchedStyleDeclaration = () => {
>+                for (let rule of nodeStyles.matchedRules) {
>+                    if (rule.selectorText === ".rule-c")
>+                        return rule.style;
>+                }
>+                InspectorTest.fail("No declaration found.");
>+                resolve();
>+            };
>+
>+            let getProperty = (propertyName) => {
>+                let styleDeclaration = getMatchedStyleDeclaration();
>+                for (let property of styleDeclaration.allProperties) {
>+                    if (property.name === propertyName)
>+                        return property;
>+                }
>+                InspectorTest.fail("No property found.");
>+                resolve();
>+            };
>+
>+            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 = `\n        /* padding-left: 2em; */\n        padding-right: 0px;\n    `;
>+            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 = `\n        /* padding-left: 2em; */\n        /* padding-right: 0px; */\n    `;
>+            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.CommentOutAndUncommentPropertyWithoutNewlines",
>+        test(resolve, reject) {
>+            let getMatchedStyleDeclaration = () => {
>+                for (let rule of nodeStyles.matchedRules) {
>+                    if (rule.selectorText === ".rule-d")
>+                        return rule.style;
>+                }
>+                InspectorTest.fail("No declaration found.");
>+                resolve();
>+            };
>+
>+            let getProperty = (propertyName) => {
>+                let styleDeclaration = getMatchedStyleDeclaration();
>+                for (let property of styleDeclaration.allProperties) {
>+                    if (property.name === propertyName)
>+                        return property;
>+                }
>+                InspectorTest.fail("No property found.");
>+                resolve();
>+            };
>+
>+            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 +267,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..9afa53e19ae 100644
>--- a/Source/WebInspectorUI/ChangeLog
>+++ b/Source/WebInspectorUI/ChangeLog
>@@ -1,3 +1,42 @@
>+2018-12-15  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 Devin Rousso.
>+
>+        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.
>+
>+        * UserInterface/Views/SpreadsheetStyleProperty.js:
>+        (WI.SpreadsheetStyleProperty.prototype.remove):
>+        (WI.SpreadsheetStyleProperty.prototype.update):
>+        (WI.SpreadsheetStyleProperty.prototype._handleNameChange):
>+        (WI.SpreadsheetStyleProperty.prototype._handleValueChange):
>+        Style declaration should be locked while editing. Add asserts to ensure this.
>+
> 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;
>+        }
>     }
> };
> 
>diff --git a/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js b/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js
>index ef68b658e9f..9ae9fa9112f 100644
>--- a/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js
>+++ b/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js
>@@ -129,6 +129,7 @@ WI.SpreadsheetStyleProperty = class SpreadsheetStyleProperty extends WI.Object
> 
>     remove(replacement = null)
>     {
>+        console.assert(this._property.ownerStyle.locked, `Removed property was unlocked (${this._property.name})`);
>         this.element.remove();
> 
>         if (replacement)
>@@ -153,6 +154,7 @@ WI.SpreadsheetStyleProperty = class SpreadsheetStyleProperty extends WI.Object
>             this._checkboxElement.checked = this._property.enabled;
>             this._checkboxElement.tabIndex = -1;
>             this._checkboxElement.addEventListener("click", (event) => {
>+                console.assert(this._property.ownerStyle.locked, `Toggled property was unlocked (${this._property.name})`);
>                 event.stopPropagation();
>                 let disabled = !this._checkboxElement.checked;
>                 this._property.commentOut(disabled);
>@@ -651,11 +653,15 @@ WI.SpreadsheetStyleProperty = class SpreadsheetStyleProperty extends WI.Object
> 
>     _handleNameChange()
>     {
>+        console.assert(this._property.ownerStyle.locked, `Modified property was unlocked (${this._property.name})`);
>+
>         this._property.name = this._nameElement.textContent.trim();
>     }
> 
>     _handleValueChange()
>     {
>+        console.assert(this._property.ownerStyle.locked, `Modified property was unlocked (${this._property.name})`);
>+
>         this._property.rawValue = this._valueElement.textContent.trim();
>     }
>

Actions: View | Formatted Diff | Diff

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