WebKit Bugzilla
Attachment 356890 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]
WIP: single agent call
patch.txt (text/plain), 15.42 KB, created by
Nikita Vasilyev
on 2018-12-08 15:38:44 PST
(
hide
)
Description:
WIP: single agent call
Filename:
MIME Type:
Creator:
Nikita Vasilyev
Created:
2018-12-08 15:38:44 PST
Size:
15.42 KB
patch
obsolete
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog >index 565ab81a36f..72e2291d289 100644 >--- a/LayoutTests/ChangeLog >+++ b/LayoutTests/ChangeLog >@@ -1,3 +1,16 @@ >+2018-12-06 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!). >+ >+ Test commenting out and uncommenting CSS properties. >+ >+ * inspector/css/modify-css-property-expected.txt: >+ * inspector/css/modify-css-property.html: >+ > 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/modify-css-property-expected.txt b/LayoutTests/inspector/css/modify-css-property-expected.txt >index c8aa3d9cbfc..8dc2debd86b 100644 >--- a/LayoutTests/inspector/css/modify-css-property-expected.txt >+++ b/LayoutTests/inspector/css/modify-css-property-expected.txt >@@ -16,3 +16,25 @@ 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.CommentOutAndUncommentPropertyLocked >+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.CommentOutAndUncommentPropertyLocked2 >+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. >+ >+-- Running test case: ModifyCSSProperty.CommentOutAndUncommentPropertyUnlocked >+PASS: Commented out property should be disabled. >+PASS: Style declaration text should stay unchanged. >+PASS: Uncommented property should be enabled. >+PASS: Style declaration text should stay unchanged. >+PASS: Commented out property should be disabled. >+ >diff --git a/LayoutTests/inspector/css/modify-css-property.html b/LayoutTests/inspector/css/modify-css-property.html >index c35b9f03157..ca01f761862 100644 >--- a/LayoutTests/inspector/css/modify-css-property.html >+++ b/LayoutTests/inspector/css/modify-css-property.html >@@ -127,6 +127,157 @@ function test() { > } > }); > >+ suite.addTestCase({ >+ name: "ModifyCSSProperty.CommentOutAndUncommentPropertyLocked", >+ test(resolve, reject) { >+ let getMatchedStyleDeclaration = () => { >+ for (let rule of nodeStyles.matchedRules) { >+ if (rule.selectorText === ".rule-c") >+ return rule.style; >+ } >+ }; >+ >+ let getProperty = (propertyName) => { >+ let styleDeclaration = getMatchedStyleDeclaration(); >+ for (let property of styleDeclaration.allProperties) { >+ if (property.name === propertyName) >+ return property; >+ } >+ }; >+ >+ 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.CommentOutAndUncommentPropertyLocked2", >+ test(resolve, reject) { >+ let getMatchedStyleDeclaration = () => { >+ for (let rule of nodeStyles.matchedRules) { >+ if (rule.selectorText === ".rule-d") >+ return rule.style; >+ } >+ }; >+ >+ let getProperty = (propertyName) => { >+ let styleDeclaration = getMatchedStyleDeclaration(); >+ for (let property of styleDeclaration.allProperties) { >+ if (property.name === propertyName) >+ return property; >+ } >+ }; >+ >+ 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(); >+ } >+ }); >+ >+ suite.addTestCase({ >+ name: "ModifyCSSProperty.CommentOutAndUncommentPropertyUnlocked", >+ test(resolve, reject) { >+ let getMatchedStyleDeclaration = () => { >+ for (let rule of nodeStyles.matchedRules) { >+ if (rule.selectorText === ".rule-c") >+ return rule.style; >+ } >+ }; >+ >+ let getProperty = (propertyName) => { >+ let styleDeclaration = getMatchedStyleDeclaration(); >+ for (let property of styleDeclaration.allProperties) { >+ if (property.name === propertyName) >+ return property; >+ } >+ >+ >+] >+ InspectorTest.fail("No style declaration found."); >+ resolve(); >+ return; >+ } >+ }; >+ >+ let styleDeclaration = getMatchedStyleDeclaration(); >+ styleDeclaration.locked = false; >+ >+ if (!styleDeclaration) { >+ InspectorTest.fail("No style declaration found."); >+ resolve(); >+ return; >+ } >+ >+ InspectorTest.expectThat(!getProperty("padding-right").enabled, `Commented out property should be disabled.`); >+ >+ let disabled = false; >+ getProperty("padding-right").commentOut(disabled); >+ >+ const expectedStyleText = ` >+ /* padding-left: 2em; */ >+ /* padding-right: 0px; */ >+ `; >+ InspectorTest.expectEqual(styleDeclaration.text, expectedStyleText, `Style declaration text should stay unchanged.`); >+ >+ InspectorTest.expectThat(getProperty("padding-right").enabled, `Uncommented property should be enabled.`); >+ >+ disabled = true; >+ getProperty("padding-right").commentOut(disabled); >+ >+ InspectorTest.expectEqual(styleDeclaration.text, expectedStyleText, `Style declaration text should stay unchanged.`); >+ >+ InspectorTest.expectThat(!getProperty("padding-right").enabled, `Commented out property should be disabled.`); >+ >+ resolve(); >+ } >+ }); >+ > WI.domManager.requestDocument((documentNode) => { > WI.domManager.querySelector(documentNode.id, "#x", (contentNodeId) => { > if (contentNodeId) { >@@ -160,7 +311,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..d5c14fc4359 100644 >--- a/Source/WebInspectorUI/ChangeLog >+++ b/Source/WebInspectorUI/ChangeLog >@@ -1,3 +1,34 @@ >+2018-12-06 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. >+ >+ 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: >+ >+ /* 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..f7dda2220cb 100644 >--- a/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js >+++ b/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js >@@ -270,7 +270,16 @@ WI.CSSProperty = class CSSProperty extends WI.Object > get inherited() { return this._inherited; } > get valid() { return this._valid; } > get variable() { return this._variable; } >- get styleSheetTextRange() { return this._styleSheetTextRange; } >+ >+ get styleSheetTextRange() >+ { >+ return this._styleSheetTextRange; >+ } >+ >+ set styleSheetTextRange(value) >+ { >+ this._styleSheetTextRange = value; >+ } > > get editable() > { >@@ -313,6 +322,11 @@ WI.CSSProperty = class CSSProperty extends WI.Object > > get relatedLonghandProperties() { return this._relatedLonghandProperties; } > >+ getTextRangeWithinOwnerStyle() >+ { >+ return this._styleSheetTextRange.relativeTo(this._ownerStyle.styleSheetTextRange.startLine, this._ownerStyle.styleSheetTextRange.startColumn); >+ } >+ > addRelatedLonghandProperty(property) > { > this._relatedLonghandProperties.push(property); >@@ -360,9 +374,11 @@ WI.CSSProperty = class CSSProperty extends WI.Object > > let styleText = this._ownerStyle.text || ""; > >+ styleText = this._prependSemicolonIfNeeded(styleText); >+ > // _styleSheetTextRange is the position of the property within the stylesheet. > // range is the position of the property within the rule. >- let range = this._styleSheetTextRange.relativeTo(this._ownerStyle.styleSheetTextRange.startLine, this._ownerStyle.styleSheetTextRange.startColumn); >+ let range = this.getTextRangeWithinOwnerStyle(); > > // Append a line break to count the last line of styleText towards endOffset. > range.resolveOffsets(styleText + "\n"); >@@ -375,7 +391,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,10 +403,43 @@ WI.CSSProperty = class CSSProperty extends WI.Object > this._ownerStyle.shiftPropertiesAfter(this, lineDelta, columnDelta, propertyWasRemoved); > } > >- _appendSemicolonIfNeeded(styleText) >+ _prependSemicolonIfNeeded(styleText) > { >- 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) { >+ let range = property.getTextRangeWithinOwnerStyle(); >+ >+ // Append a line break to count the last line of styleText towards endOffset. >+ range.resolveOffsets(styleText + "\n"); >+ >+ let oldText = styleText.slice(range.startOffset, range.endOffset); >+ let newText = oldText.trimRight() + ";" + match[1]; >+ >+ let lineDelta = newText.lineCount - oldText.lineCount; >+ let columnDelta = newText.lastLine.length - oldText.lastLine.length; >+ property.styleSheetTextRange = property.styleSheetTextRange.cloneAndModify(0, 0, lineDelta, columnDelta); >+ >+ let propertyWasRemoved = false; >+ >+ // Text ranges are temporary inaccurate until property._ownerStyle.text is updated. >+ property._ownerStyle.shiftPropertiesAfter(property, lineDelta, columnDelta, propertyWasRemoved); >+ >+ let prefix = styleText.slice(0, range.startOffset); >+ let postfix = styleText.slice(range.endOffset); >+ >+ if (WI.settings.enableStyleEditingDebugMode.value) >+ 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`); >+ >+ styleText = prefix + newText + postfix; >+ } >+ >+ break; >+ } > > return styleText; > }
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:
nvasilyev
:
review-
nvasilyev
:
commit-queue-
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 192396
:
356761
|
356785
|
356790
|
356850
|
356890
|
356891
|
357397