Last Comment Bug 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)
: DevTools CSS Rules Inspector incorrectly collapses list-valued properties who...
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: CSS Rules Inspector (show other bugs)
: unspecified
: Unspecified Unspecified
P1 normal (vote)
: Firefox 51
Assigned To: Tom Tromey :tromey
:
: Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-08-24 16:23 PDT by Daniel Holbert [:dholbert]
Modified: 2017-01-09 02:03 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
testcase 1 (346 bytes, text/html)
2016-08-24 16:23 PDT, Daniel Holbert [:dholbert]
no flags Details
screenshot #1 (107.89 KB, image/png)
2016-08-24 16:40 PDT, Daniel Holbert [:dholbert]
no flags Details
Bug 1297890 - don't allow token pasting in parseDeclarations; (58 bytes, text/x-review-board-request)
2016-08-29 07:44 PDT, Tom Tromey :tromey
pbrosset: review+
Details | Review

Description User image Daniel Holbert [:dholbert] 2016-08-24 16:23:03 PDT
Created attachment 8784607 [details]
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 User image Daniel Holbert [:dholbert] 2016-08-24 16:40:30 PDT
Created attachment 8784614 [details]
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 User image Patrick Brosset <:pbro> (PTO, back on Feb 27th) 2016-08-26 08:15:05 PDT
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 User image Tom Tromey :tromey 2016-08-29 07:37:36 PDT
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 User image Tom Tromey :tromey 2016-08-29 07:44:28 PDT Comment hidden (mozreview-request)
Comment 5 User image Tom Tromey :tromey 2016-08-29 07:50:35 PDT
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 User image Patrick Brosset <:pbro> (PTO, back on Feb 27th) 2016-08-30 05:35:34 PDT
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 User image Patrick Brosset <:pbro> (PTO, back on Feb 27th) 2016-08-30 05:36:14 PDT
Comment on attachment 8785963 [details]
Bug 1297890 - don't allow token pasting in parseDeclarations;

https://reviewboard.mozilla.org/r/74972/#review73128
Comment 8 User image Pulsebot 2016-08-30 10:20:09 PDT
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/18f8a36a4ed2
don't allow token pasting in parseDeclarations; r=pbro
Comment 9 User image Ryan VanderMeulen [:RyanVM] 2016-08-31 06:57:32 PDT
https://hg.mozilla.org/mozilla-central/rev/18f8a36a4ed2
Comment 10 User image Md.Tarikul Islam Oashi 2017-01-06 07:47:18 PST
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 User image Md.Majedul isalm 2017-01-07 07:00:47 PST
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 User image Bogdan Maris, QA [:bogdan_maris] 2017-01-09 02:03:34 PST
Thanks guys for verifying this bug.

Note You need to log in before you can comment on or make changes to this bug.