419319 - Global chrome overrides are enabled for all themes (aero theme broke all third-party themes)

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect, P1)

Description

[Pardal Freudenthal (:ShareBird)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12pre) Gecko/20080130 BonEcho/2.0.0.12pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022404 Minefield/3.0b4pre

The fix for Bug 416531 - Use chrome overrides to display Aero style icons on Vista breaks all third-party themes on Vista. See. http://forums.mozillazine.org/viewtopic.php?t=630829&start=0&postdays=0&postorder=asc&highlight=

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Ed Hume]

In the Mozillazine thread I posted a list of chrome.manifest override codes which were designed to reverse the override codes in classic.manifest. dansx6 tried them out and they failed. 

The only way we themers will have to fix this problem in the short term is to duplicate all of these graphics files, appending -aero to each.

Longer term - to keep down the size of our theme files - we will have to hunt down all occurrences of css calls for each of the 22 files that were reassigned . . . assuming that none of them have been coded in areas not accessible by theme css files.

Each developer will have to learn about this issue through unhappy feedback from theme users, then figure out what is wrong, then fix it. One can just imagine how many man-hours will be chewed up correcting this.

Comment 2

[Reed Loden [:reed]]

Hmmph, I didn't see this bug when I filed bug 419380.

Comment 4

[(no longer active)]

Hmmm, I think a nice solution to this problem would be adding an "override-if-exists" rule to the chrome registration code, and use it instead of override for Aero icons.  The override-if-exists rule checks to make sure the target chrome URI points to an existing resource, and overrides the specified resource *only* if the target exists.

With this solution, theme designers who don't provide -aero images get the same look on both XP and Vista, and those who wish to provide a Vista-specific theme can create the -aero image files as well, and their Vista-specific theme magically works without them needing to add anything to their manifest files.

Mano, Benjamin: what do you think?  I'm willing to try to create a patch for override-if-exists.

Comment 5

[(no longer active)]

Oh, and note to drivers: if Beta4 ships without this being fixed, it may potentially break any Fx3 compatible theme and also prevent theme designers to create and test their themes with the latest Fx3 beta.

Comment 6

[Asaf Romano (gone)]

The solution is to only apply a theme rule when that theme is installed.

Comment 7

[Richard Marti (:Paenglab)]

Better would be only to apply a theme rule when the theme is used.

Comment 8

[(no longer active)]

