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)

VERIFIED FIXED in Firefox 51

Status

()

Firefox
Developer Tools: CSS Rules Inspector
P1
normal
VERIFIED FIXED
8 months ago
3 months ago

People

(Reporter: dholbert, Assigned: tromey)

Tracking

unspecified
Firefox 51
Points:
---

Firefox Tracking Flags

(firefox51 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

8 months ago
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.
(Reporter)

Comment 1

8 months ago
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.
(Reporter)

Updated

8 months ago
Summary: DevTools CSS Rules inspector incorrectly collapses list-valued properties whose list entries are only separated by CSS comments → 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)
(Reporter)

Updated

8 months ago
Summary: 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 whose list entries are only separated by CSS comments (sometimes changing valid values into invalid ones)
P1 because the code is valid and we show a bogus output.

Tom: do you mind giving pointers about how this can be solved?
Flags: needinfo?(ttromey)
Priority: -- → P1
(Assignee)

Comment 3

8 months ago
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.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Flags: needinfo?(ttromey)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

8 months ago
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.
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

8 months ago
mozreview-review
Comment on attachment 8785963 [details]
Bug 1297890 - don't allow token pasting in parseDeclarations;

https://reviewboard.mozilla.org/r/74972/#review73128
Attachment #8785963 - Flags: review?(pbrosset) → review+

Comment 8

8 months ago
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/18f8a36a4ed2
don't allow token pasting in parseDeclarations; r=pbro
https://hg.mozilla.org/mozilla-central/rev/18f8a36a4ed2
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
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

4 months ago
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]
Thanks guys for verifying this bug.
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
You need to log in before you can comment on or make changes to this bug.