222654 - global/scrollbars.css for native scrollbars different on Mac (many Firefox themes break scrollbars on Mac)

Status: RESOLVED FIXED

(SeaMonkey :: Themes, defect)

Description

[aaron spuler]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4) Gecko/20030624
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4) Gecko/20030624

If you look at the classic.jar for MacOS X and all other platforms, the code is
different.  To achieve native scrollbars, it (to me anyway) is impossible to use
a non-mac classic.jar scrollbars.css file.  To test this out, I have 2 themes
that use native scrollbars, named Rain and Smoke.  You may find them at
http://mozthemes.tk, and if you try to install the non-Mac version on a Mac,
everything is correct except for the scrollbars (they don't appear).  However,
installing the Mac-specific builds corrects this problem.  I have not modified
the code global/scrollbars.css -- I just copied the file straight from the
classic.jar file.  For the Mac-specific version, I pulled it from the Mac
classic.jar, and for the non-Mac version, I pulled it from the Windows classic.jar.
I am not the only themer noticing this problem, others on the Mozillazine forums
have noticed this issue.

Reproducible: Always

Steps to Reproduce:
1.Install non-Mac version of Rain or Smoke themes from http://mozthemes.css
2.Restart Mozilla

Actual Results:  
Scrollbars missing.

Expected Results:  
Native scrollbars.

Then to get native scrollbars, install the Mac-specific version, restart
Mozilla, and there you go.  Since Mozilla is cross-platform, themes should not
have to be platform-specific.

Comment 1

[aaron spuler]

Sorry, in "Steps to reproduce, step 1", it should say http://mozthemes.tk
instead of http://mozthemes.css!

Comment 2

[Samir L. Boulema]

also got this problem when making my qute status and crystal status themes...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Themes may need to be platform-specific if you want a different look on
different platforms, which is the case here...

Comment 4

[aaron spuler]

But the only thing different about the themes is the scrollbars... all other os
widgets are cross-platform on the themes.  And here's another thing:  If I take
a non-Mac theme and change the -moz-appearance for all instances of scrollbar
code in scrollbars.css to -moz-appearance: none, then there ARE scrollbars
appearing in the mail folder pane and mail message pane!  But nowhere else :(

Comment 5

[alanjstr]

This is about themes being cross-platform, which seems to no longer be the case.
 This doesn't seem to be about using OSX specific widgets and currently only
appears to be a problem with scrollbars.  

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6a) Gecko/20031025
Firebird/0.7+ (Mike Tigas (Nova); SVG-DOMi-Venkman; i686)

Comment 6

[aaron spuler]

No progress yet on this bug??  I still have to maintain two versions of each
theme that uses Native OS Scrollbars, and the only difference is
global\scrollbars.css -- This behavior occurs on Mozilla, Mozilla Firefox, and
Mozilla Thunderbird.

Comment 7

[Jason O'Halloran]

I agree with this.  For all the themes I've hacked to get to work with OS X all
i've had to do was replace scrollbars.css  Us Mac users shouldn't have to do
that.  And the theme authors shouldn't have to create a seperate theme just for
us.  

Comment 8

[CatThief]

Maintaining two versions of the same theme just to solve the missing OS X scrollbars issue is more work than should be necessary. For what it's worth I vote this issue be fixed.

Comment 9

[aaron spuler]

Asa, how about nominating this as a blocker for 1.9???

Please?

Comment 10

[Wolf [:wolf]]

Requesting blocking-aviary1.0mac on this bug.. Unless it's intentional that
themes aren't cross-platform for MacOSX on Firefox and Tbird. 

Bug 252294, was filed against Mozilla Update asking that the Cross-Platform
"All" themes be hidden for MacOSX users, because of this bug. As most
cross-platform themes that don't take Mac into consideration appearantly are
failing to show up appropriately. I'd rather see this bug fixed than have to
implement such a workaround for Update.

Comment 11

[Steffen Wilberg]

The only place using chrome://global/skin/scrollbars.css is
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsLayoutStylesheetCache.cpp#83
(there's also mozilla/extensions/inspector/resources/content/tests/allskin.xul,
but that's only used by inspector/jar.mn).

We could do this:
-NS_LITERAL_CSTRING("chrome://global/skin/scrollbars.css"));
+#idef XP_MACOSX
+  NS_LITERAL_CSTRING("chrome://global/skin/nativescrollbars.css"));
+#else
+  NS_LITERAL_CSTRING("chrome://global/skin/xulscrollbars.css"));
+#endif

Include both css files in the appropriate jar.mn (and clean up some cruft in
makefiles). Each theme could provide both css files then, and the app would use
the one appropriate for its platform.

