254508 - CSS platform classes are not working

Status: RESOLVED FIXED

(SeaMonkey :: Help Viewer, defect)

Description

[u60234]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040805 Firefox
0.9.1+

The hiding of help content with "class="noWin"" and the like, is not working.

Steps to reproduce:
1. Go to Help -> Contents -> Keyboard Shortcuts

Actual result:
The keyboard shortcut for Page Info is listed as "Ctrl+J  I".

Expected result:
On Windows, the keyboard shortcut for Page Info should be listed as "Ctrl+J".

Comment 1

[Jeff Walden (gone starting June 8)]

Attachment: Untested patch to add preprocessed CSS rules

As far as I can tell without actually building (which I don't think I can do
because of my hard drive partitions but will still try), this patch should hide
and display correctly for the following classes:

unix
noUnix
win
noWin
mac
noMac

...by setting display:none; when the proper variable from
XP_UNIX/XP_MACOSX/XP_WIN32 is defined or undefined (nothing is set when the
preprocessed #ifdef or #ifndef isn't met).

Comment 2

[u60234]

Does this not mean that the Help content will depend on the default skin?

Should'nt this classes be defined in content instead?

Comment 3

[Jeff Walden (gone starting June 8)]

(In reply to comment #2)
> Does this not mean that the Help content will depend on the default skin?

No.  The altered file is the one we've always used to store stylistic
information for Help documents, and I haven't noticed any breakage using other
themes.  Additionally, because of the way the file's stored it's accessible at
chrome://help/skin/helpFileLayout.css, and skins can only place content within a
semi-rigid hierarchy within chrome://browser/skin/ as far as I know.  I'm sure
someone else can explain it better than I can, but I'm sure Help won't depend on
Winstripe.

Also, I might as well add rlk to the CC list, because he's probably the best bet
to test this.

Comment 4

[Steffen Wilberg]

XP_UNIX is true on Mac as well, see http://www.mozilla.org/build/mac.html at the
bottom.
You can use e.g.
#ifdef XP_UNIX
#ifdef XP_MACOSX
(Mac-specific code)
#else
(Linux-specific code)
#endif
#endif

Comment 5

[R.J. Keller]

Comment on attachment 155337 [details] [diff] [review]
Untested patch to add preprocessed CSS rules

Jeff, this won't work with themes. Remember that preprocessing is done at
compile-time, not run-time. If someone installs a new theme, it'd goof this up.

What we need is a CSS file in the Content directory and import it to
helpFileLayout.css. I thought I did this, but I guess not (it might've been on
the MozDev tree but never checked in).

This is what we should do:
http://lxr.mozilla.org/mozilla/source/extensions/help/resources/content/platfor
mClasses.css
http://lxr.mozilla.org/mozilla/source/extensions/help/resources/locale/en-US/he
lpFileLayout.css#36

Comment 6

[Jeff Walden (gone starting June 8)]

(In reply to comment #5)
> What we need is a CSS file in the Content directory and import it to
> helpFileLayout.css. I thought I did this, but I guess not (it might've been on
> the MozDev tree but never checked in).

Aside from seeing a difference in method here, I'm not seeing how it makes any
difference how it's done.  That said, if the classes shouldn't be within the
skinning area, why does @import-ing the platforms CSS from the Help layout CSS
skin make any more sense?  From what I understand you're expecting all theme
authors to change their themes by adding a new line to their CSS files. 
Wouldn't simply adding in a new <link rel="stylesheet" type="text/css"
href="chrome://help/content/platformClasses.css"/> in each of the files with
platform-specific information be a better idea?  Then theme authors wouldn't
have to do anything.

In any case I'm safe to upload a copy of platformClasses.css here for addition
to the tree, probably at toolkit/components/help/content/platformClasses.css. 
Actually using the file can be another patch which would need to work either as
initially described or as I just suggested, and I need to know which is more
correct first.

Comment 7

[Jeff Walden (gone starting June 8)]

(In reply to comment #4)
> XP_UNIX is true on Mac as well, see http://www.mozilla.org/build/mac.html at
> the bottom.

I was assuming the "unix" class applied to all unix-ish platforms, hence the
absence of any Mac OS X filtering.  I'll add some to the file to be uploaded next.

Comment 8

[R.J. Keller]

(In reply to comment #6)
> Aside from seeing a difference in method here, I'm not seeing how it makes any
> difference how it's done.  That said, if the classes shouldn't be within the
> skinning area, why does @import-ing the platforms CSS from the Help layout CSS
> skin make any more sense?  From what I understand you're expecting all theme
> authors to change their themes by adding a new line to their CSS files. 
> Wouldn't simply adding in a new <link rel="stylesheet" type="text/css"
> href="chrome://help/content/platformClasses.css"/> in each of the files with
> platform-specific information be a better idea?  Then theme authors wouldn't
> have to do anything.

yes, but it'd be more work. That little line doesn't mean much.

The issue with having it under skins is you would have different skin files on
Unix, Windows, and MacOS. Skinners would have to make 3 different themes, which
is unreasonable. You could do more work to include this in all the help docs,
but I'd just do it this way. Maybe a smarter idea might be to import
helpFileLayout.css to platformClasses.css and then link to platformClasses. You
never know when we might want to add another CSS file.

> 
> In any case I'm safe to upload a copy of platformClasses.css here for addition
> to the tree, probably at toolkit/components/help/content/platformClasses.css. 

That'd be fine.

Comment 9

[Jeff Walden (gone starting June 8)]

Attachment: Updated toolkit/components/help/content/platformClasses.css

I changed the conditionals to match those in the previously-mentioned platform
CSS file.  The ones in that file were substantially simpler (although I think
less clear, because preprocessor instructions don't nest clearly).  The only
change is the addition of an extra #endif at the end of the file, because the
original code was missing one.

Note that the file will still need to be added to jar.mn as a preprocessed file
before it will be built with Help.  It also will still need hookup to the Help
docs in one of the ways previously mentioned before it'll do anything.

Comment 10

[Jeff Walden (gone starting June 8)]

Comment on attachment 155368 [details]
Updated toolkit/components/help/content/platformClasses.css

The only part of this CSS I don't particularly like is the stuff apparently for
OS2 (about which I know nothing).  Aside from that, there shouldn't be any
problems, particularly as the code is already tested elsewhere in the tree.

Comment 11

[Jeff Walden (gone starting June 8)]

(In reply to comment #8)
> Maybe a smarter idea might be to import helpFileLayout.css to
> platformClasses.css and then link to platformClasses. You
> never know when we might want to add another CSS file.

It's a good idea, but it'd require changes to every bit of Help documentation we
have, and we're running out of time.  I say push this idea to After Firefox 1.0
and simply add the @import line to helpFileLayout.css.

So, for the record, what I think makes sense for after 1.0 is this:
-every file links to chrome://help/content/helpFileLayout.css (or another
 suitable name - I think platformClasses.css is a little too specific and
 doesn't anticipate any future needs)
  -the file contains lines that can't be changed, e.g., platform classes
-a file like chrome://help/skin/helpFileLayout.css is @import-ed for skinners

The really important stuff that's build-specific or is required for some other
reason is in content, but everything else is modifiable through the skinnable file.

For now, tho, let's just add the @import to the top of the current file.  Patch
coming up ASAP to hook up the previously-uploaded platformClasses.css file...

Comment 12

[Jeff Walden (gone starting June 8)]

Attachment: @import the stylesheet referenced above

This patch does an @import of platformClasses.css, which is attachment 155368 [details].

Reminder: *must* remember to post a message to Themes forum on Mozillazine and
add a mention to the Theme Development article in the knowledge base so that
themers know about this change (which unfortunately probably makes all their
themes obsolete, even if it's in a nitpicky way that won't make a whole lot of
difference visually).

Comment 13

[R.J. Keller]

Comment on attachment 155648 [details] [diff] [review]
@import the stylesheet referenced above

r=[e-mail] for both patchs.

Jeff, you forgot the jar.mn change but I'll add it in real quick.

Comment 14

[u60234]

Does not the Pinstripe and Gnomestripe version of helpFileLayout.css need to be
updated also?

Comment 15

[R.J. Keller]

(In reply to comment #14)
> Does not the Pinstripe and Gnomestripe version of helpFileLayout.css need to be
> updated also?

Yes, I made that change as well. Once we get aviary approval, I'll throw it in.

Not sure if I should update the Qute version. I don't think it's used anymore.

Comment 16

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 155648 [details] [diff] [review]
@import the stylesheet referenced above

a=[e-mail]

Comment 17

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 155648 [details] [diff] [review]
@import the stylesheet referenced above

a=[e-mail]

Comment 18

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 155368 [details]
Updated toolkit/components/help/content/platformClasses.css

a=[e-mail]

Comment 19

[R.J. Keller]

Fix checked in.

Comment 20

[Steffen Wilberg]

Something is broken here. On Linux, I see " Exit  Quit", and both
Edit->Preferences and Tools->Options in the menu reference.

"Exit" has .win, Options has .noUnix. They shouldn't be displayed because
chrome://help/content/platformClasses.css says:
.noUnix, .win, .mac { display: none; }

Comment 21

[Jeff Walden (gone starting June 8)]

Attachment: Remove the blemishes of my idiocy

I'm not that stupid.  Nope, not me.  There's no way I could possibly mess up
the syntax of the @import rule.  Gimme a break!  How stupid could one be to
mess up a one-line fix?  It's un-possible!  Nope, it must've been a typo.

If you call it fixing a typo, you probably don't need to get approval-aviary. 
If you *don't* call it fixing a typo and instead call it fixing stupidity, you
*do* need to get approval-aviary.  Hah!  I've cornered you by threat of
approval delay into being forced to believe I'm still infallible!

Self-flagellation aside, the revised syntax is tested, verifiable by the CSS2
specs, and correct, and *this* patch will work.

Comment 22

[Steffen Wilberg]

Checked in, blaming typo :)

Comment 23

[R.J. Keller]

Comment on attachment 155881 [details] [diff] [review]
Remove the blemishes of my idiocy

No problem Jeff. Nobody really memorizes those import statements :) (at least I
don't).

Comment 24

[Steffen Wilberg]

We mustn't forget to let themers know of the theme changes.
I just made a post: http://forums.mozillazine.org/viewtopic.php?p=719287#719287