281645 - #sidebar-box line in pinstripe's sidebar.css is stripped by the preprocessor

Status: RESOLVED FIXED

(SeaMonkey :: Help Viewer, defect)

Description

[Hiro]

In sidebar.css of help.jar, there is no line of URL. 
Therefore, the following error messages come out in JavaScript Console when the
debug option is made effective and help is opened. 

JavaScript Console message:
Error: Expected identifier for pseudo-class or pseudo-element but found ' '. 
Ruleset ignored due to bad selector.
Source File: chrome://help/skin/sidebar.css
Line: 5

Perhaps, the following definitions might have been treated as a comment. 

#sidebar-box {


If this is correct, the same problem might be brought up also in other places. 

Mac OS X 10.3.7
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b) Gecko/20050209
Firefox/1.0+

Comment 1

[Steffen Wilberg]

The winstripe version of the file uses a space before #sidebar-box:
http://lxr.mozilla.org/seamonkey/source/toolkit/themes/winstripe/help/sidebar.css#46

But preprocessing css files is just a bad idea.

Comment 2

[Anne (:annevk)]

Would removing the space fix the problem?

Comment 3

[Steffen Wilberg]

No, you'd need to add a space in the pinstripe version of the file, so that the
preprocessor doesn't treats that line as a comment. Just have a look at the
license: the preprocessor removes all the lines starting with a "#".

The winstripe file has got that space, so there's no problem on Win/Lin.

But since css files may make use of the # sign, the real solution is to stop
running css files through the preprocessor. You need to remove the "*" sign in
the respective jar.mn
(http://lxr.mozilla.org/mozilla/source/toolkit/themes/pinstripe/help/jar.mn),
and convert the #-style comments to /* */ style.

Comment 4

[Anne (:annevk)]

Attachment: patch #1

Something like this? Who needs to review this?

Comment 5

[Steffen Wilberg]

Exactly! Request review from a toolkit peer:
http://www.mozilla.org/owners.html#toolkit

Comment 6

[Mike Connor [:mconnor]]

Comment on attachment 174842 [details] [diff] [review]
patch #1

I'd rather have one extra space than carry around four extra license headers in
xulrunner chrome.  Proportion is key here.

Comment 7

[Anne (:annevk)]

Actually, one of those files is already loaded including the license (help.css).
Should that change? I also wonder why there is a different policy for CSS files
here. (html.css, quirk.css, forms.css etc. all have this included normally.)

Comment 8

[Mike Connor [:mconnor]]

those aren't in toolkit, and only browser/toolkit/mail were using the
preprocessor for a long time.

Changing over existing files is a tricky issue, but we're more likely to convert
towards stripping than away from it.

Comment 9

[Anne (:annevk)]

Attachment: patch #2

Here is a patch that does what you want and fixes this bug.

It would be nice though if some consistency was used, since not all the files
are using this kind of syntax in toolkit/themes. Shall I file a separate bug on
that? (If so, who needs to be cc'ed?)

Comment 10

[Mike Connor [:mconnor]]

Comment on attachment 174993 [details] [diff] [review]
patch #2

well, like I said, its a tricky question.  File the bug, cc me, and we'll fix
it at some point.  We really need to make a stricter rule as opposed to a
general practice.

Comment 11

[Anne (:annevk)]

Filed bug 283047. By the way, could someone check this in? I only have a CVS
account for mozilla.org.

Comment 12

[Mike Connor [:mconnor]]

Comment on attachment 174993 [details] [diff] [review]
patch #2

/toolkit doesn't need SR

Comment 13

[Josh Aas]

checked in on trunk