Web Inspector: Styles: toggling selected properties may cause data corruption
authornvasilyev@apple.com <nvasilyev@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 15 Dec 2018 10:03:19 +0000 (10:03 +0000)
committernvasilyev@apple.com <nvasilyev@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 15 Dec 2018 10:03:19 +0000 (10:03 +0000)
https://bugs.webkit.org/show_bug.cgi?id=192396
<rdar://problem/46478383>

Reviewed by Devin Rousso.

Source/WebInspectorUI:

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.

LayoutTests:

* 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.

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

LayoutTests/ChangeLog
LayoutTests/inspector/css/add-css-property-expected.txt [new file with mode: 0644]
LayoutTests/inspector/css/add-css-property.html [new file with mode: 0644]
LayoutTests/inspector/css/modify-css-property-expected.txt
LayoutTests/inspector/css/modify-css-property.html
Source/WebInspectorUI/ChangeLog
Source/WebInspectorUI/UserInterface/Models/CSSProperty.js
Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js

index a8c77ad..cc8b9a8 100644 (file)
@@ -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-14  Youenn Fablet  <youenn@apple.com>
 
         MediaRecorderPrivateAVFImpl should have a Ref<MediaRecorderPrivateWriter> as member
diff --git a/LayoutTests/inspector/css/add-css-property-expected.txt b/LayoutTests/inspector/css/add-css-property-expected.txt
new file mode 100644 (file)
index 0000000..28c39df
--- /dev/null
@@ -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 (file)
index 0000000..7a91fbd
--- /dev/null
@@ -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>
index c8aa3d9..c5374e5 100644 (file)
@@ -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.
+
index c35b9f0..5226658 100644 (file)
@@ -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>
index e344c35..9152ad4 100644 (file)
@@ -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-14  Matt Baker  <mattbaker@apple.com>
 
         Web Inspector: REGRESSION(r238599): Uncaught Exception: TypeError: null is not an object (evaluating 'treeElement.listItemElement.classList')
index d6e4ad7..4ba36ef 100644 (file)
@@ -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;
+        }
     }
 };
 
index ef68b65..9ae9fa9 100644 (file)
@@ -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();
     }