Attachment 361008 Details for Bug 192739 – Patch

[patch] Patch

patch.txt (text/plain), 20.94 KB, created by Nikita Vasilyev on 2019-02-03 04:01:56 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Nikita Vasilyev

Created: 2019-02-03 04:01:56 PST

Size: 20.94 KB

patch

obsolete

>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 395610b7ad6..7e30d24d16c 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,16 @@
>+2019-02-02  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 NOBODY (OOPS!).
>+
>+        * 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-01  Antoine Quint  <graouts@apple.com>
> 
>         Dispatch pointercancel events when content is panned or zoomed on iOS
>diff --git a/LayoutTests/inspector/css/modify-css-property-expected.txt b/LayoutTests/inspector/css/modify-css-property-expected.txt
>index c5374e54c9e..1d3b5ee4d71 100644
>--- a/LayoutTests/inspector/css/modify-css-property-expected.txt
>+++ b/LayoutTests/inspector/css/modify-css-property-expected.txt
>@@ -4,18 +4,23 @@ 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: 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".
>+-- Running test case: ModifyCSSProperty.PropertiesChangedEvent
> 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 update immediately.
>+
>+-- 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
>index 00000000000..60d5fb96f06
>--- /dev/null
>+++ b/LayoutTests/inspector/css/modify-css-property-race-expected.txt
>@@ -0,0 +1,9 @@
>+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
>+First style change.
>+PASS: Value updated to "10px".
>+PASS: CSSStyleDeclaration text should update.
>+
>diff --git a/LayoutTests/inspector/css/modify-css-property-race.html b/LayoutTests/inspector/css/modify-css-property-race.html
>new file mode 100644
>index 00000000000..0ef0a56c37e
>--- /dev/null
>+++ b/LayoutTests/inspector/css/modify-css-property-race.html
>@@ -0,0 +1,114 @@
>+<!DOCTYPE html>
>+<html>
>+<head>
>+<script src="../../http/tests/inspector/resources/inspector-test.js"></script>
>+<script>
>+let requestAnimationFrameID;
>+
>+function expand(iterationCount) {
>+    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");
>+
>+        if (i < iterationCount)
>+            requestAnimationFrameID = requestAnimationFrame(loop);
>+        else
>+            stopExpanding();
>+    };
>+
>+    loop();
>+}
>+
>+function stopExpanding() {
>+    cancelAnimationFrame(requestAnimationFrameID);
>+    TestPage.dispatchEventToFrontend("TestPage-didStopExpanding");
>+}
>+
>+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();
>+            };
>+
>+            let valueModifiedByWebInspector = false;
>+            InspectorTest.evaluateInPage("expand(50)");
>+
>+            function styleDecorationUpdated(event) {
>+                if (valueModifiedByWebInspector) {
>+                    InspectorTest.expectEqual(getProperty("height").rawValue, "10px", `Value updated to "10px".`);
>+                    InspectorTest.expectEqual(getInlineStyleDeclaration().text, "height: 10px;", "CSSStyleDeclaration text should update.");
>+
>+                    InspectorTest.evaluateInPage("stopExpanding()");
>+                    WI.CSSStyleDeclaration.removeEventListener(WI.CSSStyleDeclaration.Event.PropertiesChanged, styleDecorationUpdated);
>+                    resolve();
>+                } else {
>+                    InspectorTest.log("First style change.");
>+                    getProperty("height").rawValue = "10px";
>+                    valueModifiedByWebInspector = true;
>+                }
>+            }
>+
>+            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>
>diff --git a/LayoutTests/inspector/css/modify-css-property.html b/LayoutTests/inspector/css/modify-css-property.html
>index c1817029f76..87b0a8d4a5d 100644
>--- a/LayoutTests/inspector/css/modify-css-property.html
>+++ b/LayoutTests/inspector/css/modify-css-property.html
>@@ -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();
>         }
>@@ -93,6 +93,45 @@ 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.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) {
>@@ -117,12 +156,9 @@ 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);
>+            styleDeclaration.awaitEvent(WI.CSSStyleDeclaration.Event.PropertiesChanged).then((event) => {
>+                InspectorTest.fail("PropertiesChanged event shouldn't fire while property is being edited by a user.");
>+            });
> 
>             styleDeclaration.locked = true;
>             getProperty("width").rawValue = "64px";
>@@ -133,9 +169,45 @@ 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()`);
>+
>+            resolve();
>+        }
>+    });
>+
>+    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();
>         }
>     });
> 
>diff --git a/Source/WebInspectorUI/ChangeLog b/Source/WebInspectorUI/ChangeLog
>index 08a49fed285..bc9c71acdf5 100644
>--- a/Source/WebInspectorUI/ChangeLog
>+++ b/Source/WebInspectorUI/ChangeLog
>@@ -1,3 +1,38 @@
>+2019-02-02  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 NOBODY (OOPS!).
>+
>+        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  Joseph Pecoraro  <pecoraro@apple.com>
> 
>         Web Inspector: Make WI.ColumnChart a WI.View subclass
>diff --git a/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js b/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js
>index 1be38e5d5fe..66967838ff9 100644
>--- a/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js
>+++ b/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js
>@@ -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 || "";
>diff --git a/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js b/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js
>index 56c10677f8a..6d06c8a7d23 100644
>--- a/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js
>+++ b/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js
>@@ -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;
>     }
>diff --git a/Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js b/Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js
>index 16cf2ff3f28..c7845078fbc 100644
>--- a/Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js
>+++ b/Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js
>@@ -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
>diff --git a/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js b/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js
>index b727b04b590..b5eb8d141f6 100644
>--- a/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js
>+++ b/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js
>@@ -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)

Flags:

hi: review+
hi: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 192739: 358205 | 358212 | 358216 | 358217 | 358219 | 358447 | 358450 | 358451 | 358453 | 358466 | 358857 | 360990 | 360994 | 361001 | 361007 | 361008 | 361104