Bug 145679

Summary: REGRESSION (r184000): Web Inspector: Stripped whitespace after editing CSS in Styles sidebar
Product: WebKit Reporter: Nikita Vasilyev <nvasilyev>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Critical CC: commit-queue, ddkilzer, graouts, hi, joepeck, jonowells, mattbaker, nvasilyev, timothy, tobi+webkit, webkit-bug-importer
Priority: P1 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Animated GIF of the problem
none
[Animated GIF] Expected behavior
none
[WIP PATCH] Patch in progress.
none
[SCREENSHOT] Patch in progress screenshot.
none
[PATCH] Fix
none
[PATCH] fix 2.
none
[PATCH] fix 3.
none
[PATCH] fix 4.
none
Patch
none
Patch none

Description Nikita Vasilyev 2015-06-04 18:30:25 PDT
Created attachment 254327 [details]
Animated GIF of the problem

index.html:

<!DOCTYPE html>
<html>
<head>
    <link rel="stylesheet" href="style.css"/>
</head>
<body> blah </body>
</html>


style.css:

body {
    background: #000;
    color: #CCC;
}


Sometimes, CSS resource doesn’t update at all, see https://bugs.webkit.org/show_bug.cgi?id=143244.
Comment 1 Radar WebKit Bug Importer 2015-06-04 18:30:57 PDT
<rdar://problem/21253488>
Comment 2 Nikita Vasilyev 2015-06-04 18:52:31 PDT
I have bisected and found that it regressed in http://trac.webkit.org/changeset/184000/trunk.
Comment 3 Nikita Vasilyev 2015-06-04 18:56:19 PDT
(In reply to comment #0)
> Sometimes, CSS resource doesn’t update at all, see
> https://bugs.webkit.org/show_bug.cgi?id=143244.

To clarify:
https://bugs.webkit.org/show_bug.cgi?id=143244 isn't related to http://trac.webkit.org/changeset/184000, it was introduced before that.
Comment 4 Tobias Reiss 2015-06-08 03:39:58 PDT
Editing CSS in the Styles sidebar happens within an Editor that uses the CSSRule-Formatter for pretty printing.

Before http://trac.webkit.org/changeset/184000/trunk:
The CSSRule-Formatter was (kinda) a copy of the CSS-Formatter.

After http://trac.webkit.org/changeset/184000/trunk:
The CSSRule-Formatter was optimized for CSSRules and lost some of the CSSStyleSheet specific features. That change optimized pretty printing in the Styles sidebar.

The bug is a result of applying CSS that is optimized for CSSRules to a CSSStyleSheet.

Potential solutions:
1) Let the CSS being formatted with the CSS-Formatter before it gets copied to the CSS Resource.
2) Change the CSSRule-Formatter in a way so that it works for CSSRule and CSSStyleSheet.

I'd prefer solution 1. Not sure though what part of the code should apply the CSS-Formatter.
Comment 5 Jonathan Wells 2015-06-25 16:08:19 PDT
After talking with Tobias offline I don't think this is a regression. I just think the solution to format the actual CSS resource hasn't been implemented yet. I'm going to work with his first suggestion.
Comment 6 Nikita Vasilyev 2015-06-25 16:32:25 PDT
Created attachment 255594 [details]
[Animated GIF] Expected behavior

