RESOLVED FIXED 145679
REGRESSION (r184000): Web Inspector: Stripped whitespace after editing CSS in Styles sidebar
https://bugs.webkit.org/show_bug.cgi?id=145679
Summary REGRESSION (r184000): Web Inspector: Stripped whitespace after editing CSS in...
Nikita Vasilyev
Reported 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.
Attachments
Animated GIF of the problem (156.21 KB, image/gif)
2015-06-04 18:30 PDT, Nikita Vasilyev
no flags
[Animated GIF] Expected behavior (143.18 KB, image/gif)
2015-06-25 16:32 PDT, Nikita Vasilyev
no flags
[WIP PATCH] Patch in progress. (3.51 KB, patch)
2015-07-06 15:19 PDT, Jonathan Wells
no flags
[SCREENSHOT] Patch in progress screenshot. (155.67 KB, image/png)
2015-07-06 15:21 PDT, Jonathan Wells
no flags
[PATCH] Fix (4.72 KB, patch)
2015-07-13 17:56 PDT, Jonathan Wells
no flags
[PATCH] fix 2. (4.56 KB, patch)
2015-07-14 15:37 PDT, Jonathan Wells
no flags
[PATCH] fix 3. (4.27 KB, patch)
2015-07-14 15:57 PDT, Jonathan Wells
no flags
[PATCH] fix 4. (5.00 KB, patch)
2015-07-20 16:50 PDT, Jonathan Wells
no flags
Patch (10.22 KB, patch)
2015-08-13 11:53 PDT, Devin Rousso
no flags
Patch (10.31 KB, patch)
2015-08-13 16:58 PDT, Devin Rousso
no flags
Radar WebKit Bug Importer
Comment 1 2015-06-04 18:30:57 PDT
Nikita Vasilyev
Comment 2 2015-06-04 18:52:31 PDT
I have bisected and found that it regressed in http://trac.webkit.org/changeset/184000/trunk.
Nikita Vasilyev
Comment 3 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.
Tobias Reiss
Comment 4 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.
Jonathan Wells
Comment 5 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.
Nikita Vasilyev
Comment 6 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.
Jonathan Wells
Comment 7 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.
Jonathan Wells
Comment 8 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.
Timothy Hatcher
Comment 9 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?
Jonathan Wells
Comment 10 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?
Timothy Hatcher
Comment 11 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?
Jonathan Wells
Comment 12 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.
Timothy Hatcher
Comment 13 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.
Jonathan Wells
Comment 14 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!
Jonathan Wells
Comment 15 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.
Jonathan Wells
Comment 16 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.
Jonathan Wells
Comment 17 2015-07-06 15:21:13 PDT
Created attachment 256246 [details] [SCREENSHOT] Patch in progress screenshot.
Timothy Hatcher
Comment 18 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.
Jonathan Wells
Comment 19 2015-07-13 17:56:53 PDT
Created attachment 256751 [details] [PATCH] Fix
Tobias Reiss
Comment 20 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.
Jonathan Wells
Comment 21 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.
Jonathan Wells
Comment 22 2015-07-14 15:37:42 PDT
Created attachment 256804 [details] [PATCH] fix 2.
Tobias Reiss
Comment 23 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))
Jonathan Wells
Comment 24 2015-07-14 15:57:43 PDT
Created attachment 256806 [details] [PATCH] fix 3.
Jonathan Wells
Comment 25 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.
Joseph Pecoraro
Comment 26 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.
Jonathan Wells
Comment 27 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.
Jonathan Wells
Comment 28 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.
Jonathan Wells
Comment 29 2015-07-20 16:50:23 PDT
Created attachment 257146 [details] [PATCH] fix 4.
Brian Burg
Comment 30 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.
Timothy Hatcher
Comment 31 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.
Jonathan Wells
Comment 32 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.
Jonathan Wells
Comment 33 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.
Devin Rousso
Comment 34 2015-08-13 11:53:00 PDT
Timothy Hatcher
Comment 35 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.
Tobias Reiss
Comment 36 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 :)
Devin Rousso
Comment 37 2015-08-13 16:58:25 PDT
WebKit Commit Bot
Comment 38 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>
WebKit Commit Bot
Comment 39 2015-08-13 18:51:35 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.