REGRESSION (r188730): Web Inspector: Warning icons incorrectly positioned in CSS Rules sidebar

[Matt Baker]

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)

[Radar WebKit Bug Importer]

<rdar://problem/26356520>

[Devin Rousso]

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.

[Nikita Vasilyev]

This regressed in:
http://trac.webkit.org/changeset/188730/trunk/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js

[Nikita Vasilyev]

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.

[Timothy Hatcher]

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?

[Nikita Vasilyev]

(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.

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

Created attachment 282276 [details]
Patch

[Timothy Hatcher]

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?

[Nikita Vasilyev]

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.

[WebKit Commit Bot]

Comment on attachment 282276 [details]
Patch

Clearing flags on attachment: 282276

Committed r202589: <http://trac.webkit.org/changeset/202589>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.