395454 - [Mac] Have one file, scrollbars.css

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Colin Barrett [:cbarrett]]

Now that we no-longer use or have native scrollbars (bug 377457 bug 379319 bug 370439), we don't need "xulscrollbars.css" and "nativescrollbars.css" files on Mac. We should just have scrollbars.css, like Windows.

Nothing should be using xulscrollbars.css, we can just remove it and rename nativescrollbars.css to scrollbars.css (and remove a mac specific ifdef http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/style/nsLayoutStylesheetCache.cpp&rev=1.12#83 ).

Comment 1

[Damon Sicore (:damons)]

What's the perf win here?  I'm -'ing this.  Making it wanted.  If this is something that can get us perf, we should probably do it.

Comment 2

[Colin Barrett [:cbarrett]]

Attachment: fix v1.0

I tested this with Firefox, Seamonkey and Camino on Mac. Worked for me.

Enn, if you're not the right reviewer, let me know who is.

Comment 3

[Colin Barrett [:cbarrett]]

Comment on attachment 296461 [details] [diff] [review]
fix v1.0

Oops

Comment 4

[Neil Deakin]

Comment on attachment 296461 [details] [diff] [review]
fix v1.0

Seems OK. Technically, you probably shouldn't change the license on themes/classic/global/mac/xulscrollbars.css

The default is horizontal for scrollbars, perhaps we should reverse these:

+
+slider {
+  -moz-appearance: scrollbartrack-vertical;
+}
+
+slider[orient="horizontal"] {
+  -moz-appearance: scrollbartrack-horizontal;
+}

You should ask Neil Rashbrook for a review as well.

Comment 5

[Colin Barrett [:cbarrett]]

(In reply to comment #4)
> (From update of attachment 296461 [details] [diff] [review])
> Seems OK. Technically, you probably shouldn't change the license on
> themes/classic/global/mac/xulscrollbars.css

Ah, that happened because I moved nativescrolllbars.css to xulscrollbars.css and CVS thought I just changed the file (it doesn't know).

Suggestions?

> The default is horizontal for scrollbars, perhaps we should reverse these:

I don't know, if you think so, let's do it :)


> You should ask Neil Rashbrook for a review as well.

Will do, thanks!

Comment 6

[[e-mail]]

Comment on attachment 296461 [details] [diff] [review]
fix v1.0

Looks good to me, although
a) I did wonder why you didn't change the internal name back to scrollbars.css
b) instead of "renaming" nativescrollbars.css in the tree you can simply rename it in the jar.mn e.g.
 skin/classic/global/scrollbars.css (nativescrollbars.css)

Comment 7

[Colin Barrett [:cbarrett]]

(In reply to comment #6)
> (From update of attachment 296461 [details] [diff] [review])
> Looks good to me, although
> a) I did wonder why you didn't change the internal name back to scrollbars.css
> b) instead of "renaming" nativescrollbars.css in the tree you can simply rename
> it in the jar.mn e.g.
>  skin/classic/global/scrollbars.css (nativescrollbars.css)

I like the sound of both of those. New patch soon.

Comment 8

[Colin Barrett [:cbarrett]]

Attachment: fix v1.1

Comment 9

[Boris Zbarsky [:bzbarsky]]

Please ask someone else for sr?  I doubt I can get to this before the beta freeze, for one thing.

Comment 10

[Damon Sicore (:damons)]

Comment on attachment 297061 [details] [diff] [review]
fix v1.1

a1.9+=damons

Comment 11

[Reed Loden [:reed]]

Checking in extensions/inspector/resources/content/tests/allskin.xul;
/cvsroot/mozilla/extensions/inspector/resources/content/tests/allskin.xul,v  <--  allskin.xul
new revision: 1.7; previous revision: 1.6
done
Checking in layout/style/nsLayoutStylesheetCache.cpp;
/cvsroot/mozilla/layout/style/nsLayoutStylesheetCache.cpp,v  <--  nsLayoutStylesheetCache.cpp
new revision: 1.13; previous revision: 1.12
done
Checking in suite/themes/modern/jar.mn;
/cvsroot/mozilla/suite/themes/modern/jar.mn,v  <--  jar.mn
new revision: 1.185; previous revision: 1.184
done
Checking in themes/classic/jar.mn;
/cvsroot/mozilla/themes/classic/jar.mn,v  <--  jar.mn
new revision: 1.169; previous revision: 1.168
done
Checking in themes/classic/global/mac/nativescrollbars.css;
/cvsroot/mozilla/themes/classic/global/mac/nativescrollbars.css,v  <--  nativescrollbars.css
new revision: 1.12; previous revision: 1.11
done
Checking in toolkit/themes/pinstripe/global/jar.mn;
/cvsroot/mozilla/toolkit/themes/pinstripe/global/jar.mn,v  <--  jar.mn
new revision: 1.40; previous revision: 1.39
done
Checking in toolkit/themes/pinstripe/global/nativescrollbars.css;
/cvsroot/mozilla/toolkit/themes/pinstripe/global/nativescrollbars.css,v  <--  nativescrollbars.css
new revision: 1.10; previous revision: 1.9
done
Checking in toolkit/themes/pmstripe/global/jar.mn;
/cvsroot/mozilla/toolkit/themes/pmstripe/global/jar.mn,v  <--  jar.mn
new revision: 1.8; previous revision: 1.7
done
Checking in toolkit/themes/winstripe/global/jar.mn;
/cvsroot/mozilla/toolkit/themes/winstripe/global/jar.mn,v  <--  jar.mn
new revision: 1.46; previous revision: 1.45
done

Comment 12

[KLB]

One undesirable side effect of this change is that while Mac Firefox adopted old nativescrollbars.css as the new scrollbars.css, Windows Firefox adopted the old xulscrollbars.css as the new scrollbars.css.  

The problem is if the old xulscrollbars.css is used as the new scrollbars.css on a cross OS custom theme, Mac Firefox loses its scrollbars.  If the old nativescrollbars.css is used for the new scrollbars.css for a custom theme on Windows XP using the "Windows XP Style" desktop a stray black border appears around the scrollbars due to WinXP Firefox not handling "-moz-appearance: scrollbar" properly(see bug 423780).

Since the old scrollbar style sheets have been "merged", we really need Firefox to be fixed such that the Windows Firefox can use "-moz-appearance: scrollbar" without causing a black border.  This fix would allow Windows Firefox to use the old nativescrollbars.css as the new scrollbars.css as Mac does.  This in turn would make cross OS theme development much easier.

Comment 13

[Josh Aas]

Neil, do we need to do something about this?