(In reply to comment #6)
> The solution is to only apply a theme rule when that theme is installed.

Any ideas how one should attempt to fix this?  I really don't know where to start...

Comment 9

[(no longer active)]

(In reply to comment #7)
> Better would be only to apply a theme rule when the theme is used.

I guess that's what mano meant, otherwise the existing problem would persist.

Comment 10

[ronin.achilles]

Just porting my comment from Bug #419380 since that bug has been duplicated.

IMO we should just go ahead with a single theme i.e. Aero. The only major difference between Aero and XP theme is the color of keyhole; and the Aero keyhole is way better than XP. As far as the other icons/buttons are concerned, Aero's ones are again much better.

Hence, why can't we just ship the Aero theme as default for all Window versions?

Comment 11

[Asaf Romano (gone)]

(In reply to [e-mail], comment #10)

Please stop this spam, now. This is a platform bug about chrome overrides, if you don't know what these are, or if you don't have anything to contribute in order to fix it, do not comment here. Thanks.

Comment 12

[Dave Townsend [:mossop]]

About the only feasible way I can see of changing the chrome override mechanism to make this work would be to add an additional manifest flag that enables the override based on the currently selected skin. This would be fairly simple to implement but to be honest I really don't see much benefit to having such a flag in the platform beyond fixing this unique situation.

The alternative would be to change the Firefox theme to not use overrides, instead it would probably have the jar contents split into multiple sets, either a straight effectively two skins packaged or a single main set of files and then the two OS specific areas.

Comment 13

[Dave Townsend [:mossop]]

I'd add that this is not really a theme problem. The default theme is not actually shipped as a theme, if it were it would be unable to use overrides at all. It is just a default set of chrome registrations, previously only skin registrations.

Comment 14

[Asaf Romano (gone)]

Dave, as things stand, if a theme sets overrides, it simply breaks the next theme you switch too. If we decide not to fix this situation, we should disallow override rules in themes.

Comment 15

[Dave Townsend [:mossop]]

(In reply to comment #14)
> Dave, as things stand, if a theme sets overrides, it simply breaks the next
> theme you switch too. If we decide not to fix this situation, we should
> disallow override rules in themes.

We do disallow override rules in themes. As I say, the default theme is not packaged as a theme as such.

Comment 16

[Pardal Freudenthal (:ShareBird)]

Could it be possible to make it with a binding, checking if OS is Vista and adding class="aero" to those elements? With this class it would be easy to skin separately elements for Vista. Just one thought...

Comment 17

[Pardal Freudenthal (:ShareBird)]

Or a similar approach. Adding "osversion" as an attribute for some element in the DOM tree. Supposing for example this element being the main-window (just as example, it could also be a child element), it would open fantastic possibilities to third-party themes be more "cross-platformish". I mean it would be possible to have rules like #main-window[osversion="6"] #back-button {list-style-image: myaerobutton.png}

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Blocking for resolution before Beta 4.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #16)
> Could it be possible to make it with a binding, checking if OS is Vista and
> adding class="aero" to those elements? With this class it would be easy to skin
> separately elements for Vista. Just one thought...

CSS selectors were considered, IIRC, and there were problems with that solution. The ideas in comment 6 and comment 7 seem like rightthinking to me, with comment 4 being a good fallback.

Comment 20

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch

What this patch does is add a new 'platform' skin package to the theme.
The advantages of using this method are that it requires no action on the part of third parties to override (they just need to be aware of what's going on when copying CSS from the default theme and change chrome URIs as necessary), it limits the amount of duplicate content to only images that are actually different between luna and aero and it reduces maintenance overhead by requiring the minimum amount of work to add and change new icons.

This not only converts winstripe icons to the new namespace, but pinstripe and gnomestripe as well. I've done this in the interests of keeping the themes close enough so that they become no harder to patch than they are now and because there are a number of places where icons are referenced in the source and figuring out exactly which to move in pinstripe and gnomestripe would've made cleaning up after my global search and replace slightly more annoying :). Plus it makes it a bit easier if we ever decide to do tango/oxygen and leopard/nextbigcat icon sets later on.

Comment 21

[Reed Loden [:reed]]

The |#ifndef MINIMO|s are not needed, as Minimo is not supported any longer. Please do not add new ones, and remove the ones you find. :)

Comment 22

[Ryan Flint [:rflint] (ping via IRC for reviews)]

(In reply to comment #21)
> The |#ifndef MINIMO|s are not needed, as Minimo is not supported any longer.
> Please do not add new ones, and remove the ones you find. :)
> 

I do recall a bug on file for that but being that it's 5am the thought was fleeting enough that I didn't care all that much :)

Comment 23

[Ryan Flint [:rflint] (ping via IRC for reviews)]

(and I'll take care of them when I checkin)

Comment 24

[Ryan Flint [:rflint] (ping via IRC for reviews)]

(In reply to comment #20)
>...it requires no action on the part of third parties to override
Except for those places where the icons are hard-coded in the source... (argh.)

Dave brought up a good point on IRC that if we're going to need people to update their themes anyway to accommodate the hard-coded icons that it might be easier for some to have toolkit and browser split out into two unique resource-like namespaces for manifest mapping to existing directory structures.
I'm of the opinion that it's probably easier to copy the resource folder wholesale along with our manifest declarations now in the process of updating themes for fx3 and not need to worry much about it for future releases.

In either case, this is probably going to end with me needing to cleanup and add a lot to the theme docs on DevMo.

Comment 25

[Asaf Romano (gone)]

Comment on attachment 305980 [details] [diff] [review]
Patch

Should this be browser-platform and global-platform instead?

Comment 26

[Dave Townsend [:mossop]]

(In reply to comment #25)
> (From update of attachment 305980 [details] [diff] [review])
> Should this be browser-platform and global-platform instead?

That was pretty much my thought, though it is an unfortunate mouthful. But if we do that then all current themes have to change to work is add a mapping for browser-platform to wherever they currently have browser, same for global, same for mozapps etc.

Comment 27

[mcdavis941 (sporadically reading bugmail)]

(In reply to comment #24)
> (In reply to comment #20)
> >...it requires no action on the part of third parties to override
> Except for those places where the icons are hard-coded in the source... (argh.)
> 

Are you talking about places where for example in C++ source there are references to a chrome URL in a skin package?

If that's true, it's probably worth pointing out that, from a theme developer's perspective, it's vastly preferable for the code to create elements that expose state information through element attributes, and then for skin CSS to apply the image (through something like list-style-image or background-image). 

Applying images through skin CSS gives theme-writers and anybody doing UI programming a lot more flexibility, and lets you do things like use different versions of the same icon depending on whatever background color it's on (on-light in some places, on-dark in others), or for another example to vary it based on hover state even if the default theme doesn't.

Comment 28

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Multiple package approach

This should make the migration for themes slightly easier as there's no need for them to change their existing directory structure - only simple skin registrations mapping the platform-* packages to their existing counterparts are required.

Comment 29

[Ryan Flint [:rflint] (ping via IRC for reviews)]

(In reply to comment #27)
> Are you talking about places where for example in C++ source there are
> references to a chrome URL in a skin package?
> 

Yes. We certainly don't like to do so but in most cases we're limited by what APIs provide - some only expose a way to pass in an icon (instead of, say, a class name) and at times that's the best way to do it. There are certainly some of them we can improve, but that's likely out of scope for this release.

Comment 30

[mcdavis941 (sporadically reading bugmail)]

(In reply to comment #29)
> (In reply to comment #27)
> > Are you talking about places where for example in C++ source there are
> > references to a chrome URL in a skin package?
> > 
> 
> Yes. We certainly don't like to do so but in most cases we're limited by what
> APIs provide - some only expose a way to pass in an icon (instead of, say, a
> class name) and at times that's the best way to do it. There are certainly some
> of them we can improve, but that's likely out of scope for this release.
> 

OK.  I appreciate the clarification.

Comment 31

[mcdavis941 (sporadically reading bugmail)]

(In reply to comment #30)
> > Yes. We certainly don't like to do so but in most cases we're limited by what
> > APIs provide - some only expose a way to pass in an icon (instead of, say, a
> > class name) and at times that's the best way to do it. There are certainly some
> > of them we can improve, but that's likely out of scope for this release.

> OK.  I appreciate the clarification.

And thanks for not ending with "and welcome to software development, Captain Obvious!" :) I guess what I said is a bit of a no-brainer, but I'd never heard it so stated and didn't know whether that was the perspective.  Great to hear it.

Comment 32

[Dave Townsend [:mossop]]

Comment on attachment 306188 [details] [diff] [review]
Multiple package approach

First run through, I still have to produce builds and test with this working:

tab-arrow-end.png or tab-arrow-start.png are lost in gnomestripe, can't see that they are used anymore, is that correct? If so the files should be removed as well I guess.

Same for bookmarksToolbar.png in winstripe, yet you didn't remove it from pinstripe?

Is there a reason for not removing the find.png and wrap.png icons from the general winstripe area?

chrome://browser/skin/identity.png needs to change in browser/themes/gnomestripe/browser/browser.css
chrome://browser/skin/feeds/feedIcon.png needs to change in browser/components/preferences/applications.js
chrome://browser/skin/feeds/feedIcon16.png needs to change in browser/themes/gnomestripe/browser/browser.css
chrome://browser/skin/feeds/feedIcon16.png needs to change in browser/themes/gnomestripe/browser/places/places.css

extensions/cck uses chrome://browser/skin/page-livemarks.png, file a bug to get it updated.

Comment 33

[Dave Townsend [:mossop]]

With those changes I'll be pretty much happy that these will work, though I don't have a vista machine to verify with.

My only concern would be the package names though they seem appropriate and the moving of the icons in gnomestripe and pinstripe as well, they aren't really necessary for the moment I think (well maybe a bunch of the gnomestripe ones are).

While I can see your argument that it /could/ ease things in the future, the same could be said for moving other images in winstripe. We hit the point where everytime we add a new image we have to decide whether to put it into the general or the osversion specific areas. It might even be most sensible to have all the images in the osversion area. That'd certainly make communicating the change easier.

Check in with gavin or another toolkit peer that he's happy with the package names, pinstripe changes and general structure here please but I'm otherwise fine with it.

Comment 34

[Asaf Romano (gone)]

I prefer not to change pinstripe and gnomestripe at this point.

Comment 35

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Simplest approach

Don't know why didn't think of doing it this way before even though I mentioned it in comment 28...

This leaves the luna/gnomestripe/pinstripe icons in place and then simply maps the platform-* packages to where they need to go. This creates a nice fallback for the CCK and other extensions and makes it so existing themes only need to copy over our non-aero platform-* manifest rules if they're similar enough to the default theme.

> tab-arrow-end.png or tab-arrow-start.png are lost in gnomestripe, can't see
> that they are used anymore, is that correct? If so the files should be removed
> as well I guess.
Yep, gnomestripe's using native arrows on the scroll buttons. I'll remove the images on checkin.

> Same for bookmarksToolbar.png in winstripe, yet you didn't remove it from
> pinstripe?
Done. I'll remove the files as well.

Comment 36

[Dave Townsend [:mossop]]

Attachment: inneficient but zero impact [Checkin: Comment 39]

This just doubles up the theme so no need to tinker with existing chrome paths or themes.

Comment 37

[Dave Townsend [:mossop]]

Expected size impact from this change: The current classic.jar is some 810kb. Some 540kb of that is the images. So my approach gives about at additional 260kb of arguably unnecessary duplication. Though apparently there will be some style changes coming as well but I guess not many to bring that number down. This assumes that every image will be different between XP and Vista. This is apparently not far off the truth.

Comment 38

[Mike Connor [:mconnor]]

Comment on attachment 306524 [details] [diff] [review]
inneficient but zero impact
[Checkin: Comment 39]

moa=me on the size increase, gavin looked at the details already, but the overall approach is sane and appropriately conservative for this point.

bombs away?

Comment 39

[Dave Townsend [:mossop]]

Landed.

Checking in browser/themes/winstripe/browser/jar.mn;
/cvsroot/mozilla/browser/themes/winstripe/browser/jar.mn,v  <--  jar.mn
new revision: 1.73; previous revision: 1.72
done
Checking in toolkit/themes/winstripe/global/jar.mn;
/cvsroot/mozilla/toolkit/themes/winstripe/global/jar.mn,v  <--  jar.mn
new revision: 1.43; previous revision: 1.42
done
Checking in toolkit/themes/winstripe/mozapps/jar.mn;
/cvsroot/mozilla/toolkit/themes/winstripe/mozapps/jar.mn,v  <--  jar.mn
new revision: 1.26; previous revision: 1.25
done

Comment 40

[Walter Meinl]

This hosed partially the UI of linux and OS/2 builds as their toolkit/..stripe/global/jar.mn packages put the overrides still in the toplevel of skin/classic but winstripe/global/jar.mn puts it in aero and luna.
Dirlisting of OS/2 skin\classic: aero browser communicator global help luna mozapps.
Dirlisting of a hourly windows build skin\classic: aero communicator help luna.
Either we would have to adjust in toolkit the jar.mn of pmstripe (OS/2) and gnomestripe or the jar.mn of winstripe

Comment 41

[Reed Loden [:reed]]

(In reply to comment #40)
> This hosed partially the UI of linux and OS/2 builds as their
> toolkit/..stripe/global/jar.mn packages put the overrides still in the toplevel
> of skin/classic but winstripe/global/jar.mn puts it in aero and luna.

Mind filing a new bug for tracking that and make it block this bug?

Comment 42

[Bill Gianopoulos [:WG9s]]

(In reply to comment #41)
> (In reply to comment #40)
> > This hosed partially the UI of linux and OS/2 builds as their
> > toolkit/..stripe/global/jar.mn packages put the overrides still in the toplevel
> > of skin/classic but winstripe/global/jar.mn puts it in aero and luna.
> 
> Mind filing a new bug for tracking that and make it block this bug?
> 
Bug 420440 had already been filed on this issue.

Comment 43

[Bill Gianopoulos [:WG9s]]

This has broken the default theme on Linux, which is one of the officially supported platforms. in order to make third party themes work better under Vista.

This needs to be backed out.

Comment 44

[Walter Meinl]

I've added a https://bugzilla.mozilla.org/attachment.cgi?id=306723 bug420440. OS/2 would probably go with the luna theme. However, it is the question if  attachment 306524 [details] [diff] [review] is the final solution.
Other platforms or seamonkey would have to adjust in their jar.mn files by
s|global|luna/global|

Comment 45

[Bill Gianopoulos [:WG9s]]

(In reply to comment #43)
> This has broken the default theme on Linux, which is one of the officially
> supported platforms. in order to make third party themes work better under
> Vista.
> 
> This needs to be backed out.
> 

Oh, I see too late for backout until this is sorted.  There are already subsequent patches dependent on this.  Nevermind.

Comment 46

[Bill Gianopoulos [:WG9s]]

I decided to file a separate bug on the Linux/gnomestripe issue.  It is bug 420460.

Comment 47

[Reed Loden [:reed]]

Ryan and I will have a patch for both bustages (gnomestripe/pmstripe being broken and Vista not getting the aero icons) soon. Just waiting on builds...

Comment 48

[Reed Loden [:reed]]

Attachment: Fix gnomestripe/pmstripe and make Vista use aero icons - v1

Comment 49

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Comment on attachment 306791 [details] [diff] [review]
Fix gnomestripe/pmstripe and make Vista use aero icons - v1

>Index: toolkit/themes/winstripe/global/jar.mn
>===================================================================
>+        skin/classic/global/richlistbox.css
>+# chrome://global/skin/nativescrollbars.css is used on Mac
>+        skin/classic/aero/global/nativescrollbars.css                    (../../pinstripe/global/nativescrollbars.css)
>+        skin/classic/global/xulscrollbars.css

s/aero\/// here and add it to the #ifdef XP_WIN one below it.

Comment 50

[Ed Hume]

This morning's nightly works well and does not break 3d-party Win-based themes. I was concerned about some hard-coded icons, but everything looks good.

Comment 51

[Reed Loden [:reed]]

Attachment: Fix gnomestripe/pmstripe and make Vista use aero icons - v2

Comment 52

[Reed Loden [:reed]]

Attachment: Fix gnomestripe/pmstripe and make Vista use aero icons - v2.1 [Checkin: Comment 55]

Comment 53

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

Comment on attachment 306823 [details] [diff] [review]
Fix gnomestripe/pmstripe and make Vista use aero icons - v2.1
[Checkin: Comment 55]

r=me, please land with CLOBBER

Comment 54

[Mike Connor [:mconnor]]

Comment on attachment 306823 [details] [diff] [review]
Fix gnomestripe/pmstripe and make Vista use aero icons - v2.1
[Checkin: Comment 55]

land and clobber ftw

Comment 55

[Reed Loden [:reed]]

Checking in browser/themes/winstripe/browser/jar.mn;
/cvsroot/mozilla/browser/themes/winstripe/browser/jar.mn,v  <--  jar.mn
new revision: 1.75; previous revision: 1.74
done
Checking in toolkit/themes/winstripe/global/jar.mn;
/cvsroot/mozilla/toolkit/themes/winstripe/global/jar.mn,v  <--  jar.mn
new revision: 1.45; previous revision: 1.44
done
Checking in toolkit/themes/winstripe/mozapps/jar.mn;
/cvsroot/mozilla/toolkit/themes/winstripe/mozapps/jar.mn,v  <--  jar.mn
new revision: 1.28; previous revision: 1.27
done
Checking in tools/tinderbox-configs/firefox/linux/CLOBBER;
/cvsroot/mozilla/tools/tinderbox-configs/firefox/linux/CLOBBER,v  <--  CLOBBER
new revision: 1.40; previous revision: 1.39
done
Checking in tools/tinderbox-configs/firefox/win32/CLOBBER;
/cvsroot/mozilla/tools/tinderbox-configs/firefox/win32/CLOBBER,v  <--  CLOBBER
new revision: 1.72; previous revision: 1.71
done

Comment 56

[AKIHIRO Misaki (a.k.a Kuden)]

Mr. Emura pointed this out. 

classic.manifest

bad ("mozapps" cannot be overwrited):
skin global classic/1.0 jar:classic.jar!/skin/classic/luna/global/
skin global classic/1.0 jar:classic.jar!/skin/classic/aero/global/ os=WINNT osversion>=6
skin mozapps classic/1.0 jar:classic.jar!/skin/classic/aero/mozapps/ os=WINNT osversion>=6
skin mozapps classic/1.0 jar:classic.jar!/skin/classic/luna/mozapps/
skin help classic/1.0 jar:classic.jar!/skin/classic/help/
skin browser classic/1.0 jar:classic.jar!/skin/classic/luna/browser/
skin browser classic/1.0 jar:classic.jar!/skin/classic/aero/browser/ os=WINNT osversion>=6
skin communicator classic/1.0 jar:classic.jar!/skin/classic/communicator/

fine:
skin global classic/1.0 jar:classic.jar!/skin/classic/luna/global/
skin global classic/1.0 jar:classic.jar!/skin/classic/aero/global/ os=WINNT osversion>=6
skin mozapps classic/1.0 jar:classic.jar!/skin/classic/luna/mozapps/
skin mozapps classic/1.0 jar:classic.jar!/skin/classic/aero/mozapps/ os=WINNT osversion>=6
skin help classic/1.0 jar:classic.jar!/skin/classic/help/
skin browser classic/1.0 jar:classic.jar!/skin/classic/luna/browser/
skin browser classic/1.0 jar:classic.jar!/skin/classic/aero/browser/ os=WINNT osversion>=6
skin communicator classic/1.0 jar:classic.jar!/skin/classic/communicator/

Comment 57

[AKIHIRO Misaki (a.k.a Kuden)]

I'm sorry. This problem had been fixed. 
{Build ID: 2008030122}

Comment 58

[Serge Gautherie (:sgautherie)]

Comment on attachment 306823 [details] [diff] [review]
Fix gnomestripe/pmstripe and make Vista use aero icons - v2.1
[Checkin: Comment 55]

>Index: toolkit/themes/winstripe/global/jar.mn
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/themes/winstripe/global/jar.mn,v
>retrieving revision 1.44
>diff -u -8 -p -r1.44 jar.mn
>--- toolkit/themes/winstripe/global/jar.mn	1 Mar 2008 03:52:25 -0000	1.44
>+++ toolkit/themes/winstripe/global/jar.mn	2 Mar 2008 02:51:56 -0000
>@@ -1,237 +1,246 @@
> classic.jar:
>+% skin global classic/1.0 %skin/classic/global/ os=WINNT osversion<6

Here and elsewhere, why is this not moved inside the |#ifdef XP_WIN| ?

>+#ifdef XP_WIN
>+classic.jar:
> % skin global classic/1.0 %skin/classic/aero/global/ os=WINNT osversion>=6