Created attachment 279305 [details] [Image] Misplaced icons * SUMMARY Warning icons incorrectly positioned in CSS Rules sidebar. * STEPS TO REPRODUCE 1. Inspect about:blank 2. Goto Elements > Styles — Rules 3. Past the following into the empty body {} rule: direction: blue; colors: red; => Both warning icons appear on the same line (see screenshot)
<rdar://problem/26356520>
I think that this may be happening because, since inline styles effectively have a single line of code, the CodeMirror resource is having to pretty print for us, but the warning icons are added before that happens, meaning that both of them are added to the first line in the first position. I tested this with the exact same steps but in a new stylesheet rule instead and there was no issue.
This regressed in: http://trac.webkit.org/changeset/188730/trunk/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js
Created attachment 282146 [details] WIP text.trim() introduced in r188730 removed the first line break. This caused the regression. Before r188730: \n direction: blue;\n colors: red;\n After r188730: direction: blue;\n colors: red; Prepending a line break fixes the issues. However, I don't think it's right to require a CSS rules string to start with a line break. I'm looking at CSSAgent.setStyleText to find the root cause.
Comment on attachment 282146 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=282146&action=review > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:189 > - let trimmedText = text.trim(); > + let trimmedText = "\n" + text.trim(); This might negatively impact single line style rules. .foo { color: red; } Would become: .foo { color: red; } Right?
(In reply to comment #5) > Comment on attachment 282146 [details] > WIP > > View in context: > https://bugs.webkit.org/attachment.cgi?id=282146&action=review > > > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:189 > > - let trimmedText = text.trim(); > > + let trimmedText = "\n" + text.trim(); > > This might negatively impact single line style rules. > > .foo { color: red; } > > Would become: > > .foo { > color: red; } > > Right? We already insert a line break after "{" and before "}". div {color: red} Changing color to "green" in the Styles sidebar changes the original formatting to: div { color: green } We could improve that by guessing original formatting. However, that's outside of the scope of this bug.
https://github.com/WebKit/webkit/blob/dea05a5f9291f1c0154b53d9eb31fb130bc4a076/Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js#L791-L795 // Adjust the line position for the missing prefix line. if (this._prefixWhitespace) { --from.line; --to.line; } Since this._prefixWhitespace is always "\n", line numbers become off by one.
Created attachment 282276 [details] Patch
Comment on attachment 282276 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=282276&action=review > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:189 > + let trimmedText = WebInspector.CSSStyleDeclarationTextEditor.PrefixWhitespace + text.trim(); Why does this not need to be WebInspector.CSSStyleDeclarationTextEditor.PrefixWhitespace + text.trim() + WebInspector.CSSStyleDeclarationTextEditor.SuffixWhitespace?
Comment on attachment 282276 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=282276&action=review >> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:189 >> + let trimmedText = WebInspector.CSSStyleDeclarationTextEditor.PrefixWhitespace + text.trim(); > > Why does this not need to be WebInspector.CSSStyleDeclarationTextEditor.PrefixWhitespace + text.trim() + WebInspector.CSSStyleDeclarationTextEditor.SuffixWhitespace? This only applies to inline styles. See below: if (trimmedText === WebInspector.CSSStyleDeclarationTextEditor.PrefixWhitespace || this._type === WebInspector.CSSStyleDeclaration.Type.Inline) text = trimmedText; This patch is essentially fixes the bug without making too much changes in how we handle spacing and indentation in CSS. In the future, we shouldn't append or prepend "\n" to inline styles at all. This would require more changes to be made, which can potentially break things.
Comment on attachment 282276 [details] Patch Clearing flags on attachment: 282276 Committed r202589: <http://trac.webkit.org/changeset/202589>
All reviewed patches have been landed. Closing bug.