Bug 146178

Summary:

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

Product:

WebKit

Reporter:

Joseph Pecoraro <joepeck>

Component:

Web Inspector

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, commit-queue, graouts, hi, joepeck, jonowells, mattbaker, nvasilyev, rniwa, timothy, tobi+webkit, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Expected result	none
Patch	none
Patch	none
Archive of layout-test-results from ews104 for mac-mavericks-wk2	none
Patch: Remove css strikethrough on multi-line properties.	none
Before r184000	none
Fixed the issue	none
Added suggestions to fix	none
Patch	none

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.