625989 - Support tabs in title bar on Mac OS X

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Alex Limi (:limi) — Firefox UX Team]

(this is post-Firefox 4 material, but filing it so we can take a look at it for the next release)

In Firefox 4, we decided to support tabs in title bar on Windows when the window was in a maximized state.

We'd like to add this capability to the OS X version too, but in a slightly different way: Since OS X has no concept of "maximized" (it only has "zoomed"), it should probably be an explicit switch in the preference panel or the Customize palette.

The downside of having tabs in titlebar on Mac is of course that you get less of a draggable area for moving the window, but on the upside, you can use any empty area in the main UI for moving the window already, so it will be less of an issue than doing the same on Windows.

With OS X 10.7 moving to a structure where the close/maximize/minimize buttons are centered in the topmost toolbar, I'm pretty sure we can pull this off and make it look good too.

Comment 3

[Markus Stange [:mstange]]

(In reply to comment #0)
> The downside of having tabs in titlebar on Mac is of course that you get less
> of a draggable area for moving the window, but on the upside, you can use any
> empty area in the main UI for moving the window already,

Bug 615807 wants to change that, by the way.

I've started on the implementation of this and now I'm uncertain what to do with the window title. Should the title also be hidden in tabs-on-bottom mode? (Hiding it always would be the simplest to implement.)
On Windows it seems to be the case that the title is hidden iff the Firefox button is in the title bar, i.e. if the window is in draw-in-titlebar mode.

Comment 4

[Lars Olofsson]

is this only to be in 10.7 Lion? fx6 or 7,8?

Comment 5

[Stephen Horlander [:shorlander]]

(In reply to comment #3)
> I've started on the implementation of this and now I'm uncertain what to do
> with the window title. Should the title also be hidden in tabs-on-bottom
> mode? (Hiding it always would be the simplest to implement.)
> On Windows it seems to be the case that the title is hidden iff the Firefox
> button is in the title bar, i.e. if the window is in draw-in-titlebar mode.

Would it be possible to have the titlebar as a toolbar you can toggle?

Like this: http://www.stephenhorlander.com/pages/configs/main-window-configurations-mac.html

Comment 6

[Markus Stange [:mstange]]

Yes, that's possible. Having the title as a XUL label would require more XUL/JS changes but actually simplify things from the widget perspective. I like the idea.

Comment 7

[emailkurtis]

Lion is now released and I am admittedly disappointed that there is no status update on this bug - this should be a priority fix for Firefox 7 as the lack of cross-platform UI compatibility is obviously lacking with regards to the tabs. The most annoying thing is that "browser.tabs.drawInTitlebar" is set to TRUE in about:config and the UI doesn't reflect this setting. Chrome, and a few of Apple's new Lion apps, reflect the titlebar space saving. This shouldn't be something that is postponed for Firefox 8 - it took the Chrome devs only 4 days to released Lion updates for full-screen, gesture and, yes, proper tabs in titlebar support.

Comment 8

[Lars Olofsson]

Anyone working on this? I´ve seen all new mockups have tabs in title bar on Mac.

Comment 9

[Markus Stange [:mstange]]

Not at the moment, no. But if anybody wants to pick this up, some of the harder parts of the code can be found in bug 647216.

Comment 11

[Josh Aas]

Reassigning this to nobody, this is just a tracking bug. I'm happy to work on any dep bugs in Gecko. I just posted a new patch for the biggest remaining blocker, bug 647216.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

I don't see any other dependencies for this bug now that bug 647216 is fixed, so marking it as RESO-FIXED.

Comment 13

[Dão Gottwald [:dao]]

This still needs browser changes.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Attachment: Always draw in the titlebar on OSX - v1

This patch is dependent on the patch in bug 647216.

This makes Firefox always draw in the titlebar. When not using a lw-theme, the titlebar gets a style of -moz-window-titlebar, which I've added support for in widget/cocoa.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 700610 [details] [diff] [review]
Always draw in the titlebar on OSX - v1

I haven't been able to connect with Markus Stange, so requesting feedback from you Steven - hope that's OK!

Comment 16

[Steven Michaud [:smichaud] (Retired)]

> hope that's OK!

No problem.  But I'm not going to get to this soon.

On top of the usual craziness I'm always trying to stay on top of, I've now got a bunch of fires from the FF 18 release to put out or otherwise deal with.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 700610 [details] [diff] [review]
Always draw in the titlebar on OSX - v1

Hello Stephen - rstrong said you might be able to take a look at this and give me feedback.

Comment 18

[Josh Aas]

Comment on attachment 700610 [details] [diff] [review]
Always draw in the titlebar on OSX - v1

Review of attachment 700610 [details] [diff] [review]:
-----------------------------------------------------------------

I'm fine with this strategy.

Comment 19

[Stephen A Pohl [:spohl]]

Comment on attachment 700610 [details] [diff] [review]
Always draw in the titlebar on OSX - v1

Review of attachment 700610 [details] [diff] [review]:
-----------------------------------------------------------------

I too like this approach and it seems to work well from my limited testing. A few things that I'd like to point out:

1. I've observed a rare glitch where a solid light grey bar is drawn underneath the title bar but above the tabs bar, with a height of approximately 10px. When this occurs, the bar doesn't disappear until the browser is restarted. I was able to reproduce a few times when switching from lightweight themes to no theme and meant to take a screenshot, but I've been unable to reproduce more recently. This may be unrelated to this patch, but I thought it was worth pointing out.
2. I'm not a huge fan of the break between the titlebar and the tabs bar. I would prefer the gradient that you pointed out in bug 647216 in comment 69 (http://i.imgur.com/qEzCw.png). If we could fix the gradient to be the same as in the current nightly, that would be ideal.
3. When switching to fullscreen mode (by clicking the button on the right side of the title bar), the tabs bar jumps noticeably (best observed with no theme applied). When going to fullscreen, the tabs bar jumps to the top when the transition finishes. When returning from fullscreen, the tabs bar is drawn on top of the title bar until the transition ends and then jumps back down to underneath the title bar. The nightly has a smooth transition for both scenarios.

::: toolkit/content/LightweightThemeConsumer.jsm
@@ +116,3 @@
>        root.setAttribute("drawintitlebar", "true");
> +    }
> +    else if (this._hasActivated && !this._drawInTitlebarDefault) {

Is there ever a need to reset the attribute to the actual value of the default if one was present? Currently, the behavior is correct if the default had a value of true, but we don't restore the attribute to false if the default was false.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Attachment: Always draw in the titlebar on OSX - v1.1

(In reply to Stephen Pohl [:spohl] from comment #19)
> Comment on attachment 700610 [details] [diff] [review]
> Always draw in the titlebar on OSX - v1
> 
> Review of attachment 700610 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I too like this approach and it seems to work well from my limited testing.
> A few things that I'd like to point out:
> 
> 1. I've observed a rare glitch where a solid light grey bar is drawn
> underneath the title bar but above the tabs bar, with a height of
> approximately 10px. When this occurs, the bar doesn't disappear until the
> browser is restarted. I was able to reproduce a few times when switching
> from lightweight themes to no theme and meant to take a screenshot, but I've
> been unable to reproduce more recently. This may be unrelated to this patch,
> but I thought it was worth pointing out.

Hrm. No luck reproducing this one yet...

> 2. I'm not a huge fan of the break between the titlebar and the tabs bar. I
> would prefer the gradient that you pointed out in bug 647216 in comment 69
> (http://i.imgur.com/qEzCw.png). If we could fix the gradient to be the same
> as in the current nightly, that would be ideal.

Weird - the whole titlebar was flipped when applied this patch on a fresh pull from m-c. I've fixed this patch so that we don't flip anymore. The result is a smooth gradient transition from the titlebar to toolbar, a la that screenshot.


> 3. When switching to fullscreen mode (by clicking the button on the right
> side of the title bar), the tabs bar jumps noticeably (best observed with no
> theme applied). When going to fullscreen, the tabs bar jumps to the top when
> the transition finishes. When returning from fullscreen, the tabs bar is
> drawn on top of the title bar until the transition ends and then jumps back
> down to underneath the title bar. The nightly has a smooth transition for
> both scenarios.
> 

Yes, this is not ideal, and I'm open to suggestions. I'm already way out of my depth with this patch - I have no idea how native fullscreen works, and I could use some pointers. I find it particularly strange that (with this latest patch) when initting full-screen, the titlebar gets inverted before disappearing.

> ::: toolkit/content/LightweightThemeConsumer.jsm
> @@ +116,3 @@
> >        root.setAttribute("drawintitlebar", "true");
> > +    }
> > +    else if (this._hasActivated && !this._drawInTitlebarDefault) {
> 
> Is there ever a need to reset the attribute to the actual value of the
> default if one was present? Currently, the behavior is correct if the
> default had a value of true, but we don't restore the attribute to false if
> the default was false.

If the default value was false, I believe this is the behaviour:

1) On _update called with active = true, we sample the drawintitlebar attribute and record it (so this._drawInTitlebarDefault = false, since drawintitlebar is not there).

2) On a subsequent call with active = false, if !this._drawInTitlebarDefault, the attribute is removed.

Is my logic flawed somehow?

Comment 21

[Stephen A Pohl [:spohl]]

(In reply to Mike Conley (:mconley) from comment #20)
> Created attachment 705093 [details] [diff] [review]
> Always draw in the titlebar on OSX - v1.1
> 
> (In reply to Stephen Pohl [:spohl] from comment #19)
> > Comment on attachment 700610 [details] [diff] [review]
> > Always draw in the titlebar on OSX - v1
> > 
> > Review of attachment 700610 [details] [diff] [review]:
> > -----------------------------------------------------------------
> 
> > ::: toolkit/content/LightweightThemeConsumer.jsm
> > @@ +116,3 @@
> > >        root.setAttribute("drawintitlebar", "true");
> > > +    }
> > > +    else if (this._hasActivated && !this._drawInTitlebarDefault) {
> > 
> > Is there ever a need to reset the attribute to the actual value of the
> > default if one was present? Currently, the behavior is correct if the
> > default had a value of true, but we don't restore the attribute to false if
> > the default was false.
> 
> If the default value was false, I believe this is the behaviour:
> 
> 1) On _update called with active = true, we sample the drawintitlebar
> attribute and record it (so this._drawInTitlebarDefault = false, since
> drawintitlebar is not there).
> 
> 2) On a subsequent call with active = false, if
> !this._drawInTitlebarDefault, the attribute is removed.
> 
> Is my logic flawed somehow?

I'll have a look at the rest a bit later, but for this one, I think you would want to use root.getAttribute("drawintitlebar"); instead of root.hasAttribute("drawintitlebar"); on line 113, handling the case gracefully when the attribute is not present. Then, the behavior is as you outlined. Currently, you're sampling the presence, but not the value of the attribute. I hope this makes sense.

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Attachment: Always draw in the titlebar on OSX - v1.2

(In reply to Stephen Pohl [:spohl] from comment #21)
> (In reply to Mike Conley (:mconley) from comment #20)
> > Created attachment 705093 [details] [diff] [review]
> > Always draw in the titlebar on OSX - v1.1
> > 
> > (In reply to Stephen Pohl [:spohl] from comment #19)
> > > Comment on attachment 700610 [details] [diff] [review]
> > > Always draw in the titlebar on OSX - v1
> > > 
> > > Review of attachment 700610 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > 
> > > ::: toolkit/content/LightweightThemeConsumer.jsm
> > > @@ +116,3 @@
> > > >        root.setAttribute("drawintitlebar", "true");
> > > > +    }
> > > > +    else if (this._hasActivated && !this._drawInTitlebarDefault) {
> > > 
> > > Is there ever a need to reset the attribute to the actual value of the
> > > default if one was present? Currently, the behavior is correct if the
> > > default had a value of true, but we don't restore the attribute to false if
> > > the default was false.
> > 
> > If the default value was false, I believe this is the behaviour:
> > 
> > 1) On _update called with active = true, we sample the drawintitlebar
> > attribute and record it (so this._drawInTitlebarDefault = false, since
> > drawintitlebar is not there).
> > 
> > 2) On a subsequent call with active = false, if
> > !this._drawInTitlebarDefault, the attribute is removed.
> > 
> > Is my logic flawed somehow?
> 
> I'll have a look at the rest a bit later, but for this one, I think you
> would want to use root.getAttribute("drawintitlebar"); instead of
> root.hasAttribute("drawintitlebar"); on line 113, handling the case
> gracefully when the attribute is not present. Then, the behavior is as you
> outlined. Currently, you're sampling the presence, but not the value of the
> attribute. I hope this makes sense.

Yikes - you're absolutely right; I thought the widget listener was just checking for the presence of drawintitlebar and not actually paying attention to its value. Now I for the actual *value* of drawintitlebar; not just its presence.

Comment 23

[Markus Stange [:mstange]]

Comment on attachment 705109 [details] [diff] [review]
Always draw in the titlebar on OSX - v1.2

>+            [NSNumber numberWithBool:NO], @"is.flipped",

YES was actually correct. There was a bug in the patch in bug 647216 which caused title bar rendering to be flipped.

Other than that, this patch looks very good to me.
There's one piece which I think is still missing, which would allow us to make the titlebar smaller without destroying the gradient: We should take the position of the top edge of the toolbar box into consideration when calculating the unified toolbar height. I'll play around with this a little.

Comment 24

[Markus Stange [:mstange]]

Attachment: more robust unification

Here's what I have so far. This patch, applied on top of yours, makes the gradient look good even if I remove padding-top from #titlebar. There's one part which needs to be looked at again, it's marked with a XXX comment.

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Attachment: Always draw in the titlebar on OSX - v1.3

De-bitrotted after changes in the patches in bug 647216. I've also folded in mstange's patch.

mstange's patch appears to have fixed the issue with the toolbar separator appearing between the titlebar and the nav-bar when transitioning to full-screen mode. Thanks!

Comment 26

[Markus Stange [:mstange]]

Attachment: Always draw in the titlebar on OSX - v1.4

I've made some more changes to handle HiDPI mode correctly.

Comment 27

[Markus Stange [:mstange]]

Comment on attachment 705828 [details] [diff] [review]
Always draw in the titlebar on OSX - v1.4

The patch looks good to me, but I'm not a reviewer for browser.js (Dão should do that part) and the widget part should be reviewed by Josh, too.

Comment 28

[Mike Conley (:mconley) (:⚙️)]

(In reply to Markus Stange from comment #27)
> Comment on attachment 705828 [details] [diff] [review]
> Always draw in the titlebar on OSX - v1.4
> 
> The patch looks good to me, but I'm not a reviewer for browser.js (Dão
> should do that part) and the widget part should be reviewed by Josh, too.

I've noticed that with this last patch, when transitioning to the full-screen state, a toolbar separator briefly appears between the titlebar and the tabstrip. I don't recall seeing this separator in patch v1.3.

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Attachment: Always draw in the titlebar on OSX v1.5 (r=josh, feedback=mstange)

De-bitrotting.

Comment 30

[Matthew N. [:MattN]]

Comment on attachment 712910 [details] [diff] [review]
Always draw in the titlebar on OSX v1.5 (r=josh, feedback=mstange)

Review of attachment 712910 [details] [diff] [review]:
-----------------------------------------------------------------

I have some drive-by comments since I wanted to double-check you handled the ifdef in LWTConsumer while I was reviewing bug 813802.

::: browser/base/content/browser.js
@@ +4984,5 @@
>      // the resize event (handled in tabbrowser.xml).
>      this.allowedBy("sizemode", false);
>  
> +#ifdef XP_MACOSX
> +    document.documentElement.setAttribute("drawintitlebar", "true");

Do we really need another attribute instead of using @tabsintitlebar?

::: browser/themes/pinstripe/browser.css
@@ +42,1 @@
>  #main-window:not([drawintitlebar=true]) > #titlebar {

I think we should avoid supporting drawintitlebar/tabsintitlebar = false on pinstripe unless there is a clear need to support it.  Adding new variables is a maintenance burden.  If some theme/add-on wants to support the false value then they are free to do so but I don't think we should in pinstripe.

In other words, I think the attribute checks should be removed.

Comment 31

[Mike Conley (:mconley) (:⚙️)]

(In reply to Matthew N. [:MattN] from comment #30)
> Comment on attachment 712910 [details] [diff] [review]
> Always draw in the titlebar on OSX v1.5 (r=josh, feedback=mstange)
> 
> Review of attachment 712910 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have some drive-by comments since I wanted to double-check you handled the
> ifdef in LWTConsumer while I was reviewing bug 813802.
> 
> ::: browser/base/content/browser.js
> @@ +4984,5 @@
> >      // the resize event (handled in tabbrowser.xml).
> >      this.allowedBy("sizemode", false);
> >  
> > +#ifdef XP_MACOSX
> > +    document.documentElement.setAttribute("drawintitlebar", "true");
> 
> Do we really need another attribute instead of using @tabsintitlebar?
> 

We've been using this attribute for a while - it's our way of signalling to the Cocoa window that we want to draw in the titlebar. If we want tabs in titlebar to be optional via a pref, we will still need this attribute in order for lw-themes to work properly (since we must set drawintitlebar to true in order to draw lw-themes in there).


> ::: browser/themes/pinstripe/browser.css
> @@ +42,1 @@
> >  #main-window:not([drawintitlebar=true]) > #titlebar {
> 
> I think we should avoid supporting drawintitlebar/tabsintitlebar = false on
> pinstripe unless there is a clear need to support it.  Adding new variables
> is a maintenance burden.  If some theme/add-on wants to support the false
> value then they are free to do so but I don't think we should in pinstripe.
> 
> In other words, I think the attribute checks should be removed.

I would agree if it was true that we don't want drawing of the tabs in the titlebar to be optional. Is this the case?

Comment 32

[Dão Gottwald [:dao]]

Comment on attachment 712910 [details] [diff] [review]
Always draw in the titlebar on OSX v1.5 (r=josh, feedback=mstange)

>@@ -4979,16 +4979,19 @@ var TabsInTitlebar = {
> #ifdef CAN_DRAW_IN_TITLEBAR
>     this._readPref();
>     Services.prefs.addObserver(this._prefName, this, false);
> 
>     // Don't trust the initial value of the sizemode attribute; wait for
>     // the resize event (handled in tabbrowser.xml).
>     this.allowedBy("sizemode", false);
> 
>+#ifdef XP_MACOSX
>+    document.documentElement.setAttribute("drawintitlebar", "true");
>+#endif
>     this._initialized = true;
> #endif
>   },

TabsInTitlebar.init being called doesn't actually mean that we're drawing tabs in the title bar. That's the point of the allowedBy calls. You should set the attribute in TabsInTitlebar._update.

Comment 33

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dão Gottwald [:dao] from comment #32)
> Comment on attachment 712910 [details] [diff] [review]
> Always draw in the titlebar on OSX v1.5 (r=josh, feedback=mstange)
> 
> >@@ -4979,16 +4979,19 @@ var TabsInTitlebar = {
> > #ifdef CAN_DRAW_IN_TITLEBAR
> >     this._readPref();
> >     Services.prefs.addObserver(this._prefName, this, false);
> > 
> >     // Don't trust the initial value of the sizemode attribute; wait for
> >     // the resize event (handled in tabbrowser.xml).
> >     this.allowedBy("sizemode", false);
> > 
> >+#ifdef XP_MACOSX
> >+    document.documentElement.setAttribute("drawintitlebar", "true");
> >+#endif
> >     this._initialized = true;
> > #endif
> >   },
> 
> TabsInTitlebar.init being called doesn't actually mean that we're drawing
> tabs in the title bar. That's the point of the allowedBy calls. You should
> set the attribute in TabsInTitlebar._update.

Hm, I guess we'll have to be careful then about removing the drawintitlebar attribute if allowed=false, since we might have a lightweight theme installed.

Comment 34

[Mike Conley (:mconley) (:⚙️)]

Actually, I think my original plan was to *always* draw in the titlebar (hence the name of the patch) - even if we're not drawing tabs up there.

That's why I added the drawintitlebar in TabsInTitlebar.init - but I could just have easily put it directly onto the window node itself.

Is there a good reason to *not* always draw in the titlebar?

Comment 35

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) from comment #33)
> Hm, I guess we'll have to be careful then about removing the drawintitlebar
> attribute if allowed=false, since we might have a lightweight theme
> installed.

You can use mozMatchesSelector and :-moz-lwtheme for that. There's also the problem that a lightweight theme can be applied and removed in different states, making your _drawInTitlebarDefault property unreliable.

(In reply to Mike Conley (:mconley) from comment #34)
> Actually, I think my original plan was to *always* draw in the titlebar
> (hence the name of the patch) - even if we're not drawing tabs up there.

So the idea is to always render #titlebar (except in fullscreen mode) such that contend doesn't shift into the title bar area accidentally, and then have the tabs overlay #titlebar when appropriate?

> That's why I added the drawintitlebar in TabsInTitlebar.init - but I could
> just have easily put it directly onto the window node itself.

Right, putting it in TabsInTitlebar.init doesn't make sense either way.

Comment 36

[Mike Conley (:mconley) (:⚙️)]

> So the idea is to always render #titlebar (except in fullscreen mode) such
> that contend doesn't shift into the title bar area accidentally, and then
> have the tabs overlay #titlebar when appropriate?
> 

Yes, that was my idea.

> > That's why I added the drawintitlebar in TabsInTitlebar.init - but I could
> > just have easily put it directly onto the window node itself.
> 
> Right, putting it in TabsInTitlebar.init doesn't make sense either way.

Alright, I'll move it into the window node.

> 
> You can use mozMatchesSelector and :-moz-lwtheme for that. There's also the
> problem that a lightweight theme can be applied and removed in different
> states, making your _drawInTitlebarDefault property unreliable.
> 

What states are you referring to? Either the window has drawintitlebar set on it by default, or it doesn't - and I don't believe you can (or should) be able to transition from one to the other.

What am I missing here?

Comment 37

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) from comment #36)
> > You can use mozMatchesSelector and :-moz-lwtheme for that. There's also the
> > problem that a lightweight theme can be applied and removed in different
> > states, making your _drawInTitlebarDefault property unreliable.
> > 
> 
> What states are you referring to? Either the window has drawintitlebar set
> on it by default, or it doesn't - and I don't believe you can (or should) be
> able to transition from one to the other.

The state handled in TabsInTitlebar._update resulting from the TabsInTitlebar.allowedBy calls. That is, if you don't set the tabsintitlebar attribute unconditionally in browser.xul.

Comment 38

[Mike Conley (:mconley) (:⚙️)]

Hm - so, I remember now - the way nsXULElement works is that it will only initiate drawing into the titlebar on OSX if it sees us set the attribute (via nsXULElement:AfterSetAttr).

If the node just starts with the attribute, the signal to Cocoa to let us draw in there is not sent.

Markus - I'm not really familiar with the guts of nsXULElement. Do you know if there's a nice way we can read the drawsintitlebar attribute when the element is first manifested, instead of waiting for the attribute to be set?

Comment 39

[Josh Matthews [:jdm]]

I encountered the same thing with my changes in bug 829360. You need to add a similar check to nsXULElement::Create, since it doesn't flow through AfterSetAttr when creating an element from a prototype node.

Comment 40

[Markus Stange [:mstange]]

(In reply to Mike Conley (:mconley) from comment #38)
> Markus - I'm not really familiar with the guts of nsXULElement. Do you know
> if there's a nice way we can read the drawsintitlebar attribute when the
> element is first manifested, instead of waiting for the attribute to be set?

Yes, this should be done in nsXULWindow::SyncAttributesToWidget(). I've attempted to do that in bug 573973 once, but something held me up there. Maybe you can fix up the patch in that bug.

Comment 41

[Mike Conley (:mconley) (:⚙️)]

Attachment: Always draw in the titlebar on OSX v2 (r=josh, feedback=mstange)

Instead of using drawintitlebar, I make nsCocoaWindow respond to chromemargin (albeit in a very simple way).

chromemargin is now set for OSX directly in the main-window node instead of adding it in TabsInTitlebar.init.

Comment 42

[Mike Conley (:mconley) (:⚙️)]

Attachment: Always draw in the titlebar on OSX v2.1 (r=josh, feedback=mstange)

Just realized that we don't need gPrivateBrowsingUI to switch us to drawing in the titlebar, since we *always* draw there with this patch.

Comment 43

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 716568 [details] [diff] [review]
Always draw in the titlebar on OSX v2.1 (r=josh, feedback=mstange)

Jim,

In bug 573973, you asked Markus to switch us over from using drawintitlebar to chromemargin. I've done so here - though my approach might be a bit simplistic.

But does it satisfy your request?

-Mike

Comment 44

[Markus Stange [:mstange]]

Hah, I didn't think it would be this simple. Looks good to me, though.

Comment 45

[Jim Mathies [:jimm]]

Comment on attachment 716568 [details] [diff] [review]
Always draw in the titlebar on OSX v2.1 (r=josh, feedback=mstange)

I'm not the right person for reviewing mac widget code -> smichaud. Front end changes should be reviews by a fx front end peer, Dao maybe?

Comment 46

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 716568 [details] [diff] [review]
Always draw in the titlebar on OSX v2.1 (r=josh, feedback=mstange)

Sorry Jim - I should have used feedback? instead.

I really just want to know if you're happy with my approach for using chromemargin instead of drawintitlebar, since you requested it in bug 573973.

Comment 47

[Jim Mathies [:jimm]]

Comment on attachment 716568 [details] [diff] [review]
Always draw in the titlebar on OSX v2.1 (r=josh, feedback=mstange)

The chromemargin usage looks correct. Although I don't fully understand the LightweightThemeConsumer changes. I'm sure Dao will though!

Comment 48

[Mike Conley (:mconley) (:⚙️)]

(In reply to Jim Mathies [:jimm] from comment #47)
> Comment on attachment 716568 [details] [diff] [review]
> Always draw in the titlebar on OSX v2.1 (r=josh, feedback=mstange)
> 
> The chromemargin usage looks correct. Although I don't fully understand the
> LightweightThemeConsumer changes. I'm sure Dao will though!

Cool, thanks Jim.

Comment 49

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 716568 [details] [diff] [review]
Always draw in the titlebar on OSX v2.1 (r=josh, feedback=mstange)

One small problem in nsNativeThemeCocoa::DrawNativeTitlebar():

+  CGContextSaveGState(aContext);
+
+  if (aTitlebarRect.size.width * aTitlebarRect.size.height > CUIDRAW_MAX_AREA) {
+    return;
+  }

You should put the call to CGContextSaveGState() after the possible early return.

Comment 50

[Dão Gottwald [:dao]]

Comment on attachment 716568 [details] [diff] [review]
Always draw in the titlebar on OSX v2.1 (r=josh, feedback=mstange)

> LightweightThemeConsumer.prototype = {
>   _lastData: null,
>   _lastScreenWidth: null,
>   _lastScreenHeight: null,
>+#ifdef XP_MACOSX
>+  _hasActivated: false,
>+  _chromemarginDefault: "",
>+#endif

You can get rid of _hasActivated by setting _chromemarginDefault to null or undefined initially.

>+    else if (this._hasActivated && !this._drawInTitlebarDefault) {
>+      root.setAttribute("chromemargin", this._chromemarginDefault);
>+    }

_drawInTitlebarDefault doesn't exist.

Comment 51

[Mike Conley (:mconley) (:⚙️)]

Attachment: Always draw in the titlebar on OSX v2.2 (r=josh, feedback=mstange)

Fixed smichaud's CGContextSaveGState(aContext) bug in here, and made sure that the private browsing window mask still draws.

(In reply to Dão Gottwald [:dao] from comment #50)
> 
> You can get rid of _hasActivated by setting _chromemarginDefault to null or
> undefined initially.
> 
> >+    else if (this._hasActivated && !this._drawInTitlebarDefault) {
> >+      root.setAttribute("chromemargin", this._chromemarginDefault);
> >+    }
> 
> _drawInTitlebarDefault doesn't exist.

Ach, how embarrassing. Is this better?

Comment 52

[Dão Gottwald [:dao]]

Comment on attachment 717178 [details] [diff] [review]
Always draw in the titlebar on OSX v2.2 (r=josh, feedback=mstange)

>+    if (this._chromemarginDefault === undefined) {
>+      this._chromemarginDefault = root.getAttribute("chromemargin");
>+    }

nit: curly brackets can be dropped per local style

>+    if (active) {
>+      root.setAttribute("chromemargin", "0,-1,-1,-1");
>+    }
>+    else if (this._chromemarginDefault) {
>+      root.setAttribute("chromemargin", this._chromemarginDefault);
>+    } else {
>+      root.removeAttribute("chromemargin");
>+    }

nit: I think formatting it as follows would make it easier to comprehend, given that 'active' is a binary switch:

    if (active) {
      root.setAttribute("chromemargin", "0,-1,-1,-1");
    } else {
      if (this._chromemarginDefault)
        root.setAttribute("chromemargin", this._chromemarginDefault);
      else
        root.removeAttribute("chromemargin");
    }

Comment 53

[Mike Conley (:mconley) (:⚙️)]

Attachment: Always draw in the titlebar on OSX v2.2 (r=josh,dao, feedback=mstange)

Thanks! Fixed. Will push to inbound once the tree re-opens.

Comment 54

[Mike Conley (:mconley) (:⚙️)]

So I pushed this to try, and got some test failures:

https://tbpl.mozilla.org/?tree=Try&rev=683e4d6e8092

I'm currently investigating these - though I think it's taking me into layout-land, so if anybody has any suggestions or advice, I'm all ears.

Comment 55

[Mike Conley (:mconley) (:⚙️)]

Hey Markus,

I'm not making much headway fixing these tests.

The one I'm focusing on particular, is the selectAtPoint test - you can execute this with:

./mach mochitest-chrome dom/tests/mochitest/chrome/test_selectAtPoint.html

Note that this test doesn't just fail with this patch - if we draw in the titlebar on trunk (say, but adding a lw-theme), and then run this test again, we fail in the same way.

Perhaps you know what's going wrong? jimm suggested that some of our widgetry code might be wrong about their origins or bounds due to our drawing in the titlebar...but, as usual with this stuff, I'm a little out of my depth.

Comment 56

[Markus Stange [:mstange]]

I've filed bug 845595 on the test_selectAtPoint.html failure.

About the "ABORT: retaining manager changed out from under us ... HELP!" crash you'll want to talk to Matt Woodrow. I don't know much about that.

Also, I've noticed that in windows without a toolbar, the titlebar separator line is on the top instead of on the bottom of the titlebar. I haven't had a chance to look into that yet, though.

Comment 57

[Jared Wein [:jaws] (please needinfo? me)]

Matt, are you able to help out with the crash mentioned in comment #56?

Comment 58

[Matt Woodrow (:mattwoodrow)]

What do I need to do to reproduce that error?

I've built with the patch, and nothing seemed to be different.

Enabling browser.tabs.drawInTitlebar moves the tabs up (though the draw underneath the close/minimize etc controls).

I only see 'GContextRestoreGState: invalid context 0x0' errors with that, not the abort.

Comment 59

[Markus Stange [:mstange]]

Hi Matt, the crash happens during mochitest in debug builds, for example here: https://tbpl.mozilla.org/php/getParsedLog.php?id=20004022&tree=Try&full=1#error1

Comment 60

[Matt Woodrow (:mattwoodrow)]

Alright, I see it.

I really don't like this change, because titlebar painting on mac is *really* slow.

Because we need to paint the titlebar into a separate CGContext to that of the main window (and one that we can't draw with OpenGL), we need to maintain two retained layer managers.

One (OpenGL backed) for the window area, and one (BasicLayers) for the titlebar. Both of these build display lists and layers for the entire browser area, but only do painting for their specific areas.

So enabling this is doubling the cost of display list building, layer construction.

This particular error is because the remote frame display item doesn't like being in two retained layer trees at once. I'm not sure exactly why this requirement exists, cjones?

One option is to try restrict the area that we build display lists for to only the required area, such that they don't overlap. This would fix this current error, but it's still possible (I believe) for an item to lie across the boundary, and thus be required to exist in both. It would also improve the performance a lot.

Is it at all possible to draw the titlebar content into the same context as the rest of the window? This would make life a lot easier, especially when we have OpenGL (essentially all mac systems have this enabled).

Comment 61

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Matt Woodrow (:mattwoodrow) from comment #60)
> This particular error is because the remote frame display item doesn't like
> being in two retained layer trees at once. I'm not sure exactly why this
> requirement exists, cjones?

Shadow layers are bound to a single layer manager, like non-shadow layers are.  If the layer manager changes, we have to ensure any existing layer tree is thrown out and the shadow-layer machinery is rebuilt for the new manager.  We haven't implemented that code yet because we haven't needed it, sorry :(.

Comment 62

[Markus Stange [:mstange]]

Thanks.

(In reply to Matt Woodrow (:mattwoodrow) from comment #60)
> but it's still possible (I believe) for an item to lie across
> the boundary,

Tabs will, for example.

> Is it at all possible to draw the titlebar content into the same context as
> the rest of the window? This would make life a lot easier, especially when
> we have OpenGL (essentially all mac systems have this enabled).

It is, but it will take quite some work to get there. I filed bug 676241 on that some time ago, and I have a tiny demo app lying around somewhere which demonstrates that it's possible, but I don't have the time to write the Firefox patches at the moment.

Comment 63

[Matt Woodrow (:mattwoodrow)]

I really think it's worth doing so, both because of the significant complexity required to make the current method work (on top of the existing complexity that exists for it), plus the expected performance regression for the entire browser.

Comment 64

[Cameron Kaiser [:spectre]]

Would that essentially require hardware acceleration then? TenFourFox and the various legacy PowerPC Firefox forks are almost entirely rendered in software.

Or is the concept simply to unify the window under one context, which could be a CGContext *or* OpenGL?

Comment 65

[Markus Stange [:mstange]]

The latter.

Comment 66

[Mike Conley (:mconley) (:⚙️)]

Attachment: Always draw in the titlebar on OSX v2.3 (r=josh,dao, feedback=mstange)

De-bitrotting

Comment 67

[Mike Conley (:mconley) (:⚙️)]

Attachment: Puts tabs in the titlebar on UX branch

UX would like to test the interaction of tabs in the titlebar on OSX, so I've whipped up a really quick 'n dirty patch that puts the tabs in there.

Comment 68

[Mike Conley (:mconley) (:⚙️)]

Just so we're clear, attachment 722823 [details] [diff] [review] is purely to test on the UX branch, and is not intended for mozilla-central ever.

Comment 69

[Mike Conley (:mconley) (:⚙️)]

Attachment: Puts tabs in the titlebar on UX branch

This is the patch that I landed (adds comments about how these changes should not leak into mozilla-central).

Comment 70

[Josiah Bruner [:jsbruner]]

Perhaps someone could use this. I discovered a way to move the three buttons down very easily. It's a little hacky, but might work. I covered the process here:

http://infinite-josiah.blogspot.com/2013/03/obj-c-hack-to-re-position-window-buttons.html

Hopefully with some adoption that could help out here.

Comment 71

[Matthew N. [:MattN]]

Comment on attachment 722830 [details] [diff] [review]
Puts tabs in the titlebar on UX branch

This temporary UX branch patch may have caused bug 853415. I'm noting that here as its something to look for when we make the real patch for m-c.

Comment 72

[Steven Michaud [:smichaud] (Retired)]

I hear rumors that Mike Conley's temporary UX-branch patch for this bug (https://hg.mozilla.org/projects/ux/rev/cc13552f009f) has been removed from that branch.  See bug 853415 comment #7 and bug 853415 comment #8.

Is this true?  And if so which changeset removed it?

I'd actually prefer this remained on the UX branch.  I've been using it for testing in my work on bug 861317 and bug 851652.

Comment 73

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #72)

Oh and by the way, I now have a working fix for bug 861317, which I'll post shortly.

Comment 74

[Mike Conley (:mconley) (:⚙️)]

(In reply to Steven Michaud from comment #72)
> I hear rumors that Mike Conley's temporary UX-branch patch for this bug
> (https://hg.mozilla.org/projects/ux/rev/cc13552f009f) has been removed from
> that branch.  See bug 853415 comment #7 and bug 853415 comment #8.
> 
> Is this true?  And if so which changeset removed it?

Not exactly - looking through the UX repo, I can still see that patch in there: https://hg.mozilla.org/projects/ux/rev/cc13552f009f

> 
> I'd actually prefer this remained on the UX branch.  I've been using it for
> testing in my work on bug 861317 and bug 851652.

The patch that is missing is, I believe, attachment 722830 [details] [diff] [review]. Apply that, and you should be back to the original, hacky state.

Comment 75

[Steven Michaud [:smichaud] (Retired)]

Turns out it was this changeset that turned off drawing in the titlebar:
https://hg.mozilla.org/projects/ux/rev/34e823eae687

Comment 76

[Josiah Bruner [:jsbruner]]

Attachment: Always draw in the titlebar on OSX v2.3.1 (r=josh,dao, feedback=mstange)

Updated for current trunk.

Comment 77

[Josiah Bruner [:jsbruner]]

Attachment: Always draw in the titlebar on OSX v2.3.1 (r=josh,dao, feedback=mstange)

Missed something. *Now* it's updated for the current trunk.

Comment 78

[Mike Conley (:mconley) (:⚙️)]

According to smichaud, the patch that we're waiting on (bug 851652) still needs more research, and will not be ready for M6. So, slipping to M7.

Comment 79

[Mike Conley (:mconley) (:⚙️)]

Removing the items from M7 that do not block landing on m-c.

Comment 82

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/35d68446e000

Comment 84

[Bogdan Maris, Desktop QA]

Since the target milestone is Firefox 28 are there any fixes to verify in Firefox 28 beta or the fixes are for Australis only?

Comment 85

[Mike Conley (:mconley) (:⚙️)]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #84)
> Since the target milestone is Firefox 28 are there any fixes to verify in
> Firefox 28 beta or the fixes are for Australis only?

No, there aren't - while some of the platform-y things exist in Firefox 28, the front-end code that makes use of it is not. TL;DR - no verification required here for 28.

Comment 86

[Bogdan Maris, Desktop QA]

Thanks Mike, dropping verifyme based on comment 85.