303110 - add unified toolbar feature to Cocoa widgets

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, enhancement)

Description

[mark]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050802 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050802 Firefox/1.0+

Newest Camino also uses it.

Reproducible: Always

Steps to Reproduce:

Comment 1

[mark]

Attachment: Prefs example

Screenshot of Omniweb Prefs
(http://www.hicksdesign.co.uk/downloads/?c=BrowserResources)

Comment 2

[mark]

Attachment: Prefs example

Screenshot of Omniweb Prefs
(http://www.hicksdesign.co.uk/downloads/?c=BrowserResources)

Comment 3

[Jo Hermans]

(In reply to comment #0)
> Newest Camino also uses it.

Firefox is using Carbon widgets, not Cocoa. That's why Firefox and Camino are
different.

That will change in the future though. See
<http://wiki.mozilla.org/Mac:Cocoa_Widgets>

Comment 4

[Håkan Waara]

(In reply to comment #3)
> (In reply to comment #0)
> > Newest Camino also uses it.
> 
> Firefox is using Carbon widgets, not Cocoa. That's why Firefox and Camino are
> different.

Are you sure that's all? Is it really possible to get this look without a native toolbar (NSToolbar) cooperating with the window? 

Note that the theme authors would have to provide one 10.3-look for the toolbar and one 10.4-look (unified).

> That will change in the future though. See
> <http://wiki.mozilla.org/Mac:Cocoa_Widgets>

I don't see the unified toolbar look mentioned there.

I'd still love to see this fixed though, but I don't know how...

Comment 5

[Colin Barrett [:cbarrett]]

It's possible to fake the appearance of a unified toolbar, yes. In fact any arbitrary pattern or image that you want to draw in the title bar area and below is possible. I have a test app that does exactly this in less than 100 lines of code without any really scary undocumented tricks (there are two undocumented methods used, but those are to control the toolbar pill button that shows/hides the effect -- and yes, this same code can give us back that toolbar show/hide button in cocoa widgets :).

I don't see integrating it into nsCocoaWindow being a huge problem. The "tough" part, at least from my view, is figuring out the interface you want to expose for this to themes. I don't really know about that aspect of the themeing API.

FWIW, my feeling is that we really only want the unified gradient to extend to our main toolbar, as Camino does.

I'm not entirely sure a 10.3-only theme is entirely worth it. Most of our users are on 10.4, and with Leopard shipping in October, even fewer will remain on 10.3.

Comment 6

[Dão Gottwald [:dao]]

(In reply to comment #5)
> I'm not entirely sure a 10.3-only theme is entirely worth it. Most of our users
> are on 10.4, and with Leopard shipping in October, even fewer will remain on
> 10.3.

10.3 isn't supported anymore.

Comment 7

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

Colin: how much work is this, and what effect would making it blocking have on your ability to hit all your other commitments for M8?

Comment 8

[Colin Barrett [:cbarrett]]

Summary: I'd love to see this happen, but it's not a trivial thing. Decisions need to be made regarding the scope of this, and questions about themeing (both OS-level and Firefox-level) need to be made. Also, Mac OS X Leopard complicates things further. As for my M8 commitments, Gecko is already in feature-freeze, and my other commitments at this time are bug fixes.

I wrote a demo program that shows off "faking" a unified toolbar in a normal cocoa app, by using the setBackgroundColor method of NSWindow (I wrote this at the last all hands). I can use that same approach in Firefox. I experimented around with using it in the actual chrome a couple weekends ago and ran into a problem.

Since we don't have any support for translucent windows on Mac, setting the background color of an element to transparent in CSS just draws white -- it doesn't actually allow the background of the window to show through. I'm assuming that there's a switch to flip somewhere to turn that on, but I couldn't find it readily.

Beyond that, the following issues add complexity.
1) The toolbar show/hide button in the top right needs to behave correctly. Since we take over (some of the) drawing in the titlebar, we end up being responsible for drawing the "normal" appearance of the titlebar as well.
2) People who theme their OS would see their themed buttons in the titlebar, but a "normal" appearance for the titlebar and unified toolbar. It's possible to work around this but it makes things more complicated.
3) At the same time we want to support people who want to theme Firefox. As I understand it, getting an image out of CSS and turning that into a cocoa NSImage is a difficult process. Would opting-in for unified mean you basically just get the unified style of Pinstripe? Is this acceptable to themers?

Additionally, visual differences between the current version of Mac OS X (Tiger) and the upcoming version (Leopard) will cause a host of problems for making Firefox look native, and this will be an affected area for sure. I can't say more on a public bug, sorry.

Comment 9

[Håkan Waara]

(In reply to comment #8)
<snip>
> 
> Since we don't have any support for translucent windows on Mac, setting the
> background color of an element to transparent in CSS just draws white -- it
> doesn't actually allow the background of the window to show through. I'm
> assuming that there's a switch to flip somewhere to turn that on, but I
> couldn't find it readily.

I'm probably missing something, but what does translucent windows have to do with a unified toolbar?

In case it is relevant: OS X does have good support for translucent windows (since Tiger); use a NSView with the background filled with [NSColor clearColor] as contentView and draw on that using any opacity.

Comment 10

[Colin Barrett [:cbarrett]]

Gecko does not support translucent windows though, and attempting to use translucency as the background color 

The hack for a unified toolbar is, in a nutshell: setting the backgroundColor property of NSWindow to an NSColor which contains an NSImage. You then move an NSView in place to clip the tiling and place it at the very back of the view hierarchy.

I need the top level nsChildView to draw it's background color as transparent, essentially, and I couldn't get it to do that from CSS. I suspect this is because Gecko doesn't support translucent windows on Mac OS X, bug 307204.

(As an annoying side effect, it would mean that you couldn't have parts of a top level window be translucent, i.e. cutting a hole in the window. However, most of the use cases for translucent windows are for popups or windows with no chrome, so it shouldn't be *that* big of an issue).

Comment 11

[Stephen Horlander [:shorlander]]

(In reply to comment #8)

> Additionally, visual differences between the current version of Mac OS X
> (Tiger) and the upcoming version (Leopard) will cause a host of problems for
> making Firefox look native, and this will be an affected area for sure. I can't
> say more on a public bug, sorry.

Well I think if you went with a Leopard style toolbar it would look "native" to Leopard and acceptable in Panther. The look is more or less the current look of iTunes and iLife 08 so it wouldn't exactly be doing something that was completely foreign to the user. However Leopard is supposed to be "unifying" all toolbars and titlebars system wide. Which means that an application without a unified toolbar would appear rather foreign. So I believe that having a Leopard style appearance in Tiger would be preferable to having a Tiger style appearance in Leopard.

Also as you point out the result of this could negatively impact people using custom themes. I would think, and I don't have data to back this up, that most Mac Firefox users aren't huge theme users. I don't know any personally at least. In addition to that one of the more persistent complaints about Firefox on a Mac is its non-nativeness. So if you had to weigh some 3rd party theme issues vs. going with as native a look as possible, I personally would consider nativeness to weigh quite heavily.

Of course there are technical and practical issues to consider as well. If those outweigh the benefits then it's hard to lobby for this. Still I think it is one of those "little things" that would go a long way towards making Firefox more "Mac-like".

Comment 12

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

I'm weary of making this a blocker just so Colin can work on it. This is a really-really-really want for Firefox 3, but I'm not sure that it's a blocker (to be honest, the blocker feels like bug 307204, which when fixed would make this a lot easier to fix as I understand things)

Also, I agree with Stephen: we should make this look good for 10.5, and that way we're ahead of the game ... like iTunes! :)

Comment 13

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

Connor convinced me that getting unified was a requirement for Mac platform-parity, which means that this blocks.

Comment 14

[Colin Barrett [:cbarrett]]

Just noticed the ETA needed marker.

I've got a 95% complete patch for this that I've been working on on weekends. Hopefully Friday I will be able to get it up, I need to finish hooking up the API for themes.

Comment 15

[Colin Barrett [:cbarrett]]

Attachment: gecko changes v1.0

This patch adds a new property to the XUL window element, titlebarcolor. It also adds a new method to nsIWidget, SetWindowTitlebarColor. This is implemented only on Mac OS X by a trick I learned that allows you to get colors or patterns to draw in the titlebar of a window. This can be used by a theme to simulate a smooth, continuous (unified) gradient between the titlebar and the toolbar.

Some things I changed in this patch that probably require UI-review (they all have come out of discussions I've had with mconnor and beltzner):

 - Windows without the titlebarcolor attribute set will draw using an iTunes 7 / Leopard look. I made a guess at the values using publicly available screenshots, to avoid breaking any NDAs
 - There is no longer a pill button to show/hide the toolbar. If I were to add it back in, I would want to have collapsing the toolbars just have the window use the default appearance. This change would not be that hard to put in -- but it might look strange if someone was doing a non-grey theme.

I first demoed a proof of concept of this back at the April on-site (I wrote a test app demonstrating the titlebar painting technique) and then again at the August on-site (where I demoed an early version of this patch, without any of the content or nsIWidget work). It's been quite a long, slow road implementing this. Thanks to everyone who has helped out along the way (so far!) including:

Gus Mueller (inspiration for the original demo)
bz (for help with the content & XUL portions)
mconnor & beltzner & faaborg & sdwilsh (for not shutting up about this bug ;)

In a patch I'll post shortly I'll show an example of how to use XBL to apply the titlebarcolor attribute to make the titlebar orange for all browser windows.

Comment 16

[Colin Barrett [:cbarrett]]

Attachment: browser changes (demo, please don't ship this) 1.0

Here's the sample usage I talked about in the above comment. It's the best my poor design skills could muster ;)

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 281276 [details] [diff] [review]
gecko changes v1.0

>+++ mozilla.fd42a14408f5/content/xul/content/src/nsXULElement.cpp	2007-09-17 21:57:19.000000000 -0700
>+            attrValue.ParseColor(*aValue, GetCurrentDoc());

This will crash if GetCurrentDoc() returns null.  You probably want to skip this whole block if it's null, right?

>+nsresult
>+nsXULElement::SetTitlebarColor(nscolor aColor)

Just make this a void method.  No one looks at the return value anyway.

>+    if (!doc || doc->GetRootContent() != this)
>+      return NS_ERROR_UNEXPECTED;

Please put curlies in content code even around one-line if bodies.  Same elsewhere in this patch.

>+++ mozilla.fd42a14408f5/widget/public/nsIWidget.h	2007-09-17 21:57:19.000000000 -0700
>+    /**
>+     * Set the color of the window titlebar for this widget.

Which color?  Foreground?  Background?  Something else?

>+     * @param aColor Any color with an alpha of 0 (fully transparent) will cause
>+     *               the window to draw with the default style for the platform

And what about an alpha > 0 but < 1?  Treated as 1?

I assume that Josh will review the cocoa changes...

Comment 18

[Josh Aas]

"Connor convinced me that getting unified was a requirement for Mac platform-parity, which means that this blocks."

What does "platform parity" mean in this context?

Comment 19

[Colin Barrett [:cbarrett]]

(In reply to comment #17)
> (From update of attachment 281276 [details] [diff] [review])
> >+++ mozilla.fd42a14408f5/content/xul/content/src/nsXULElement.cpp	2007-09-17 21:57:19.000000000 -0700
> >+            attrValue.ParseColor(*aValue, GetCurrentDoc());
> 
> This will crash if GetCurrentDoc() returns null.  You probably want to skip
> this whole block if it's null, right?

Yeah.

> >+nsresult
> >+nsXULElement::SetTitlebarColor(nscolor aColor)
> 
> Just make this a void method.  No one looks at the return value anyway.
> 
> >+    if (!doc || doc->GetRootContent() != this)
> >+      return NS_ERROR_UNEXPECTED;
> 
> Please put curlies in content code even around one-line if bodies.  Same
> elsewhere in this patch.

Will do.

> >+++ mozilla.fd42a14408f5/widget/public/nsIWidget.h	2007-09-17 21:57:19.000000000 -0700
> >+    /**
> >+     * Set the color of the window titlebar for this widget.
> 
> Which color?  Foreground?  Background?  Something else?

On OS X it is probably most accurately the "background" of the titlebar that is being set -- buttons draw on top of the color.
 
> >+     * @param aColor Any color with an alpha of 0 (fully transparent) will cause
> >+     *               the window to draw with the default style for the platform
> 
> And what about an alpha > 0 but < 1?  Treated as 1?

What scale are you talking about, 0 - 1 or 0 - 255 (nscolor uses 0-255, Cocoa uses 0-1, fwiw)?

> I assume that Josh will review the cocoa changes...

Yeah, that was the intent (sorry if that wasn't clear)

Comment 20

[Mike Connor [:mconnor]]

Josh, I think that means "platform-nativeness" rather than parity.

Comment 21

[Colin Barrett [:cbarrett]]

Attachment: gecko patch 1.1

address bz's review comments. Other than that, I fixed an oversight where colors with a coverage channel would have it ignored. This wasn't my intention -- I think that got left over from debugging or something. Only the coverage value of 0 is "magic".

Comment 22

[Boris Zbarsky [:bzbarsky]]

I'm still not happy with the "primary color" thing.  First, it does not say whether we're talking about background or foreground.  Second, what does this function do on platforms where the default titlebar background is a gradient?

I really think it should be clearly spelled out what callers can expect and what implementors need to do if they decide to implement this method, basically.

> What scale are you talking about, 0 - 1 or 0 - 255

The latter, since this is nscolor.  Does one get partially transparent titlebars?  Or does something else happen?

Comment 23

[Colin Barrett [:cbarrett]]

(In reply to comment #22)

> Second, what does this
> function do on platforms where the default titlebar background is a gradient?

It sets it to a single solid color. That should be spelled out, yes.

> > What scale are you talking about, 0 - 1 or 0 - 255
> 
> The latter, since this is nscolor.  Does one get partially transparent
> titlebars?  Or does something else happen?

On OS X it is possible to set the titlebar to a color with alpha. It might look good if you set it to a slightly opaque color? But at that point what you really want is translucent XUL windows. I'm not sure if we should allow or disallow it, so I defaulted to allow. Input from beltzner or mconnor on that would be fantastic.

By the way, this particular patch makes translucent XUL toplevel/dialog windows difficult to do on Mac (popups are made no harder, and that is the vast majority of the use cases, according to mfinkle), but it doesn't look like translucent windows will make it on mac for 1.9, so we'll have time to do additional work on that later.

I've got a revised patch with the documentation spelled out a little clearer.

Comment 24

[Colin Barrett [:cbarrett]]

Attachment: gecko patch 1.2

aforementioned patch

Comment 25

[Håkan Waara]

Why does this make it harder to make translucent XUL windows?

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 281456 [details] [diff] [review]
gecko patch 1.2

>+++ mozilla/widget/public/nsIWidget.h	2007-09-19 
> ...                  On Mac, the
>+     *               default style is a grey gradient and a single pixel border

I don't think you need that sentence.

r=bzbarsky with that.

Comment 27

[Colin Barrett [:cbarrett]]

(In reply to comment #25)
> Why does this make it harder to make translucent XUL windows?

It works by taking advantage of NSWindow drawing the NSColor returned by [window backgroundColor] through the entire area of the window, including the titlebar, when you set the Textured (metal) bit on the window. There's a bunch (more) documentation in the patch.

Part of translucent XUL windows is having the backgroundColor of the window be [NSColor clearColor], no? We can't do that for toplevel/dialog windows, because we need them to use the ToolbarWindow subclass.

Although now that you mention it, I suppose the algorithm for determining when to use the ToolbarWindow subclass should look at the style mask instead of checking the window type, so as not to use the subclass where it's not needed.

(In reply to comment #26)
> (From update of attachment 281456 [details] [diff] [review])
> >+++ mozilla/widget/public/nsIWidget.h	2007-09-19 
> > ...                  On Mac, the
> >+     *               default style is a grey gradient and a single pixel border
> 
> I don't think you need that sentence.
> 
> r=bzbarsky with that.

Done.

Comment 28

[Stephen Horlander [:shorlander]]

Attachment: Window Widget Alignment after patch.

Just some nit-picky style/appearance comments:

When you have a window without a toolbar the window widgets (Close/Minimize/Zoom) are pushed down 1 px. too far. This leaves about 5 px. from the top and 1 px. from the bottom making them look misaligned. Currently in Tiger(10.4) all textured windows are like this(because they either have a toolbar or are all one piece) but in Leopard(from what I have seen) this won't be the case. Looks like all window widgets will be 4 px. from the top and 2 px. from the bottom. See attachment.

I was thinking it would be a non-issue if they get their positioning from the OS because Leopard will fix it. However since Tiger compatibility is being maintained it will still look "off" there.

Also in regards to color values you can pull those from iTunes/iLife NDA free.

Light(top) gradient color appears to be: r=196, g=196, b=196
Bottom(dark) gradient color appears to be:  r=149, g=149, b=149
Separator line appears to be: r=64, g=64, b=64

Might be slightly variable but I think that is pretty close.

And finally a question: How is this going to work exactly for making it look unified?

I looked at the patch as I applied it but it isn't obvious to me how this is going to work. The patch without styling creates the default grey look but the styling is for just the titlebar not the toolbar. After styling with "titlebarcolor='orange'" it creates a solid orange titlebar. Does that attribute take other properties besides solid colors? How do you style it to have the requisite faux unified gradient? Sorry if that is obvious :)

Comment 29

[Colin Barrett [:cbarrett]]

(In reply to comment #28)
> Created an attachment (id=281682) [details]
> Window Widget Alignment after patch.
> 
> Just some nit-picky style/appearance comments:
> 
> When you have a window without a toolbar the window widgets
> (Close/Minimize/Zoom) are pushed down 1 px. too far. This leaves about 5 px.
> from the top and 1 px. from the bottom making them look misaligned. Currently
> in Tiger(10.4) all textured windows are like this(because they either have a
> toolbar or are all one piece) but in Leopard(from what I have seen) this won't
> be the case. Looks like all window widgets will be 4 px. from the top and 2 px.
> from the bottom. See attachment.

Yeah. I know :( I have a potential fix for this (which also should help the XUL transparent window issue), but I'm really hesitant to post it, since it involves overriding a method on a private class (NSGrayFrame's +drawBezel:inFrame:topCornerRounded:bottomCornerRounded:, which I just changed to call its superclass's implementation, which worked OK). I also haven't tested it on Leopard.

> I was thinking it would be a non-issue if they get their positioning from the
> OS because Leopard will fix it. However since Tiger compatibility is being
> maintained it will still look "off" there.
> 
> Also in regards to color values you can pull those from iTunes/iLife NDA free.
> 
> Light(top) gradient color appears to be: r=196, g=196, b=196
> Bottom(dark) gradient color appears to be:  r=149, g=149, b=149
> Separator line appears to be: r=64, g=64, b=64

Thanks!

> Might be slightly variable but I think that is pretty close.
> 
> And finally a question: How is this going to work exactly for making it look
> unified?
> 
> I looked at the patch as I applied it but it isn't obvious to me how this is
> going to work. The patch without styling creates the default grey look but the
> styling is for just the titlebar not the toolbar. After styling with
> "titlebarcolor='orange'" it creates a solid orange titlebar. Does that
> attribute take other properties besides solid colors? How do you style it to
> have the requisite faux unified gradient? Sorry if that is obvious :)

It's not obvious, and it's not the best of solutions, but the idea is to set the titlebar to a solid color and then start a gradient with that color below it. (I think you want to set the background image of #toolbox?). This was quicker to implement, and we are quite late in the cycle. Hopefully it's better than nothing...

Comment 30

[Colin Barrett [:cbarrett]]

Attachment: fix v1.3

Address bz's last comment, and make the change I talked about above.

I wasn't just able to check the title bit of the features mask, though, since we're marking sheets as having titlebars (and other window features as well. AFAIK that's entirely unnecessary. I saw mentioned in another bug (can't find it now) that we need to clean up the way we map XUL window features / border styles. Is this still true? Anyway, that's a different bug.

Also changed the colors to match Steven's comments.

Comment 31

[Stephen Horlander [:shorlander]]

Attachment: Unified test

(In reply to comment #29)
> It's not obvious, and it's not the best of solutions, but the idea is to set
> the titlebar to a solid color and then start a gradient with that color below
> it. (I think you want to set the background image of #toolbox?). This was
> quicker to implement, and we are quite late in the cycle. Hopefully it's better
> than nothing...

Yes, this makes sense. Although for some reason I thought that it would be possible to put some kind of image background into the titlebar. I guess what I was envisioning from the description was ending up with two gradients, one in the titlebar and one in the toolbar, that butted up against each other to create the illusion of a true unified window.

Anyway :) I messed around with it a bit after your suggestion and got a pretty good result.

Comment 32

[Colin Barrett [:cbarrett]]

Attachment: fix v1.3.1

Update for trunk (josh changed nsIWidget, so it wasn't applying cleanly).

Comment 33

[Josh Aas]

Comment on attachment 282603 [details] [diff] [review]
fix v1.3.1

One thing that I find worrisome is that all top-level windows and dialogs use this new window setup where you set NSTexturedBackgroundWindowMask, use an obscure  non-public API, and insert a foreign view into our hierarchy, unified or not. This makes me extremely nervous. There is a good enough chance that this is going to cause problems with Firefox, but even more likely is that we'll cause problems for others using the XUL platform, who aren't using unified. Those people aren't going to find out the trouble they are in until they've moved their apps to Gecko 1.9, which won't happen to enough of an extent before Gecko 1.9 is released.

If people want to depend on this hack (and that is what this is, even if it is clever) by turning on the unified toolbar window, they should be the only ones affected by it. So if somebody doesn't have the unified toolbar setup, I don't want this code used for them.

Also, what happens to users who don't want to use unified but do want to use the pill button in the top right the way we were? Firefox prefs? The platform capability for that should be there whether or not Firefox uses it anyway. We shouldn't be forcing people to live in a unified world.

Another major problem, which might have to get solved per-app if we're going to do this, is that the entire toolbar in a unified app drags the window. We have to do that in Firefox. Otherwise we're setting up that functional expectation for our users and then failing them with a half-baked impl. Is there a plan for fixing this? We should have one in place before we commit to this. IIRC songbird figured out how to do it at one point though I'm not sure if what they did is acceptable for Firefox.

That's it for my first high-level technical review of this patch. On a more subjective note, I don't like this at all. We're making heavy hack modifications to a major part of our Mac OS X platform for some visual bling. Also, I don't buy it that we can't look like a first-class mac app without unified. The thin line between a window's content and the title bar can't be that much of a design obstacle.

We may or may not be able to overcome some of the major problems I outlined here, but either way I recommend dropping this unified idea. It is going to be a lot of dangerous work.

Comment 34

[Aronnax]

Hi Josh, 
i can understand that you have a problem with a "hack" - with limited developer resources - a narrows timeline and so on 

Butthis is a question between black and white and everything depends on this bug.

Its not only the thin line. The title-bar on Mac OS X 10.4 is much more brightly then the one from the upcoming 10.5.
A theme which should really work well with both systems is not possible - surely not . 
The 2.0 theme is even now a compromise and nobody is happy with it. A compromise which should work then with these very different title-bar styles will be even much more a very "dirty" hack. 

And Firefox on 10.4 has now more or less only another variation below many different toolbar styles (Aqua, Brushed Metal, the brightly and the dark Unified and the Pro Apps. style) and this will be changed with Mac OS X 10. 5.  Almost very app. will have then the dark Unified Toolbar. 
Everyone knows that the Mac user reactions on the current default Firefox variation are not as good as they should be. But this would be nothing compared with the upcoming reactions as soon 10.5 arrives. A different toolbar will then attract much attention - and together with a "dirty" 10.4 and 10.5 compromise - it will looks much, much more out of place as already.

Only the unified toolbar patch make it possible to build a theme which fit well on both systems.
Only the unified toolbar patch will make it possible that Firefox 3.0 will fit well in Leopard.

And other third part themes on a much higher level are possible.
With these patch have the themes the control of the look of the title-bar. Complete different colored themes are then for example possible. A huge advantage and i can tell it only for myself - with these patch will i build themes with the Leopard grey and complete blue variations  - maybe others as well  - without the unified toolbar patch will i very likely quit my "GrApple" themes (because of the same reasons as mentioned before which affect the as well default theme) A theme which bring back the 2.0 look is then as well no problem. 

---
and 
" .. the pill button in the top right the way we were?"
come on, it is no reason - have Safari a pill button? no - have someone a problem with it: no 

"We shouldn't be forcing people to live in a unified world."
A missing unified toolbar patch forces every Mac Firefox user to live with a unified "dirty" compromise - only this patch will makes a huge spectrum  of variations possible 

"that the entire toolbar in a unified app drags the window" 
this is still a big flaw - can maybe fixed - maybe not - but it is a only a big flaw on windows like the Places-Organizer -  the normal  browser window is full of buttons and other stuff that nobody will really be tempted to drags the window there - in other word: it s a bigger bug, but it is easy to life with it 

Cheers 

Comment 35

[Colin Barrett [:cbarrett]]

I removed the pill button after speaking with beltzner and mconnor. I could easily add it back. The theme would be responsible for any styling changes, though.

I'd like to point out, though, that my fix for the pill button that landed a number of weeks ago also uses an "obscure, private API" (I found it through classdump and fscript-anywhere). However, unlike that technique, the unified technique is quite well documented:

http://www.rogueamoeba.com/utm/posts/Article/polished-metal-2005-09-10-10-00.html
http://www.gusmueller.com/blog/archives/2005/9/8.html#1323
http://mattgemmell.com/2005/09/09/itunes-5-window-style-code

Perhaps it wasn't clear, btw, but when the theme specifies no titlebar color, we use a normal looking window header, but it is dark in color, Leopard-style. An app like SeaMonkey will look fine and not require any theme changes.

I agree about dragging by the toolbar. It's one of the issues that I had not solved yet.

As for inserting a foreign view, I can remove that as well. I wrote an NSColor subclass that handles everything for us nicely. I was hoping to do that in a follow up bug, though.

As for wether or not this is necessary to blend in, take a look at all of the apps Apple is shipping on Leopard. Count the number that don't have a unified toolbar, and count the number that do. I think it's pretty evident that it's the way of the future.

So, Josh, what do I need to do to get this r+'d from you? Let's figure out a way to address your concerns.

Comment 36

[Josh Aas]

"I'd like to point out, though, that my fix for the pill button that landed a
number of weeks ago also uses an "obscure, private API" (I found it through
classdump and fscript-anywhere)."

The big difference is that we needed to use that private API to fix a regression, and not having unified is not a regression. I still don't like what we had to do there but we had no choice. Here we have a choice.

I'll look into what you said more in a bit, I just wanted to clarify my comment about using private APIs. We should always think long and hard about depending on private APIs when we have a choice.

Comment 37

[Josh Aas]

As for this being a well-documented technique, first of all this is a few guys making blog posts about neat tricks and secondly being documented in this sense does not mean well-tested. I suspect the number of apps that have done this and shipped to customers is a very small number, probably zero. Not comforting at all for a windowing technique we're thinking about building Gecko's Mac OS X support on.

Comment 38

[Josh Aas]

I think the next two steps that need to be taken towards getting this ready (though I'm not promising anything) are:

1) Come up with a technique for window dragging via toolbar chrome. If there isn't a good way to do this then everyone who uses unified is going to use it in a half-baked state so adding unified is going to encourage bad OS integration and functional behavior and won't be worth it. This technique doesn't necessarily need to be part of this patch so long as window dragging via the title bar isn't broken for regular windows, but we should have a technique for apps that use unified to use, preferably one that can be implemented in our toolkit's generic toolbar impl. I want to know that this exists before we add the infrastructure for a feature that won't be complete without it.

2) Get rid of the view insertion and use an NSColor subclass. I'm not going to take it on faith that it is possible to do this in a reasonable way later.

Comment 39

[Colin Barrett [:cbarrett]]

(In reply to comment #37)
> I suspect the number of apps that have done this and
> shipped to customers is a very small number, probably zero.

Untrue. VoodooPad (http://flyingmeat.com/voodoopad) uses it. Rogue Amoeba is shipping their brand new app, Radioshift (http://rogueamoeba.com/radioshift/) using it as well. The reputations of both Flying Meat and of Rogue Amoeba speak for themselves.

(In reply to comment #38) 
> 1) Come up with a technique for window dragging via toolbar chrome. If there
> isn't a good way to do this then everyone who uses unified is going to use it
> in a half-baked state so adding unified is going to encourage bad OS
> integration and functional behavior and won't be worth it. This technique
> doesn't necessarily need to be part of this patch so long as window dragging
> via the title bar isn't broken for regular windows,

Dragging by the titlebar works fine right now.

> but we should have a
> technique for apps that use unified to use, preferably one that can be
> implemented in our toolkit's generic toolbar impl. I want to know that this
> exists before we add the infrastructure for a feature that won't be complete
> without it.

I'll create another bug on this, and mark it as blocking this one.

> 2) Get rid of the view insertion and use an NSColor subclass. I'm not going to
> take it on faith that it is possible to do this in a reasonable way later.

OK.

Comment 40

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

>On a more subjective note, I don't like this at all. We're making heavy hack
>modifications to a major part of our Mac OS X platform for some visual bling.

If we want to look at this quantitatively, here are the top 5 requested changes to Firefox based on the >1000 emails Colin received for his blog post (http://iamthewalr.us/blog/2007/04/20/firefox-on-the-mac/)

1. performance	21%
2. theme	12%
3. native widgets in the content area	11%
4. keychain support	10%
5. startup time	9% 

I personally don't look at the unified toolbar as "some visual bling" as much as our second most reported issue impacting overall user experience on OS X.  Now of course the reporters were entirely self selecting, and the demographic is purely digg/reddit/tuaw readers, but if anything I figured that would increase the scores for more technical issues, and not the overall visual appearance.

Comment 41

[Josh Aas]

"complaints about theme" != "complaints about not having a unified toolbar"

Not having a unified toolbar is by no stretch of the imagination equivalent to our #2 complaint. Our theme has way more issues than not having a unified toolbar, looking at it that way is placing the blame way too heavily/unfairly on one issue.

Anyway, we'll hopefully get unified into acceptable shape soon.

Comment 42

[Wayne Woods]

True that a blog like that is selecting for the kinds of people who are technically picky about such things. But even then, to me "theme" as a popular concern suggests "the current theme doesn't look good enough", not "the current theme is technically at odds with Apple's upcoming GUI".

I can understand why people want this to look as compatible as possible with Leopard since the releases will be close, but this sounds like a huge change very late in the game. And if Josh thinks this is too high risk, maybe we should concentrate on getting a great Mac theme out of our current codebase rather than risk introducing unforeseen bugs and compatibility issues so close to the end. I'm not having a stab at the quality of the code... there's no way in a million years I could produce it... but it could just be a case of it being a shame it didn't land sooner when the trunk was more experimental.

Comment 43

[Colin Barrett [:cbarrett]]

Attachment: fix v2.0

No content/ changes, so bz's review should still stand.

I've done a couple things in this patch:
1) Ditched the whole foreign view thing. We now use an NSColor subclass that overrides -set to set a pattern as the fill. We then draw our titlebar color in the top 22 pixels for the window and the background color in the rest, in the pattern's draw callback. Also unbreaks future work on XUL translucent windows.
2) Added back in the toolbar pill button.
3) Improved saftey when calling undocumented methods.
4) Added a method for people who want to test the background color of the content area that's usable on both NSWindows and ToolbarWindows.

I have some ideas on how to do dragging by the toolbar. Heres one: when a mouse down event comes in to a frame, we would check if the frame has a "dragging this moves the window" bit set on it. If so, we would signal back to widget that the drag that's about to begin is going to move the window. (I don't think we can do that kind of testing of frame types in widget) Widget then would watch the mouse moved events during the drag and update the window position accordingly.

Honestly I'm surprised I was able to finish these changes so quickly. I had them prototyped in a test app, but I didn't expect it to go this smoothly!

Comment 44

[Asaf Romano (gone)]

Attachment: required front-end changes

These are the required front-end changes for this to work across all windows.

With that applied, |titlebarcolor| seems to have no affect on "dialog" windows (window opened with the "dialog" feature) or maybe it's the different root-element that's confusing you, I didn't read the content/ changes.

Comment 45

[Asaf Romano (gone)]

Also, the default color (i.e. non-unified) seems to be much darker than 10.4's non-unified titlebar-color.

Comment 46

[Asaf Romano (gone)]

And, color="-moz-Dialog" doesn't seem to work either :(

Comment 47

[Aronnax]

(In reply to comment #45)
> Also, the default color (i.e. non-unified) seems to be much darker than 10.4's
> non-unified titlebar-color.
> 
this is not a bug - it`s a necessary feature to create one theme for Mac 10.4 and 10.5 which looks then all of a piece  

Comment 48

[Colin Barrett [:cbarrett]]

(In reply to comment #44)
> Created an attachment (id=283898) [details]
> required front-end changes
> 
> These are the required front-end changes for this to work across all windows.
> 
> With that applied, |titlebarcolor| seems to have no affect on "dialog" windows
> (window opened with the "dialog" feature) or maybe it's the different
> root-element that's confusing you, I didn't read the content/ changes.

It's an oversight on my part. Updated patch coming up soon.

Comment 49

[Colin Barrett [:cbarrett]]

Attachment: patch v2.1

This is a change to the changes in content/, that allow titlebarcolor to be set on any root XUL node (not just window).

It's a very small change, here's the interdiff:

diff -r 8430dd6a3660 content/xul/content/src/nsXULElement.cpp
--- a/content/xul/content/src/nsXULElement.cpp  Sat Oct 06 03:04:26 2007 -0700
+++ b/content/xul/content/src/nsXULElement.cpp  Mon Oct 08 12:56:42 2007 -0700
@@ -1049,9 +1049,10 @@ nsXULElement::AfterSetAttr(PRInt32 aName
             HideWindowChrome(aValue && NS_LITERAL_STRING("true").Equals(*aValue));
         }
 
+        // titlebarcolor is settable on any root node (windows, dialogs, etc)
         nsIDocument *document = GetCurrentDoc();
         if (aName == nsGkAtoms::titlebarcolor &&
-            mNodeInfo->Equals(nsGkAtoms::window) && document) {
+            document && document->GetRootContent() == this) {
 
             nscolor color = NS_RGBA(0, 0, 0, 0);
             nsAttrValue attrValue;
@@ -1304,7 +1305,7 @@ nsXULElement::UnsetAttr(PRInt32 aNameSpa
         }
 
         if (aName == nsGkAtoms::titlebarcolor &&
-            mNodeInfo->Equals(nsGkAtoms::window)) {
+            doc && doc->GetRootContent() == this) {
             // Use 0, 0, 0, 0 as the "none" color.
             SetTitlebarColor(NS_RGBA(0, 0, 0, 0));
         }

Comment 50

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 284026 [details] [diff] [review]
patch v2.1

Makes sense.

Comment 51

[Josh Aas]

Comment on attachment 284026 [details] [diff] [review]
patch v2.1

+  if (![mWindow isKindOfClass:[ToolbarWindow class]])
+    return NS_ERROR_NOT_IMPLEMENTED;

This seems like the wrong error. This is a problem with the caller, not an acknowledged or intended lack of implementation.

+  float result = (*aIn) * (196/255.0) + (1.0f - *aIn) * (149/255.0);

Please explain your use of those constants in a comment.

+  float titlebarHeight = 22.0f;

Can you find a dynamic way of arriving at that number? What if an OS release changes the height of a standard titlebar?

+    [[NSColor colorWithDeviceWhite:64/255.0f alpha:1.0f] set];

Explain the numbers used here in a comment.

+// This is an internal Apple class, which we need to work around a bug in. It is
+// the class responsible for drawing the titlebar for metal windows. It actually
+// is a few levels deep in the inhertiance graph, but we don't need to know about
+// all its superclasses.
+@interface NSGrayFrame : NSObject

This is not good. There is no way to do this in a more sane way? This is a perfect example of what I meant when I said once you go down the path of relying on a total hack you end up digging yourself deeper and deeper. If there is nothing you can do about this go ahead and fix all the other stuff while I decide whether or not this is acceptable.

Comment 52

[Colin Barrett [:cbarrett]]

(In reply to comment #51)
> (From update of attachment 284026 [details] [diff] [review])
> +  if (![mWindow isKindOfClass:[ToolbarWindow class]])
> +    return NS_ERROR_NOT_IMPLEMENTED;
> 
> This seems like the wrong error. This is a problem with the caller, not an
> acknowledged or intended lack of implementation.

Should the documentation be changed then too? Right now it says that it returns NOT_IMPLEMENTED if the widget does not support it (and a widget that is not a ToolbarWindow does not support this). Should it return NS_ERROR_FAIL? In what circumstances?
 
> +  float result = (*aIn) * (196/255.0) + (1.0f - *aIn) * (149/255.0);
> 
> Please explain your use of those constants in a comment.
> 
> +    [[NSColor colorWithDeviceWhite:64/255.0f alpha:1.0f] set];
> 
> Explain the numbers used here in a comment.

Sorry, I had both of these in #defines until just recently. Should I put them back that way, or just add a comment?

 
> +  float titlebarHeight = 22.0f;
> 
> Can you find a dynamic way of arriving at that number? What if an OS release
> changes the height of a standard titlebar?

Would it be acceptable to calculate this once, and store it in a static variable?  


> +// This is an internal Apple class, which we need to work around a bug in. It
> is
> +// the class responsible for drawing the titlebar for metal windows. It
> actually
> +// is a few levels deep in the inhertiance graph, but we don't need to know
> about
> +// all its superclasses.
> +@interface NSGrayFrame : NSObject
> 
> This is not good. There is no way to do this in a more sane way? This is a
> perfect example of what I meant when I said once you go down the path of
> relying on a total hack you end up digging yourself deeper and deeper. If there
> is nothing you can do about this go ahead and fix all the other stuff while I
> decide whether or not this is acceptable.

This is only really needed for a titled window with a transparent content area background color. It isn't a problem until we do that, but when we do, it becomes very noticeable. 

Note that I am very unhappy with this solution as well. There are perhaps some other solutions to this problem if this one is completely unacceptable (perhaps some trickery with the clipping rect or frame sizes?), but I have to say I don't have any super promising leads that are less hacky.



One solution that we should definitely consider for the future (post 1.9( is to handle all of the titlebar drawing ourselves. We can get the stoplight buttons easily (with a public API). I had talked with Mano about doing all of our titlebar drawing with XUL. It would make us more flexible in terms of the unified impl for themes and would simplify the dragging code as well. However, we would need XUL transparent windows to do the rounded corners. I also don't think it's an acceptable solution for 1.9 given how far we are.

Comment 53

[Josh Aas]

> Should the documentation be changed then too?

If you change the impl, change the docs. I don't know if fail is the right choice, there are other error types in or repertoire right?

> Sorry, I had both of these in #defines until just recently. Should I put them
> back that way, or just add a comment?

Do what you think is best I guess. At least a comment. Maybe const instead of a macro is another option?

> Would it be acceptable to calculate this once, and store it in a static
> variable?

Please do, but make sure the solution takes into account small title bars (think font/color panels). I don't think there is a way for that to be an issue in Gecko but you should make sure.

Comment 54

[Colin Barrett [:cbarrett]]

Attachment: Patch v2.2

(In reply to comment #53)
> > Should the documentation be changed then too?
> 
> If you change the impl, change the docs. I don't know if fail is the right
> choice, there are other error types in or repertoire right?

I've implemented your other suggestions, but the only thing I can find for this is NS_ERROR_ILLEGAL_VALUE which is also NS_ERROR_INVALID_ARG. Not that happy with it. What do you think?

Comment 55

[Josh Aas]

Comment on attachment 284801 [details] [diff] [review]
Patch v2.2

About NS_ERROR_NOT_IMPLEMENTED, can we just go with NS_ERROR_FAILURE and write something out to the console? Maybe via NS_WARNING.

This patch no longer applies.

Comment 56

[Colin Barrett [:cbarrett]]

(In reply to comment #55)
> (From update of attachment 284801 [details] [diff] [review])
> About NS_ERROR_NOT_IMPLEMENTED, can we just go with NS_ERROR_FAILURE and write
> something out to the console? Maybe via NS_WARNING.

Sounds reasonable.
 
> This patch no longer applies.

OK.

Comment 57

[Josh Aas]

This bug is now a widget bug, concerning the feature being added to Cocoa widgets. This is not about Firefox's use of the feature. The Firefox folks will track that through another bug, probably the theme bug for the theme that will use this feature.

Comment 58

[Colin Barrett [:cbarrett]]

(In reply to comment #56)
> (In reply to comment #55)
> > (From update of attachment 284801 [details] [diff] [review] [details])
> > About NS_ERROR_NOT_IMPLEMENTED, can we just go with NS_ERROR_FAILURE and write
> > something out to the console? Maybe via NS_WARNING.
> 
> Sounds reasonable.

Since the specific method return value is a) largely inconsequential to callers and b) is not irregular (from the rest of nsIWidget anyway), I removed the section of the documentation describing the return value. 

I changed the return value though as per your suggestion, though.

Patch coming up.

Comment 59

[Colin Barrett [:cbarrett]]

Attachment: fix v2.3

Comment 60

[Josh Aas]

> @end
>+
>+@end
>+

You shouldn't need two here. See compiler warning for nsCocoaWindow.mm.

Comment 61

[Josh Aas]

Comment on attachment 286339 [details] [diff] [review]
fix v2.3

+@interface TitlebarAndBackgroundColor : NSColor
+{
+	NSColor *mTitlebarColor;
+	NSColor *mBackgroundColor;
+	NSWindow *mWindow; // [WEAK] (we are owned by the window)
+  float mTitlebarHeight;
+}

You're mixing tabs and double spaces here.

++ (void)drawBevel:(NSRect)bevel inFrame:(NSRect)frame topCornerRounded:(BOOL)top bottomCornerRounded:(BOOL)bottom
+{
+	if ([self respondsToSelector:@selector(drawBevel:inFrame:topCornerRounded:)])
+		[self drawBevel:bevel inFrame:frame topCornerRounded:top];
+}

You're also using tabs here.

+- (void)delloc

Umm... can I get a "dealloc"?

Comment 62

[Josh Aas]

+// This is the grey for the border at the bottom of the titlebar
+static const float sTitlebarBorderGrey = 64/255.0f;
+
+// Callback used by the default titlebar shading.

Additionally, please consistently write comments with the same structure. If it is a full sentence, capitalize and end with a period.

Comment 63

[Colin Barrett [:cbarrett]]

Attachment: patch v2.4

Address review comments. Should apply with -p0 this time.

Comment 64

[Josh Aas]

Comment on attachment 286606 [details] [diff] [review]
patch v2.4

+  else
+    [(ToolbarWindow*)mWindow setTitlebarColor:[NSColor colorWithDeviceRed:NS_GET_R(aColor)/255.0
+                                                                    green:NS_GET_G(aColor)/255.0
+                                                                     blue:NS_GET_B(aColor)/255.0
+                                                                    alpha:NS_GET_A(aColor)/255.0]];

Where something spans more that one line please use block braces for the "if" statement, even if it isn't strictly necessary. Remember to add braces to everything then, the "if" and the "else".

+        && (features & NSTitledWindowMask))

The && goes at the end of the preceding line.

+  TitlebarAndBackgroundColor *s = (TitlebarAndBackgroundColor*)aInfo;

"s" is a confusing variable name. Please make it more descriptive (even "c" would make more sense, but we can do even better probably).

+  float component = 1;

That should be "1.0" since we're talking about a float.

Problems are getting less serious, getting closer.

Comment 65

[Colin Barrett [:cbarrett]]

Attachment: patch2.5

Comment 66

[Josh Aas]

Comment on attachment 286630 [details] [diff] [review]
patch2.5

"self" is pretty much more confusing than "s", change to "color" on checkin

Comment 67

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

Comment on attachment 286630 [details] [diff] [review]
patch2.5

I can tell this is evil, but there's nothing else obviously wrong with it.

Comment 68

[Colin Barrett [:cbarrett]]

Checking in content/base/src/nsGkAtomList.h;
/cvsroot/mozilla/content/base/src/nsGkAtomList.h,v  <--  nsGkAtomList.h
new revision: 3.85; previous revision: 3.84
done
Checking in content/xul/content/src/nsXULElement.cpp;
/cvsroot/mozilla/content/xul/content/src/nsXULElement.cpp,v  <--  nsXULElement.cpp
new revision: 1.737; previous revision: 1.736
done
Checking in content/xul/content/src/nsXULElement.h;
/cvsroot/mozilla/content/xul/content/src/nsXULElement.h,v  <--  nsXULElement.h
new revision: 1.262; previous revision: 1.261
done
Checking in widget/public/nsIWidget.h;
/cvsroot/mozilla/widget/public/nsIWidget.h,v  <--  nsIWidget.h
new revision: 3.163; previous revision: 3.162
done
Checking in widget/src/cocoa/nsCocoaWindow.h;
/cvsroot/mozilla/widget/src/cocoa/nsCocoaWindow.h,v  <--  nsCocoaWindow.h
new revision: 1.43; previous revision: 1.42
done
Checking in widget/src/cocoa/nsCocoaWindow.mm;
/cvsroot/mozilla/widget/src/cocoa/nsCocoaWindow.mm,v  <--  nsCocoaWindow.mm
new revision: 1.112; previous revision: 1.111
done
Checking in widget/src/xpwidgets/nsBaseWidget.cpp;
/cvsroot/mozilla/widget/src/xpwidgets/nsBaseWidget.cpp,v  <--  nsBaseWidget.cpp
new revision: 1.163; previous revision: 1.162
done
Checking in widget/src/xpwidgets/nsBaseWidget.h;
/cvsroot/mozilla/widget/src/xpwidgets/nsBaseWidget.h,v  <--  nsBaseWidget.h
new revision: 1.88; previous revision: 1.87
done

Comment 69

[Colin Barrett [:cbarrett]]

So the Lk numbers (trace_malloc) went up from 957KB to 1.06MB. Could be an OS leak, could be a lot of thing.

In fact, we don't really have a good idea of what that number is, and what contributes to it at all. I'll investigate soon.

Comment 70

[Håkan Waara]

Colin, does this still affect the transparent window bug in any way?

Comment 71

[Colin Barrett [:cbarrett]]

I don't think so.

If ever you look at the return value of -backgroundColor, use -windowBackgroundColor instead, but I don't think that happens in your patch.

Comment 72

[Colin Barrett [:cbarrett]]

See bug 401677 for work on the Lk numbers.

Comment 73

[Marcia Knous [:marcia]]

Colin: Can I get some help with steps to verify this bug?

Comment 74

[Marcia Knous [:marcia]]

I just spotted your blog post, other than the default titlebar appearance, is there anything else?

(In reply to comment #73)
> Colin: Can I get some help with steps to verify this bug?
> 

Comment 75

[Colin Barrett [:cbarrett]]

This is at this point is mostly just adding an API, so no.

Comment 76

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007103004 Minefield/3.0a9pre. I verified that the default titlebar appearance now defaults to the same style as iTunes or Leopard, which is a much darker gray than normal Tiger windows.