In Fullscreen mode, move caption buttons to top

RESOLVED FIXED in Firefox 4.0b10

Status

()

defect
--
minor
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: imradyurrad, Assigned: at.light)

Tracking

(Blocks 1 bug, {polish})

unspecified
Firefox 4.0b10
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Reporter

Description

9 years ago
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
Reporter

Comment 1

9 years ago
Posted image clipboard01
Reporter

Updated

9 years ago
Severity: normal → minor
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?

Comment 3

9 years ago
I can reproduce this Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; en-US; rv:2.0b2pre) Gecko/20100710 Minefield/4.0b2pre
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Ever confirmed: true
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.
blocking2.0: ? → -
Keywords: polish
Duplicate of this bug: 580845

Updated

9 years ago
Duplicate of this bug: 591406
Component: General → Toolbars
QA Contact: general → toolbars
Duplicate of this bug: 598255

Updated

9 years ago
Duplicate of this bug: 597611

Updated

9 years ago
Duplicate of this bug: 600831

Updated

9 years ago
Blocks: 544820
Assignee

Comment 11

9 years ago
Posted patch patch (obsolete) — Splinter Review
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?
Assignee: nobody → at.light
Attachment #499641 - Flags: review?(dietrich)
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?
Attachment #499641 - Flags: review?(dietrich) → review-
Assignee

Comment 13

9 years ago
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.
Assignee

Comment 15

9 years ago
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.
Assignee

Comment 16

9 years ago
Posted patch patch, v. 2 (obsolete) — Splinter Review
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).
Attachment #499641 - Attachment is obsolete: true
Attachment #502185 - Flags: review?(dao)
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");
Assignee

Comment 18

9 years ago
Posted patch patch, v. 3 (obsolete) — Splinter Review
Thanks once again, Dão, for putting up with a relative newbie to the Mozilla codebase!
Attachment #502185 - Attachment is obsolete: true
Attachment #502207 - Flags: review?(dao)
Attachment #502185 - Flags: review?(dao)
Blocks: 620064
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!
Attachment #502207 - Flags: review?(dao) → review+
Assignee

Comment 21

9 years ago
Posted patch patch, v. 4 (obsolete) — Splinter Review
Attachment #502207 - Attachment is obsolete: true
Attachment #502662 - Flags: review+
Assignee

Comment 22

9 years ago
Posted patch patch, v. 4aSplinter Review
This patch actually applies properly.
Attachment #502662 - Attachment is obsolete: true
Attachment #502673 - Flags: review+
Assignee

Updated

9 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/4413ed6ba5a5
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b10
Depends on: 624685
Duplicate of this bug: 620064
I filed follow up bug 625312 with some additional changes

Comment 26

9 years ago
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.

Updated

8 years ago
Depends on: 637049
You need to log in before you can comment on or make changes to this bug.