Web Inspector: Styles: fix race conditions when editing
authornvasilyev@apple.com <nvasilyev@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 4 Feb 2019 23:30:49 +0000 (23:30 +0000)
committernvasilyev@apple.com <nvasilyev@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 4 Feb 2019 23:30:49 +0000 (23:30 +0000)
https://bugs.webkit.org/show_bug.cgi?id=192739
<rdar://problem/46752925>

Reviewed by Devin Rousso.

Source/WebInspectorUI:

Editing CSS property in the style editor syncronously updates CSSStyleDeclaration on the front-end
and asyncronously updates the backend by calling CSSAgent.setStyleText. After the new style text is applied
on the backend, CSSStyleDeclaration (on the front-end) gets updated.

Unsure there's no race conditions by introducing `_updatesInProgressCount`:

  - Increment it before calling CSSAgent.setStyleText.
  - Decrement it after CSSAgent.setStyleText is finished.

Prevent updates of CSSStyleDeclaration when _updatesInProgressCount isn't 0.

* UserInterface/Models/CSSProperty.js:
(WI.CSSProperty.prototype._updateOwnerStyleText):
* UserInterface/Models/CSSStyleDeclaration.js:
(WI.CSSStyleDeclaration):
(WI.CSSStyleDeclaration.prototype.set text): Removed.
(WI.CSSStyleDeclaration.prototype.setText): Added.
Change the setter to a method since it has side effects including an asynchronous backend call.

* UserInterface/Models/DOMNodeStyles.js:
(WI.DOMNodeStyles.prototype.changeStyleText):

* UserInterface/Views/SpreadsheetStyleProperty.js:
(WI.SpreadsheetStyleProperty.prototype.get nameTextField): Removed.
(WI.SpreadsheetStyleProperty.prototype.get valueTextField): Removed.
Drive-by: remove unused code.
LayoutTests:

* inspector/css/modify-css-property-expected.txt:
* inspector/css/modify-css-property-race-expected.txt: Added.
* inspector/css/modify-css-property-race.html: Added.
* inspector/css/modify-css-property.html:

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

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

index bbfe58a..bcf139f 100644 (file)
@@ -1,3 +1,16 @@
+2019-02-04  Nikita Vasilyev  <nvasilyev@apple.com>
+
+        Web Inspector: Styles: fix race conditions when editing
+        https://bugs.webkit.org/show_bug.cgi?id=192739
+        <rdar://problem/46752925>
+
+        Reviewed by Devin Rousso.
+
+        * inspector/css/modify-css-property-expected.txt:
+        * inspector/css/modify-css-property-race-expected.txt: Added.
+        * inspector/css/modify-css-property-race.html: Added.
+        * inspector/css/modify-css-property.html:
+
 2019-02-04  Simon Fraser  <simon.fraser@apple.com>
 
         Async overflow scroll with border-radius renders incorrectly
index c5374e5..ec16073 100644 (file)
@@ -4,18 +4,27 @@ Testing that CSSStyleDeclaration update immediately after modifying its properti
 == Running test suite: ModifyCSSProperty
 -- Running test case: Update value when CSSStyleDeclaration is not locked
 PASS: "font-size" property value should update immediately.
-PASS: Style declaration text should stay unchanged.
+PASS: Style declaration text should update immediately.
 
 -- Running test case: Update value when CSSStyleDeclaration is locked
 PASS: "font-size" property value should update immediately.
 PASS: Style declaration text should update immediately.
 
+-- Running test case: ModifyCSSProperty.PropertiesChangedEvent
+PASS: Style declaration is unlocked.
+PASS: "width" property value should update to "200px".
+PASS: Inline style declaration text should update when not locked.
+
 -- Running test case: Update inline style value when CSSStyleDeclaration locked and not locked
 PASS: Style declaration text should update immediately.
-PASS: Style declaration text should stay "width: 64px".
-PASS: "width" property value should update to "200px".
+PASS: Style declaration text should update immediately.
+PASS: Style declaration is unlocked.
+PASS: "width" property value should update to "128px".
 PASS: Inline style declaration text should update when not locked.
 
