419383 - Extract native Vista toolbar styles and expose to Themes

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P2)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

We would like to use the silver toolbar appearance on Vista for the main window of Firefox and the media toolbar appearance (black) in the Library window.

Vlad wrote an app to explore what parts of the native OS theme we can extract, so he will be able to provide more specific details.

Comment 1

[Rob Arnold [:robarnold]]

What we want are rebars, not toolbars. The right theme to use depends on the appearance:
media -> "Media::Rebar"
communcations -> "Communcations::Rebar"
alternate -> "Rebar" or "Alternate::Rebar"

part: RP_BACKGROUND
state: 0

So how should we expose this to themers?

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yep; we need to define new appearance properties.  I think right now the 'toolbox' appearance is the base rebar; we should define something like media-toolbox and communications-toolbox.  The nice thing is that "Media::Rebar" falls back to "Rebar" if the specific subclass isn't found, so we don't have to do anything special for windows.  For now, we can ignore those properties on other platforms I think, or just treat them the same as a normal 'toolbox'.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: add new appearance types

Adds three new appearance types, media-toolbox, communications-toolbox, and browsertabbar-toolbox.  Note that these will all fall back to the normal 'Rebar' style if the specific subclass is not available.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How many of those Vista things are there? Will we eventually want to add a huge number of theme constants to cover them all?

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Nope, should just be these three, at least for 1.9.  If we have time i'd like to revisit how we do native theme stuff for moz2+.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'd like to hear your thoughts about that, I don't have any :-)

Shouldn't you be adding something to nsNativeThemeWin::CloseData?

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated patch

Updated patch, with something now done in CloseTheme().  Also got rid of a bogus hunk that snuck in here that should've been done in the other theme patch (and now is).

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 306204 [details] [diff] [review]
updated patch

+#define CLOSE_THEME(_x)  if (_x) { closeTheme(_x); _x = NULL; }

Macros suck, would you mind "static void CloseTheme(HANDLE* aTheme)"?

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 306204 [details] [diff] [review]
updated patch

One more to request approval on..

roc, I'd rather not do a function since then I can't assign NULL back to the pointer (unless I do a ref to a pointer, but ick).  I mean, I can expand out the macro, just seems cleaner this way.  (Really, I -almost- made a little helper class so you could do  WindowsThemeData mButtonTheme("Button");, but decided that would probably be overkill for now :)

Comment 10

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

Comment on attachment 306204 [details] [diff] [review]
updated patch

a1.9b4=beltzner

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in.

Comment 12

[Jim Jeffery not reading bug-mail 1/2/11]

Unless its something to come later, I am not seeing the 'black' bar in the Library on my Vista HP.  

Today's build:
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4pre) Gecko/2008030106 Minefield/3.0b4pre Firefox/3.0 ID:2008030106

Comment 13

[Wesley Johnston (:wesj)]

Its not hooked up yet, but the patch works with some userchrome.

On another note, while this degrades nicefully in XP, it doesn't theme the text color on the toolbox, meaning that you have to use special theme overrides for XP and Vista to fix it anyway. Is that possible to extract from the theme? Or maybe theres an easier to way to make sure the text is visible that I don't know of.

Comment 14

[Lee Houghton]

From what I can tell, Communications::Rebar has a text colour defined in the theme, while Media::Rebar doesn't... 

There's also the issue that the "Media" and "Communications" subclasses could disappear entirely in the next version of Windows, so if you do then explicitly set the text colour to another colour, it might look a bit odd on newer versions of windows.

Comment 15

[Alex Faaborg [:faaborg] (Firefox UX)]

>There's also the issue that the "Media" and "Communications" subclasses could
>disappear entirely in the next version of Windows

They are encouraging other applications to use these toolbar styles in their user experience guidelines, so they probably will keep them around for legacy applications with Windows 7.  If not we will have plenty of time to create an update while it is in beta.