Comment 12

[alanjstr]

Is it in the same place for Firefox?

Comment 13

[PikeUK]

*** Bug 254783 has been marked as a duplicate of this bug. ***

Comment 14

[Asaf Romano (gone)]

*** Bug 256280 has been marked as a duplicate of this bug. ***

Comment 15

[Asaf Romano (gone)]

*** Bug 262174 has been marked as a duplicate of this bug. ***

Comment 16

[alanjstr]

Steffen -

Can you please turn your idea into a formal patch?

Comment 17

[alanjstr]

This is really causing a problem for UMO.  Unfortunately this didn't make it for
1.0, which means a lot of frustrated users.

Comment 18

[Justin Wood (:Callek)]

I can make a formal patch out of the idea, not "now" though, not sure who to r?
though, or if that patch is optimal...but it will likely be a much better
work-around than what is currently in place.

Comment 19

[Justin Wood (:Callek)]

Re-Assigning to me, so I dont loose.

Comment 20

[Hiro]

*** Bug 272307 has been marked as a duplicate of this bug. ***

Comment 21

[Anne (:annevk)]

Attachment: patch #1 - first part of what has been described in comment 11

Comment 22

[Asaf Romano (gone)]

*** Bug 285084 has been marked as a duplicate of this bug. ***

Comment 23

[Justin Wood (:Callek)]

Comment on attachment 175042 [details] [diff] [review]
patch #1 - first part of what has been described in comment 11

hmm forgive me for talking this late at night, but do we not need the #ifdef
etc as teh first character on the line, at least the # anyway, with possible
space between that and teh command.

Plus #idef XP_MACOSX is wrong, mistyped.

I am unsure of which new-file scrollbars.css will be best shipped to, though a
new file for the other will be needed, and an edit of the jar.mn for those CSS
files.

Comment 24

[Chris Neale]

Attachment: Tweaked patch addressing #idef and #ifdef etc at the start of the line

Of course this doesn't help the theme developer who wants to have the
scrollbars on every OS; e.g. Modern Pinball

Comment 25

[Chris Neale]

Attachment: Tweaked patch addressing #idef and #ifdef etc at the start of the line

Of course this doesn't help the theme developer who wants to have the same
scrollbars on every OS; e.g. Modern Pinball

Comment 26

[Chris Neale]

Comment on attachment 184227 [details] [diff] [review]
Tweaked patch addressing #idef and #ifdef etc at the start of the line

I'll diff -u next time : )

Comment 27

[Chris Neale]

Attachment: diff -u tweaked patch

Regarding comment 24 [25]

nativescrollbars.css would only need to be :

@import(xulscrollbars.css); /* absolute paths if absolutely necessary */

or

@import(chrome://global/skin/xulscrollbars.css);

To get around that.

Apologies for excessive bugspam. Been up since approx 4am

Comment 28

[Steffen Wilberg]

Attachment: complete patch

The above patches are nowhere near complete, so lLet's start again.

The goal is to allow themes to provide two css files, one for Mac
(nativescrollbars.css) and one for the other platforms (xulscrollbars.css). The
advantage is that you can provide a single theme for all platforms.

This patch changes the reference from the generic
chrome://global/skin/scrollbars.css to
chrome://global/skin/nativescrollbars.css and
chrome://global/skin/xulscrollbars.css.

On the Seamonkey Classic theme, there are already two files,
nativescrollbars.css and xulscrollbars.css. So I only need to change the jar.mn
packaging.
On the Seamonkey Modern theme, there's only scrollbars.css, providing xul
scrollbars on all platforms. The jar.mn file packages it as
nativescrollbars.css on Mac, and as xulscrollbars.css on the other platforms.
On Pinstripe, which is used by Firefox on Mac, there's a scrollbars.css file,
which I move to nativescrollbars.css, and pack that.
On Qute, which is used by Thunderbird on all platforms, there's only
scrollbars.css, providing xul scrollbars on all platforms. The jar.mn file
packages it as nativescrollbars.css on Mac, and as xulscrollbars.css on the
other platforms.
On Winstripe, which is used by Firefox on non-Mac platforms, there's a
scrollbars.css file, which I move to xulscrollbars.css, and pack that.

The allskin.xul file is the only other file referencing
chrome://global/skin/scrollbars.css.

Comment 29

[Steffen Wilberg]

I tested this with Firefox on Linux. There's no difference to Firefox/Windows.
I'd be glad if someone could test this on Mac and on Seamonkey.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 184242 [details] [diff] [review]
complete patch

I'm not the right reviewer for this.

Comment 31

[[e-mail]]

Comment on attachment 184242 [details] [diff] [review]
complete patch

>-              NS_LITERAL_CSTRING("chrome://global/skin/scrollbars.css"));
>+#ifdef XP_MACOSX
>+              NS_LITERAL_CSTRING("chrome://global/skin/nativescrollbars.css"));
>+#else
>+              NS_LITERAL_CSTRING("chrome://global/skin/xulscrollbars.css"));
>+#endif
On IRC bz suggested changing this to chrome://global/content/scrollbars.css
then using preprocessing to @import the relevant skin file. I think it's too
much pain for too little gain. But you should still get some sort of layout
sign-off either way.

