1114929 - Unminifying CSS lines starting by a comment.

Status: RESOLVED FIXED

(DevTools :: Style Editor, defect, P3)

Description

[Karl Dubost💡 :karlcow]

1. Go to http://weather.yahoo.com/
2. Inspect Element 
3. Open the style Editor
4. Some of the combo "files" are not being unminified.

It seems that all lines starting with

/* some comments */ .something {…}

are not being parsed for reflowing them.

Comment 1

[Tom Tromey :tromey]

The prettifier uses a heuristic to decide what to do:

https://dxr.mozilla.org/mozilla-central/source/devtools/shared/styleinspector/css-logic.js#984

The combo file I saw not being prettified consisted of a number of comment blocks
with minified CSS, like:

/*
 * multi-line
 * comment
 * here
 */
minified { css; ... }

This fools the heuristic.


While I'm not totally convinced this heuristic is valid, it seems to me that this is
another situation where a prettify button would help, see bug 1155358.

Comment 2

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 3

[Patrick Brosset <:pbro>]

Attachment: prettify-css.html

I'm trying to construct a test case that will show the issue, because the problem does not seem to occur anymore on the URL provided in comment 0.
However I can't find a way to trick the prettifier into thinking a stylesheet is already pretty when it's not.
This is my latest attempt.

Tom: do you think it's possible we fixed this "by accident" or are there still ways to reproduce the issue.

Comment 4

[Tom Tromey :tromey]

