WebKit Bugzilla
Attachment 357397 Details for
Bug 192396
: Web Inspector: Styles: toggling selected properties may cause data corruption
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch
patch.txt (text/plain), 22.06 KB, created by
Nikita Vasilyev
on 2018-12-15 01:21:20 PST
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
Nikita Vasilyev
Created:
2018-12-15 01:21:20 PST
Size:
22.06 KB
patch
obsolete
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog >index 565ab81a36f..4b486277f45 100644 >--- a/LayoutTests/ChangeLog >+++ b/LayoutTests/ChangeLog >@@ -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-06 Wenson Hsieh <wenson_hsieh@apple.com> > > [iOS] WKWebView should match UITextView behavior when editing text with an RTL keyboard >diff --git a/LayoutTests/inspector/css/add-css-property-expected.txt b/LayoutTests/inspector/css/add-css-property-expected.txt >new file mode 100644 >index 00000000000..28c39df157c >--- /dev/null >+++ b/LayoutTests/inspector/css/add-css-property-expected.txt >@@ -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 >index 00000000000..7a91fbd0fa2 >--- /dev/null >+++ b/LayoutTests/inspector/css/add-css-property.html >@@ -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> >diff --git a/LayoutTests/inspector/css/modify-css-property-expected.txt b/LayoutTests/inspector/css/modify-css-property-expected.txt >index c8aa3d9cbfc..c5374e54c9e 100644 >--- a/LayoutTests/inspector/css/modify-css-property-expected.txt >+++ b/LayoutTests/inspector/css/modify-css-property-expected.txt >@@ -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. >+ >diff --git a/LayoutTests/inspector/css/modify-css-property.html b/LayoutTests/inspector/css/modify-css-property.html >index c35b9f03157..52266588482 100644 >--- a/LayoutTests/inspector/css/modify-css-property.html >+++ b/LayoutTests/inspector/css/modify-css-property.html >@@ -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> >diff --git a/Source/WebInspectorUI/ChangeLog b/Source/WebInspectorUI/ChangeLog >index 698900f5b78..9afa53e19ae 100644 >--- a/Source/WebInspectorUI/ChangeLog >+++ b/Source/WebInspectorUI/ChangeLog >@@ -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-06 Matt Baker <mattbaker@apple.com> > > Web Inspector: REGRESSION(r238602): Elements: collapsing a DOM node with the left arrow doesn't work >diff --git a/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js b/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js >index d6e4ad71535..4ba36efc8c3 100644 >--- a/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js >+++ b/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js >@@ -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; >+ } > } > }; > >diff --git a/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js b/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js >index ef68b658e9f..9ae9fa9112f 100644 >--- a/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js >+++ b/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js >@@ -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(); > } >
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 192396
:
356761
|
356785
|
356790
|
356850
|
356890
|
356891
| 357397