>+#ifndef XP_MACOSX
>+# use xul scrollbars on non-Mac platforms
>+  skin/classic/global/xulscrollbars.css                       (global/win/scrollbars.css)
>+#endif
I'd rather that this line be moved into the #else section of the #ifdef
XP_MACOSX instead of having its own #if block.

>+#ifdef XP_MACOSX
>+# referenced as chrome://global/skin/nativescrollbars.css on Mac
>+  skin/modern/global/nativescrollbars.css                          (global/scrollbars.css)
>+#else
>+# referenced as chrome://global/skin/xulscrollbars.css on non-Mac platforms
>+  skin/modern/global/xulscrollbars.css                             (global/scrollbars.css)
>+#endif
I think I'd prefer to package it twice for Modern, so that the platforms get
identical .jar files (which is the point).

>-+  skin/classic/global/scrollbars.css
>++  skin/classic/global/nativescrollbars.css
I assume this would be done with a CVS move... unless your toolkit theme
reviewer will let you get away with renaming it in jar.mn instead ;-)

>-<?xml-stylesheet href="chrome://global/skin/scrollbars.css"?>
>+#ifdef XP_MACOSX
>+<?xml-stylesheet href="chrome://global/skin/nativescrollbars.css"?>
>+#else
>+<?xml-stylesheet href="chrome://global/skin/xulscrollbars.css"?>
>+#endif
Since the layout code always loads the scrollbars anyway (except for the
sidebar in the modern theme, which I doubt applies here), you could probably
get away with simply removing that line.

sr=me with these nits fixed.

Comment 32

[Chris Neale]

Attachment: Like attachment 184242 addressing many of neil's concerns from comment 31

