Bug 157869 - REGRESSION (r188730): Web Inspector: Warning icons incorrectly positioned in CSS Rules sidebar
Summary: REGRESSION (r188730): Web Inspector: Warning icons incorrectly positioned in ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-05-18 15:58 PDT by Matt Baker
Modified: 2016-06-28 14:33 PDT (History)
9 users (show)

See Also:


Attachments
[Image] Misplaced icons (189.86 KB, image/png)
2016-05-18 15:58 PDT, Matt Baker
no flags Details
WIP (914 bytes, patch)
2016-06-27 11:15 PDT, Nikita Vasilyev
nvasilyev: review-
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
Patch (5.09 KB, patch)
2016-06-28 13:23 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2016-05-18 15:58:18 PDT
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)
Comment 1 Radar WebKit Bug Importer 2016-05-18 15:59:06 PDT
<rdar://problem/26356520>
Comment 2 Devin Rousso 2016-05-26 22:16:08 PDT
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.
Comment 4 Nikita Vasilyev 2016-06-27 11:15:41 PDT
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 5 Timothy Hatcher 2016-06-27 11:18:18 PDT
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?
Comment 6 Nikita Vasilyev 2016-06-27 11:43:26 PDT
(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.
Comment 7 Nikita Vasilyev 2016-06-27 13:41:43 PDT
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.
Comment 8 Nikita Vasilyev 2016-06-28 13:23:38 PDT
Created attachment 282276 [details]
Patch
Comment 9 Timothy Hatcher 2016-06-28 13:34:58 PDT
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 10 Nikita Vasilyev 2016-06-28 14:12:29 PDT
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 11 WebKit Commit Bot 2016-06-28 14:33:31 PDT
Comment on attachment 282276 [details]
Patch

Clearing flags on attachment: 282276

Committed r202589: <http://trac.webkit.org/changeset/202589>
Comment 12 WebKit Commit Bot 2016-06-28 14:33:36 PDT
All reviewed patches have been landed.  Closing bug.