(In reply to Patrick Brosset <:pbro> from comment #3)

> Tom: do you think it's possible we fixed this "by accident" or are there
> still ways to reproduce the issue.

As I recall the sheet in question used to hit the early return here:

https://searchfox.org/mozilla-central/rev/adec563403271e78d1a057259b3e17fe557dfd91/devtools/shared/inspector/css-logic.js#192-196

In particular, it had multiple lines of text due to a comment, but really just one long line for all the rules.
So, while it passed that heuristic, the result was not very good for users.

I still tend to think the style inspector should follow the debugger and require explicit action
to prettify.

Comment 5

[Tom Tromey :tromey]

I suspect the attached document just needs a few more newlines in the intro comment.
By my count it has 9 rules and 7 lines, but would need 9 lines to take the early return.

Comment 6

[Patrick Brosset <:pbro>]

Attachment: prettify-css.html

(In reply to Tom Tromey :tromey from comment #4)
> https://searchfox.org/mozilla-central/rev/
> adec563403271e78d1a057259b3e17fe557dfd91/devtools/shared/inspector/css-logic.
> js#192-196
Thanks, that's exactly what I needed. You're right, my test case just needed more lines than rules. And now I can see the problem.

Comment 7

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1114929: Bug 1114929: Minimize CSS text before prettifying. r?pbro

Comment 8

[Daisuke Akatsuka (:daisuke)]

Hi Patrick!
I have made a prototype, couldn't you give your feedback?
If this approach is not bad, can we change the our tests as well? Or should we keep the spec?

Thanks!

Comment 9

[Tom Tromey :tromey]

It seems to me that this patch lexes the input CSS twice.
That is going to be somewhat expensive.

Maybe a better heuristic here would be to only count newlines
that aren't immediately before or after a comment. That could
be done without lexing twice.

Comment 10

[Daisuke Akatsuka (:daisuke)]

Thank you very much for the feedback, Tom!
Yes, I agree this is expensive, so I'll do without the rexer.

The reason why I had choosen this approach is that I found similar issues and want to fix at same time.
1 . For example, when we see the below example in StyleEditor, the CSS properties will not be prettified.

#information_adchoices_link{
float:left;padding-right:5px;color:#000;display:none}

However, in case of below, we can see prettified contents.

#information_adchoices_link{float:left;padding-right:5px;color:#000;display:none}

2 . Also, below example as well will not be prettified.

/*
 * comment
 */#information_adchoices_link{float:left;padding-right:5px;color:#000;display:none}

I thought the expected result is like below:

/*
 * comment
 */
#information_adchoices_link{
  float:left;
  padding-right:5px;
  color:#000;
  display:none;
}

Thus, before prettifying, I minimized CSS properties part and added a line after comment.

So, at first, I'll fix this comments issue with your suggestion. Then want to fix another issue like above in other patches if needed.

Comment 11

[Patrick Brosset <:pbro>]

I wonder if we need a heuristic at all. Would it be a problem if we just always beautified stylesheets?
It would, of course, if they were already formatted nicely. The formatting would change a little, and the stylesheet would end up looking a tiny bit different from the author wrote, and that might be odd.
The initial patch does that I think, right? It doesn't care about checking if the stylesheet is already formatted or not, it minifies it, and then goes from there to re-beautify it.

Of course, we could do this as Tom suggested in comment 4 and only beautify when the user requests it. So by default we wouldn't do anything, and would just wait for the user to press a button to beautify the code.
Now, this would have other consequences. Mostly benefits actually: for instance when linking CSS warnings from the console to the style-editor, the auto-beautifying messes things up as we can't find the right line/column since those changed.
But it would have an obvious user impact that might be perceived negatively too. Suddenly stylesheets aren't easy to scan anymore and one more action is required.
I know that, at least, this would make it harder for the webcompat team to do their jobs.

I think this is, ultimately, a product call. So in the meantime, it'd be good to investigate if we can, as Tom proposed, just address this bug, without extra processing cost, and without big user impacts.
If not, then let's take it to product to take a decision on whether we want to always auto-beautify, or only do it on a user action.

Comment 12

[Daisuke Akatsuka (:daisuke)]

Okay, in this time, I think I can fix only an issue of comment which has multiple lines without big effects.
Thank you very much for your direction!

Comment 13

[Daisuke Akatsuka (:daisuke)]

Hello Tom, thank you for your advice!
I will ready a first patch according to your proposal tomorrow.
So, if possible, I want to you to take this review. May I request to review to you?
Thanks!

Comment 14

[Tom Tromey :tromey]

(In reply to Daisuke Akatsuka (:daisuke) from comment #10)

1 . For example, when we see the below example in StyleEditor, the CSS properties will not be prettified.

#information_adchoices_link{
float:left;padding-right:5px;color:#000;display:none}

Yeah. The heuristic approach will always have some limitation.
I tend to think that this is fine, provided that it does the right
thing for the output of the most common minifiers. E.g., if you
wrote this example by hand, then I would say there's no real need
to handle it; but if this is the actual output of a minifier, then
an extension to the heuristic would make sense.

What do you think of that?

You should probably request review from someone else, I doubt I can
push to try any more and I'm really just doing some drive-by comments.
That said, I'm happy to look at the patches of course.

Comment 15

[Daisuke Akatsuka (:daisuke)]

(In reply to Tom Tromey :tromey from comment #14)

(In reply to Daisuke Akatsuka (:daisuke) from comment #10)

1 . For example, when we see the below example in StyleEditor, the CSS properties will not be prettified.

#information_adchoices_link{
float:left;padding-right:5px;color:#000;display:none}

Yeah. The heuristic approach will always have some limitation.
I tend to think that this is fine, provided that it does the right
thing for the output of the most common minifiers. E.g., if you
wrote this example by hand, then I would say there's no real need
to handle it; but if this is the actual output of a minifier, then
an extension to the heuristic would make sense.

Thank you for replying!
I understood the thinking. For me, I'm a bit concerned about the consistency of whether we apply prettifying or not by depending on the CSS. Thus, as you and Patrick said, I think that it makes sense that at first CSS displays as it is, then prettify when user acts explicitly. I'd like to discuss more in another bug.

You should probably request review from someone else, I doubt I can
push to try any more and I'm really just doing some drive-by comments.
That said, I'm happy to look at the patches of course.

Okay, understood!

Comment 16

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1114929: Apply line count of comments. r?pbro

Comment 17

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1114929: Add a test for multiline comment. r?pbro

Depends on D24118

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c404dce8474
Apply line count of comments. r=pbro
https://hg.mozilla.org/integration/autoland/rev/168ced563105
Add a test for multiline comment. r=pbro

Comment 19

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/9c404dce8474
https://hg.mozilla.org/mozilla-central/rev/168ced563105