Breaking this out of bug 625989, since that one seems to be mostly concerned with changes to platform and toolkit to support putting the tabs in the titlebar. This bug will track the patch that will actually put the tabs into the titlebar. There are three parts to this: 1) Move the tabs into the titlebar 2) Blank out the window title for browser windows 3) Find a way to sample the widths of the window buttons and full screen button, and indent the tabs inwards that amount.

> 2) Blank out the window title for browser windows Don't worry about this. I've already got a way to hook the method that draws the titlebar cell, and can stop it drawing when we need that. We *don't* want to just blank out a window's title (by setting its title to @""). That would break external accessibility apps, which examine windows' titles and "speak" them. This might also break our own accessibility code.

This patch alters the OSX CSS to put the tabs back into the titlebar, and adds the CSS hooks that will (hopefully) allow me to sample the widths of the window buttons and full-screen button.

Hey Steven, So now I've reached the point where I need your help. I've added two CSS values for -moz-appearance: "-moz-mac-button-box" and "-moz-mac-fullscreen-button". Right now, I've just hard-coded in the widths returned from nsNativeThemeCocoa.mm. How should I move forward to get the actual widths of the window buttons and the fullscreen button? The patch *should* apply directly to a recent pull from UX. Thanks, -Mike

I won't have a fix for bug 851652 ready for at least another week. I could wait until that's finished to replace your hard-coded widths with actual widths. Or I could add just enough of it here to measure the actual widths. For that matter, if you give me a hook to tell me when the window title should be hidden (not drawn), I could add code for that too. This would need to be next week (since I have tomorrow off).

