* SUMMARY Styles sidebar editing with incomplete property looks poor in UI. * STEPS TO REPRODUCE 1. Inspect <body> 2. In Styles sidebar for Style attribute type: "color; display: none" => confusing, poor UI.
<rdar://problem/19856887>
Created attachment 248087 [details] this looks poor how it looks like now
Created attachment 248089 [details] this looks better how it's supposed to look like
We do attempt to maintain the author formatting. If it is all on one line I think we will force it to break on multiple lines. That must be where the bug is.
From CSSStyleDeclarationTextEditor.js: // Pretty print the content if there are more properties than there are lines. // This could be an option exposed to the user; however, it is almost always // desired in this case. I think case the first property is an error and does not count, so we have 1 line and 1 property.
Created attachment 248213 [details] patch
Created attachment 248216 [details] patch
Breaks style declarations into multiple lines. Try: color; /* color: blue; display: none; */ text-align: left;
Created attachment 248238 [details] patch
Regarding patch update: I added some more comments and fixed an issue where style declarations with already existing line-breaks were accidentally ignored.
I'm still not happy with my patch. What I really want to do is the following: - Listen on onLineChange and onPaste - Parse the line and add missing new line characters, colons and semi-colons. A parser should be able to convert: - key: value => key: value;\n - key; => key:;\n - /* key:value; key2:value2 */ => /* key: value; */\n/* key2: value2; */\n - key:value /* key: value; */ => key: value; /* key: value; */ Questions: - Where can I listen on "onLineChange" and "onPaste"? _reset doesn't seem to be the correct place. It's not called onLineChange. - Are you okay with the mentioned spec for the parser?
Comment on attachment 248238 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=248238&action=review > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:217 > + // The parser detects multiple style declarations in one line, > + // corrects invalid style declarations and adds missing line breaks Style: WebKit style is for comments to end with periods. This comment applies to all comments in this patch. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:218 > + _parseStyleDeclarations: function(string) The name _parse implies that you are parsing the string into some data structure / representation. Here you are just returning a formatted string, so perhaps this should be named _formatStyleDeclarations. The comment above the function shouldn't be needed with a clear name, but if you want to keep it can go inside the function. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:230 > + style = mode.token(stream, state); Style: WebKit style for JavaScript is to just use var at the declaration point. You can move the `var`s from the for loop initializer section in front of the variables `stream` and `style`. Imagine them moving to `let` eventually. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:233 > + if (style === "comment m-css") { We typically test style.test(/\bcomment\b) to be resilient against additional styles getting added to the token. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:235 > + parsed.push(stream.current()); > + // Otherwise parse line and consider it containing multiple declarations Comment indentation here is confusing. You can add a `continue;` here, to reduce the `else if` to just an `if` with the comment. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:253 > + current = stream.current().trim(); > + // Detect opening comment > + if (current.indexOf("/*") > -1) { > + // Remove opening comment and whitespace > + current = current.replace(/\s*\/\*\s*/, ""); > + state.isInlineComment = true; > + } > + // Detect ending comment > + if (current.indexOf("*/") > -1) { > + // Remove ending comment and whitespace > + current = current.replace(/\s*\*\/\s*/, ""); > + state.isInlineComment = false; > + } > + // Add missing colon in case of a property without a value > + // E.g.: "color;" turns into "color:;" > + if (current.indexOf(":") === -1) > + current += ":"; I see the logic here, but because we are using a CSS tokenizer we should take advantage of it. stream.skipTo looks like it will just search characters, not tokens. So the ';' that it finds could be in the middle of a string or comment: /* foo; bar */ div { background: url(some;thing); Likewise for the comment searching.
(In reply to comment #11) > I'm still not happy with my patch. What I really want to do is the following: > > - Listen on onLineChange and onPaste > - Parse the line and add missing new line characters, colons and semi-colons. > > A parser should be able to convert: > - key: value => key: value;\n > - key; => key:;\n > - /* key:value; key2:value2 */ => /* key: value; */\n/* key2: value2; */\n > - key:value /* key: value; */ => key: value; /* key: value; */ That seems reasonable to me. I like the idea of splitting up the comments. > Questions: > - Where can I listen on "onLineChange" and "onPaste"? _reset doesn't seem to > be the correct place. It's not called onLineChange. I don't think CodeMirror has "onLineChange" it has more generic change events, which we have registered for: http://codemirror.net/doc/manual.html#events this._codeMirror.on("change", this._contentChanged.bind(this)); I don't remember off hand, but I would guess that pastes trigger a change as well.
The goal of this change is to format the style sidebar, correct? If so, I wonder if we should do this for everything. We currently try to preserve the developer's formatting except for the case I mentioned in comment #5. I think we might want to improve the auto-completion to auto insert semicolons and newlines as you type and not try to clean up the existing code. That will be a much more attainable goal. Adding a parser / formatter is costly to the project and can be a performance bottleneck. Was the original code in the attached images from a page or typed in the Inspector via live editing?
> I see the logic here, but because we are using a CSS tokenizer we should > take advantage of it. In case we do go down the parser route: There is a tool I use to see CodeMirror's tokenizer / parser output. Open up Source/WebInspectorUI/Tools/PrettyPrinting/index.html in your browser and play around with it. Switch the language to CSS, and write some code and you can see what CodeMirror's tokenizer/parser state looks like. For instance: div { /* comment */ color; color: red; } Outputs: Token: 'tag' Stack: 'undefined' Current: 'div' Token: null Stack: 'undefined' Current: ' ' Token: null Stack: 'rule' Current: '{' Token: null Stack: 'rule' Current: ' ' Token: 'comment' Stack: 'rule' Current: '/* comment */' Token: null Stack: 'rule' Current: ' ' Token: 'property' Stack: 'rule' Current: 'color' Token: null Stack: 'rule' Current: ';' Token: null Stack: 'rule' Current: ' ' Token: 'property' Stack: 'rule' Current: 'color' Token: 'operator' Stack: 'propertyValue' Current: ':' Token: null Stack: 'propertyValue' Current: ' ' Token: 'keyword' Stack: 'propertyValue' Current: 'red' Token: null Stack: 'rule' Current: ';' Token: null Stack: 'undefined' Current: '}' So it seems like using CodeMirror's CSS mode you should be able to detect comments and "property;" and "property: value" patterns rather easily without direct string searching. As for the cost, I have a feeling it is not very costly because CodeMirror is already parsing so that we can get syntax highlighting.
One other reason I don't really like this, is it hides errors from the developer. Your patch is fixing up the code in the sidebar, but that bad CSS is still being applied to the page. If you click through the the CSS resource you will see the original bad code (or on the DOM node in the style attribute). Fixing errors as the developers pastes or types is fine, but papering over them after the CSS is already on the page is not okay and just hides the problem. We should consider using the existing CSS pretty printer and adding a pretty print toggle to the sidebar if we want to have a way to pretty up the code. But the pretty printer does not fix developer errors.
Created attachment 249322 [details] Describes different behavior of details sidebar's pretty print As described in the video "_resetContent" handles pretty print differently depending on how many "visible" properties are available in one line. My questions now are: - Is this a bug? - Shall we pretty print invalid properties and comments or remove them? Timothy, you pointed out that we should only pretty print and not modify styles because the bad css is still available in the code, right? In that case I would consider this behavior being a bug.
(In reply to comment #17) > Created attachment 249322 [details] > Describes different behavior of details sidebar's pretty print > > As described in the video "_resetContent" handles pretty print differently > depending on how many "visible" properties are available in one line. My > questions now are: > > - Is this a bug? > - Shall we pretty print invalid properties and comments or remove them? Looks and sounds like a bug. As a user, I wouldn't expect content to suddenly be removed, even if it is invalid. > Timothy, you pointed out that we should only pretty print and not modify > styles because the bad css is still available in the code, right? In that > case I would consider this behavior being a bug. Correct.
Created attachment 250537 [details] patch
Created attachment 250538 [details] Video that shows fixes Most of the errors are fixed. Only breaking multiple styles within comments into multiple comments is missing.
Comment on attachment 250537 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=250537&action=review I'll let Joe look at this for actual substance. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:790 > + } > + > _resetContent() Nit: Extra newline. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:853 > + }); > + return; Nit: Newline. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:859 > + // Now the Formatter pretty prints the styles. > + editor.setValue(this._formattedContentFromEditor()); I'd put a newline before the comment. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:896 > + this._markLinesWithCheckboxPlaceholder(); > + > } Nit: Extra newline. > Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:34 > + return false; > + }, Nit: Newline. > Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:312 > + }, > shouldHaveSpaceBeforeToken: function(lastToken, lastContent, token, state, content, isComment) Ditto. > Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:439 > + }, > + shouldHaveSpaceBeforeToken: function(lastToken, lastContent, token, state, content, isComment) Ditto.
Created attachment 250590 [details] patch
Created attachment 250617 [details] patch
Comment on attachment 250617 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=250617&action=review You mentioned you had tests for this as well? > Source/WebInspectorUI/UserInterface/Controllers/FormatterContentBuilder.js:189 > + _undo() > { > var removed = this._formattedContent.pop(); > this._formattedContentLength -= removed.length; The next line in this function pops from formattedLineEndings. However you call undo from two places, removeLastNewline and removeLastWhitespace. Seems like this should be: if (removed === "\n") this._formattedLineEndings.pop() Or, perhaps better, move the pop up into removeLastNewline so there is no if check at all. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:829 > + var editor = this._codeMirror; This shouldn't be necessary. We prefer using the member variable directly instead of storing into a local. This code is not particularly hot so performance shouldn't be a concern. Likewise this is a common enough idiom that I would imagine the engine optimizes it efficiently. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:881 > + var to = {line: lineNumber}; Nice, you realized dropping the "ch" would get you to the end of the line =) > Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:439 > + var isLastComment = /\bcomment\b/.test(lastToken); > + var isLastSemicolon = lastContent === ";" && lastToken === null; > + var isSemicolon = content === ";" && token === null; > + return isComment || isLastComment || isLastSemicolon || isSemicolon; I'd prefer these be early returns to avoid unnecessary work. In pretty printing these functions are called very frequently. > Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:472 > + var isRegularProperty = /\bproperty\b/.test(token) && !(/\bmeta\b/.test(lastToken)); > + var isPrefixedProperty = /\bmeta\b/.test(token) && lastContent !== ":"; > + var isInvalidAfterComment = /\batom\b/.test(token) && /\bcomment\b/.test(lastToken); > + return isComment || isPrefixedProperty || isRegularProperty || isInvalidAfterComment; For example in this each check is for a distinct type of token. If you are one of those types, you shouldn't need to run the others. Likewise it gives an opportunity to provide comments describing the case. I don't actually know what "isPrefixedProperty" means.
Comment on attachment 250617 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=250617&action=review >> Source/WebInspectorUI/UserInterface/Controllers/FormatterContentBuilder.js:189 >> this._formattedContentLength -= removed.length; > > The next line in this function pops from formattedLineEndings. However you call undo from two places, removeLastNewline and removeLastWhitespace. > > Seems like this should be: > > if (removed === "\n") > this._formattedLineEndings.pop() > > Or, perhaps better, move the pop up into removeLastNewline so there is no if check at all. Good catch! >> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:829 >> + var editor = this._codeMirror; > > This shouldn't be necessary. We prefer using the member variable directly instead of storing into a local. This code is not particularly hot so performance shouldn't be a concern. Likewise this is a common enough idiom that I would imagine the engine optimizes it efficiently. Okay >> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:881 >> + var to = {line: lineNumber}; > > Nice, you realized dropping the "ch" would get you to the end of the line =) I thought it's a bug. How can this be considered as convenient API? :D >> Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:439 >> + return isComment || isLastComment || isLastSemicolon || isSemicolon; > > I'd prefer these be early returns to avoid unnecessary work. In pretty printing these functions are called very frequently. Done. >> Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:472 >> + return isComment || isPrefixedProperty || isRegularProperty || isInvalidAfterComment; > > For example in this each check is for a distinct type of token. If you are one of those types, you shouldn't need to run the others. > > Likewise it gives an opportunity to provide comments describing the case. I don't actually know what "isPrefixedProperty" means. Done.
Created attachment 250785 [details] patch
Comment on attachment 250785 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=250785&action=review > Source/WebInspectorUI/Tools/PrettyPrinting/css-rule-tests/add-whitespace-after-colon-expected.css:1 > +display: none; Suggestion: Let's make sure that we don't need to create a file for every test case in the new testing environment. I'd prefer the style `assert.equal('display:none;', 'display: none;', 'Add whitespace after colon')`. https://nodejs.org/api/assert.html#assert_assert_equal_actual_expected_message
Comment on attachment 250785 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=250785&action=review Looks good. I tried it out and caught one issue. I think this is a step in the right direction, I'd like to see another patch addressing these comments. > Source/WebInspectorUI/ChangeLog:16 > + Result of running `ruby Source/WebInspectorUI/Scripts/update-pretty-printer.rb Tools`. A more generic comment might make sense in the ChangeLog. Updated shared sources in the PrettyPrinting tool. > Source/WebInspectorUI/ChangeLog:19 > + Align Test setup to be able to run "css-rule" Formatter tests. "Align" may not be the best word here. Maybe "Enable" would be better? >> Source/WebInspectorUI/Tools/PrettyPrinting/css-rule-tests/add-whitespace-after-colon-expected.css:1 >> +display: none; > > Suggestion: Let's make sure that we don't need to create a file for every test case in the new testing environment. I'd prefer the style `assert.equal('display:none;', 'display: none;', 'Add whitespace after colon')`. > https://nodejs.org/api/assert.html#assert_assert_equal_actual_expected_message Yeah, that sounds reasonable. > Source/WebInspectorUI/Tools/PrettyPrinting/index.html:13 > <script src="css.js"></script> > + <script> > + CodeMirror.defineMode("css-rule", CodeMirror.modes.css); > + CodeMirror.extendMode("css-rule", { > + token: function extendedCSSToken(stream, state) To prevent duplication we should be able to just include a file to get this. Even if it means moving the original out of CodeMirrorAdditions.js and into a new file like CodeMirrorModeAdditions.js and including that from here. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:782 > + var builder = new FormatterContentBuilder(mapping, [], [], 0, 0, " "); I'd prefer if we broke that space out with the comment: // <rdar://problem/10593948> Provide a way to change the tab width in the Web Inspector var indentString = " "; That will make it very easy for us to find all the places we do this once we add the setting! > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:833 > + var findWhitespace = /\s/g; I wonder if /\s+/g could be more efficient. Consider: str = " test " If you do /\s/g, that will match 4 times. If you do /\s+/g that will match 2 times. Probably doesn't matter much. Feel free to benchmark. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:868 > + var cssPropertiesMap = Object.create(null); We have been actively moving toward actually using Maps. We should move this to: var cssPropertiesMap = new Map; ... > Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:31 > + removeLastWhitespace: function(lastToken, lastContent, token, state, content, isComment) Style: It would be nicer to have this next to removeLastNewline instead of here at the top. > Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:454 > + // Remove whitespace before semicolon. Like `prop: value ;`. > + if (content === ";" && token === null) > + return true; > + > + // Remove whitespace before colon. Like `prop : value;`. > + if (content === ":" && token === null) > + return true; Here is another case, you can combine an if: if (token === null) return content === ";" || content === ":"; Also, for style, I think we have been doing "if (!token)" for these cases instead of explicit comparison to null. > Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:467 > + return lastContent === ":" && lastToken === null; > + }, > + > + shouldHaveSpaceAfterLastToken: function(lastToken, lastContent, token, state, content, isComment) > + { > + return lastContent === "," && lastToken === null; > + }, Style: "lastToken === null" => "!lastToken" > Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:500 > + // Add new line before a regular property like `display`. > + if (/\bproperty\b/.test(token) && !(/\bmeta\b/.test(lastToken))) > + return true; > + > + // Add new line before a prefixed property like `-webkit-animation`. > + if (/\bmeta\b/.test(token) && lastContent !== ":") > + return true; > + > + // Add new line after comment > + if (/\bcomment\b/.test(lastToken)) > + return true; > + > + // Add new line before comments. > + if (isComment) > + return true; I would expect these to be of the form: if (/\bproperty\b/.test(token)) return !(/\bmeta\b/.test(lastToken) So that once a token is matched once, it doesn't need to drop to all of the other cases that do not matter. ---- I'd prefer the isComment up at the top, as it is a cheap check that might prevent unnecessary regex work. ---- The second case adds a newline before any "meta" prefixed thing. It may not be a property, it may be part of the value. For example: border: solid blue -webkit-calc(1px); Would incorrectly put a newline before -webkit-calc. Instead of `lastContent !== ":"` you should check the parser's state. I think here you can check if the state is `prop` (state.state === "prop") to know where you are.
Created attachment 251898 [details] patch
Created attachment 252045 [details] patch
> "Align" may not be the best word here. Maybe "Enable" would be better? Done. > To prevent duplication we should be able to just include a file to get this. Even if it means moving the original out of CodeMirrorAdditions.js and into a new file like CodeMirrorModeAdditions.js and including that from here. Good point. It turned out that requiring WebInspector.js was sufficient. > var indentString = " "; Done. > I wonder if /\s+/g could be more efficient. Done. Although multiple spaces should't exist at that point. > var cssPropertiesMap = new Map; Done. Didn't know that. Great! > Here is another case, you can combine an if: Done. > if (!token) Done. > So that once a token is matched once, it doesn't need to drop to all of the other cases that do not matter. Done. > I'd prefer the isComment up at the top, as it is a cheap check that might prevent unnecessary regex work. Done. > border: solid blue -webkit-calc(1px); Good catch! I fixed it and added a test case accordingly.
Comment on attachment 252045 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=252045&action=review r=me > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:783 > + var indentString = " "; We should have that FIXME comment here. // FIXME: <rdar://problem/10593948> Provide a way to change the tab width in the Web Inspector So we don't miss this later.
Created attachment 252638 [details] patch
> We should have that FIXME comment here. > // FIXME: <rdar://problem/10593948> Provide a way to change the tab width in the Web Inspector > So we don't miss this later. Done. Sry, I totally missed that I should add that as a comment.
Comment on attachment 252638 [details] patch Clearing flags on attachment: 252638 Committed r184000: <http://trac.webkit.org/changeset/184000>
All reviewed patches have been landed. Closing bug.
This caused a regression: https://bugs.webkit.org/show_bug.cgi?id=145679