-    NS_ASSERTION(gStyleCache->mScrollbarsSheet, "Could not load
scrollbars.css.");
+    NS_ASSERTION(gStyleCache->mScrollbarsSheet, "Could not load
[native]scrollbars.css.");

bit still looks wrong since it's now xulscrollbars.css ... and then they're
sort of "native" since Firefox etc on Linux pick up the GTK scrollbars.

Are native scrollbars native or pseudo-native ?

Comment 33

[Steffen Wilberg]

Attachment: complete patch v1.1

This patch addresses Neil's comments except bz's suggestion, because that would
result in shipping three css files: The nativescrollbars.css,
xulscrollbars.css, and scrollbars.css, the latter having either of the first
two @import'ed.

I also splitted the NS_ASSERTION to report the proper file names.

Comment 34

[[e-mail]]

Comment on attachment 184444 [details] [diff] [review]
complete patch v1.1

Looks OK now but I was just wondering whether we should try to load
scrollbars.css just in case.

Comment 35

[Boris Zbarsky [:bzbarsky]]

I'll try to look at this in the next week.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 184444 [details] [diff] [review]
complete patch v1.1

>Index: mozilla/themes/classic/jar.mn

Wouldn't it make sense to package both files no matter what the target OS is? 
That way theme developers who make a copy of this theme as a basis for their
theme won't run into issues...

Or is that what the patch is doing?

>Index: mozilla/toolkit/themes/pinstripe/global/scrollbars.css

Please get this repository moved (so as not to lose the CVS history) or just
leave it be and only change the packaging.

Again, it might make sense to package under both names so people developing
themes based on this don't get screwed over.

>Index: mozilla/toolkit/themes/qute/global/jar.mn

Please package under both names no matter what the target platform is, like
Modern does.

>Index: mozilla/toolkit/themes/winstripe/global/scrollbars.css

Same comments here as for Pinstripe.

sr=bzbarsky with those comments addressed.

Comment 37

[Kevin Gerich]

Comment on attachment 184444 [details] [diff] [review]
complete patch v1.1

Excuse the delay. Skin changes look good, though I haven't gotten a chance to
test this on Mac since we don't build on Tiger yet

Comment 38

[alanjstr]

Let me see if I have this straight from a theme developer's point of view:

Old:  
OSX:  scrollbars.css
Other: scrollbars.css

New:
OSX:  nativescrollbars.css
Other: xulscrollbars.css

Comment 39

[alanjstr]

Comment on attachment 184444 [details] [diff] [review]
complete patch v1.1

>Index: mozilla/extensions/inspector/resources/content/tests/allskin.xul
>===================================================================
> <?xml-stylesheet href="chrome://global/skin/radio.css"?>
>-<?xml-stylesheet href="chrome://global/skin/scrollbars.css"?>
> <?xml-stylesheet href="chrome://global/skin/splitter.css"?>

Shouldn't you have added the others back in?

And I agree with bz, you should always package both native and xulscrollbars
since people base their themes on the included one.

Many thanks to Anne and cdn for really getting this done.

Comment 40

[Asa Dotzler [:asa]]

what's missing here? is this ready to land?

Comment 41

[Steffen Wilberg]

Attachment: ship both files on all platforms

Sorry for the delay. This patch addresses bz's comments by shipping both
xulscrollbars.css and nativescrollbars.css on all platforms.

Comment 38 is correct.

Tested with Firefox on Linux.

Comment 42

[Steffen Wilberg]

Attachment: ship both files on all platforms

Bugzilla's patch-viewer doesn't like long lines.
Shorter comments this time.

Comment 43

[Steffen Wilberg]

Hmm, the patch-viewer ("Diff") still suppresses two lines from the
pinstripe/global/jar.mn part.

Comment 44

[[e-mail]]

Comment on attachment 189573 [details] [diff] [review]
ship both files on all platforms

>-    NS_ASSERTION(gStyleCache->mScrollbarsSheet, "Could not load scrollbars.css.");
>+#ifdef XP_MACOSX
>+    NS_ASSERTION(gStyleCache->mScrollbarsSheet, "Could not load nativescrollbars.css.");
>+#else
>+    NS_ASSERTION(gStyleCache->mScrollbarsSheet, "Could not load xulscrollbars.css.");
>+#endif
I always hate trailing dots after filenames ;-)

Comment 45

[Steffen Wilberg]

Chase, can you please do repository copies (so as not to lose the CVS history) of
mozilla/toolkit/themes/pinstripe/global/scrollbars.css to nativescrollbars.css, and
mozilla/toolkit/themes/winstripe/global/scrollbars.css to xulscrollbars.css, as
requested by bz in comment 36?
Thanks.

Comment 46

[Steffen Wilberg]

Justdave, can you help here please? See previous comment.

Comment 47

[Uri Bernstein]

*** Bug 294365 has been marked as a duplicate of this bug. ***

Comment 48

[Chase Phillips]

CVS copy being tracked in bug 304354.

Comment 49

[Steffen Wilberg]

Attachment: unbitrotted patch

Thanks for the cvs copies.

This is an unbitrotted patch which doesn't add the already copied files, but
modifies them in place.

I don't have spare time today, so I'd appreciate if anybody could test this and
check it in.

Comment 50

[Steffen Wilberg]

Checked in trunk and 1.8-branch.

Comment 51

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #50)
> Checked in trunk and 1.8-branch.

Seems like you may have mistakenly checked in a few files on the branch.

Comment 52

[Steffen Wilberg]

> Seems like you may have mistakenly checked in a few files on the branch.
I don't think so, but bonsai's display of files removed from a branch is indeed
a bit strange:
mozilla/ toolkit/ themes/ pinstripe/ global/ Tag:
mozilla/ toolkit/ themes/ pinstripe/ global/ MOZILLA_1_8_BRANCH
mozilla/ toolkit/ themes/ winstripe/ global/ Tag:
mozilla/ toolkit/ themes/ winstripe/ global/ MOZILLA_1_8_BRANCH

Comment 53

[Shriramana Sharma]

I'm running Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1)
Gecko/20050822 SeaMonkey/1.0a and the fix for this bug has caused the scrollbars
to disappear on all of the themes from www.spuler.us (OP of this thread).
There's a fix sort of discussed over here:
http://forums.mozillazine.org/viewtopic.php?t=305665, so what do we do now about
this bugfix? Do I start another bug?

Comment 54

[Benjamin Smedberg]

Shriramana, those themes need to be updated, there is no need to open a bugzilla
bug.

Comment 55

[Stephen Donner [:stephend] Not actively reading bugmail]

*** Bug 305864 has been marked as a duplicate of this bug. ***

Comment 56

[Stephen Donner [:stephend] Not actively reading bugmail]

*** Bug 306329 has been marked as a duplicate of this bug. ***

Comment 57

[Stephen Germany]

*** Bug 307000 has been marked as a duplicate of this bug. ***

Comment 58

[Steffen Wilberg]

This has been reverted by bug 379319 because we don't need different scrollbars.css files anymore.