* SUMMARY Multiline CSS in Styles Sidebar is marked as invalid * STEPS TO REPRODUCE 1. Inspect <body> on about:blank 2. Paste into the style attribute section: background: repeating-radial-gradient( circle, purple, purple 10px, #4b026f 10px, #4b026f 20px ); 3. Click outside the editor => editor reformats and marks style as invalid (but the page does update)
<rdar://problem/21472245>
Hmm, seems to be another regression from r184000. Seems not all newlines can be considered the end of the line.
Created attachment 255573 [details] Expected result
Created attachment 255578 [details] Patch
Comment on attachment 255578 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255578&action=review > Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:482 > removeLastNewline: function(lastToken, lastContent, token, state, content, isComment, firstTokenOnLine) > { > + // Remove new line before keywords like "purple" and atoms like "circle" or hex-colors > + // background: repeating-radial-gradient( > + // purple > + // ); > + if (firstTokenOnLine) > + return content === ")" || (/\bkeyword|atom\b/.test(token)); > + > return false; > }, We would want to make sure that this covers any newlines / unnecessary whitespace. For example: border : 1px solid black; color: red; Should format to: border: 1px solid black; color: red; Lets test these. I feel like these cases are a bit limited. Maybe the keyword|atom is enough? Also, /\b(?:keyword|atom)\b/ otherwise we are just checking /\bkeyword/ or /atom\b/.
Comment on attachment 255578 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255578&action=review >> Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:482 >> }, > > We would want to make sure that this covers any newlines / unnecessary whitespace. For example: > > border > : > 1px > solid > black; > color: red; > > Should format to: > > border: 1px solid black; > color: red; > > Lets test these. I feel like these cases are a bit limited. Maybe the keyword|atom is enough? > > Also, /\b(?:keyword|atom)\b/ otherwise we are just checking /\bkeyword/ or /atom\b/. Thinking about this, basically we may want to be removing newlines while inside a rule and inserting newlines after declarations within a rule?
Created attachment 255584 [details] Patch
Comment on attachment 255584 [details] Patch Attachment 255584 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4681773665484800 New failing tests: http/tests/security/storage-blocking-strengthened-plugin.html http/tests/security/storage-blocking-loosened-plugin.html
Created attachment 255598 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 255584 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255584&action=review > Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:442 > + if (/\bkeyword|atom|number\b/.test(lastToken) && /\bkeyword|atom|number\b/.test(token)) Shouldn't this be: /\b(keyword|atom|number)\b/ Otherwise, it's matching \bkeyword OR atom OR number\b, which isn't what I think you want. > Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:488 > + Same as above: /\b(keyword|atom)\b/ Also, you have unnecessary parenthesis around the second argument.
Created attachment 255611 [details] Patch: Remove css strikethrough on multi-line properties. Even if the multi-line CSS is properly formatted, just in case of weird situations we shouldn't apply the CSS strikethrough to multi-line CSS.
> Thinking about this, basically we may want to be removing newlines while inside a rule and inserting newlines after declarations within a rule? @Joe: I agree and that would simplify the condition! In case there are other edge cases (e.g.: comments, invalid rules) you should see an existing test that would verify your result. > /\b(?:keyword|atom)\b/ @Joe, @drousso: Good catch and shame on me! non-capturing-group makes sense as I do not capture anything. > Even if the multi-line CSS is properly formatted, just in case of weird situations we shouldn't apply the CSS strikethrough to multi-line CSS. @drousso: Nice idea but I personally wouldn't do that because it hides the error but doesn't fix the original problem. Internally the Rule is still invalid (you cannot comment and un-comment the Rule using the checkbox for example).
Created attachment 255620 [details] Before r184000 > Hmm, seems to be another regression from r184000. Seems not all newlines can be considered the end of the line. @Joe: Name it regression, but actually the Rule was invalid before r184000. It just wasn't visible because we applied the wrong Formatter (CSS instead of CSSRule). Now that we have a proper Formatter the bug became visible.
@drousso Do you think you could take that bug? I just don't have the time to fix that thoroughly. That would be helpful, thanks!
(In reply to comment #14) > @drousso Do you think you could take that bug? I just don't have the time to > fix that thoroughly. That would be helpful, thanks! Sure thing. I'll try to fix it today.
Created attachment 255656 [details] Fixed the issue
Comment on attachment 255656 [details] Fixed the issue View in context: https://bugs.webkit.org/attachment.cgi?id=255656&action=review > Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:442 > + var regexp = /\b(keyword|atom|number)\b/; This should be non-capturing. /\b(?:keyword|atom|number)\b/ Including number here makes be wonder if "rgb(0, 0, 0)" will turn into "rgb( 0, 0, 0)".
Comment on attachment 255656 [details] Fixed the issue View in context: https://bugs.webkit.org/attachment.cgi?id=255656&action=review >> Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:442 >> + var regexp = /\b(keyword|atom|number)\b/; > > This should be non-capturing. /\b(?:keyword|atom|number)\b/ > > Including number here makes be wonder if "rgb(0, 0, 0)" will turn into "rgb( 0, 0, 0)". Might just need a check for lastContent === "(" && !lastToken.
Comment on attachment 255656 [details] Fixed the issue View in context: https://bugs.webkit.org/attachment.cgi?id=255656&action=review > Source/WebInspectorUI/Tools/PrettyPrinting/css-rule-tests/add-whitespace-between-values.css:3 > +border: 1px > + solid > + black; Seems it would be more useful to test this without leading spaces on the subsequent lines. Likewise we should have a test for multiple rules, even multiple rules on the same line > Source/WebInspectorUI/UserInterface/Views/CodeMirrorFormatters.js:483 > + // There should be no extra newlines after css-rules in the sidebar. They should always display on one line. Style: Just a single space after a period. This comment is somewhat misleading and contains superfluous text. How about: // Each property should be formatted to one line each with no extra newlines.
Created attachment 255763 [details] Added suggestions to fix
Comment on attachment 255763 [details] Added suggestions to fix View in context: https://bugs.webkit.org/attachment.cgi?id=255763&action=review > Source/WebInspectorUI/ChangeLog:1 > -2015-06-26 Devin Rousso <drousso@apple.com> > +2015-06-29 Devin Rousso <drousso@apple.com> Does not apply because of this. You will need to upload a new patch.
Created attachment 256149 [details] Patch
Comment on attachment 256149 [details] Patch Clearing flags on attachment: 256149 Committed r186281: <http://trac.webkit.org/changeset/186281>
All reviewed patches have been landed. Closing bug.