WebKit Bugzilla
Attachment 356891 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), 20.62 KB, created by
Nikita Vasilyev
on 2018-12-08 16:00:26 PST
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
Nikita Vasilyev
Created:
2018-12-08 16:00:26 PST
Size:
20.62 KB
patch
obsolete
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog >index 565ab81a36f..05d01cf7734 100644 >--- a/LayoutTests/ChangeLog >+++ b/LayoutTests/ChangeLog >@@ -1,3 +1,19 @@ >+2018-12-08 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 NOBODY (OOPS!). >+ >+ * 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..e3947d4b86c >--- /dev/null >+++ b/LayoutTests/inspector/css/add-css-property-expected.txt >@@ -0,0 +1,17 @@ >+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.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..0a1ccc3df80 >--- /dev/null >+++ b/LayoutTests/inspector/css/add-css-property.html >@@ -0,0 +1,162 @@ >+<!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"); >+ >+ suite.addTestCase({ >+ name: "AddCSSProperty.AppendAfterMissingSemicolon", >+ test(resolve, reject) { >+ let getMatchedStyleDeclaration = () => { >+ for (let rule of nodeStyles.matchedRules) { >+ if (rule.selectorText === ".rule-a") >+ return rule.style; >+ } >+ >+ InspectorTest.fail("No declaration found."); >+ reject(); >+ }; >+ >+ let styleDeclaration = getMatchedStyleDeclaration(); >+ 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."); >+ >+ resolve(); >+ } >+ }); >+ >+ suite.addTestCase({ >+ name: "AddCSSProperty.InsertBeforeAndAfter", >+ test(resolve, reject) { >+ let getMatchedStyleDeclaration = () => { >+ for (let rule of nodeStyles.matchedRules) { >+ if (rule.selectorText === ".rule-b") >+ return rule.style; >+ } >+ >+ InspectorTest.fail("No declaration found."); >+ reject(); >+ }; >+ >+ let styleDeclaration = getMatchedStyleDeclaration(); >+ 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 = getMatchedStyleDeclaration(); >+ 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."); >+ >+ resolve(); >+ } >+ }); >+ >+ suite.addTestCase({ >+ name: "AddCSSProperty.AppendAfterCommentedOutProperty", >+ test(resolve, reject) { >+ let getMatchedStyleDeclaration = () => { >+ for (let rule of nodeStyles.matchedRules) { >+ if (rule.selectorText === ".rule-c") >+ return rule.style; >+ } >+ >+ InspectorTest.fail("No declaration found."); >+ reject(); >+ }; >+ >+ let styleDeclaration = getMatchedStyleDeclaration(); >+ 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."); >+ >+ resolve(); >+ } >+ }); >+ >+ suite.addTestCase({ >+ name: "AddCSSProperty.AppendAfterCommentedOutPropertyWithoutSemicolon", >+ test(resolve, reject) { >+ let getMatchedStyleDeclaration = () => { >+ for (let rule of nodeStyles.matchedRules) { >+ if (rule.selectorText === ".rule-d") >+ return rule.style; >+ } >+ >+ InspectorTest.fail("No declaration found."); >+ reject(); >+ }; >+ >+ let styleDeclaration = getMatchedStyleDeclaration(); >+ 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."); >+ >+ 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 { >+ font-size: 14px; >+ /* color: red; */ >+} >+.rule-d { >+ font-size: 14px; >+ /* color: red */ >+} >+</style> >+<div id="x" class="rule-a rule-b rule-c rule-d" 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..07b4ae0b3fa 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.CommentOutAndUncommentProperty >+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.CommentOutAndUncommentProperty2 >+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..92723b141df 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."); >+ reject(); > }; > > let getProperty = (propertyName) => { >@@ -31,6 +33,8 @@ function test() { > if (property.name === propertyName) > return property; > } >+ InspectorTest.fail("No property found."); >+ reject(); > }; > > let styleDeclaration = getMatchedStyleDeclaration(); >@@ -54,6 +58,8 @@ function test() { > if (rule.selectorText === ".rule-a") > return rule.style; > } >+ InspectorTest.fail("No declaration found."); >+ reject(); > }; > > let getProperty = (propertyName) => { >@@ -62,6 +68,8 @@ function test() { > if (property.name === propertyName) > return property; > } >+ InspectorTest.fail("No property found."); >+ reject(); > }; > > let styleDeclaration = getMatchedStyleDeclaration(); >@@ -93,6 +101,8 @@ function test() { > if (styleDeclaration.type === styleDeclaration.constructor.Type.Inline) > return styleDeclaration; > } >+ InspectorTest.fail("No declaration found."); >+ reject(); > }; > > let getProperty = (propertyName) => { >@@ -101,6 +111,8 @@ function test() { > if (property.name === propertyName) > return property; > } >+ InspectorTest.fail("No property found."); >+ reject(); > }; > > let styleDeclaration = getInlineStyleDeclaration(); >@@ -127,6 +139,107 @@ function test() { > } > }); > >+ suite.addTestCase({ >+ name: "ModifyCSSProperty.CommentOutAndUncommentProperty", >+ test(resolve, reject) { >+ let getMatchedStyleDeclaration = () => { >+ for (let rule of nodeStyles.matchedRules) { >+ if (rule.selectorText === ".rule-c") >+ return rule.style; >+ } >+ InspectorTest.fail("No declaration found."); >+ reject(); >+ }; >+ >+ let getProperty = (propertyName) => { >+ let styleDeclaration = getMatchedStyleDeclaration(); >+ for (let property of styleDeclaration.allProperties) { >+ if (property.name === propertyName) >+ return property; >+ } >+ InspectorTest.fail("No property found."); >+ reject(); >+ }; >+ >+ 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 = ` >+ /* padding-left: 2em; */ >+ padding-right: 0px; >+ `; >+ 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 = ` >+ /* padding-left: 2em; */ >+ /* padding-right: 0px; */ >+ `; >+ 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.CommentOutAndUncommentProperty2", >+ test(resolve, reject) { >+ let getMatchedStyleDeclaration = () => { >+ for (let rule of nodeStyles.matchedRules) { >+ if (rule.selectorText === ".rule-d") >+ return rule.style; >+ } >+ InspectorTest.fail("No declaration found."); >+ reject(); >+ }; >+ >+ let getProperty = (propertyName) => { >+ let styleDeclaration = getMatchedStyleDeclaration(); >+ for (let property of styleDeclaration.allProperties) { >+ if (property.name === propertyName) >+ return property; >+ } >+ InspectorTest.fail("No property found."); >+ reject(); >+ }; >+ >+ 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 +273,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..100f29b0f71 100644 >--- a/Source/WebInspectorUI/ChangeLog >+++ b/Source/WebInspectorUI/ChangeLog >@@ -1,3 +1,35 @@ >+2018-12-08 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 NOBODY (OOPS!). >+ >+ 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. >+ > 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; >+ } > } > }; >
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
Flags:
hi
:
review+
hi
:
commit-queue-
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 192396
:
356761
|
356785
|
356790
|
356850
|
356890
|
356891
|
357397