Closed Bug 851652 Opened 11 years ago Closed 11 years ago

Change position of the standardWindowButtons for Australis

Categories

(Core :: Widget: Cocoa, enhancement)

All
macOS
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: MattN, Assigned: Gijs)

References

()

Details

(Whiteboard: [Australis:M9][Australis:P4])

Attachments

(3 files, 20 obsolete files)

264.31 KB, image/png
Details
1.33 MB, image/png
Details
25.58 KB, patch
jaws
: review+
Details | Diff | Splinter Review
We can use [aWindow standardWindowButton:NSWindowCloseButton] with setFrameOrigin to move them.

See https://developer.apple.com/library/mac/documentation/Cocoa/Reference/ApplicationKit/Classes/NSWindow_Class/Reference/Reference.html#//apple_ref/doc/uid/20000013-SW99 for the other NSWindowButton constants.

Source: http://infinite-josiah.blogspot.com/2013/03/obj-c-hack-to-re-position-window-buttons.html?showComment=1363376510210#c1789580280464753476

The open question I have is how are we going to decide when to move the buttons? Should we use a define that browser will use? What about existing CAN_DRAW_IN_TITLEBAR or SetDrawsInTitlebar? We need to keep in mind that not all Windows need this.

Josiah, is this something you want to work on?
Flags: needinfo?(josiah)
(In reply to Matthew N. [:MattN] from comment #0)
> We can use [aWindow standardWindowButton:NSWindowCloseButton] with
> setFrameOrigin to move them.
> 
> See
> https://developer.apple.com/library/mac/documentation/Cocoa/Reference/
> ApplicationKit/Classes/NSWindow_Class/Reference/Reference.html#//apple_ref/
> doc/uid/20000013-SW99 for the other NSWindowButton constants.
> 
> Source:
> http://infinite-josiah.blogspot.com/2013/03/obj-c-hack-to-re-position-window-
> buttons.html?showComment=1363376510210#c1789580280464753476
> 
> The open question I have is how are we going to decide when to move the
> buttons? Should we use a define that browser will use? What about existing
> CAN_DRAW_IN_TITLEBAR or SetDrawsInTitlebar? We need to keep in mind that not
> all Windows need this.
> 
> Josiah, is this something you want to work on?

Sure, I can take a stab at it. The most difficult question is indeed where and when do we do this? I need to take a look at this standardWindowButton thing. Perhaps I will create a quick Obj-C program to see how this works. I was having trouble with that before, though I wasn't aware of setFrameOrigin. Do we use CocoaWindow.mm (I believe that was the name of it), somewhere else? 

I believe when I did use standardWindowButton I was having problems with it not detecting hover and drawing properly. It appears even Chrome has this problem and they are doing a lot of detection and catching.
Flags: needinfo?(josiah)
Assignee: nobody → josiah
Status: NEW → ASSIGNED
Alright, so I used the standardWindowButton using the code I added to my other blog post. http://infinite-josiah.blogspot.com/2013/03/obj-c-hack-to-re-position-window-buttons.html

As mentioned there, this works well (Thanks Markus!) except for the hover detection. At this point in time it would appear that I will need to set up some kind of hover detection that sends notifications or something. Trying to find the most streamlined way right now...
So I figured out how we can reset the hover capability. It seems that when the window resizes, it will reset the hover detection to the buttons' current location. Well, so when the window loads I can use:

[_window setFrame:NSMakeRect(_window.frame.origin.x, _window.frame.origin.y, _window.frame.size.width, _window.frame.size.height - 1) display:YES animate:YES];
  [_window setFrame:NSMakeRect(_window.frame.origin.x, _window.frame.origin.y, _window.frame.size.width, _window.frame.size.height + 1) display:YES animate:YES];
}

To move the window infinitely quickly, which does the job. Is this a fine approach?
No, that's not acceptable. We should find out the real reason why this happens. Is it a problem with invalidation? Is setNeedsDisplayInRect called in the right places with the right rects? Or is it a problem with actually detecting the mouse, i.e. are the NSTrackingAreas wrong? Does calling _updateButtons on the border view do anything helpful? How will moving our ChildView from being a subview of the contentView to being a subview of the border view change things?
I haven't had a chance to look at the Chromium code yet, but I expect we'll need similar code to solve similar problems.
Not positive about this, but I looked through Chrome's source, and nothing is standing out that would solve this. However, they do something similar to what I did, using the following:

  // Force re-layout of the window buttons by wiggling the size of the frame
  // view.
  NSView* frameView = [[self contentView] superview];
  BOOL frameViewDidAutoresizeSubviews = [frameView autoresizesSubviews];
  [frameView setAutoresizesSubviews:NO];
  NSRect oldFrame = [frameView frame];
  [frameView setFrame:NSZeroRect];
  [frameView setFrame:oldFrame];
  [frameView setAutoresizesSubviews:frameViewDidAutoresizeSubviews];

Could this be what they are using? This does seem quite hacky and odd.
Markus, this does look like what Chrome is doing. Resizing the frame causes the buttons to reset. But reset's what is the question?