+-- Running test case: ModifyCSSProperty.ConsequentValueChanges
+PASS: Style declaration text should contain most recent value.
+
 -- Running test case: ModifyCSSProperty.CommentOutAndUncommentPropertyWithNewlines
 PASS: Commented out property should be disabled.
 PASS: Style declaration text should update immediately with uncommented property.
diff --git a/LayoutTests/inspector/css/modify-css-property-race-expected.txt b/LayoutTests/inspector/css/modify-css-property-race-expected.txt
new file mode 100644 (file)
index 0000000..b4e857d
--- /dev/null
@@ -0,0 +1,11 @@
+Testing that changes to "style" attribute made from page's JavaScript are ignored when there's a pending change made via Web Inspector.
+
+
+== Running test suite: ModifyCSSProperty
+-- Running test case: ModifyCSSPropertyRace.ChangeInlineStyle
+PASS: expectGreaterThan(43, 42)
+PASS: expectGreaterThan(43, 42)
+PASS: Value updated to "10px".
+PASS: CSSStyleDeclaration text should update.
+PASS: expectGreaterThanOrEqual(10, 10)
+
diff --git a/LayoutTests/inspector/css/modify-css-property-race.html b/LayoutTests/inspector/css/modify-css-property-race.html
new file mode 100644 (file)
index 0000000..83c691d
--- /dev/null
@@ -0,0 +1,117 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../../http/tests/inspector/resources/inspector-test.js"></script>
+<script>
+let requestAnimationFrameID;
+
+function expand() {
+    let element = document.getElementById("x");
+    let initialHeight = parseInt(element.style.height);
+    let i = 0;
+
+    let loop = () => {
+        ++i;
+        let height = initialHeight + i;
+        element.style.height = height + "px";
+        TestPage.dispatchEventToFrontend("TestPage-styleChanged", height + "px");
+
+        requestAnimationFrameID = requestAnimationFrame(loop);
+    };
+
+    loop();
+}
+
+function stopExpanding() {
+    cancelAnimationFrame(requestAnimationFrameID);
+}
+
+function test() {
+    InspectorTest.redirectRequestAnimationFrame();
+
+    let nodeStyles = null;
+    let suite = InspectorTest.createAsyncSuite("ModifyCSSProperty");
+
+    InspectorTest.addEventListener("TestPage-styleChanged", (event) => {
+        // Normally, this would get called if the styles sidebar is visible.
+        // This doesn't work in tests.
+        nodeStyles.refresh();
+    });
+
+    suite.addTestCase({
+        name: "ModifyCSSPropertyRace.ChangeInlineStyle",
+        test(resolve, reject) {
+            let getInlineStyleDeclaration = () => {
+                for (let styleDeclaration of nodeStyles.orderedStyles) {
+                    if (styleDeclaration.type === styleDeclaration.constructor.Type.Inline)
+                        return styleDeclaration;
+                }
+                InspectorTest.fail("No declaration found.");
+                resolve();
+            };
+
+            let getProperty = (propertyName) => {
+                let styleDeclaration = getInlineStyleDeclaration();
+                for (let property of styleDeclaration.properties) {
+                    if (property.name === propertyName)
+                        return property;
+                }
+                InspectorTest.fail("No property found.");
+                resolve();
+            };
+
+            InspectorTest.evaluateInPage("expand()");
+            let updateCount = 0;
+
+            function styleDecorationUpdated(event) {
+                if (!updateCount) {
+                    let valueNumber = parseInt(getProperty("height").rawValue);
+                    InspectorTest.expectGreaterThan(valueNumber, 42);
+                } else if (updateCount === 1) {
+                    let valueNumber = parseInt(getProperty("height").rawValue);
+                    InspectorTest.expectGreaterThan(valueNumber, 42);
+                    getProperty("height").rawValue = "10px";
+                } else if (updateCount === 2) {
+                    InspectorTest.expectEqual(getProperty("height").rawValue, "10px", `Value updated to "10px".`);
+                    InspectorTest.expectEqual(getInlineStyleDeclaration().text, "height: 10px;", "CSSStyleDeclaration text should update.");
+                } else {
+                    let valueNumber = parseInt(getProperty("height").rawValue);
+                    InspectorTest.expectGreaterThanOrEqual(valueNumber, 10);
+
+                    InspectorTest.evaluateInPage("stopExpanding()");
+                    WI.CSSStyleDeclaration.removeEventListener(WI.CSSStyleDeclaration.Event.PropertiesChanged, styleDecorationUpdated);
+                    resolve();
+                }
+                ++updateCount;
+            }
+
+            WI.CSSStyleDeclaration.addEventListener(WI.CSSStyleDeclaration.Event.PropertiesChanged, styleDecorationUpdated);
+        }
+    });
+
+    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 changes to "style" attribute made from page's JavaScript are ignored when there's a pending change made via Web Inspector.</p>
+    <div id="x" style="height: 42px"></div>
+</body>
+</html>
index c181702..32ffd9a 100644 (file)
@@ -44,7 +44,7 @@ function test() {
 
             InspectorTest.expectEqual(getProperty("font-size").rawValue, "10px", `"font-size" property value should update immediately.`);
 
-            InspectorTest.expectEqual(styleDeclaration.text, `font-size: 12px; color: antiquewhite`, `Style declaration text should stay unchanged.`);
+            InspectorTest.expectEqual(styleDeclaration.text, `font-size: 10px; color: antiquewhite`, `Style declaration text should update immediately.`);
 
             resolve();
         }
@@ -94,6 +94,46 @@ function test() {
     });
 
     suite.addTestCase({
+        name: "ModifyCSSProperty.PropertiesChangedEvent",
+        test(resolve, reject) {
+            let getInlineStyleDeclaration = () => {
+                for (let styleDeclaration of nodeStyles.orderedStyles) {
+                    if (styleDeclaration.type === styleDeclaration.constructor.Type.Inline)
+                        return styleDeclaration;
+                }
+                InspectorTest.fail("No declaration found.");
+                resolve();
+            };
+
+            let getProperty = (propertyName) => {
+                let styleDeclaration = getInlineStyleDeclaration();
+                for (let property of styleDeclaration.properties) {
+                    if (property.name === propertyName)
+                        return property;
+                }
+                InspectorTest.fail("No property found.");
+                resolve();
+            };
+
+            let styleDeclaration = getInlineStyleDeclaration();
+
+            styleDeclaration.awaitEvent(WI.CSSStyleDeclaration.Event.PropertiesChanged).then((event) => {
+                InspectorTest.expectThat(!styleDeclaration.locked, `Style declaration is unlocked.`);
+                InspectorTest.expectEqual(getProperty("width").rawValue, "200px", `"width" property value should update to "200px".`);
+                InspectorTest.expectEqual(styleDeclaration.text, `width: 200px;`, `Inline style declaration text should update when not locked.`);
+                resolve();
+            });
+
+            styleDeclaration.locked = true;
+            // WI.CSSStyleDeclaration.Event.PropertiesChanged event should not fire when the style declaration is locked.
+            InspectorTest.evaluateInPage(`makeNarrow()`);
+
+            styleDeclaration.locked = false;
+            InspectorTest.evaluateInPage(`makeWide()`);
+        }
+    });
+
+    suite.addTestCase({
         name: "Update inline style value when CSSStyleDeclaration locked and not locked",
         test(resolve, reject) {
             let getInlineStyleDeclaration = () => {
@@ -117,12 +157,12 @@ function test() {
 
             let styleDeclaration = getInlineStyleDeclaration();
 
-            styleDeclaration.awaitEvent(WI.CSSStyleDeclaration.Event.PropertiesChanged)
-                .then((event) => {
-                    InspectorTest.expectEqual(getProperty("width").rawValue, "200px", `"width" property value should update to "200px".`);
-                    InspectorTest.expectEqual(styleDeclaration.text, `width: 200px;`, `Inline style declaration text should update when not locked.`);
-                })
-                .then(resolve, reject);
+            WI.CSSStyleDeclaration.awaitEvent(WI.CSSStyleDeclaration.Event.PropertiesChanged).then((event) => {
+                InspectorTest.expectThat(!styleDeclaration.locked, `Style declaration is unlocked.`);
+                InspectorTest.expectEqual(getProperty("width").rawValue, "128px", `"width" property value should update to "128px".`);
+                InspectorTest.expectEqual(styleDeclaration.text, `width: 128px;`, `Inline style declaration text should update when not locked.`);
+                resolve();
+            });
 
             styleDeclaration.locked = true;
             getProperty("width").rawValue = "64px";
@@ -133,13 +173,47 @@ function test() {
 
             styleDeclaration.locked = false;
             getProperty("width").rawValue = "128px";
-            InspectorTest.expectEqual(styleDeclaration.text, `width: 64px;`, `Style declaration text should stay "width: 64px".`);
+            InspectorTest.expectEqual(styleDeclaration.text, `width: 128px;`, `Style declaration text should update immediately.`);
 
             InspectorTest.evaluateInPage(`makeWide()`);
         }
     });
 
     suite.addTestCase({
+        name: "ModifyCSSProperty.ConsequentValueChanges",
+        test(resolve, reject) {
+            let getInlineStyleDeclaration = () => {
+                for (let styleDeclaration of nodeStyles.orderedStyles) {
+                    if (styleDeclaration.type === styleDeclaration.constructor.Type.Inline)
+                        return styleDeclaration;
+                }
+                InspectorTest.fail("No declaration found.");
+                resolve();
+            };
+
+            let getProperty = (propertyName) => {
+                let styleDeclaration = getInlineStyleDeclaration();
+                for (let property of styleDeclaration.properties) {
+                    if (property.name === propertyName)
+                        return property;
+                }
+                InspectorTest.fail("No property found.");
+                resolve();
+            };
+
+            let styleDeclaration = getInlineStyleDeclaration();
+            styleDeclaration.locked = false;
+
+            for (let i = 0; i <= 20; ++i)
+                getProperty("width").rawValue = i + "px";
+
+            InspectorTest.expectEqual(styleDeclaration.text, `width: 20px;`, `Style declaration text should contain most recent value.`);
+
+            resolve();
+        }
+    });
+
+    suite.addTestCase({
         name: "ModifyCSSProperty.CommentOutAndUncommentPropertyWithNewlines",
         test(resolve, reject) {
             let getMatchedStyleDeclaration = () => {
index bcd979d..d999e2b 100644 (file)
@@ -1,3 +1,38 @@
+2019-02-04  Nikita Vasilyev  <nvasilyev@apple.com>
+
+        Web Inspector: Styles: fix race conditions when editing
+        https://bugs.webkit.org/show_bug.cgi?id=192739
+        <rdar://problem/46752925>
+
+        Reviewed by Devin Rousso.
+
+        Editing CSS property in the style editor syncronously updates CSSStyleDeclaration on the front-end
+        and asyncronously updates the backend by calling CSSAgent.setStyleText. After the new style text is applied
+        on the backend, CSSStyleDeclaration (on the front-end) gets updated.
+
+        Unsure there's no race conditions by introducing `_updatesInProgressCount`:
+
+          - Increment it before calling CSSAgent.setStyleText.
+          - Decrement it after CSSAgent.setStyleText is finished.
+
+        Prevent updates of CSSStyleDeclaration when _updatesInProgressCount isn't 0.
+
+        * UserInterface/Models/CSSProperty.js:
+        (WI.CSSProperty.prototype._updateOwnerStyleText):
+        * UserInterface/Models/CSSStyleDeclaration.js:
+        (WI.CSSStyleDeclaration):
+        (WI.CSSStyleDeclaration.prototype.set text): Removed.
+        (WI.CSSStyleDeclaration.prototype.setText): Added.
+        Change the setter to a method since it has side effects including an asynchronous backend call.
+
+        * UserInterface/Models/DOMNodeStyles.js:
+        (WI.DOMNodeStyles.prototype.changeStyleText):
+
+        * UserInterface/Views/SpreadsheetStyleProperty.js:
+        (WI.SpreadsheetStyleProperty.prototype.get nameTextField): Removed.
+        (WI.SpreadsheetStyleProperty.prototype.get valueTextField): Removed.
+        Drive-by: remove unused code.
+
 2019-02-01  Devin Rousso  <drousso@apple.com>
 
         Web Inspector: create icons for media event types instead of using a blue circle
index 1be38e5..6696783 100644 (file)
@@ -380,6 +380,10 @@ WI.CSSProperty = class CSSProperty extends WI.Object
             return;
         }
 
+        console.assert(this._ownerStyle);
+        if (!this._ownerStyle)
+            return;
+
         this._prependSemicolonIfNeeded();
 
         let styleText = this._ownerStyle.text || "";
index 56c1067..6d06c8a 100644 (file)
@@ -41,6 +41,7 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object
         this._inherited = inherited || false;
 
         this._initialState = null;
+        this._updatesInProgressCount = 0;
         this._locked = false;
         this._pendingProperties = [];
         this._propertyNameMap = {};
@@ -106,9 +107,22 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object
     update(text, properties, styleSheetTextRange, options = {})
     {
         let dontFireEvents = options.dontFireEvents || false;
-        let suppressLock = options.suppressLock || false;
 
-        if (this._locked && !suppressLock && text !== this._text)
+        // When two consequent setText calls happen (A and B), only update when the last call (B) is finished.
+        //               Front-end:   A B
+        //                Back-end:       A B
+        // _updatesInProgressCount: 0 1 2 1 0
+        //                                  ^
+        //                                  update only happens here
+        if (this._updatesInProgressCount > 0 && !options.forceUpdate) {
+            if (WI.settings.enableStyleEditingDebugMode.value && text !== this._text)
+                console.warn("Style modified while editing:", text);
+
+            return;
+        }
+
+        // Allow updates from the backend when text matches because `properties` may contain warnings that need to be shown.
+        if (this._locked && !options.forceUpdate && text !== this._text)
             return;
 
         text = text || "";
@@ -199,11 +213,24 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object
         if (!trimmedText.length || this._type === WI.CSSStyleDeclaration.Type.Inline)
             text = trimmedText;
 
-        // Update text immediately when it was modified via the styles sidebar.
-        if (this._locked)
-            this._text = text;
+        this._text = text;
+        ++this._updatesInProgressCount;
+
+        let timeoutId = setTimeout(() => {
+            console.error("Timed out when setting style text:", text);
+            styleTextDidChange();
+        }, 2000);
+
+        let styleTextDidChange = () => {
+            if (!timeoutId)
+                return;
+
+            clearTimeout(timeoutId);
+            timeoutId = null;
+            this._updatesInProgressCount = Math.max(0, this._updatesInProgressCount - 1);
+        };
 
-        this._nodeStyles.changeStyleText(this, text);
+        this._nodeStyles.changeStyleText(this, text, styleTextDidChange);
     }
 
     get enabledProperties()
@@ -322,7 +349,7 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object
         for (let index = propertyIndex + 1; index < this._properties.length; index++)
             this._properties[index].index = index;
 
-        this.update(this._text, this._properties, this._styleSheetTextRange, {dontFireEvents: true, suppressLock: true});
+        this.update(this._text, this._properties, this._styleSheetTextRange, {dontFireEvents: true, forceUpdate: true});
 
         return property;
     }
index 16cf2ff..c784507 100644 (file)
@@ -449,21 +449,25 @@ WI.DOMNodeStyles = class DOMNodeStyles extends WI.Object
         return result.promise;
     }
 
-    changeStyleText(style, text)
+    changeStyleText(style, text, callback)
     {
-        if (!style.ownerStyleSheet || !style.styleSheetTextRange)
+        if (!style.ownerStyleSheet || !style.styleSheetTextRange) {
+            callback();
             return;
+        }
 
         text = text || "";
 
-        function styleChanged(error, stylePayload)
-        {
-            if (error)
+        let didSetStyleText = (error, stylePayload) => {
+            if (error) {
+                callback(error);
                 return;
-            this.refresh();
-        }
+            }
+
+            this.refresh().then(callback);
+        };
 
-        CSSAgent.setStyleText(style.id, text, styleChanged.bind(this));
+        CSSAgent.setStyleText(style.id, text, didSetStyleText);
     }
 
     // Private
index b727b04..b5eb8d1 100644 (file)
@@ -75,8 +75,6 @@ WI.SpreadsheetStyleProperty = class SpreadsheetStyleProperty extends WI.Object
 
     get element() { return this._element; }
     get property() { return this._property; }
-    get nameTextField() { return this._nameTextField; }
-    get valueTextField() { return this._valueTextField; }
     get enabled() { return this._property.enabled; }
 
     set index(index)