WebKit Bugzilla
Attachment 358466 Details for
Bug 192739
: Web Inspector: Styles: fix race conditions when editing
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch
patch.txt (text/plain), 15.90 KB, created by
Nikita Vasilyev
on 2019-01-06 12:34:49 PST
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
Nikita Vasilyev
Created:
2019-01-06 12:34:49 PST
Size:
15.90 KB
patch
obsolete
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog >index f884bd836f6..1bfec960b9f 100644 >--- a/LayoutTests/ChangeLog >+++ b/LayoutTests/ChangeLog >@@ -1,3 +1,14 @@ >+2019-01-05 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.html: >+ > 2019-01-04 Devin Rousso <drousso@apple.com> > > Web Inspector: Audit: disable breakpoints when running Audit >diff --git a/LayoutTests/inspector/css/modify-css-property-expected.txt b/LayoutTests/inspector/css/modify-css-property-expected.txt >index c5374e54c9e..0f17979a483 100644 >--- a/LayoutTests/inspector/css/modify-css-property-expected.txt >+++ b/LayoutTests/inspector/css/modify-css-property-expected.txt >@@ -4,18 +4,20 @@ 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.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.html b/LayoutTests/inspector/css/modify-css-property.html >index 52266588482..a70fe70e5fa 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,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.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) { >@@ -119,10 +159,8 @@ function test() { > > 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); >+ InspectorTest.fail("PropertiesChanged event shouldn't fire while property is being edited by a user."); >+ }); > > styleDeclaration.locked = true; > getProperty("width").rawValue = "64px"; >@@ -133,9 +171,11 @@ 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(); > } > }); > >diff --git a/Source/WebInspectorUI/ChangeLog b/Source/WebInspectorUI/ChangeLog >index f05867e772b..7cb375de296 100644 >--- a/Source/WebInspectorUI/ChangeLog >+++ b/Source/WebInspectorUI/ChangeLog >@@ -1,3 +1,44 @@ >+2019-01-05 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. >+ >+ When two consequent style changes happen, only update CSSStyleDeclaration 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 >+ >+ 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 property): >+ Drive-by: remove unused code. >+ > 2019-01-04 Joseph Pecoraro <pecoraro@apple.com> > > Web Inspector: subclasses of WI.ClusterContentView don't save/restore content views after the initial view >diff --git a/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js b/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js >index 4ba36efc8c3..62ca03b8ca0 100644 >--- a/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js >+++ b/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js >@@ -358,6 +358,10 @@ WI.CSSProperty = class CSSProperty extends WI.Object > return; > } > >+ console.assert(this._ownerStyle); >+ if (!this._ownerStyle) >+ return; >+ > this._prependSemicolonIfNeeded(); > > let styleText = this._ownerStyle.text || ""; >@@ -383,7 +387,8 @@ WI.CSSProperty = class CSSProperty extends WI.Object > let columnDelta = newText.lastLine.length - oldText.lastLine.length; > this._styleSheetTextRange = this._styleSheetTextRange.cloneAndModify(0, 0, lineDelta, columnDelta); > >- this._ownerStyle.text = newStyleText; >+ // This causes a backend call which is asynchronous yet changes in the model on the front-end are synchronous. >+ this._ownerStyle.setText(newStyleText); > > let propertyWasRemoved = !newText; > this._ownerStyle.shiftPropertiesAfter(this, lineDelta, columnDelta, propertyWasRemoved); >diff --git a/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js b/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js >index 62f11405844..e6bc1c670e9 100644 >--- a/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js >+++ b/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js >@@ -40,6 +40,7 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object > this._node = node || null; > this._inherited = inherited || false; > >+ this._updatesInProgressCount = 0; > this._locked = false; > this._pendingProperties = []; > this._propertyNameMap = {}; >@@ -102,9 +103,23 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object > update(text, properties, styleSheetTextRange, options = {}) > { > let dontFireEvents = options.dontFireEvents || false; >- let suppressLock = options.suppressLock || false; >+ let forceUpdate = options.forceUpdate || false; >+ >+ // 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 && !forceUpdate) { >+ if (WI.settings.enableStyleEditingDebugMode.value && text !== this._text) >+ console.warn("Style modified while editing:", text); > >- if (this._locked && !suppressLock && text !== this._text) >+ return; >+ } >+ >+ // Allow updates from the backend when text matches because `properties` may contain warnings that need to be shown. >+ if (this._locked && !forceUpdate && text !== this._text) > return; > > text = text || ""; >@@ -168,7 +183,7 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object > > // Don't fire the event if there is text and it hasn't changed. > if (oldText && this._text && oldText === this._text) { >- if (!this._locked || suppressLock) { >+ if (!this._locked && !forceUpdate) { > // We shouldn't have any added or removed properties in this case. > console.assert(!addedProperties.length && !removedProperties.length); > } >@@ -201,7 +216,7 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object > return this._text; > } > >- set text(text) >+ setText(text) > { > if (this._text === text) > return; >@@ -213,11 +228,25 @@ 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 styleTextDidChange = () => { >+ if (!timeoutId) >+ return; >+ >+ clearTimeout(timeoutId); >+ timeoutId = null; >+ this._updatesInProgressCount--; >+ console.assert(this._updatesInProgressCount >= 0, "_updatesInProgressCount cannot be a negative number."); >+ }; >+ >+ let timeoutId = setTimeout(() => { >+ console.error("Timed out when setting style text:", text); >+ styleTextDidChange(); >+ }, 1000); > >- this._nodeStyles.changeStyleText(this, text); >+ this._nodeStyles.changeStyleText(this, text, styleTextDidChange); > } > > get properties() >@@ -332,7 +361,7 @@ WI.CSSStyleDeclaration = class CSSStyleDeclaration extends WI.Object > for (let index = propertyIndex + 1; index < this._allProperties.length; index++) > this._allProperties[index].index = index; > >- this.update(this._text, this._allProperties, this._styleSheetTextRange, {dontFireEvents: true, suppressLock: true}); >+ this.update(this._text, this._allProperties, 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 f44b05628a1..4ecc8a7bfc0 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().finally(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 d454fa6020f..ebfdaa7b586 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)
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
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