146178 – REGRESSION(r184000): Web Inspector: Multiline CSS in Styles Sidebar is marked as invalid

RESOLVED FIXED 146178

REGRESSION(r184000): Web Inspector: Multiline CSS in Styles Sidebar is marked as invalid

https://bugs.webkit.org/show_bug.cgi?id=146178

Summary REGRESSION(r184000): Web Inspector: Multiline CSS in Styles Sidebar is marked...

Joseph Pecoraro

Reported 2015-06-19 19:03:32 PDT

* 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)

Attachments
Expected result (18.82 KB, image/png) 2015-06-25 14:42 PDT, Tobias Reiss	no flags	Details
Patch (3.55 KB, patch) 2015-06-25 15:06 PDT, Tobias Reiss	no flags	Details Formatted Diff Diff
Patch (6.15 KB, patch) 2015-06-25 15:53 PDT, Tobias Reiss	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (732.92 KB, application/zip) 2015-06-25 17:02 PDT, Build Bot	no flags	Details
Patch: Remove css strikethrough on multi-line properties. (1.91 KB, patch) 2015-06-25 19:36 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Before r184000 (1.01 MB, video/quicktime) 2015-06-26 00:37 PDT, Tobias Reiss	no flags	Details
Fixed the issue (5.90 KB, patch) 2015-06-26 12:08 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Added suggestions to fix (4.53 KB, patch) 2015-06-29 11:28 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (7.37 KB, patch) 2015-07-04 09:58 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2015-06-19 19:03:46 PDT

<rdar://problem/21472245>

Joseph Pecoraro

Comment 2 2015-06-19 19:09:37 PDT

Hmm, seems to be another regression from r184000. Seems not all newlines can be considered the end of the line.

Tobias Reiss

Comment 3 2015-06-25 14:42:05 PDT

Created attachment 255573 [details] Expected result

Tobias Reiss

Comment 4 2015-06-25 15:06:41 PDT

Created attachment 255578 [details] Patch

Joseph Pecoraro

Comment 5 2015-06-25 15:20:04 PDT

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/.

Joseph Pecoraro

Comment 6 2015-06-25 15:21:06 PDT

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?

Tobias Reiss

Comment 7 2015-06-25 15:53:12 PDT

Created attachment 255584 [details] Patch

Build Bot

Comment 8 2015-06-25 17:02:15 PDT

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

Build Bot

Comment 9 2015-06-25 17:02:20 PDT

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

Devin Rousso

Comment 10 2015-06-25 19:32:35 PDT

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.

Devin Rousso

Comment 11 2015-06-25 19:36:39 PDT

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.

Tobias Reiss

Comment 12 2015-06-26 00:28:57 PDT

> 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).

Tobias Reiss

Comment 13 2015-06-26 00:37:01 PDT

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.

Tobias Reiss

Comment 14 2015-06-26 00:50:12 PDT

@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!

Devin Rousso

Comment 15 2015-06-26 10:22:08 PDT

(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.

Devin Rousso

Comment 16 2015-06-26 12:08:37 PDT

Created attachment 255656 [details] Fixed the issue

Timothy Hatcher

Comment 17 2015-06-26 13:28:23 PDT

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)".

Timothy Hatcher

Comment 18 2015-06-26 13:34:51 PDT

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.

Joseph Pecoraro

Comment 19 2015-06-26 14:07:36 PDT

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.

Devin Rousso

Comment 20 2015-06-29 11:28:08 PDT

Created attachment 255763 [details] Added suggestions to fix

Timothy Hatcher

Comment 21 2015-07-04 07:50:50 PDT

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.

Devin Rousso

Comment 22 2015-07-04 09:58:40 PDT

Created attachment 256149 [details] Patch

WebKit Commit Bot

Comment 23 2015-07-04 14:42:28 PDT

Comment on attachment 256149 [details] Patch Clearing flags on attachment: 256149 Committed r186281: <http://trac.webkit.org/changeset/186281>

WebKit Commit Bot

Comment 24 2015-07-04 14:42:33 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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware All

OS All

Product WebKit

Component Web Inspector

Assignee

Nobody

Reported

2015-06-19 19:03 PDT

Modified

2015-07-04 14:42 PDT History

CC List

12 users Show

URL

Keywords InRadar

Depends on

Blocks