This is pre-r184000 build and it works as expected.
Comment 7 Jonathan Wells 2015-06-28 16:49:35 PDT
Thanks for that, Nikita. Trying to figure out which piece was doing the formatting on the resource itself.
Comment 8 Jonathan Wells 2015-06-29 14:55:23 PDT
In CSSStyleDeclarationTextEditor.js it might be possible to store the prefixed whitespace separately for each line and restore them upon formatting in using _formattedContent.
Comment 9 Timothy Hatcher 2015-06-30 09:27:06 PDT
(In reply to comment #8)
> In CSSStyleDeclarationTextEditor.js it might be possible to store the
> prefixed whitespace separately for each line and restore them upon
> formatting in using _formattedContent.

That would be ideal. How would you track new or deleted lines?
Comment 10 Jonathan Wells 2015-06-30 12:55:07 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > In CSSStyleDeclarationTextEditor.js it might be possible to store the
> > prefixed whitespace separately for each line and restore them upon
> > formatting in using _formattedContent.
> 
> That would be ideal. How would you track new or deleted lines?

Maybe we could add/remove the prefixed whitespace data to the array using [].splice() as you edit?
Comment 11 Timothy Hatcher 2015-06-30 15:01:35 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > In CSSStyleDeclarationTextEditor.js it might be possible to store the
> > > prefixed whitespace separately for each line and restore them upon
> > > formatting in using _formattedContent.
> > 
> > That would be ideal. How would you track new or deleted lines?
> 
> Maybe we could add/remove the prefixed whitespace data to the array using
> [].splice() as you edit?

That would be slick. I assume newlines would inherit the indent from the previous line?
Comment 12 Jonathan Wells 2015-06-30 15:08:00 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (In reply to comment #8)
> > > > In CSSStyleDeclarationTextEditor.js it might be possible to store the
> > > > prefixed whitespace separately for each line and restore them upon
> > > > formatting in using _formattedContent.
> > > 
> > > That would be ideal. How would you track new or deleted lines?
> > 
> > Maybe we could add/remove the prefixed whitespace data to the array using
> > [].splice() as you edit?
> 
> That would be slick. I assume newlines would inherit the indent from the
> previous line?

Yes. This would be congruent with how most text editors behave by default as you add new lines. 

We'll have to consider the case where all lines are removed and then one added back, but this might be where we just use the shortest prefix whitespace that was stored before.
Comment 13 Timothy Hatcher 2015-06-30 15:12:26 PDT
(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (In reply to comment #9)
> > > > (In reply to comment #8)
> > > > > In CSSStyleDeclarationTextEditor.js it might be possible to store the
> > > > > prefixed whitespace separately for each line and restore them upon
> > > > > formatting in using _formattedContent.
> > > > 
> > > > That would be ideal. How would you track new or deleted lines?
> > > 
> > > Maybe we could add/remove the prefixed whitespace data to the array using
> > > [].splice() as you edit?
> > 
> > That would be slick. I assume newlines would inherit the indent from the
> > previous line?
> 
> Yes. This would be congruent with how most text editors behave by default as
> you add new lines. 
> 
> We'll have to consider the case where all lines are removed and then one
> added back, but this might be where we just use the shortest prefix
> whitespace that was stored before.

Yeah, or just a default 4 spaces. We might add a setting for indent width in the future which we would use.
Comment 14 Jonathan Wells 2015-06-30 15:26:47 PDT
(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (In reply to comment #10)
> > > > (In reply to comment #9)
> > > > > (In reply to comment #8)
> > > > > > In CSSStyleDeclarationTextEditor.js it might be possible to store the
> > > > > > prefixed whitespace separately for each line and restore them upon
> > > > > > formatting in using _formattedContent.
> > > > > 
> > > > > That would be ideal. How would you track new or deleted lines?
> > > > 
> > > > Maybe we could add/remove the prefixed whitespace data to the array using
> > > > [].splice() as you edit?
> > > 
> > > That would be slick. I assume newlines would inherit the indent from the
> > > previous line?
> > 
> > Yes. This would be congruent with how most text editors behave by default as
> > you add new lines. 
> > 
> > We'll have to consider the case where all lines are removed and then one
> > added back, but this might be where we just use the shortest prefix
> > whitespace that was stored before.
> 
> Yeah, or just a default 4 spaces. We might add a setting for indent width in
> the future which we would use.

Works for me!
Comment 15 Jonathan Wells 2015-07-01 13:36:10 PDT
This is working I think. The problem though is that I've lost the utility of the code mirror instance to retrieve lines of text and such for me (since I'm working directly on the string), so I'm having to implement a lot of that.
Comment 16 Jonathan Wells 2015-07-06 15:19:09 PDT
Created attachment 256245 [details]
[WIP PATCH] Patch in progress.

I'm posting my progress and a screenshot of an issue I'm having. I could use help because I'm unclear on how the checkbox positions are calculated. Right now I'm trying to save the whitespace for each line before the styles are edited. While I haven't yet accounted for new lines that are added, I first am trying to resolve an issue where the checkboxes now appear in the wrong place. Here is the patch in progress.
Comment 17 Jonathan Wells 2015-07-06 15:21:13 PDT
Created attachment 256246 [details]
[SCREENSHOT] Patch in progress screenshot.
Comment 18 Timothy Hatcher 2015-07-09 12:26:23 PDT
Comment on attachment 256245 [details]
[WIP PATCH] Patch in progress.

View in context: https://bugs.webkit.org/attachment.cgi?id=256245&action=review

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1385
> +            this._linePrefixWhitespace = [];

The bug you are seeing with the checkboxes is due to _updateTextMarkers using _linePrefixWhitespace as a string, and not indexing into the array you add here. The markers need to account for the whitespace in character offsets.
Comment 19 Jonathan Wells 2015-07-13 17:56:53 PDT
Created attachment 256751 [details]
[PATCH] Fix
Comment 20 Tobias Reiss 2015-07-14 03:14:27 PDT
Comment on attachment 256751 [details]
[PATCH] Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=256751&action=review

Glad you found a way to fix this! Looks like magic to me.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:507
> +            if (!hasEndingSemicolon && trimmedLine.length && !this._textAtCursorIsComment(this._codeMirror, cursor))

Maybe I'm reading that wrong but considering `trimmedLine.length` is 0, why should we handle that case at all and instead not just return early? Could it be that L499 takes care of that already?

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1513
> +

Similar to how the variables are named in `_handleEnterKey` you could consider renaming those to:

var styleText = this._style.text;
var trimmedStyleText = styleText.trim();

Yep, `styleText` changes its meaning.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1523
> +            // Capture the whitespace before and after the set of properties, and before the first property

Consider putting all this in a separate function. Reasons:
- encapsulate logic
- reveal side-effects
- allow more meaningful variable names

function setLinePrefixSuffixWhitespace(styleText) {…}
setLinePrefixSuffixWhitespace(this._style.text);

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1525
> +            var prefixWhitespaceRegExp = /^[ \t]*\n/;

You might want to also match no-break space.
Comment 21 Jonathan Wells 2015-07-14 15:09:48 PDT
Comment on attachment 256751 [details]
[PATCH] Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=256751&action=review

>> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:507
>> +            if (!hasEndingSemicolon && trimmedLine.length && !this._textAtCursorIsComment(this._codeMirror, cursor))
> 
> Maybe I'm reading that wrong but considering `trimmedLine.length` is 0, why should we handle that case at all and instead not just return early? Could it be that L499 takes care of that already?

I'll make sure but hitting enter on a blank line should only put a \n without the ; on that line. I think this is needed but I'll make sure.
Comment 22 Jonathan Wells 2015-07-14 15:37:42 PDT
Created attachment 256804 [details]
[PATCH] fix 2.
Comment 23 Tobias Reiss 2015-07-14 15:51:50 PDT
Comment on attachment 256804 [details]
[PATCH] fix 2.

View in context: https://bugs.webkit.org/attachment.cgi?id=256804&action=review

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1508
> +            if (!this instanceof WebInspector.CSSStyleDeclarationTextEditor)

Attention! This will always evaluate to false.

if (!(this instanceof WebInspector.CSSStyleDeclarationTextEditor))
Comment 24 Jonathan Wells 2015-07-14 15:57:43 PDT
Created attachment 256806 [details]
[PATCH] fix 3.
Comment 25 Jonathan Wells 2015-07-16 12:21:34 PDT
Comment on attachment 256804 [details]
[PATCH] fix 2.

View in context: https://bugs.webkit.org/attachment.cgi?id=256804&action=review

>> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1508
>> +            if (!this instanceof WebInspector.CSSStyleDeclarationTextEditor)
> 
> Attention! This will always evaluate to false.
> 
> if (!(this instanceof WebInspector.CSSStyleDeclarationTextEditor))

Oh yeah I put this in when I was doing this differently and there was a chance it could be something else. Will remove.
Comment 26 Joseph Pecoraro 2015-07-16 16:10:32 PDT
Comment on attachment 256806 [details]
[PATCH] fix 3.

View in context: https://bugs.webkit.org/attachment.cgi?id=256806&action=review

Looking better. I caught an issue in testing.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1472
> +        var styleText = this._style.text;

At some point we have to trim. After testing, it appears as if this may constantly add whitespace to the end of a rule. I ended up with a CSS file that looked like:

>    body {
>        background: #000;
>        color: red;
>        
>        
>        
>        
>        
>        
>        
>        
>        
>    }

If I kept clicking in and out of the styles sidebar which looked reasonable:

>        background: #000;
>        color: red;

r- until we can fix that issue.

* STEPS TO REPRODUCE
1. Inspect <body> on original test case
2. Click at the end of the "color" link in the style sidebar editor => adds a newline
3. Click outside the editor => removes the newline in the sidebar
4. Repeat steps 2 and 3 a half dozen times
5. Show CSS resource
  => don't expect lots of trailing newlines.

Not sure where it would be best to fix this.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1477
> +        var prefixWhitespaceRegExp = /^[\s\t]*\n/;
> +        var suffixWhitespaceRegExp = /\n[\s\t]*$/;

I think just \s gives you what you want. No need for the [\s\t] group:

    /\s/.test("\t") // true

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1497
> +        else if (styleText.match(/^.*?\n/))
> +            this._linePrefixWhitespace = "";

What is this case handling or trying to handle?

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1551
> +            this._setLinePrefixSuffixWhitespace();

I think "updateFoo" is a better name. It does set values but I would expect a "setFoo" function to be passed values.
Comment 27 Jonathan Wells 2015-07-20 13:20:48 PDT
Comment on attachment 256806 [details]
[PATCH] fix 3.

View in context: https://bugs.webkit.org/attachment.cgi?id=256806&action=review

>> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1472
>> +        var styleText = this._style.text;
> 
> At some point we have to trim. After testing, it appears as if this may constantly add whitespace to the end of a rule. I ended up with a CSS file that looked like:

Yes, clicking into the rule automatically adds a newline at the end, and those aren't being removed. I'm trying to figure out why.

>> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1497
>> +            this._linePrefixWhitespace = "";
> 
> What is this case handling or trying to handle?

I don't remember. May not be necessary.
Comment 28 Jonathan Wells 2015-07-20 14:30:29 PDT
Comment on attachment 256806 [details]
[PATCH] fix 3.

View in context: https://bugs.webkit.org/attachment.cgi?id=256806&action=review

>>> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1497
>>> +            this._linePrefixWhitespace = "";
>> 
>> What is this case handling or trying to handle?
> 
> I don't remember. May not be necessary.

I remember now what I was trying to match. This is identifying that there is a rule on the first line with no prefix whitespace and thus there should not be any prefix whitespace on any other rules.
Comment 29 Jonathan Wells 2015-07-20 16:50:23 PDT
Created attachment 257146 [details]
[PATCH] fix 4.
Comment 30 Brian Burg 2015-07-20 17:11:55 PDT
Comment on attachment 257146 [details]
[PATCH] fix 4.

View in context: https://bugs.webkit.org/attachment.cgi?id=257146&action=review

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:2
> + * Copyright (C) 2015 Apple Inc. All rights reserved.

2013, 2015

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1489
> +        // FIXME: <rdar://problem/10593948> Provide a way to change the tab width in the Web Inspector

this is now duplicated in constructor.
Comment 31 Timothy Hatcher 2015-07-21 09:11:43 PDT
Comment on attachment 257146 [details]
[PATCH] fix 4.

View in context: https://bugs.webkit.org/attachment.cgi?id=257146&action=review

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:50
> -        this._linePrefixWhitespace = "";
> +
> +        // FIXME: <rdar://problem/10593948> Provide a way to change the tab width in the Web Inspector
> +        this._linePrefixWhitespace = "    ";

Not needed here since _updateLinePrefixSuffixWhitespace is called before it is used. Keeping this._linePrefixWhitespace = "" would be best.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:59
> -            indentWithTabs: true,
> +            indentWithTabs: false,

Intentional? This is true everywhere else we use CodeMirror.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1476
> +        var prefixWhitespaceRegExp = /^[\s]*\n/;

A newline should not be required. For a single line style rule that is:

.foo { bar: 42; baz: 24; }

The _prefixWhitespace should end up being " ", this regex will fail and cause _prefixWhitespace to be "".

So I think you just want /^\s*/. No need for the character class brackets ([]) either.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1477
> +        var suffixWhitespaceRegExp = /\n[\s]*$/;

Ditto for _suffixWhitespace. /\s*$/

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1496
> +        else if (styleText.match(/^./))

What is this for? "." matches anything. styleText.length be a better check, and do the same thing.
Comment 32 Jonathan Wells 2015-07-22 15:43:22 PDT
Comment on attachment 257146 [details]
[PATCH] fix 4.

View in context: https://bugs.webkit.org/attachment.cgi?id=257146&action=review

>> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:59
>> +            indentWithTabs: false,
> 
> Intentional? This is true everywhere else we use CodeMirror.

This is to keep code mirror's behavior consistent with our default: this._linePrefixWhitespace = "    "; This there being tabs sometimes and spaces sometimes as a prefix whitespace default, particularly when adding the first rule to an empty selector.
Comment 33 Jonathan Wells 2015-07-22 15:50:05 PDT
Comment on attachment 257146 [details]
[PATCH] fix 4.

View in context: https://bugs.webkit.org/attachment.cgi?id=257146&action=review

>> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1476
>> +        var prefixWhitespaceRegExp = /^[\s]*\n/;
> 
> A newline should not be required. For a single line style rule that is:
> 
> .foo { bar: 42; baz: 24; }
> 
> The _prefixWhitespace should end up being " ", this regex will fail and cause _prefixWhitespace to be "".
> 
> So I think you just want /^\s*/. No need for the character class brackets ([]) either.

When i make this change, it breaks cases like this:

body {
    text-align: left;
    font-size: 12px;
}

Which becomes:

body {
text-align: left;
font-size: 12px;
}

That's because the simple rule /^\s*/ eats up the line prefix whitespace from the first line, thus making the assumption that should be the whitespace for EVERY line. I need to somehow test for the single line selector as a special case.
Comment 34 Devin Rousso 2015-08-13 11:53:00 PDT
Created attachment 258908 [details]
Patch
Comment 35 Timothy Hatcher 2015-08-13 14:39:17 PDT
Comment on attachment 258908 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258908&action=review

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:-1617
> -

Keep empty line, since the previous line is a function call. If it was the let readOnly = ... line then it could touch the if (readOnly).

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1668
> +            let findWhitespace = /\s+/g;

whitespaceRegex would be a better name. find implies this is a function.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1686
> +            // Recommit the changes to the style sheet.
> +            this._commitChanges();

We shouldn't commit changes here. We don't want to change things just by viewing the Styles sidebar. It should only happen if a user makes a change.
Comment 36 Tobias Reiss 2015-08-13 16:28:12 PDT
Comment on attachment 258908 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258908&action=review

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1615
> +        let readOnly = !this._style || !this._style.editable || !this._style.styleSheetTextRange;

Looks like the variable "readOnly" shouldn't be changed within its scope? If that's the case I would recommend using `const` instead of `let`. I'm using `const` now for quite some time in production (with Babel) and it makes the code easier to read (and review) since I have the certainty that the variable can’t be re-assigned.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1643
>  

I like the variable name! :) Consider using `const`.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1673
> +            if (styleTextPrefixWhitespace && trimmedStyleText.includes("\n")) {

Please double-check: Do you want to find "\n" in styleTextPrefixWhitespace? If so, shouldn't the condition be styleTextPrefixWhitespace && styleTextPrefixWhitespace[0].includes("\n")? Could be that I'm reading that wrong. A comment would be helpful!

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1674
> +                let linePrefixWhitespaceMatch = styleTextPrefixWhitespace[0].match(/[^\S\n]+$/);

Isn't styleTextPrefixWhitespace[0] supposed to include only whitespace-like characters (\s) or new-line? Why do you try to match non-whitespace-like characters (\S)? A comment would be helpful, at least for me :)
Comment 37 Devin Rousso 2015-08-13 16:58:25 PDT
Created attachment 258957 [details]
Patch
Comment 38 WebKit Commit Bot 2015-08-13 18:51:30 PDT
Comment on attachment 258957 [details]
Patch

Clearing flags on attachment: 258957

Committed r188427: <http://trac.webkit.org/changeset/188427>
Comment 39 WebKit Commit Bot 2015-08-13 18:51:35 PDT
All reviewed patches have been landed.  Closing bug.