Unminifying CSS lines starting by a comment.

RESOLVED FIXED in Firefox 68

Status

defect
P3
normal
RESOLVED FIXED
5 years ago
3 months ago

People

(Reporter: karlcow, Assigned: daisuke)

Tracking

(Blocks 1 bug)

unspecified
Firefox 68
x86
macOS
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Reporter

Description

5 years ago
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.

Updated

4 years ago
See Also: → 1182044
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.
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P3

Updated

Last year
Product: Firefox → DevTools
Blocks: 1493094
Posted file prettify-css.html (obsolete) —
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.
Flags: needinfo?(ttromey)

Comment 4

6 months ago
(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.
Flags: needinfo?(ttromey)

Comment 5

6 months ago
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.
Posted file 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.
Attachment #9029768 - Attachment is obsolete: true
Assignee

Comment 8

3 months ago

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!

Flags: needinfo?(pbrosset)

Comment 9

3 months ago

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.

Assignee

Comment 10

3 months ago

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.

Assignee: nobody → dakatsuka
Status: NEW → ASSIGNED

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.

Flags: needinfo?(pbrosset)
Assignee

Comment 12

3 months ago

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!

Assignee

Comment 13

3 months ago

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!

Flags: needinfo?(tom)

(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.

Flags: needinfo?(tom)
Assignee

Comment 15

3 months ago

(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!

Attachment #9052162 - Attachment description: Bug 1114929: Apply line number of comments. r?pbro → Bug 1114929: Apply line count of comments. r?pbro
Attachment #9051623 - Attachment is obsolete: true

Comment 18

3 months ago
Pushed by dakatsuka@mozilla.com:
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

3 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
You need to log in before you can comment on or make changes to this bug.