If this is what Chrome is using, should we take a similar approach and "silently" resize the window during it's creation? Or do we go farther than Chrome, and find the reasoning behind this and readjust things ourselves?
OK, this does look like there's no obvious non-hacky alternative. In any case, resizing the frameView is much better than resizing the window, so I'd be fine with using Chrome's solution.
Alright. Now... Where to do this? Directly in CocoaWindow?
(In reply to Matthew N. [:MattN] from comment #0)
> The open question I have is how are we going to decide when to move the
> buttons? Should we use a define that browser will use? What about existing
> CAN_DRAW_IN_TITLEBAR or SetDrawsInTitlebar? We need to keep in mind that not
> all Windows need this.
> 

Just my 2c on this - we probably want to move the buttons when chromemargin on the window starts with "0", indicating that the content area extends to the very top of the window.

We can be very static about this, and simply move the buttons down by the amount required in the spec: http://people.mozilla.com/~shorlander/files/australis-designSpecs/australis-designSpecs-osx-mainWindow.html

At least, unless there's a good reason to be a little more flexible - but I can't think of any at the moment.
I decided to use CocoaWindow.mm for this. I have successfully moved the position of the buttons, but I need to test with the new tab style. I will upload a patch when the Australis patches have been updated for the current trunk and I can test on them.
Attached image Issue with draw over
I am encountering an annoying issue. I can now move the buttons, as seen from the picture, and on a separate Cocoa App I made it works fine. However, it would appear that on Firefox, moving the button out of the titlebar causes issues, as something is drawing over them.

Apparently we aren't using native toolbars here, and are drawing our own, but this is causing the inability to place the buttons above the drawing.

I am using:

  [[self frameView]addSubview:button];

Which should place it on the top of all other views, which tells me that this is not just about stacking, but that I will need to draw over this draw over. How I am to do this is unclear at the moment.

Any ideas/facts would be appreciated. I do not know a ton of how the views and drawing is working on this.
I think we need to fix bug 676241 first. What covers the buttons is the OpenGL view. Does your patch work correctly if you turn off hardware acceleration?
(In reply to Markus Stange from comment #12)
> I think we need to fix bug 676241 first. What covers the buttons is the
> OpenGL view. Does your patch work correctly if you turn off hardware
> acceleration?

Ah ha. Yep, works perfectly with hardware acceleration off. Thanks Markus! So I guess we need bug 676241 to fix that first. I'll work on moving the fullscreen button down next for now.
Depends on: 676241
Attached image Round 1, move NSButtons down. (obsolete) —
Demonstrates what the buttons look like now without OpenGL drawing over them.
(In reply to Josiah Bruner [:JosiahOne] from comment #14)
> Created attachment 727171 [details]
> Round 1, move NSButtons down.
> 
> Demonstrates what the buttons look like now without OpenGL drawing over them.

This looks *fantastic*. You're my hero.
(In reply to Josiah Bruner [:JosiahOne] from comment #14)
> Created attachment 727171 [details]
> Round 1, move NSButtons down.
> 
> Demonstrates what the buttons look like now without OpenGL drawing over them.

Looking great! Nice work. Can't wait for bug 676241 to be fixed. :)
Great work!
Could you reapply this magic for the OS X full screen button, too?
(In reply to Morpheus3k from comment #17)
> Great work!
> Could you reapply this magic for the OS X full screen button, too?

Working on that currently. However, this is quite more difficult. For some reason the fullscreen button is actually not created as part of the window, therefore I can't use the same class. The only way to access is from the browser's app delegate/controller class. However, all source I have seen so far are just NSWindow's and their subclasses.

Markus, do you know of the equivalent to the App Delegate class/ the controller class?
I don't know where you're making the adjustment, but in Firefox, the place where the full screen button is added is in nsCocoaWindow::SetShowsFullScreenButton (the setCollectionBehavior line). Try making the adjustment after that line.

Whether the adjustments works shouldn't have anything to do with what class you're in when you make the adjustment. It may have to do with the point in time, and it's possible that in your example app the delegate or controller methods are called at a later time than whatever method you are moving the other buttons in.
(In reply to Markus Stange from comment #19)
> I don't know where you're making the adjustment, but in Firefox, the place
> where the full screen button is added is in
> nsCocoaWindow::SetShowsFullScreenButton (the setCollectionBehavior line).
> Try making the adjustment after that line.
> 
> Whether the adjustments works shouldn't have anything to do with what class
> you're in when you make the adjustment. It may have to do with the point in
> time, and it's possible that in your example app the delegate or controller
> methods are called at a later time than whatever method you are moving the
> other buttons in.

Thanks, that helped a lot. Got it pretty much working. There are still a few things I need to address though.

1. The three window buttons are being placed in window during full screen. 

2. For some reason the full screen button is being moved correctly at launch, but after a few seconds, it moves down. After that any actions that are called will re-align the button, but there is someplace I am missing that causes a re-layout of the window.
Please attach your patch.
Attached patch Ugly Patch (obsolete) — Splinter Review
Here is a very rough and ugly patch. So don't get mad at me for the random line spaces and what not. I did a lot of experimenting. :)

This works with three problems.

The two mentioned a little above and the fact that the buttons move on every toolbar window, which includes the preference pane, and others, which shouldn't have moved buttons.
Attached patch Ugly Patch V2 (obsolete) — Splinter Review
Updated patch (formatting is still bad), that fixes the issue of the window button showing in fullscreen mode.
Attachment #727816 - Attachment is obsolete: true
Attached patch Patch (V 0.1) (obsolete) — Splinter Review
Patch positions the fullscreen button correctly now using windowDidUpdate. If this is the best approach I am not sure.

Still to address is the moving of buttons only on the main browser window.
Attachment #727981 - Attachment is obsolete: true
Here's a screenshot of the current state now. In HiDPI.
Attachment #727171 - Attachment is obsolete: true
Attached patch Sucessful Implementation (obsolete) — Splinter Review
A successful implementation. Fixes all issues. Requesting review and ui-review.
Attachment #728359 - Attachment is obsolete: true
Attachment #729103 - Flags: ui-review?(shorlander)
Attachment #729103 - Flags: review?(smichaud)
Comment on attachment 729103 [details] [diff] [review]
Sucessful Implementation

Just starting my review (and tests), but I already have a question:

> - (void)windowDidChangeScreen:(NSNotification *)aNotification
> {
>+  BaseWindow* window = [aNotification object];
>+
>   if (!mGeckoWindow)
>     return;

This doesn't appear to do anything.  Did you leave it in by mistake?
(In reply to Steven Michaud from comment #27)
> Comment on attachment 729103 [details] [diff] [review]
> Sucessful Implementation
> 
> Just starting my review (and tests), but I already have a question:
> 
> > - (void)windowDidChangeScreen:(NSNotification *)aNotification
> > {
> >+  BaseWindow* window = [aNotification object];
> >+
> >   if (!mGeckoWindow)
> >     return;
> 
> This doesn't appear to do anything.  Did you leave it in by mistake?

Yes, just realized there might be a few of those. I had those for when I was moving the fullscreen button, but later just used windowDidUpdate, which made checking in each of those functions obsolete. I apparently forgot about removing the window pointers.
Also, it should be noted that Stephen Horlander mentioned another thing I must address. The tab bar can auto hide, so I will update my patch to not move buttons when in that state.
Comment on attachment 729103 [details] [diff] [review]
Sucessful Implementation

Another question:

Updates to the location of the fullscreen button happen in -[WindowDelegate windowDidUpdate] (on the NSWindowDidUpdateNotification notification).  But updates to the other buttons (the close, minimize and zoom buttons) happen on the NSViewFrameDidChangeNotification.

Did you have a reason for this?  Did you try updating the fullscreen button on the NSViewFrameDidChangeNotification and find that didn't work properly?

Doing all the updates on NSViewFrameDidChangeNotification is probably more efficient, if it works properly.
(In reply to Steven Michaud from comment #30)
> Comment on attachment 729103 [details] [diff] [review]
> Sucessful Implementation
> 
> Another question:
> 
> Updates to the location of the fullscreen button happen in -[WindowDelegate
> windowDidUpdate] (on the NSWindowDidUpdateNotification notification).  But
> updates to the other buttons (the close, minimize and zoom buttons) happen
> on the NSViewFrameDidChangeNotification.
> 
> Did you have a reason for this?  Did you try updating the fullscreen button
> on the NSViewFrameDidChangeNotification and find that didn't work properly?
> 
> Doing all the updates on NSViewFrameDidChangeNotification is probably more
> efficient, if it works properly.

The reason for this is that the fullscreen button is not allocated and initialized when all other buttons are. Because of this, I can not get a pointer reliably and although I forgot most of the details, could not set up a notification observer on fullscreen button, when it may or may not be available.

Using windowDidUpdate, if the button returns null, then simply nothing will happen, and there are no side-effects.
Thanks for the info.

I'm still going to try to find a way to have the fullscreen button update on the NSViewFrameDidChangeNotification -- possibly by intervening in nsCocoaWindow::SetShowsFullScreenButton.
So, although I can detect the autoHide preference, I have no way to tell if the tab bar is being hidden and/or how many tabs there are. Before I come up with some more complicated way to do this, is there a way to detect a tab bar that is not visible and/or how many tabs there are currently?
Attached patch Sucessful Implementation V 2 (obsolete) — Splinter Review
Updated patch. Fixes the issue with the buttons being moved down when there is no tab bar.

It should be noted though, that there is still an issue with Private Browsing Mode. PBM will move the buttons to normal position at *all* times. This issue can be fixed relatively easily. Therefore, another bug should be proposed that fixes this by creating an API or something similar that will allow access to the current tab count.

This patch uses the mUnifiedTitlebarHeight, but on PBM the window always will return 0, which is used to move the buttons back into place.

When this API is added to allow tab count, yet another bug should be filed to move the buttons with this API instead of using the mUnfifiedTitlebarHeight, as mUnifiedTitlebarHeight is quite temperamental.

Flagging Stephen and Stephen for review and ui-review.
Attachment #729103 - Attachment is obsolete: true
Attachment #729103 - Flags: ui-review?(shorlander)
Attachment #729103 - Flags: review?(smichaud)
Attachment #730214 - Flags: ui-review?(shorlander)
Attachment #730214 - Flags: review?(smichaud)
Attached patch Sucessful Implementation V 2.1 (obsolete) — Splinter Review
Fixed oversight.
Attachment #730214 - Attachment is obsolete: true
Attachment #730214 - Flags: ui-review?(shorlander)
Attachment #730214 - Flags: review?(smichaud)
Attachment #730228 - Flags: ui-review?(shorlander)
Attachment #730228 - Flags: review?(smichaud)
Attached patch Sucessful Implementation V 2.1.1 (obsolete) — Splinter Review
Fixed a little bit of formatting issues... And removed the extra NSLog statement. Carrying previous flags.
Attachment #730228 - Attachment is obsolete: true
Attachment #730228 - Flags: ui-review?(shorlander)
Attachment #730228 - Flags: review?(smichaud)
Attachment #730787 - Flags: ui-review?(shorlander)
Attachment #730787 - Flags: review?(smichaud)
Attached patch Sucessful Implementation V 2.1.2 (obsolete) — Splinter Review
Updated for current trunk.
Attachment #730787 - Attachment is obsolete: true
Attachment #730787 - Flags: ui-review?(shorlander)
Attachment #730787 - Flags: review?(smichaud)
Attachment #731984 - Flags: ui-review?(shorlander)
Attachment #731984 - Flags: review?(smichaud)
Josiah, I've started reviewing your patch and found some problems -- some pretty trivial, and some not so.

The most serious one is that, fiddling only a little with your patch, it's quite easy to get into an infinite recursion.  There's also a large performance hit.

So I'm going to try to find other ways to fix this bug.

I'll go into more detail when I find a better solution ... or if, after spending a few days, I'm not able to.
(In reply to Steven Michaud from comment #38)
> Josiah, I've started reviewing your patch and found some problems -- some
> pretty trivial, and some not so.
> 
> The most serious one is that, fiddling only a little with your patch, it's
> quite easy to get into an infinite recursion.  There's also a large
> performance hit.
> 
> So I'm going to try to find other ways to fix this bug.
> 
> I'll go into more detail when I find a better solution ... or if, after
> spending a few days, I'm not able to.

The performance issue is quite annoying. Although I now remember something can could be changed to address that.

No matter what, right now at least, windowDidUpdate must be used in order to constantly check for changes in the titlebar state. However, this gets called a LOT. Instead, I would prefer to use the notification center for frameDidChange, but frameDidChange can't check when I need it to right now.

That said, sine I am using windowDidUpdate, setting up a window observer is probably not necessary. I really should just remove that.

Eventually, using windowDidUpdate will become obsolete when the titlebar does not change its location and any window that returns YES for canDrawInTitlebar, and still allows room for a change in button position. Right now windowDidUpdate is the only way to constantly check.

Once this is done, windowDidUpdate will be pointless and using an observer would be preferred. Perhaps this bug should wait on another bug which allows room for window controls to move down on every window which canDrawInTitlebar (Panorama, new customize window)

Thoughts?
> Thoughts?

None for the moment.

For now I'm looking for more fundamental solutions, and fundamentally different ones.  I may just have stumbled across one.

NSThemeFrame has a _setButtonsShown: method, present on OS X 10.7 and up, that can be used to stop the OS from displaying any buttons in the titlebar.  It isn't present on OS X 10.6, and I haven't (yet) found a way to stop the OS from drawing the window's title.

But let's say I find a way to stop the OS from drawing anything in the titlebar except its background, or better yet anything at all.  Would it be feasible for us to draw those buttons (and other titlebar contents) ourselves, in Gecko (in cross-platform code)?  This might also make it easier to resolve bug 676241.

This question is directed particularly to mstange and shorlander.
I've got the basic workings of a new patch for this bug, but it isn't ready to post yet.

The reason is that I think we need a better way to detect whether or not at least one tab is present than looking for specific values in mUnifiedToolbarHeight.  Finding this won't be easy, and may take a while.  In fact I may need to add something to cross-platform code to make this information easily detectable from widget code.

So I'm posting this to show I'm still working on this bug, and haven't forgotten it.
(In reply to Steven Michaud from comment #41)
> I've got the basic workings of a new patch for this bug, but it isn't ready
> to post yet.
> 
> The reason is that I think we need a better way to detect whether or not at
> least one tab is present than looking for specific values in
> mUnifiedToolbarHeight.  Finding this won't be easy, and may take a while. 
> In fact I may need to add something to cross-platform code to make this
> information easily detectable from widget code.
> 
> So I'm posting this to show I'm still working on this bug, and haven't
> forgotten it.

Thanks Stephen. Something will definitely need to be added to use other than mUnifiedTitlebarHeight. I myself tried to find an alternative and found that you will need to write something that can retrieve this information however the js implementations are done.

Is it safe to say that you are taking this from here?
> Is it safe to say that you are taking this from here?

Yes, I suppose so :-)

Thanks for your work so far on this, and I may still have more questions for you.

In fact I have one now:

I noticed that all the lowered buttons have their bottoms cut off, even on the UX branch (with your patch or with the one I'm working on).  Sometimes this is because other objects partially cover them (for example the first tab, if there is one, partially covers the close, minimize and zoom buttons).  Sometimes (as with the fullscreen button) it's just because the bottoms don't get drawn when hardware acceleration is on (as it is by default).

I assume you've seen all this.  Otherwise please comment.

Some, at least, of the "cut off" problems will have to be dealt with in other bugs.
Assignee: josiah → smichaud
> the first tab, if there is one, partially covers the close, minimize
> and zoom buttons

Actually this only happens on trunk.

Which raises another question/problem:  We'll need to have a way to
detect from widget code whether or not Australis is "on".  If it's
"off" the buttons won't get moved.
(In reply to Steven Michaud from comment #44)
> > the first tab, if there is one, partially covers the close, minimize
> > and zoom buttons
> 
> Actually this only happens on trunk.
> 
> Which raises another question/problem:  We'll need to have a way to
> detect from widget code whether or not Australis is "on".  If it's
> "off" the buttons won't get moved.

I believe canDrawInTitlebar will work for this, but if not, it isn't terribly important. At least, from what I know, Australis will not be replaceable. However canDrawInTitlebar should return FALSE if Australis is off (Or windows such as popups, preference pane, etc)

If it is returning true on the regular trunk, then that is an issue.

And yes, hardware acceleration does draw over the buttons, and is being addressed in a dependent bug.
Attached patch Work in progress (obsolete) — Splinter Review
Here's a patch that avoids all the performance problems I described in comment #38.  But it's not yet finished, and I'm going to have to put this bug aside for a while.

My patch avoids using either the NSWindowDidUpdateNotification or the NSFrameDidChangeNotification -- so there's no performance hit at all.  To accomplish this it uses some undocumented APIs.  But all of them are used by Apple, in ways that are unchanged since at least OS X 10.5.  So it should be quite safe to use them.

It's also possible that the strategy I've used for this bug will help with some of the other titlebar bugs.

This patch isn't finished.  For example it moves all the buttons the same distance, unconditionally -- whether or not we're compiling Australis, whether or not canDrawInTitlebar is true, and whether or not at least one tab is showing.  I did this so people can have maximum leeway to play with it -- on the UX branch or on the trunk.  (And by the say, though the patch is against the trunk, it should compile with offsets on the UX branch.)

The largest task that remains is to find some way (other than looking for specific values in mUnifiedToolbarHeight) to query Gecko about how many tabs are present.  I still don't know for sure, but I suspect some cross-platform code will need to be added to make this possible.
Josiah, is AUSTRALIS_TITLEBAR_BUTTON_OFFSET correct in my patch?  Should its value be '12'?
(In reply to Steven Michaud from comment #47)
> Josiah, is AUSTRALIS_TITLEBAR_BUTTON_OFFSET correct in my patch?  Should its
> value be '12'?

Yes, 12 is correct. BUT, only if you are subtracting that from it's normal height.  Buttons should be centered 12 below their normal amount, not simply 12.
No longer depends on: 676241
Comment on attachment 731984 [details] [diff] [review]
Sucessful Implementation V 2.1.2

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

Cancelling this review for now until there is a final patch.
Attachment #731984 - Flags: ui-review?(shorlander)
Comment on attachment 731984 [details] [diff] [review]
Sucessful Implementation V 2.1.2

Clearing review flag, since we are taking a new approach anyway.
Attachment #731984 - Flags: review?(smichaud)
Whiteboard: [Australis:M4]
This patch seems to work well with the patch from bug 676241, the buttons stay at the new position even after window resizes.
> the buttons stay at the new position even after window resizes

Not for me.  I tested on OS X 10.7.5 with a UX build made with your latest patch from bug 676241 (attachment 745420 [details] [diff] [review]) plus your patch from comment #51.

Do you have a more recent patch for bug 676241 that you haven't yet posted there?
Interesting. I'm on 10.8, maybe that's the difference.
I do have a more recent patch for bug 676241 now but comment 51 was tested with the patch that's posted there.
Steven, does the fullscreen button show the snap-back-on-resize behavior, too?
> Steven, does the fullscreen button show the snap-back-on-resize behavior, too?

Yes.

Just a sec and I'll test on OS X 10.8.3.
On 10.8.3 the buttons don't snap back on resize.

I'll try to figure out what's going on.
I figured out what was happening on OS X 10.7.5 -- we were swizzling each of our methods twice!  We were swizzling in NSGrayFrame and then in its superclass NSThemeFrame.  But it's NSThemeFrame that actually implements both methods, so we were swizzling the same methods twice.  Swizzling methods is just swapping pointers -- do it more than once and you're in trouble :-)

Here's an update to Markus' patch that fixes the problem, and also makes a few cosmetic changes.  It applies to the UX branch on top of Markus' latest patch for bug 676241 (attachment 746621 [details] [diff] [review]).

But our work isn't yet done, because we still need to figure out how to query Gecko to find out how many tabs are present.
Attachment #733092 - Attachment is obsolete: true
Attachment #745500 - Attachment is obsolete: true
Comment on attachment 746709 [details] [diff] [review]
Prevent swizzling same method twice

Hah, nice.
The need to determine the number of tabs in the tabstrip is no longer necessary, since the ability to autohide the tabstrip has been removed.

The case we *do* need to worry about, is when the chromehidden attribute of the main window includes "toolbar". This happens for popup windows, like the Mozilla Persona log-in window. We'd kinda broken these popups already, and I've got a patch to fix them in bug 869660, and that's why I remembered about this case.

I don't see anything in widget/cocoa paying attention to root:chromehidden, but that's what I'd do - monitor that attribute, and not move the buttons if that attribute string contains "toolbar".
Tested on OSX 10.6.8 along with the patch in bug 676241, and it works nicely!

The only slight glitch is that the size for the (non-existent) full-screen button is still defaulting to 20, where it should really be 0 in this case.
> I don't see anything in widget/cocoa paying attention to
> root:chromehidden, but that's what I'd do - monitor that attribute,
> and not move the buttons if that attribute string contains
> "toolbar".

Any suggestions how to do this?

Places in other widget code where this already happens?  Places in
Cocoa widget code where we already do something similar (though not
identical)?

Thanks in advance!
> The only slight glitch is that the size for the (non-existent)
> full-screen button is still defaulting to 20, where it should really
> be 0 in this case.

I'm not entirely sure what you mean.  Are you saying that the
non-existent full-screen button somehow "takes up space"?

And in any case I suspect this is a problem that needs to be fixed
elsewhere -- perhaps in a new bug spun off from bug 676241 or bug
865374.

My current patch for this bug does precisely nothing wrt to the
(non-existent) full-screen button on OS X 10.6.  Since there is no
_fullScreenButtonOrigin method on SnowLeopard, it doesn't get hooked.
(In reply to Steven Michaud from comment #61)
> > I don't see anything in widget/cocoa paying attention to
> > root:chromehidden, but that's what I'd do - monitor that attribute,
> > and not move the buttons if that attribute string contains
> > "toolbar".
> 
> Any suggestions how to do this?
> 
> Places in other widget code where this already happens?  Places in
> Cocoa widget code where we already do something similar (though not
> identical)?

The handling of the chromemargin attribute in nsXULWindow.cpp and nsXULElement.cpp.

(In reply to Steven Michaud from comment #62)
> > The only slight glitch is that the size for the (non-existent)
> > full-screen button is still defaulting to 20, where it should really
> > be 0 in this case.
> 
> I'm not entirely sure what you mean.  Are you saying that the
> non-existent full-screen button somehow "takes up space"?

Oops, my fault. I hardcoded the size to (20, 0) in bug 868211, regardless of platform...
Whiteboard: [Australis:M4] → [Australis:M5]
(Following up comment #61)

Note to myself:

There's an nsIWidget::HideWindowChrome(bool aShouldHide) method, implemented by nsCocoaWindow, that might do what we need.
Whiteboard: [Australis:M5] → [Australis:M6]
Depends on: 676241
For the record, here's a way to display a window that has no toolbars (whose titlebar buttons we shouldn't move):

1) Open the Error Console (under Tools : Web Developer).

2) Paste one of the following strings into the Code box (at the top):

window.open("https://www.google.ca", "_blank", "toolbars=no, width=300, height=300")
window.open("https://www.google.ca", "_blank", "toolbars=no, location=yes, width=300, height=300")

3) Click on the Evaluate button.
Here's a way to display the Persona login window (which works even if you don't have a Persona account):

1) Visit https://current.trovebox.com/user/login.
2) Click on the Mozilla Persona button.
Attached patch Patch v0.95 (obsolete) — Splinter Review
Here's a work-in-progress patch that I hope is close to its final
form.

The trickiest part was finding out in widget code what Mike mentioned
in comment #59 -- whether "our" window includes "toolbar" in its
"chromehidden" attribute, or (to put this another way) whether it's a
toplevel window that was created with a "feature" of "toolbar[s]=no".

Gecko has always made this difficult, and widget programmers have
always been discouraged from simply looking "up" the frame/view/widget
hierarchy to find an object that can be queried for the necessary
information (on the grounds that this breaks abstraction, or requires
ad-hoc assumptions).

I quickly found out that my hunch from comment #64 is wrong --
nsIWidget::HideWindowChrome() doesn't help at all.

I also found out that the information I seek (unlike that provided by
calls to nsIWidget::HideWindowChrome()) isn't dynamic.  The contents
of a window's "chromehidden" attribute are decided before the window
is shown for the first time.  This kind of information is best passed
to widget code as some kind of parameter to nsIWidget::Create().  And
since we're talking about an attribute of a window's chrome, it seemed
best to do this by creating an additional nsBorderStyle type.

Some information about window chrome is dynamic -- for example whether
all of it should be hidden or shown (as determined by calls from Gecko
to nsIWidget::HideWindowChrome()).  Other information may be both
static and dynamic (though I need to spend some more time next week
figuring this out).  But as best I can tell, the only way to get a
window without any toolbar is to create it with a feature of
"toolbar[s]=no".  There's no setting (either available in the UI or
hidden somewhere in about:config) that can be used to change its
presence or absence on a given window dynamically.

If this turns out to be incorrect, I'll need to add an
nsIWidget::HideWindowToolbar() method.

This patch is against mozilla-central, and is deliberately incorrect
for a tree where Australis hasn't yet been landed -- it draws the
titlebar buttons in their Australis locations whenever we draw in the
titlebar (as we do for lightweight themes) in a window that doesn't
have "toolbar" in its "chromehidden" attribute.  I did this because I
think it's useful to test changing those buttons' locations
dynamically -- which you can do by enabling or disabling a lightweight
theme.  I found I needed to add some code to make this work properly.

I'll start a tryserver build, which should be available in a few
hours.

But I'm taking a long weekend, so I won't be around to work on this
bug until next week.  That's when I'll start asking for reviews.
Attachment #746709 - Attachment is obsolete: true
Attached patch Patch v1.0 (obsolete) — Splinter Review
Here's what I hope is the final patch.

It's against mozilla-central (on which Australis hasn't yet landed),
so I needed to add some #ifdef AUSTRALIS blocks.

> as best I can tell, the only way to get a window without any toolbar
> is to create it with a feature of "toolbar[s]=no".

I haven't found anything to contradict this.

> Other information may be both static and dynamic (though I need to
> spend some more time next week figuring this out).

Though it's not strictly speaking relevant to this bug, I spent
several more hours trying to figure out how static chrome settings
(e.g. the window features "location=yes/no" or "personalbar=yes/no")
work with chrome elements that can appear or disappear dynamically.
As best I can tell, (static) chromehidden settings
(e.g. "location=no") always override dynamic settings.  But "positive"
chrome settings (e.g. "location=yes") can be overridden by dynamic
settings.  The dynamic settings appear to always be properties of
nsXULElement objects -- so if we wanted this information in widget
code, we'd need to add code to nsXULElement to pass it to the
appropriate nsIWidget object.
Attachment #756272 - Attachment is obsolete: true
Attachment #758888 - Flags: review?(mstange)
(In reply to Steven Michaud from comment #69)
> It's against mozilla-central (on which Australis hasn't yet landed),
> so I needed to add some #ifdef AUSTRALIS blocks.

Why not a name which describes the feature rather that the name of a theme in a specific application? I don't think widget code should know about the codename of browser redesign.

In this case I don't see the need for the #ifdef as the blocks inside them are fairly straightforward and unlikely to be bitrotten so a separate UX branch patch should work fine.
> Why not a name which describes the feature rather that the name of a
> theme in a specific application?

See bug 857642.

> In this case I don't see the need for the #ifdef

We do need them on mozilla-central.  Otherwise the titlebar buttons
will be moved down whenever a lightweight theme is enabled.
(In reply to Steven Michaud from comment #71)
> > Why not a name which describes the feature rather that the name of a
> > theme in a specific application?
> 
> See bug 857642.

I've read the bug and it doesn't answer the question. This is the last Australis cocoa feature, isn't it?

> > In this case I don't see the need for the #ifdef
> 
> We do need them on mozilla-central.  Otherwise the titlebar buttons
> will be moved down whenever a lightweight theme is enabled.

I don't see how. The m-c patch would not contain the stuff currently in #ifdef AUSTRALIS whereas the UX patch would not (only the |else| contents).
(In reply to Markus Stange from comment #63)
> (In reply to Steven Michaud from comment #62)
> > > The only slight glitch is that the size for the (non-existent)
> > > full-screen button is still defaulting to 20, where it should really
> > > be 0 in this case.
> > 
> > I'm not entirely sure what you mean.  Are you saying that the
> > non-existent full-screen button somehow "takes up space"?
> 
> Oops, my fault. I hardcoded the size to (20, 0) in bug 868211, regardless of
> platform...

Filed bug 880124 for the fullscreen button size on 10.6
Comment on attachment 758888 [details] [diff] [review]
Patch v1.0

This looks good to me. I have no opinion on the ifdef, it's only going to be temporary anyway.

However, there's another approach which occurred to me now, which might be preferable to this one: We could make -moz-appearance:-moz-window-button-box and -moz-appearance:-mac-fullscreen-button more magical and have their position control the position of the actual window buttons. That way, the offset of the buttons would be controllable from CSS, just by adding margins to #titlebar-buttonbox and #titlebar-fullscreen-button. Implementation-wise it would be fairly simple because there's already existing machinery which reports whitelisted -moz-appearance box geometries to the widget (nsChildView::UpdateThemeGeometries).
Matt, would you prefer such an approach?
Attachment #758888 - Flags: review?(mstange) → review+
(In reply to Markus Stange from comment #74)
> However, there's another approach which occurred to me now, which might be
> preferable to this one: We could make -moz-appearance:-moz-window-button-box
> and -moz-appearance:-mac-fullscreen-button more magical and have their
> position control the position of the actual window buttons. That way, the
> offset of the buttons would be controllable from CSS, just by adding margins
> to #titlebar-buttonbox and #titlebar-fullscreen-button. Implementation-wise
> it would be fairly simple because there's already existing machinery which
> reports whitelisted -moz-appearance box geometries to the widget
> (nsChildView::UpdateThemeGeometries).
> Matt, would you prefer such an approach?

Ooh, that would definitely be a nicer approach as it would let theme, extension, and other application (e.g. Thunderbird) authors to have control over the position and it avoids the hard-coded magic numbers in widget code which is theme/application specific. 

It seems like that approach would also remove the need for the ifdef if the default position was the old native value.
Attached patch new approach (obsolete) — Splinter Review
New patch. The problem with this approach is that it only works if the titlebar buttons are considered "visible". Clipping them away completely by giving #titlebar { height: 9px; overflow: hidden; } would make them invisible and thus not shift the window buttons.
I've tried to solve this by doing #titlebar { height: 38px; margin-bottom: -29px; }, but this margin-bottom is unset by TabsInTitlebar._update. I don't really know what it's doing and whether it's worth tweaking. Matt, maybe you can find a better approach on the CSS side.
I like the fact that the vertical centering of the buttons is now expressed as -moz-box-align: center.

Steven, sorry that I'm undoing much of your work again, it's not intentional :(
Attachment #759078 - Flags: feedback?(mnoorenberghe+bmo)
Comment on attachment 759078 [details] [diff] [review]
new approach

This patch applies on top of bug 880124 and bug 880153 on mozilla-ux.
(In reply to Markus Stange from comment #74)
> Comment on attachment 758888 [details] [diff] [review]
> Patch v1.0
> 
> This looks good to me. I have no opinion on the ifdef, it's only going to be
> temporary anyway.
> 
> However, there's another approach which occurred to me now, which might be
> preferable to this one: We could make -moz-appearance:-moz-window-button-box
> and -moz-appearance:-mac-fullscreen-button more magical and have their
> position control the position of the actual window buttons. That way, the
> offset of the buttons would be controllable from CSS, just by adding margins
> to #titlebar-buttonbox and #titlebar-fullscreen-button. Implementation-wise
> it would be fairly simple because there's already existing machinery which
> reports whitelisted -moz-appearance box geometries to the widget
> (nsChildView::UpdateThemeGeometries).
> Matt, would you prefer such an approach?

I also like this idea - I like the more fine-grained control this gives front-end.
> However, there's another approach which occurred to me now, which might be
> preferable to this one: We could make -moz-appearance:-moz-window-button-box
> and -moz-appearance:-mac-fullscreen-button more magical and have their
> position control the position of the actual window buttons. That way, the
> offset of the buttons would be controllable from CSS, just by adding margins
> to #titlebar-buttonbox and #titlebar-fullscreen-button. Implementation-wise
> it would be fairly simple because there's already existing machinery which
> reports whitelisted -moz-appearance box geometries to the widget
> (nsChildView::UpdateThemeGeometries).

I like this approach better, too.  I only wish you'd thought of it earlier :-)

I need to learn more about CSS.
Attachment #758888 - Attachment is obsolete: true
(In reply to comment #72)

We need to use #ifdef blocks to land this work (mine or Markus's) on mozilla-central now.  And since it's entirely motivated by the wish to land Australis, AUSTRALIS is the best name to use.

And no, this bug isn't the last to land on m-c -- the main Australis patch (bug 865374) hasn't yet landed there, either.

But maybe it's best *not* to land this work on m-c until it can be landed together with 865374.  Then we wouldn't need any ifdef blocks.
(In reply to Steven Michaud from comment #80)
> We need to use #ifdef blocks to land this work (mine or Markus's)

When there are no boxes with -moz-appearance: -moz-window-button-box or -mac-fullscreen-button, my approach defaults to "use default button position", so I don't think I'd need an #ifdef. We would have to land the rest of the implementation for these -moz-appearance values on mozilla-central first, though. That doesn't conflict with anything on mozilla-central, as far as I know.
(In reply to comment #81)

OK, then.  I stand corrected.
Whiteboard: [Australis:M6] → [Australis:M7]
Comment on attachment 759078 [details] [diff] [review]
new approach

Apart from the margin-bottom bug on the front-end, this should be ready to review. One thing I don't really like is the way I implemented the default position handling: The swizzled [frameView _closeButtonOrigin] calls [window windowButtonsPosition], which calls [window defaultWindowButtonsPosition], which calls the original [frameView FrameView__closeButtonOrigin]. Steven, maybe you have a better idea how to handle that case.
Attachment #759078 - Flags: review?(smichaud)
-[ToolbarWindow windowButtonsPosition] and -[ToolbarWindow fullScreenButtonPosition] are only called from the swizzled [frameView _...ButtonOrigin] methods, from which you can easily get the default positions by calling super.  So why not just add a defaultPosition:(NSRect)aDefaultPosition parameter to those two methods?

Otherwise this looks fine to me, but I'd like a chance to test it.  I should be done with my tests later today.
(In reply to Steven Michaud from comment #84)
> -[ToolbarWindow windowButtonsPosition] and -[ToolbarWindow
> fullScreenButtonPosition] are only called from the swizzled [frameView
> _...ButtonOrigin] methods, from which you can easily get the default
> positions by calling super.

Well, not super, but the original swizzled-away FrameView_* method. OK.

> So why not just add a
> defaultPosition:(NSRect)aDefaultPosition parameter to those two methods?

Good idea. But I still need to get the default position in place*Button if I want to calculate the offset.
Or I drop the idea with the offsets and just store the actual, absolute position. I was worried about them getting out-of-sync during window resizing, but I think that's not a problem.
Attached patch new approach (obsolete) — Splinter Review
This uses the default button arguments. Untested - I need to leave now and would have to do a full rebuild because I updated my tree
Attachment #759078 - Attachment is obsolete: true
Attachment #759078 - Flags: review?(smichaud)
Attachment #759078 - Flags: feedback?(mnoorenberghe+bmo)
> But I still need to get the default position in place*Button if I want to
> calculate the offset.

I missed that.  Sorry.

There's actually nothing wrong with your original approach.  It just looks a bit odd.  Which means that if you keep it you should probably add a comment explaining what the calls to FrameView__...ButtonOrigin mean.
> There's actually nothing wrong with your original approach.

With one quibble:  Calling the original FrameView ...buttonOrigin methods ourselves might conceivably lead to undefined behavior.  It'd be safer (perhaps only very marginally safer) to only use the results of the calls (to the original methods) made by the OS.
> Calling the original FrameView ...buttonOrigin methods ourselves might
> conceivably lead to undefined behavior.

If we call them at the "wrong" time.
I tested both versions of your patch, Markus (attachment 759805 [details] [diff] [review] and attachment 759078 [details] [diff] [review]), on OS X 10.6.8, 10.7.5 and 10.8.3.  I applied them to the current UX branch.

The titlebar buttons always appear in their correct locations, but there's definitely other weirdness:

On first opening a new window, the toolbar (which contains the tabs) is much lower than it should be.

It gets moved to the correct location when you click the zoom button twice (to maximize it and then restore it to its original size).  But then the top few pixels are drawn in a color that's slightly too light (whether or not the window is active).

I see these problem on all the versions of OS X I tested on.

Does your patch depend on other work that hasn't yet landed on the UX branch?
> The titlebar buttons always appear in their correct locations, but there's
> definitely other weirdness:

And no, those problems don't happen without your patch (in fully current UX-branch code).
(In reply to Steven Michaud from comment #87)
> There's actually nothing wrong with your original approach.  It just looks a
> bit odd.

I agree, and I think the approach with the defaultPosition argument looks better. The method names are just a bit long, maybe.

(In reply to Steven Michaud from comment #90)
> On first opening a new window, the toolbar (which contains the tabs) is much
> lower than it should be.

Yes, that's the "margin-bottom problem" I mentioned in comment 76 and comment 83. It needs to be fixed in CSS or JavaScript code, so I'd like to leave it up to Matt (or anybody else from the Australis team).
(In reply to Markus Stange from comment #92)
> Yes, that's the "margin-bottom problem" I mentioned in comment 76 and
> comment 83. It needs to be fixed in CSS or JavaScript code, so I'd like to
> leave it up to Matt (or anybody else from the Australis team).

Yeah, I was just taking a look at this. Hopefully I'll have something later today.
mconley, exactly how urgent is this bug?

And MattN, how soon do you think you're likely to have a fix for the "margin-bottom problem"?

If this bug is urgent enough, and a solution that uses CSS for the titlebar button positions is far enough off, we should consider falling back to my "patch v1.0".  That would give us more time to implement the CSS solution.
(In reply to Steven Michaud from comment #94)
> mconley, exactly how urgent is this bug?
> 

It blocks shipping Australis, but I don't think it blocks landing on mozilla-central.

So, in terms of urgency, "the sooner the better", but you're not blocking us from moving forward.
(In reply to Steven Michaud from comment #94)
> And MattN, how soon do you think you're likely to have a fix for the
> "margin-bottom problem"?

This is still near the top of my list. Hopefully I can get to it today.
Flags: needinfo?(mnoorenberghe+bmo)
I think we can go ahead with this new approach (at least from the browser perspective)

Changes from mstange's patch:
* Do an early return in TabsInTitlebar._update for OS X as the rest is not relevant for OS X AFAICT and is additional computation.
* Use calc() to make the origin of the numbers more clear and depend on @tabHeight@. I assumed 2px is related to the window border and highlight. Was this right?
** Unfortunately this means moving that stuff below the include or else we need to move the defines of tabs.inc.css to tabs.inc?
* I think putting the margins on the placeholders/buttons is less error-prone. e.g. when the fullscreen button gets display: none, it wouldn't leave a 7px gap behind.

TODO: We should have the buttons in their OS default positions regardless of the the browser theme but since the -moz-appearance's are set in browser/base/content/browser.css, the controls are in the corners by default. I guess we can move the -moz-appearance to browser/themes/osx/ to avoid this but that is inconsistent with Windows.
TODO: check 10.6 (without the fullscreen button)
Attachment #761344 - Flags: feedback?(mstange)
Attachment #761344 - Flags: feedback?(mconley)
Flags: needinfo?(mnoorenberghe+bmo)
(In reply to Matthew N. [:MattN] from comment #97)
> I assumed 2px is related to the window border and highlight.
> Was this right?

I arrived at 2px experimentally by increasing it from 0 until the button position matched the mockup. But now that I've seen your calc()ulations, this can probably be simplified: Instead of setting height: 31px (tabHeight) + 9px (spacingAboveTabbar) - 2px; and padding-top: 2px; we can probably just set height: 31px + 9px; padding-top: 0; and arrive at the same button position, right? I'll give that a try now.

> ** Unfortunately this means moving that stuff below the include or else we
> need to move the defines of tabs.inc.css to tabs.inc?

What's wrong with moving it below the include?

> * I think putting the margins on the placeholders/buttons is less
> error-prone. e.g. when the fullscreen button gets display: none, it wouldn't
> leave a 7px gap behind.

Agreed.

> TODO: We should have the buttons in their OS default positions regardless of
> the the browser theme but since the -moz-appearance's are set in
> browser/base/content/browser.css, the controls are in the corners by
> default. I guess we can move the -moz-appearance to browser/themes/osx/ to
> avoid this but that is inconsistent with Windows.

Oh. Yeah, I think #ifdefing out the -moz-appearance in content/browser.css for OS X would be the right solution here, even if it means it's inconsistent. But the default button positions really are different on these systems. Alternatively, I guess we could change the widget part in such a way that the default position that we get from OS X are used as a minimum distance from the window frame. Then we'd clamp the allowed positions to be no nearer to the window frame than the default.

> TODO: check 10.6 (without the fullscreen button)

Oh, I guess the horizontal 7px margin will still apply there. The box still exists, it's just sized to 0x0. Maybe add a @media not mac-lion-theme display:none?
Attached patch new approach (obsolete) — Splinter Review
This patch removes the "2px" stuff and modifies the widget part to make sure that buttons don't ever get nearer to the window frame than they are by default.

Should the placeholders in the tab bar have 7px margin to the inside of the window, too? I think that just reduces the available tabbar width unnecessarily.
Attachment #759805 - Attachment is obsolete: true
Attachment #761344 - Attachment is obsolete: true
Attachment #761344 - Flags: feedback?(mstange)
Attachment #761344 - Flags: feedback?(mconley)
Attachment #761380 - Flags: feedback?(mnoorenberghe+bmo)
Attachment #761380 - Flags: feedback?(mconley)
Comment on attachment 761380 [details] [diff] [review]
new approach

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

This does the job really nicely! The browser.js and browser.css changes look sane to me too.

::: browser/themes/osx/browser.css
@@ +2120,5 @@
>  
> +%define spaceAboveTabbar 9px
> +
> +#titlebar {
> +  height: calc(@spaceAboveTabbar@ + @tabHeight@);

Ah, this is very nice. Because of the missing menu, OS X gets off easy with this. :)
Attachment #761380 - Flags: feedback?(mconley) → feedback+
Comment on attachment 761380 [details] [diff] [review]
new approach

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

(In reply to Markus Stange from comment #99)
> Should the placeholders in the tab bar have 7px margin to the inside of the
> window, too? I think that just reduces the available tabbar width
> unnecessarily.

I just did that because it was easier for the WIP instead of having two separate rulesets for the left and right buttons. I think we're fine without the margin on the inside when comparing it to the OS X mockup. Note that while testing you should have tab overflow as the scroll arrows get closer than tabs.

The following works:

.titlebar-placeholder[type="caption-buttons"],
#titlebar-buttonbox {
  margin-left: 7px;
}

.titlebar-placeholder[type="fullscreen-button"],
#titlebar-fullscreen-button {
  margin-right: 7px;
}

(In reply to Markus Stange from comment #98)
> (In reply to Matthew N. [:MattN] from comment #97)
> > TODO: check 10.6 (without the fullscreen button)
> 
> Oh, I guess the horizontal 7px margin will still apply there. The box still
> exists, it's just sized to 0x0. Maybe add a @media not mac-lion-theme
> display:none?

Three options come to mind:
1) display: none if not mac-lion-theme
2) Wrap the fullscreen margin lines above in @media mac-lion-theme
3) Have widget make the fullscreen button display: none instead of 0,0 dimensions

I prefer option 3 followed by option 2 if that's difficult.

::: browser/themes/osx/browser.css
@@ +2117,5 @@
>  }
>  
>  %include ../shared/tabs.inc.css
>  
> +%define spaceAboveTabbar 9px

Please put this at the top of the file with the other defines.
Attachment #761380 - Flags: feedback?(mnoorenberghe+bmo) → feedback+
(In reply to Matthew N. [:MattN] from comment #101)
> 3) Have widget make the fullscreen button display: none instead of 0,0
> dimensions

This wouldn't help because the margin that actually makes a difference is set on the placeholder, and not on the button that has the -moz-appearance. Moreover, this would be the first time that -moz-appearance could make something display:none, I think.

I chose option 2.
Attached patch patch (obsolete) — Splinter Review
Let's get this in!

r?smichaud for the widget changes
Attachment #731984 - Attachment is obsolete: true
Attachment #761380 - Attachment is obsolete: true
Attachment #761896 - Flags: review?(smichaud)
Comment on attachment 761896 [details] [diff] [review]
patch

r?mattn for the theme changes
Attachment #761896 - Flags: review?(mnoorenberghe+bmo)
Comment on attachment 761896 [details] [diff] [review]
patch

r?dao for the browser.js changes
Attachment #761896 - Flags: review?(dao)
Comment on attachment 761896 [details] [diff] [review]
patch

r?roc for adding NS_THEME_WINDOW_BUTTON_BOX and NS_THEME_MOZ_MAC_FULLSCREEN_BUTTON to the RegisterThemeGeometry whitelist
Attachment #761896 - Flags: review?(roc)
Attachment #761896 - Flags: review?(mnoorenberghe+bmo) → review+
Comment on attachment 761896 [details] [diff] [review]
patch

Looks fine to me.

I tested it (applied to the UX branch) on OS X 10.8.4, 10.7.5 and 10.6.8 and saw no problems.
Attachment #761896 - Flags: review?(smichaud) → review+
Comment on attachment 761896 [details] [diff] [review]
patch

> #ifdef XP_MACOSX
>       let fullscreenButton  = $("titlebar-fullscreen-button");
>       this._sizePlaceholder("fullscreen-button", rect(fullscreenButton).width);
>+      // The rest of the calculations are not needed for OS X.
>+      return;
> #endif

By "for OS X" you mean "with our default theme for OS X"? In that case, doing this based on XP_MACOSX seems bogus.

>+#titlebar {
>+  height: calc(@spaceAboveTabbar@ + @tabHeight@);
>+  margin-bottom: -@tabHeight@;
>+}

Does this fail gracefully if @tabHeight@ doesn't match the actual tab bar height due to e.g. an increased font size or larger elements being put in the tab bar?
Attachment #761896 - Flags: review?(dao) → review-
(In reply to Dão Gottwald [:dao] from comment #108)
> Comment on attachment 761896 [details] [diff] [review]
> patch
> 
> > #ifdef XP_MACOSX
> >       let fullscreenButton  = $("titlebar-fullscreen-button");
> >       this._sizePlaceholder("fullscreen-button", rect(fullscreenButton).width);
> >+      // The rest of the calculations are not needed for OS X.
> >+      return;
> > #endif
> 
> By "for OS X" you mean "with our default theme for OS X"? In that case,
> doing this based on XP_MACOSX seems bogus.

Matt, can you answer this? I still haven't had a chance to actually read through this function and picture the implications.

> >+#titlebar {
> >+  height: calc(@spaceAboveTabbar@ + @tabHeight@);
> >+  margin-bottom: -@tabHeight@;
> >+}
> 
> Does this fail gracefully if @tabHeight@ doesn't match the actual tab bar
> height due to e.g. an increased font size or larger elements being put in
> the tab bar?

How would you increase the font-size?
And placing something big into the tabbar apparently already destroys the layout completely... I'll file a new bug for that.
Flags: needinfo?(mnoorenberghe+bmo)
(In reply to Markus Stange from comment #109)
> How would you increase the font-size?

I suppose OS X provides no preference for this, but you can easily do it with CSS, i.e. userChrome.css or an add-on stylesheet.

Preferably stuff like that would be handled by the TabsInTitlebar method where you chose to return early.
Removing the items from M7 that do not block landing on m-c.
Whiteboard: [Australis:M7] → [Australis:M?]
(In reply to Dão Gottwald [:dao] from comment #110)
> Preferably stuff like that would be handled by the TabsInTitlebar method
> where you chose to return early.

Yes, this is what TabsInTitlebar is supposed to handle - though I don't know if it has ever been tested for large fonts on OSX.
(In reply to Dão Gottwald [:dao] from comment #108)
> Comment on attachment 761896 [details] [diff] [review]
> patch
> 
> > #ifdef XP_MACOSX
> >       let fullscreenButton  = $("titlebar-fullscreen-button");
> >       this._sizePlaceholder("fullscreen-button", rect(fullscreenButton).width);
> >+      // The rest of the calculations are not needed for OS X.
> >+      return;
> > #endif
> 
> By "for OS X" you mean "with our default theme for OS X"? In that case,
> doing this based on XP_MACOSX seems bogus.

The rest of the styling was taking the menubar into account and showing our in-window menubar isn't something that we support on OS X AFAIK. If some other theme wants to allow a menubar then they'd also need code to make it work.

> Does this fail gracefully if @tabHeight@ doesn't match the actual tab bar
> height due to e.g. an increased font size or larger elements being put in
> the tab bar?

Since there is no supported way to change the font-size (by OS X or Firefox Preferences), I don't think this is an issue.

Large third-party elements isn't something I had considered but that doesn't seem like a common thing and can probably be handled in bug 884492 which Markus filed.

(In reply to Dão Gottwald [:dao] from comment #110)
> Preferably stuff like that would be handled by the TabsInTitlebar method
> where you chose to return early.

So much of that code is related to the menubar so I'd rather not run that stuff needlessly on OS X. We can wrap more of it in #ifndef XP_OSX I suppose but I still find that code is already complicated to follow IMO.
Flags: needinfo?(mnoorenberghe+bmo)
(In reply to Matthew N. [:MattN] from comment #113)
> (In reply to Dão Gottwald [:dao] from comment #108)
> > By "for OS X" you mean "with our default theme for OS X"? In that case,
> > doing this based on XP_MACOSX seems bogus.
> 
> The rest of the styling was taking the menubar into account and showing our
> in-window menubar isn't something that we support on OS X AFAIK.

That just means that menuHeight is 0 in the calculations, right? I don't see why that's reason enough to skip them entirely.

> > Does this fail gracefully if @tabHeight@ doesn't match the actual tab bar
> > height due to e.g. an increased font size or larger elements being put in
> > the tab bar?
> 
> Since there is no supported way to change the font-size (by OS X or Firefox
> Preferences), I don't think this is an issue.
> 
> Large third-party elements isn't something I had considered but that doesn't
> seem like a common thing and can probably be handled in bug 884492 which
> Markus filed.

How would you handle it other than by letting TabsInTitlebar do its calculations?
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P4]
Markus, any updates here?
Flags: needinfo?(mstange)
Summary: Change position of standardWindowButton's for Australis → Change position of the standardWindowButtons for Australis
Talked to Markus on IRC and he is currently not working on this. So what's going on with this bug now?

:MattN, are you investigating this now?
Flags: needinfo?(mstange) → needinfo?(mnoorenberghe+bmo)
Version: Trunk → unspecified
Blocks: 768516
(In reply to Dão Gottwald [:dao] from comment #114)
> (In reply to Matthew N. [:MattN] from comment #113)
> > (In reply to Dão Gottwald [:dao] from comment #108)
> > > By "for OS X" you mean "with our default theme for OS X"? In that case,
> > > doing this based on XP_MACOSX seems bogus.
> > 
> > The rest of the styling was taking the menubar into account and showing our
> > in-window menubar isn't something that we support on OS X AFAIK.
> 
> That just means that menuHeight is 0 in the calculations, right? I don't see
> why that's reason enough to skip them entirely.

Getting the dimensions of elements can lead to uninterruptible reflows which I wanted to avoid.

> > > Does this fail gracefully if @tabHeight@ doesn't match the actual tab bar
> > > height due to e.g. an increased font size or larger elements being put in
> > > the tab bar?
> > 
> > Since there is no supported way to change the font-size (by OS X or Firefox
> > Preferences), I don't think this is an issue.
> > 
> > Large third-party elements isn't something I had considered but that doesn't
> > seem like a common thing and can probably be handled in bug 884492 which
> > Markus filed.
> 
> How would you handle it other than by letting TabsInTitlebar do its
> calculations?

We can use ifdef's to have the menuHeight calculations skipped or have a static height of 0.

Someone else can go ahead and make this work without the early return since I won't get to it this week.
Flags: needinfo?(mnoorenberghe+bmo)
Unbitrotted. This changes a bit of the CSS, and doesn't touch browser.js, and works perfectly with default font settings and so on. I also tested adding a button to the tabstrip and artificially increasing its height, then running TabsInTitlebar._update(true), which correctly resized everything. Only one thing 'wrong' - the buttons don't remain centered wrt the space above the navbar in this case. I'll do a second patch in a bit which alters TabsInTitlebar._update to center the content (instead of just giving it margin-bottom).
Attachment #823298 - Flags: review?(dao)
Attachment #761896 - Attachment is obsolete: true
I misunderstood how the existing patch was centering things. This change is necessary so as not to mess things up. However, it uses TabsInTitlebar._update, and so this is still fine for larger toolbar heights.
Attachment #823374 - Flags: review?(dao)
Attachment #823298 - Attachment is obsolete: true
Attachment #823298 - Flags: review?(dao)
Assignee: smichaud → gijskruitbosch+bugs
Comment on attachment 823374 [details] [diff] [review]
"Change position of the standardWindowButtons for Australis" [

Redirecting review per discussion on IRC.
Attachment #823374 - Flags: review?(dao) → review?(jaws)
Blocks: 884492
Comment on attachment 823374 [details] [diff] [review]
"Change position of the standardWindowButtons for Australis" [

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

[sidenote: this review is for only the /browser parts of this patch]

r=me with the following nits addressed.

::: browser/base/content/browser.js
@@ +4291,5 @@
>        // No need to look up the menubar stuff on OS X:
>        let menuHeight = 0;
>        let fullMenuHeight = 0;
> +      // Instead, look up the titlebar padding:
> +      let titlebarPadding = parseInt(window.getComputedStyle(titlebar).paddingTop, 10);

I think we should use parseFloat here.

::: browser/themes/osx/browser.css
@@ +46,5 @@
>  }
>  
> +.titlebar-placeholder[type="caption-buttons"],
> +#titlebar-buttonbox {
> +  margin-left: 7px;

-moz-margin-start

@@ +52,5 @@
> +
> +@media (-moz-mac-lion-theme) {
> +  .titlebar-placeholder[type="fullscreen-button"],
> +  #titlebar-fullscreen-button {
> +    margin-right: 7px;

-moz-margin-end
Attachment #823374 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] from comment #121)
> > +.titlebar-placeholder[type="caption-buttons"],
> > +#titlebar-buttonbox {
> > +  margin-left: 7px;
> 
> -moz-margin-start
> 
> @@ +52,5 @@
> > +
> > +@media (-moz-mac-lion-theme) {
> > +  .titlebar-placeholder[type="fullscreen-button"],
> > +  #titlebar-fullscreen-button {
> > +    margin-right: 7px;
> 
> -moz-margin-end

Disregard these two nits. OSX doesn't reposition these when in RTL, so making this change will cause the margins to be placed on the wrong sides. Thanks to Gijs for mentioning this.

Can you please add a comment at both of these saying that OSX doesn't flip the position of these in RTL?
I've only changed browser/ bits of the patch (the other folks on here had given r+ to the previous patch on the other parts, AIUI), and unbitrotted the rest, so I've gone and landed this on the UX branch. Thanks!

https://hg.mozilla.org/projects/ux/rev/9704aedc2a5c
Whiteboard: [Australis:M?][Australis:P4] → [Australis:M9][Australis:P4][fixed-in-ux]
Egh, and then I neglected to apply nits. I've added a comment as requested in https://hg.mozilla.org/projects/ux/rev/98d7ee045277 .

I've not yet changed the parseInt to parseFloat. We're using parseInt throughout the titlebar calculations. If you think we should use parseFloat, we should do that everywhere in one go. I tried pinging you on IRC, but I didn't get a response and I need sleep. I can either get you a followup patch here, or file/fix a followup bug, or we can leave it. Needinfo'ing so this doesn't get lost... Sorry for not doing this before landing. :-(
Flags: needinfo?(jaws)
(In reply to :Gijs Kruitbosch from comment #124)
> I've not yet changed the parseInt to parseFloat. We're using parseInt
> throughout the titlebar calculations. If you think we should use parseFloat,
> we should do that everywhere in one go. I tried pinging you on IRC, but I
> didn't get a response and I need sleep. I can either get you a followup
> patch here, or file/fix a followup bug, or we can leave it. Needinfo'ing so
> this doesn't get lost... Sorry for not doing this before landing. :-(

I filed bug 933539 as a follow-up bug to fix all of the usages.
Depends on: 933539
Flags: needinfo?(jaws)
Do you want the window buttons come down when going into customize mode, which seemingly enlarges the tabbar height, or stay in the former position?
(In reply to Thomas Stache from comment #126)
> Do you want the window buttons come down when going into customize mode,
> which seemingly enlarges the tabbar height, or stay in the former position?

Yeah, I don't think we want that. I'll file a bug.
Depends on: 933933
Blocks: 934023
No longer blocks: 934023
Depends on: 934023
Restrict Comments: true
https://hg.mozilla.org/mozilla-central/rev/9704aedc2a5c
https://hg.mozilla.org/mozilla-central/rev/98d7ee045277
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Restrict Comments: false
Whiteboard: [Australis:M9][Australis:P4][fixed-in-ux] → [Australis:M9][Australis:P4]
Target Milestone: --- → mozilla28
Depends on: 940134
No longer depends on: 940134
Blocks: 941058
Depends on: 898875
Depends on: 942170
Depends on: 987697
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: