1297890 - DevTools CSS Rules Inspector incorrectly collapses list-valued properties whose list entries are only separated by CSS comments (sometimes changing valid values into invalid ones)

Status: VERIFIED FIXED

(DevTools :: Inspector: Rules, defect, P1)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR:
 1. Load attached testcase.
 2. Inspect any of the three lines of text, in the CSS "Rules" view.
 3. Compare to the "computed style" view to see what actually took effect.

ACTUAL RESULTS:
- The "rules" view shows the original author-provided styles, with the comments removed, which converts e.g. 1/*ThisIsAComment*/2; into "12", and "5px/**/0/**/0/**/20px/**/;" into "5px0020px".
- It also strikes out the last one ("5px0020px") as being invalid CSS, despite the fact that the actual declaration (with CSS comments) *does* parse successfully and does get honored, as you can see in the rendering & the computed-style view.

EXPECTED RESULTS:
- The "rules" view should preserve CSS comments inside of a CSS property's value.  Or, it should collapse them away to "/**/", if needed. Or replace them with a blank space -- that would be fine with these properties, at least, though I'm not sure if it's fine in general.


For comparison, Chrome preserves the comments in their entirety, in its equivalent of our DevTools "CSS Rules" view.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: screenshot #1

For illustration, here's a screenshot showing the 3rd part of the testcase -- in particular, it shows:

 - The source code (upper right)
 - DevTools' comments-stripped-out version of the "margin" declaration (lower right), struck out to indicate that it's invalid CSS.
 - The actual rendering (upper left), with the margin clearly being honored on "Me too", despite the strikethrough in the rules view.

Comment 2

[Patrick Brosset <:pbro>]

P1 because the code is valid and we show a bogus output.

Tom: do you mind giving pointers about how this can be solved?

Comment 3

[Tom Tromey :tromey]

There are two separate bugs here.

One is the token pasting.  I'll attach a patch for this momentarily.

The other bug is that the comments aren't preserved in the rule view.
This is fixable but on irc :pbro said he'd rather we not.

Comment 4

[Tom Tromey :tromey]

Attachment: Bug 1297890 - don't allow token pasting in parseDeclarations;

Review commit: https://reviewboard.mozilla.org/r/74972/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74972/

Comment 5

[Tom Tromey :tromey]

BTW if you want to try comment-preserving, the spot touched by this
patch needs to change, but output-parser also needs to change to not
drop comments.

Comment 6

[Patrick Brosset <:pbro>]

Thanks a lot Tom for your prompt analysis and patch. I think this is a rather urgent bug because it causes the rule-view to show incorrect data (which is why I had triaged it as a P1), so I lean towards fixing it the way you did first, and then revisiting this decision later if we think there's a user need to show comments in the rule-view.

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8785963 [details]
Bug 1297890 - don't allow token pasting in parseDeclarations;

https://reviewboard.mozilla.org/r/74972/#review73128

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/18f8a36a4ed2
don't allow token pasting in parseDeclarations; r=pbro

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/18f8a36a4ed2

Comment 10

[Md.Tarikul Islam Oashi]

I have to reproduced this bug with Nightly 51.0a1 (2016-08-24) on Windows 7 64 bit;

The Bug's fix is verified on beta

Build ID 	20170105155013
User Agent 	Mozilla/5.0 (Windows NT 6.1; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0

[Testday-20170106]

Comment 11

[Md. Majedul islam]

I have to reproduced this bug with Nightly 51.0a1 (2016-08-24) on Ubuntu 16.10, 64 bit!

The Bug's fix is verified on beta.

Build ID 	20170105155013
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0

[Testday-20170106]

Comment 12

[Bogdan Maris, Desktop Test Engineering]

Thanks guys for verifying this bug.