Diff
Modified: trunk/LayoutTests/ChangeLog (227369 => 227370)
--- trunk/LayoutTests/ChangeLog 2018-01-23 00:45:17 UTC (rev 227369)
+++ trunk/LayoutTests/ChangeLog 2018-01-23 00:52:32 UTC (rev 227370)
@@ -1,3 +1,14 @@
+2018-01-22 Nikita Vasilyev <nvasil...@apple.com>
+
+ Web Inspector: Styles Redesign: data corruption when updating values quickly
+ https://bugs.webkit.org/show_bug.cgi?id=179461
+ <rdar://problem/35431882>
+
+ Reviewed by Joseph Pecoraro.
+
+ * inspector/css/modify-css-property-expected.txt: Added.
+ * inspector/css/modify-css-property.html: Added.
+
2018-01-22 Matt Lewis <jlew...@apple.com>
Marked fast/forms/searchfield-heights.html as flaky on High Sierra.
Added: trunk/LayoutTests/inspector/css/modify-css-property-expected.txt (0 => 227370)
--- trunk/LayoutTests/inspector/css/modify-css-property-expected.txt (rev 0)
+++ trunk/LayoutTests/inspector/css/modify-css-property-expected.txt 2018-01-23 00:52:32 UTC (rev 227370)
@@ -0,0 +1,18 @@
+Testing that CSSStyleDeclaration update immediately after modifying its properties when it is not locked.
+
+
+== 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.
+
+-- 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".
+PASS: "width" property value should update to "200px".
+PASS: Inline style declaration text should update when not locked.
+
Added: trunk/LayoutTests/inspector/css/modify-css-property.html (0 => 227370)
--- trunk/LayoutTests/inspector/css/modify-css-property.html (rev 0)
+++ trunk/LayoutTests/inspector/css/modify-css-property.html 2018-01-23 00:52:32 UTC (rev 227370)
@@ -0,0 +1,166 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src=""
+<script>
+function makeNarrow() {
+ document.getElementById("x").style.width = "50px";
+}
+
+function makeWide() {
+ document.getElementById("x").style.width = "200px";
+}
+
+function test() {
+ let nodeStyles = null;
+ let suite = InspectorTest.createAsyncSuite("ModifyCSSProperty");
+
+ suite.addTestCase({
+ name: "Update value when CSSStyleDeclaration is not locked",
+ test(resolve, reject) {
+ let getMatchedStyleDeclaration = () => {
+ for (let rule of nodeStyles.matchedRules) {
+ if (rule.selectorText === ".rule-b")
+ return rule.style;
+ }
+ };
+
+ let getProperty = (propertyName) => {
+ let styleDeclaration = getMatchedStyleDeclaration();
+ for (let property of styleDeclaration.properties) {
+ if (property.name === propertyName)
+ return property;
+ }
+ };
+
+ let styleDeclaration = getMatchedStyleDeclaration();
+ styleDeclaration.locked = false;
+ getProperty("font-size").rawValue = "11px";
+ getProperty("font-size").rawValue = "10px";
+
+ 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.`);
+
+ resolve();
+ }
+ });
+
+ suite.addTestCase({
+ name: "Update value when CSSStyleDeclaration is locked",
+ test(resolve, reject) {
+ let getMatchedStyleDeclaration = () => {
+ for (let rule of nodeStyles.matchedRules) {
+ if (rule.selectorText === ".rule-a")
+ return rule.style;
+ }
+ };
+
+ let getProperty = (propertyName) => {
+ let styleDeclaration = getMatchedStyleDeclaration();
+ for (let property of styleDeclaration.properties) {
+ if (property.name === propertyName)
+ return property;
+ }
+ };
+
+ let styleDeclaration = getMatchedStyleDeclaration();
+ styleDeclaration.locked = true;
+ getProperty("font-size").rawValue = "15px";
+ getProperty("font-size").rawValue = "16px";
+
+ InspectorTest.expectEqual(getProperty("font-size").rawValue, "16px", `"font-size" property value should update immediately.`);
+
+ InspectorTest.expectEqual(styleDeclaration.text, `
+ font-size: 16px;
+ color: #333;
+
+ margin-left: 0;
+ margin-top: 1em;
+ `, `Style declaration text should update immediately.`);
+
+ styleDeclaration.locked = false;
+
+ resolve();
+ }
+ });
+
+ suite.addTestCase({
+ name: "Update inline style value when CSSStyleDeclaration locked and not locked",
+ test(resolve, reject) {
+ let getInlineStyleDeclaration = () => {
+ for (let styleDeclaration of nodeStyles.orderedStyles) {
+ if (styleDeclaration.type === styleDeclaration.constructor.Type.Inline)
+ return styleDeclaration;
+ }
+ };
+
+ let getProperty = (propertyName) => {
+ let styleDeclaration = getInlineStyleDeclaration();
+ for (let property of styleDeclaration.properties) {
+ if (property.name === propertyName)
+ return property;
+ }
+ };
+
+ 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.locked = true;
+ getProperty("width").rawValue = "64px";
+ InspectorTest.expectEqual(styleDeclaration.text, `\nwidth: 64px;`, `Style declaration text should update immediately.`);
+
+ // WI.CSSStyleDeclaration.Event.PropertiesChanged event should not fire when the style declaration is locked.
+ InspectorTest.evaluateInPage(`makeNarrow()`);
+
+ styleDeclaration.locked = false;
+ getProperty("width").rawValue = "128px";
+ InspectorTest.expectEqual(styleDeclaration.text, `\nwidth: 64px;`, `Style declaration text should stay "width: 64px".`);
+
+ InspectorTest.evaluateInPage(`makeWide()`);
+ }
+ });
+
+ WI.domTreeManager.requestDocument((documentNode) => {
+ WI.domTreeManager.querySelector(documentNode.id, "#x", (contentNodeId) => {
+ if (contentNodeId) {
+ let domNode = WI.domTreeManager.nodeForId(contentNodeId);
+ nodeStyles = WI.cssStyleManager.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 update immediately after modifying its properties when it is not locked.</p>
+
+ <style>
+ .rule-a {
+ font-size: 14px;
+ color: #333;
+
+ margin-left: 0;
+ margin-top: 1em;
+ }
+ .rule-b {font-size: 12px; color: antiquewhite}
+ </style>
+ <div id="x" class="test-node rule-a rule-b" style="width: 100px"></div>
+</body>
+</html>
Modified: trunk/Source/WebInspectorUI/ChangeLog (227369 => 227370)
--- trunk/Source/WebInspectorUI/ChangeLog 2018-01-23 00:45:17 UTC (rev 227369)
+++ trunk/Source/WebInspectorUI/ChangeLog 2018-01-23 00:52:32 UTC (rev 227370)
@@ -1,3 +1,58 @@
+2018-01-22 Nikita Vasilyev <nvasil...@apple.com>
+
+ Web Inspector: Styles Redesign: data corruption when updating values quickly
+ https://bugs.webkit.org/show_bug.cgi?id=179461
+ <rdar://problem/35431882>
+
+ Reviewed by Joseph Pecoraro.
+
+ Data corruption used to happen because CSSStyleDeclaration.prototype.text didn't
+ update synchronously. Making two or more quick changes resulted in corrupted data.
+
+ Imagine we modify a CSS value 3 times:
+
+ Front-end: (1)-(2)---(3)
+ Back-end: (1)-----(2)-(3)
+
+ The first response from the backend could happen after the 2nd edit. In this patch,
+ CSSStyleDeclaration is locked when its view is being edited.
+
+ To correctly display invalid and overridden properties, the backend is allowed to update
+ CSSStyleDeclaration and CSSProperty when they're locked if the text from the backend
+ matches the model's text. This should happen when the backend is caught up with the
+ front-end changes.
+
+ * UserInterface/Models/CSSProperty.js:
+ (WI.CSSProperty.prototype.update):
+ * UserInterface/Models/CSSStyleDeclaration.js:
+ (WI.CSSStyleDeclaration):
+ (WI.CSSStyleDeclaration.prototype.get locked):
+ (WI.CSSStyleDeclaration.prototype.set locked):
+ (WI.CSSStyleDeclaration.prototype.set text):
+
+ * UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:
+ (WI.SpreadsheetCSSStyleDeclarationEditor):
+ (WI.SpreadsheetCSSStyleDeclarationEditor.prototype.initialLayout):
+ (WI.SpreadsheetCSSStyleDeclarationEditor.prototype.detached):
+ (WI.SpreadsheetCSSStyleDeclarationEditor.prototype.get editing):
+ (WI.SpreadsheetCSSStyleDeclarationEditor.prototype.set focused):
+ (WI.SpreadsheetCSSStyleDeclarationEditor.prototype.set inlineSwatchActive):
+ (WI.SpreadsheetCSSStyleDeclarationEditor.prototype.stylePropertyInlineSwatchActivated):
+ (WI.SpreadsheetCSSStyleDeclarationEditor.prototype.stylePropertyInlineSwatchDeactivated):
+ (WI.SpreadsheetCSSStyleDeclarationEditor.prototype._propertiesChanged):
+ (WI.SpreadsheetCSSStyleDeclarationEditor.prototype._updateStyleLock):
+ Lock CSSStyleDeclaration when a CSS property name or value is focused or
+ an inline widget is active.
+
+ * UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:
+ (WI.SpreadsheetCSSStyleDeclarationSection):
+ (WI.SpreadsheetCSSStyleDeclarationSection.prototype._handleMouseDown):
+ (WI.SpreadsheetCSSStyleDeclarationSection.prototype._handleClick):
+ * UserInterface/Views/SpreadsheetStyleProperty.js:
+ (WI.SpreadsheetStyleProperty):
+ (WI.SpreadsheetStyleProperty.prototype._createInlineSwatch):
+ When selector is focused, clicking on the white-space should not add a new blank property.
+
2018-01-19 Ross Kirsling <ross.kirsl...@sony.com>
Web Inspector: Layers tab should do away with popovers (if possible)
Modified: trunk/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js (227369 => 227370)
--- trunk/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js 2018-01-23 00:45:17 UTC (rev 227369)
+++ trunk/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js 2018-01-23 00:52:32 UTC (rev 227370)
@@ -70,6 +70,11 @@
update(text, name, value, priority, enabled, overridden, implicit, anonymous, valid, styleSheetTextRange, dontFireEvents)
{
+ // Locked CSSProperty can still be updated from the back-end when the text matches.
+ // We need to do this to keep attributes such as valid and overridden up to date.
+ if (this._ownerStyle && this._ownerStyle.locked && text !== this._text)
+ return;
+
text = text || "";
name = name || "";
value = value || "";
Modified: trunk/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js (227369 => 227370)
--- trunk/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js 2018-01-23 00:45:17 UTC (rev 227369)
+++ trunk/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js 2018-01-23 00:52:32 UTC (rev 227370)
@@ -40,6 +40,7 @@
this._node = node || null;
this._inherited = inherited || false;
+ this._locked = false;
this._pendingProperties = [];
this._propertyNameMap = {};
@@ -49,7 +50,7 @@
this._allProperties = [];
this._allVisibleProperties = null;
- this.update(text, properties, styleSheetTextRange, true);
+ this.update(text, properties, styleSheetTextRange, {dontFireEvents: true});
}
// Public
@@ -98,8 +99,17 @@
return this._ownerRule && this._ownerRule.editable;
}
- update(text, properties, styleSheetTextRange, dontFireEvents)
+ get locked() { return this._locked; }
+ set locked(value) { this._locked = value; }
+
+ update(text, properties, styleSheetTextRange, options = {})
{
+ let dontFireEvents = options.dontFireEvents || false;
+ let suppressLock = options.suppressLock || false;
+
+ if (this._locked && !suppressLock && text !== this._text)
+ return;
+
text = text || "";
properties = properties || [];
@@ -161,8 +171,11 @@
// Don't fire the event if there is text and it hasn't changed.
if (oldText && this._text && oldText === this._text) {
- // We shouldn't have any added or removed properties in this case.
- console.assert(!addedProperties.length && !removedProperties.length);
+ if (!this._locked || suppressLock) {
+ // We shouldn't have any added or removed properties in this case.
+ console.assert(!addedProperties.length && !removedProperties.length);
+ }
+
if (!addedProperties.length && !removedProperties.length)
return;
}
@@ -209,6 +222,10 @@
this.dispatchEventToListeners(WI.CSSStyleDeclaration.Event.InitialTextModified);
}
+ // Update text immediately when it was modified via the styles sidebar.
+ if (this._locked)
+ this._text = text;
+
this._nodeStyles.changeStyleText(this, text);
}
@@ -370,8 +387,7 @@
for (let index = propertyIndex + 1; index < this._allProperties.length; index++)
this._allProperties[index].index = index;
- const suppressEvents = true;
- this.update(this._text, this._allProperties, this._styleSheetTextRange, suppressEvents);
+ this.update(this._text, this._allProperties, this._styleSheetTextRange, {dontFireEvents: true, suppressLock: true});
return property;
}
Modified: trunk/Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js (227369 => 227370)
--- trunk/Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js 2018-01-23 00:45:17 UTC (rev 227369)
+++ trunk/Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js 2018-01-23 00:52:32 UTC (rev 227370)
@@ -34,6 +34,10 @@
this._delegate = delegate;
this.style = style;
this._propertyViews = [];
+
+ this._inlineSwatchActive = false;
+ this._focused = false;
+
this._propertyPendingStartEditing = null;
this._filterText = null;
}
@@ -40,6 +44,21 @@
// Public
+ initialLayout()
+ {
+ if (!this.style.editable)
+ return;
+
+ this.element.addEventListener("focus", () => { this.focused = true; }, true);
+ this.element.addEventListener("blur", (event) => {
+ let focusedElement = event.relatedTarget;
+ if (focusedElement && focusedElement.isDescendant(this.element))
+ return;
+
+ this.focused = false;
+ }, true);
+ }
+
layout()
{
super.layout();
@@ -74,6 +93,9 @@
detached()
{
+ this._inlineSwatchActive = false;
+ this.focused = false;
+
for (let propertyView of this._propertyViews)
propertyView.detached();
}
@@ -99,6 +121,23 @@
this.needsLayout();
}
+ get editing()
+ {
+ return this._focused || this._inlineSwatchActive;
+ }
+
+ set focused(value)
+ {
+ this._focused = value;
+ this._updateStyleLock();
+ }
+
+ set inlineSwatchActive(value)
+ {
+ this._inlineSwatchActive = value;
+ this._updateStyleLock();
+ }
+
startEditingFirstProperty()
{
let firstEditableProperty = this._editablePropertyAfter(-1);
@@ -162,16 +201,6 @@
return false;
}
- isFocused()
- {
- let focusedElement = document.activeElement;
-
- if (!focusedElement || focusedElement.tagName === "BODY")
- return false;
-
- return focusedElement.isSelfOrDescendant(this.element);
- }
-
addBlankProperty(index)
{
if (index === -1) {
@@ -222,6 +251,8 @@
}
}
+ // SpreadsheetStyleProperty delegate
+
spreadsheetStylePropertyRemoved(propertyView)
{
this._propertyViews.remove(propertyView);
@@ -228,6 +259,16 @@
this.updateLayout();
}
+ stylePropertyInlineSwatchActivated()
+ {
+ this.inlineSwatchActive = true;
+ }
+
+ stylePropertyInlineSwatchDeactivated()
+ {
+ this.inlineSwatchActive = false;
+ }
+
applyFilter(filterText)
{
this._filterText = filterText;
@@ -278,12 +319,17 @@
_propertiesChanged(event)
{
- if (this.isFocused()) {
+ if (this.editing) {
for (let propertyView of this._propertyViews)
propertyView.updateStatus();
} else
this.needsLayout();
}
+
+ _updateStyleLock()
+ {
+ this.style.locked = this._focused || this._inlineSwatchActive;
+ }
};
WI.SpreadsheetCSSStyleDeclarationEditor.Event = {
Modified: trunk/Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js (227369 => 227370)
--- trunk/Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js 2018-01-23 00:45:17 UTC (rev 227369)
+++ trunk/Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js 2018-01-23 00:52:32 UTC (rev 227370)
@@ -40,7 +40,7 @@
this._mediaElements = [];
this._filterText = null;
this._shouldFocusSelectorElement = false;
- this._wasFocused = false;
+ this._wasEditing = false;
}
// Public
@@ -47,8 +47,6 @@
get style() { return this._style; }
- get propertiesEditor() { return this._propertiesEditor; }
-
get editable()
{
return this._style.editable;
@@ -152,6 +150,8 @@
this.startEditingRuleSelector();
}
+ // SpreadsheetSelectorField delegate
+
spreadsheetSelectorFieldDidChange(direction)
{
let selectorText = this._selectorElement.textContent.trim();
@@ -181,6 +181,8 @@
this._discardSelectorChange();
}
+ // SpreadsheetCSSStyleDeclarationEditor delegate
+
cssStyleDeclarationEditorStartEditingAdjacentRule(toPreviousRule)
{
if (!this._delegate)
@@ -418,12 +420,12 @@
_handleMouseDown(event)
{
- this._wasFocused = this._propertiesEditor.isFocused();
+ this._wasEditing = this._propertiesEditor.editing || document.activeElement === this._selectorElement;
}
_handleClick(event)
{
- if (this._wasFocused)
+ if (this._wasEditing)
return;
if (window.getSelection().type === "Range")
Modified: trunk/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js (227369 => 227370)
--- trunk/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js 2018-01-23 00:45:17 UTC (rev 227369)
+++ trunk/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js 2018-01-23 00:52:32 UTC (rev 227370)
@@ -355,6 +355,18 @@
this._handleValueChange();
}, this);
+ if (typeof this._delegate.stylePropertyInlineSwatchActivated === "function") {
+ swatch.addEventListener(WI.InlineSwatch.Event.Activated, () => {
+ this._delegate.stylePropertyInlineSwatchActivated();
+ });
+ }
+
+ if (typeof this._delegate.stylePropertyInlineSwatchDeactivated === "function") {
+ swatch.addEventListener(WI.InlineSwatch.Event.Deactivated, () => {
+ this._delegate.stylePropertyInlineSwatchDeactivated();
+ });
+ }
+
tokenElement.append(swatch.element, innerElement);
// Prevent the value from editing when clicking on the swatch.
