User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:2.0b2pre) Gecko/20100628 Minefield/4.0b2pre Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:2.0b2pre) Gecko/20100628 Minefield/4.0b2pre The current position and size make it difficult to hit the buttons. Plus there's no reason for them to be in the navigation bar. Reproducible: Always Steps to Reproduce: 1. Press F11

This will be dependent on removing the app menu button in bug 575155 for tabs-on-bottom. Though for Tabs-on-top can the captions be moved from the nav bar to the tab bar?

I can reproduce this Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; en-US; rv:2.0b2pre) Gecko/20100710 Minefield/4.0b2pre

The caption buttons haven't moved, but tabs on top by default only makes having the buttons on the nav bar seem like it needs another solution. With Tabs on Bottom, this still make sense to have them on the nav bar.

Not a blocker. Would definitely take a patch though.

This patch is a quick fix to make the fullscreen window controls go to the tab bar in tabs-on-top mode. It can get away with being so simple because there is no way to change tabs-on-top setting while in fullscreen mode, so no observers are needed. I suspect this doesn't work in Linux (gnomestripe) with tabs-on-bottom and the navigation toolbar in Text mode. Could someone with access to Linux please test?

Comment on attachment 499641 [details] [diff] [review] patch >- <hbox id="fullscreenflex" flex="1" hidden="true" fullscreencontrol="true"/> >- <hbox id="window-controls" hidden="true" fullscreencontrol="true"> >- <toolbarbutton id="minimize-button" >- tooltiptext="&fullScreenMinimize.tooltip;" >- oncommand="window.minimize();"/> >+ <hbox id="fullscreen-control-container" hidden="true" fullscreencontrol="true"> >+ <hbox id="fullscreenflex" flex="1"/> >+ <hbox id="window-controls"> >+ <toolbarbutton id="minimize-button" >+ tooltiptext="&fullScreenMinimize.tooltip;" >+ oncommand="window.minimize();"/> > >- <toolbarbutton id="restore-button" >- tooltiptext="&fullScreenRestore.tooltip;" >- oncommand="BrowserFullScreen();"/> >+ <toolbarbutton id="restore-button" >+ tooltiptext="&fullScreenRestore.tooltip;" >+ oncommand="BrowserFullScreen();"/> > >- <toolbarbutton id="close-button" >- tooltiptext="&fullScreenClose.tooltip;" >- oncommand="BrowserTryToCloseWindow();"/> >+ <toolbarbutton id="close-button" >+ tooltiptext="&fullScreenClose.tooltip;" >+ oncommand="BrowserTryToCloseWindow();"/> >+ </hbox> > </hbox> > </toolbar> This is bogus, fullscreenflex and window-controls are part of the nav bar's default set. >--- a/browser/themes/winstripe/browser/browser-aero.css >+++ b/browser/themes/winstripe/browser/browser-aero.css >@@ -165,16 +165,23 @@ > } > > /* ::::: fullscreen window controls ::::: */ > > #window-controls { > -moz-box-align: start; > } > >+toolbar[mode="text"] #window-controls .toolbarbutton-icon { >+ display: -moz-box; >+} >+toolbar[mode="text"] #window-controls .toolbarbutton-text { >+ display: none; >+} What does this have to do with this bug?

Thanks for the review. (In reply to comment #12) > This is bogus, fullscreenflex and window-controls are part of the nav bar's > default set. Will it break existing profiles if I remove these from defaultset and add fullscreen-control-container? > >+toolbar[mode="text"] #window-controls .toolbarbutton-icon { > >+ display: -moz-box; > >+} > >+toolbar[mode="text"] #window-controls .toolbarbutton-text { > >+ display: none; > >+} > > What does this have to do with this bug? In some circumstances when the toolbar was in Text mode and the tabs were on the bottom, the fullscreen window controls disappeared. This was a regression with the other parts of this patch, so I added this to fix it. I don't know what the root cause was, but I figured it was some side-effect of adding and removing nodes from the toolbar which weren't picking up the correct display style in some way or other. It's not nice, I admit.

Since tabs are on top by default, I suggest you just put the controls on the tab bar permanently.

The tab bar is not always displayed, when Options > Tabs > "Always show the tab bar" is false (i.e. browser.tabs.autoHide = true) and there is only one tab open. (The patch I posted actually breaks this situation.) I suppose I could do this without using the fullscreen-control-container. I'll post a new patch soon.

No change to browser.xul this time (no defaultset issue), and no CSS hack. This intentionally doesn't have any effect when the above-mentioned pref is set to true (i.e. the window controls stay in the nav bar, because we can't easily predict when the tab bar will be visible).

Comment on attachment 502185 [details] [diff] [review] patch, v. 2 >+ var isTabsOnTop = document.documentElement.getAttribute("tabsontop") == "true"; >+ // when there is a chance the tab bar may be collapsed, put window >+ // controls on nav bar >+ var tabbarMayBeHidden = gPrefService.getBoolPref("browser.tabs.autoHide"); You can simplify this (and the subsequent code): var controlsInTabbar = TabsOnTop.enabled && !gPrefService.getBoolPref("browser.tabs.autoHide");

Thanks once again, Dão, for putting up with a relative newbie to the Mozilla codebase!

Comment on attachment 502207 [details] [diff] [review] patch, v. 3 >+ // In tabs-on-top mode, move window controls to the tab bar, >+ // and in tabs-on-bottom mode, move them back to the navigation toolbar ... >+ // when there is a chance the tab bar may be collapsed, put window >+ // controls on nav bar Please merge these two comments. >+ if (fullscreenctls.parentNode.id == "nav-bar" && ctlsOnTabbar) { >+ fullscreenctls.parentNode.removeChild(fullscreenctls); >+ document.getElementById("TabsToolbar").appendChild(fullscreenctls); >+ // we don't need this space in tabs-on-top mode, so prevent it from >+ // being shown >+ fullscreenflex.setAttribute("fullscreencontrol", "false"); Make this fullscreenflex.removeAttribute("fullscreencontrol"); >+ } >+ else if (fullscreenctls.parentNode.id == "TabsToolbar" && !ctlsOnTabbar) { >+ fullscreenctls.parentNode.removeChild(fullscreenctls); >+ document.getElementById("nav-bar").appendChild(fullscreenctls); >+ fullscreenflex.setAttribute("fullscreencontrol", "true"); The two removeChild calls seem unnecessary. >+#TabsToolbar > #window-controls { >+ padding-left: 4px; This needs to be -moz-margin-start to work in right-to-left locales.

Comment on attachment 502207 [details] [diff] [review] patch, v. 3 r=me with comment 19 addressed. Thanks!

This patch actually applies properly.

I filed follow up bug 625312 with some additional changes

This is not all the way fixed. If someone changes their Persona in full screen it will move the close button back on the url bar. However, this is not a major problem due to the fact that the only way you could change it is by having the tab open already or u typed "about:addons" in the url bar of a new tab.

(In reply to comment #26) > This is not all the way fixed. If someone changes their Persona in full screen > it will move the close button back on the url bar. However, this is not a major > problem due to the fact that the only way you could change it is by having the > tab open already or u typed "about:addons" in the url bar of a new tab. Can you file a new bug for that? Thanks.