(In reply to Steven Michaud from comment #4) > Or I could add just enough of it here to measure the actual widths. I think I'd prefer that, if it didn't bit-rot you / slow you down too much. > For that matter, if you give me a hook to tell me when the window > title should be hidden (not drawn), I could add code for that too. I believe the only case where we're drawing in the titlebar is for browser windows - so I believe the value you can look for is the drawsContentsIntoWindowFrame property of the window. Does that give you what you need? > > This would need to be next week (since I have tomorrow off). Fair enough. Enjoy your day off. :)

(In reply to comment #5) I'm just about to start work on this. I should be able to post something later today.

The status here should really be NEW :-)

(In reply to Steven Michaud from comment #6) > (In reply to comment #5) > > I'm just about to start work on this. I should be able to post something > later today. Awesome, thanks!

Mike: Here's an addition to your patch that measures titlebar buttons directly and stops the title cell from displaying when we draw in the titlebar. It works. For example it still allows the window title to be drawn if we're not drawing in the titlebar (for example in the "Show All Bookmarks" window). But there's one problem: Your query from CSS code to find the width of the fullscreen button comes before widget code has has a chance to create it. One way around this is to use a default value in this case (as my current patch does). In fact (practically speaking) this may be the best we can do. But let's keep thinking about it, and we my find a better way. This patch is meant to be applied to the current UX branch on top of your WIP v1 patch.

Cleaned up a bit.

Folding our patches together.

Dao, would you mind reviewing the front-end changes I made? Benoit - Steven says you're the one to probably review his widget/ changes. Mind taking a peek? Thanks, -Mike

Comment on attachment 743691 [details] [diff] [review] Patch v1.1 Review of attachment 743691 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/src/nsThemeConstants.h @@ +245,5 @@ > #define NS_THEME_WIN_COMMUNICATIONS_TOOLBOX 221 > #define NS_THEME_WIN_MEDIA_TOOLBOX 222 > #define NS_THEME_WIN_BROWSER_TAB_BAR_TOOLBOX 223 > > +// Unified toolbar and titlebar elements on the Mac Do you have someone to review this + the next 2 hunks? I've never seen this code before. ::: widget/cocoa/nsCocoaWindow.mm @@ +2480,5 @@ > +// know beforehand exactly what the superclasses will be, each of these > +// classes is created dynamically, using low-level Objective-C runtime > +// methods. So their methods' declarations can't use Objective-C syntax. > + > +Class gMozTitleCellClass = nil; static @@ +2489,5 @@ > + NSView *controlView) > +{ > + BaseWindow *window = (BaseWindow *) [controlView window]; > + if (!window) { > + window = (BaseWindow *) [NSView focusView]; I don't understand this line. if controlView is null we will take the focused view? Wont this cause bugs if you have multiple windows? @@ +2503,5 @@ > + NSCell_drawWithFrame super = (NSCell_drawWithFrame) objc_msgSendSuper; > + super(&target, sel, cellFrame, controlView); > +} > + > +NSMutableDictionary *gFrameViewClassesByStyleMask = nil; static @@ +2507,5 @@ > +NSMutableDictionary *gFrameViewClassesByStyleMask = nil; > + > +typedef void (*NSFrameView_initTitleCell)(struct objc_super *, SEL, id); > + > +static void MozFrameView_initTitleCell(id self, SEL sel, id cell) I'm not familiar with the code (ClassPair+class_addMethod) and frameViewClassForStyleMask. Trying to see if I can get someone else more knowledge to review this.

>> +{ >> + BaseWindow *window = (BaseWindow *) [controlView window]; >> + if (!window) { >> + window = (BaseWindow *) [NSView focusView]; >> > I don't understand this line. if controlView is null we will take > the focused view? Wont this cause bugs if you have multiple windows? This is just syntactic sugar. I cast the return values of [controlView window] and [NSView focusView] to BaseWindow* before I know exactly what's been returned. But before I use any of those return values, I check (on the next line) whether or not 'window' is really a BaseWindow*. Nonetheless you *have* caught me in a mistake, and a pretty bad one: [NSView focusView] returns an NSView*. I should have written [[NSView focusView] window]. >> +Class gMozTitleCellClass = nil; > > static >> +NSMutableDictionary *gFrameViewClassesByStyleMask = nil; > > static I'll fix these. New patch coming up.

(Following up comment #14) The documentation on -[NSCell drawWithFrame:(NSRect)rect inView:(NSView*)view] says that its implementation actually ignores the value of 'view' and instead uses what's returned by [NSView focusView]. My patch is just trying to play safe. In the very unlikely case that 'view' is nil, it also checks [NSView focusView].

(Following up comment #15) I'll add a comment explaining this to my next patch revision.

How's this, Benoit?

Comment on attachment 744236 [details] [diff] [review] Patch v1.2 Oops, just saw another mistake. Another patch coming up.

How's *this*? :-)

Comment on attachment 744237 [details] [diff] [review] Patch v1.3 Review of attachment 744237 [details] [diff] [review]: ----------------------------------------------------------------- LGTM

Cool, thanks Benoit. Steven - who should we ask to review the parts that Benoit wasn't familiar with?

(In reply to Mike Conley (:mconley) from comment #21) > Cool, thanks Benoit. > > Steven - who should we ask to review the parts that Benoit wasn't familiar > with? It's probably ok. It looks like internal only defines and list.

Ok, cool, thanks Benoit.

I realized that I need to set the -moz-box-ordinal-group property on the titlebar-placeholders for Windows and Linux.

Comment on attachment 744283 [details] [diff] [review] Patch v1.4 (r+'d by bgirard) Actually, shifting to MattN, who might have some spare cycles to look at this. Matt - BenWa has already reviewed everything under widget/, so I just need you to check out the changes I made under browser/.

And I forgot to remove the old ordinal attributes from the appmenu-button titlebar-placeholders.

Comment on attachment 744286 [details] [diff] [review] Patch v1.5 (r+'d by bgirard) Review of attachment 744286 [details] [diff] [review]: ----------------------------------------------------------------- Dão: I'm not sure if I'm on the right track with the content/theme distinction and I can't find the DevTools page that helped distinguish the two. ::: browser/base/content/browser.xul @@ +494,5 @@ > </hbox> > +#ifdef XP_MACOSX > + <hbox id="titlebar-fullscreen-button-container"> > + <hbox id="titlebar-fullscreen-button"> > + </hbox> Nit: anyone reason this isn't self-closing like the .titlebar-placeholders? ::: browser/themes/linux/browser.css @@ +38,5 @@ > border-bottom: 1px solid ThreeDShadow; > } > > +.titlebar-placeholder[type="appmenu-button"] { > + -moz-box-ordinal-group: 0; I think this would be better in browser/base/ since it's not specific to the default theme and that's where most of the other -moz-box-ordinal-group properties are in browser. It might get messy with ifdefs in this case though. What do you think? ::: browser/themes/osx/browser.css @@ +45,5 @@ > + -moz-box-ordinal-group: 0; > +} > + > +#titlebar-buttonbox { > + -moz-appearance: -moz-mac-button-box; The added -moz-appearance rules here are just used for sizing IIUC to match the OS controls and so I think these should also be in browser/base/.

Comment on attachment 744286 [details] [diff] [review] Patch v1.5 (r+'d by bgirard) >--- a/browser/base/content/browser.xul >+++ b/browser/base/content/browser.xul > <hbox id="titlebar-buttonbox-container" align="start"> > <hbox id="titlebar-buttonbox"> > <toolbarbutton class="titlebar-button" id="titlebar-min" oncommand="window.minimize();"/> > <toolbarbutton class="titlebar-button" id="titlebar-max" oncommand="onTitlebarMaxClick();"/> > <toolbarbutton class="titlebar-button" id="titlebar-close" command="cmd_closeWindow"/> > </hbox> > </hbox> >+#ifdef XP_MACOSX >+ <hbox id="titlebar-fullscreen-button-container"> >+ <hbox id="titlebar-fullscreen-button"> >+ </hbox> >+ </hbox> >+#endif What's the point of the extra container? titlebar-buttonbox needed a container for align="start", but you didn't use that here. > #ifdef CAN_DRAW_IN_TITLEBAR >- <hbox class="titlebar-placeholder" type="appmenu-button" ordinal="0"/> >- <hbox class="titlebar-placeholder" type="caption-buttons" ordinal="1000"/> >+#ifdef MENUBAR_CAN_AUTOHIDE >+ <hbox class="titlebar-placeholder" type="appmenu-button"/> >+#endif >+ <hbox class="titlebar-placeholder" type="caption-buttons"/> >+#ifdef XP_MACOSX >+ <hbox class="titlebar-placeholder" type="fullscreen-button"/> >+#endif > #endif Please use ifdefs for the platform-dependent ordinal values. >--- a/browser/themes/osx/browser.css >+++ b/browser/themes/osx/browser.css >+#titlebar-buttonbox { >+ -moz-appearance: -moz-mac-button-box; >+} Yes, it might make sense to move this to content/browser.css (as -moz-window-button-box; see below). >+CSS_KEY(-moz-mac-button-box, _moz_mac_button_box) As far as I can tell, you should just implement -moz-window-button-box on Mac rather than introducing -moz-mac-button-box.

Moved the ordinal rules back into browser.xul with ifdef's as per Dao's suggestion, and I'm using -moz-window-button-box instead of -moz-mac-button-box. I also moved the -moz-appearance rules into browser/base/content/browser.css as per Dao / MattN's suggestion. Tested on Windows, and it seems to do the job. Checkpointing here to test on Linux and OSX, and then I'll request another review.

Comment on attachment 744640 [details] [diff] [review] Patch v1.6 (r+'d by bgirard) >--- a/browser/base/content/browser.css >+++ b/browser/base/content/browser.css >+%ifdef XP_MACOSX >+#titlebar-buttonbox { >+ -moz-appearance: -moz-window-button-box; >+} Can you move this away from the XP_MACOSX ifdef, remove -moz-window-button-box from themes/windows/browser.css and move -moz-window-button-box-maximized here with XP_WIN (or implement -moz-window-button-box-maximized on Mac as an alias for -moz-window-button-box)?

Comment on attachment 744640 [details] [diff] [review] Patch v1.6 (r+'d by bgirard) I'd like to review the widget parts of this patch, too.

(In reply to Dão Gottwald [:dao] from comment #30) > Comment on attachment 744640 [details] [diff] [review] > Patch v1.6 (r+'d by bgirard) > > >--- a/browser/base/content/browser.css > >+++ b/browser/base/content/browser.css > > >+%ifdef XP_MACOSX > >+#titlebar-buttonbox { > >+ -moz-appearance: -moz-window-button-box; > >+} > > Can you move this away from the XP_MACOSX ifdef, remove > -moz-window-button-box from themes/windows/browser.css and move > -moz-window-button-box-maximized here with XP_WIN (or implement > -moz-window-button-box-maximized on Mac as an alias for > -moz-window-button-box)? Yep, good plan. I've opted for the former, using the XP_WIN ifdef. Checkpointing here while I re-test this on Windows.

Comment on attachment 744662 [details] [diff] [review] Patch v1.7 (r+'d by bgirard) Ok, this seems to do the job.

Hey Markus, Thanks for looking at this. Is it possible to have a review done today? We were hoping to land this soon (yesterday, actually). Thanks, -Mike

Yes, I'll be done in a few minutes. But the changes I'd like to see don't need to block landing, they can be addressed in a follow-up bug.

Comment on attachment 744662 [details] [diff] [review] Patch v1.7 (r+'d by bgirard) Actually, land away. I'll put up a patch with my changes into a new bug and let smichaud review it; I think that will be more effective at this point.

(In reply to Markus Stange from comment #36) > Comment on attachment 744662 [details] [diff] [review] > Patch v1.7 (r+'d by bgirard) > > Actually, land away. I'll put up a patch with my changes into a new bug and > let smichaud review it; I think that will be more effective at this point. Wonderful, thanks!

By the way, have all the test failures you encountered in bug 625989 comment 54 gone away? In particular I wonder about the crash in test_browserElement_inproc_OpenMixedProcess.html - does that not happen any more? In bug 625989 comment 63 it looked like the current way of drawing in the titlebar just wouldn't work in some cases (maybe just the OMTC case?).

(In reply to Markus Stange from comment #38) > By the way, have all the test failures you encountered in bug 625989 comment > 54 gone away? In particular I wonder about the crash in > test_browserElement_inproc_OpenMixedProcess.html - does that not happen any > more? In bug 625989 comment 63 it looked like the current way of drawing in > the titlebar just wouldn't work in some cases (maybe just the OMTC case?). For better or for worse, it looks like the tack we're taking for Australis is to land, and then deal with test failures after the fact. I just re-ran the DOM tests, and test_browserElement_inproc_OpenMixedProcess.html does not happen for me. The original test failure in dom/tests/mochitest/chrome/test_selectAtPoint.html now passes. I am, however, seeing test failures in dom/tests/mochitest/chrome/test_queryCaretRect.html and dom/tests/mochitest/chrome/test_resize_move_windows.xul. I'll file follow-up bugs for those.

Ah, and it looks like those test failures are actually unrelated to this patch, as they already exist on the Jamun branch.

Landed on the UX branch as https://hg.mozilla.org/projects/ux/rev/c06fb1a7c7fb

I just discovered that the patches I uploaded here somehow didn't include several of my latest changes. This could result in problems -- particularly on SnowLeopard, where it will lead to something like a crash (SnowLeopard doesn't have fullscreen buttons). Sigh :-( I think I should just post the required changes here, and get them onto the UX branch as soon as possible. Benoit, I'll ask you once again to review. Markus, I know you may have problems with my patch. But let's deal with that later in bug 868211.

(In reply to Steven Michaud from comment #42) > > I think I should just post the required changes here, and get them onto the > UX branch as soon as possible. Yes please - before the next UX Nightly spin if possible.

Here it is.

Bustage fix landed on UX as https://hg.mozilla.org/projects/ux/rev/401c71c3732d

Bug 853415 is back again. I assume it is related to draw in the titlebar.

http://hg.mozilla.org/mozilla-central/rev/c06fb1a7c7fb http://hg.mozilla.org/mozilla-central/rev/401c71c3732d

This will be covered by qa in bug 625989.