Summary: | REGRESSION (r184000): Web Inspector: Stripped whitespace after editing CSS in Styles sidebar | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikita Vasilyev <nvasilyev> | ||||||||||||||||||||||
Component: | Web Inspector | Assignee: | 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
Nikita Vasilyev
2015-06-04 18:30:25 PDT
I have bisected and found that it regressed in http://trac.webkit.org/changeset/184000/trunk. (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. 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. 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. Created attachment 255594 [details] [Animated GIF] Expected behavior This is pre-r184000 build and it works as expected. Thanks for that, Nikita. Trying to figure out which piece was doing the formatting on the resource itself. In CSSStyleDeclarationTextEditor.js it might be possible to store the prefixed whitespace separately for each line and restore them upon formatting in using _formattedContent. (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? (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? (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? (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. (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. (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! 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. 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.
Created attachment 256246 [details]
[SCREENSHOT] Patch in progress screenshot.
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. Created attachment 256751 [details]
[PATCH] Fix
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 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. Created attachment 256804 [details]
[PATCH] fix 2.
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)) Created attachment 256806 [details]
[PATCH] fix 3.
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 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 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 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. Created attachment 257146 [details]
[PATCH] fix 4.
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 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 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 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. Created attachment 258908 [details]
Patch
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 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 :) Created attachment 258957 [details]
Patch
Comment on attachment 258957 [details] Patch Clearing flags on attachment: 258957 Committed r188427: <http://trac.webkit.org/changeset/188427> All reviewed patches have been landed. Closing bug. |