Full Screen Navigation Bar Should Have Auto Hide

RESOLVED FIXED in Firefox 3 beta5

Status

()

Firefox
Toolbars and Customization
--
enhancement
RESOLVED FIXED
14 years ago
7 years ago

People

(Reporter: Stephan T. Lavavej, Assigned: Michael Ventnor)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 3 beta5
Points:
---
Dependency tree / graph
Bug Flags:
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 15 obsolete attachments)

49.03 KB, image/png
Details
1.18 KB, patch
beltzner
: approval1.9+
Details | Diff | Splinter Review
17.75 KB, patch
beltzner
: approval1.9+
Details | Diff | Splinter Review
(Reporter)

Description

14 years ago
User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; Q312461; .NET CLR 1.0.3705; .NET CLR 1.1.4322)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6)

This is an enhancement request: The full screen navigation bar should have an 
auto hide feature, as IE's does.

Reproducible: Always
Steps to Reproduce:
(Reporter)

Comment 1

14 years ago
This is the complete Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 
5.1; en-US; rv:1.6) Gecko/20040206 Firefox/0.8

Comment 2

14 years ago
Bug 126718 is the equivalent mozilla suite bug for this. Dont know if this is a
dupe of that one or if it should be firefox specific.

Comment 3

14 years ago
Pretty sure we'd get it from them. 

*** This bug has been marked as a duplicate of 126718 ***
Status: UNCONFIRMED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → DUPLICATE

Updated

14 years ago
Status: RESOLVED → VERIFIED

Updated

11 years ago
QA Contact: bugzilla → toolbars
(Assignee)

Comment 4

11 years ago
Since this bug is in the Firefox component, and bug 126718 is in the Core component, I'm reopening this bug.

I have a patch for this, but it only modifies /browser code and is therefore Firefox-specific. So I need a Firefox bug to put it in.
Status: VERIFIED → UNCONFIRMED
Resolution: DUPLICATE → ---
Assignee: bugs → nobody
(Assignee)

Comment 5

11 years ago
Created attachment 261573 [details] [diff] [review]
Patch

Theres a lot of mess in this patch so I'll try and explain what everything does.

On fullscreen, it will hide the navigation bar and tab strip, and put an extremely tiny bar at the top of the screen. It's mainly to handle the mouseover and dragenter events, but it also provides feedback to the user that there is something of interest there.

The chrome will hide again when the mouse goes back into content, UNLESS either:
a) A popup menu is open
b) A textbox in chrome has focus (eg. location bar). Originally I was going to make the chrome collapse anyway and take focus away from the textbox, because it made hiding the chrome easier, but then I took the road of IE7 parity. I'll see what Beltzner says.
However, this is overridden when the user presses Esc, in order to make it easier for people without mice to hide chrome, or for anyone to hide chrome quickly and easily.

All keyboard shortcuts to location bar and search bar will trigger chrome.
Assignee: nobody → ventnor.bugzilla
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #261573 - Flags: ui-review?(beltzner)
Comment on attachment 261573 [details] [diff] [review]
Patch

The only things I'm concerned with are:

 1. ensuring the user knows how to get chrome back
 2. ensuring the user knows how to get out of fullscreen mode
 3. making it possible to lock the chrome in place

To solve 1, I'd suggest that the order of drawing operations be architected such that the chrome is originally drawn and then hidden (unless the mouse focus is in the area that brings it back), hopefully sliding gracefully away much like the Windows Task Bar auto-hide does things.

Solving point 2 means, I think, having both an obvious control on the toolbar and also supporting actions like ESC or having a right-click entry (I'm embarrassed to say that I haven't got a Windows build in front of me atm, and can't remember if we do this already)

The final point is currently serviced by the pref, I guess, but we might want to have an "Auto-hide" context menu item added when hovering over the toolbar.

If those are addressed by the current patch, send me a ui-r? again and I'll approve. I'd also love to see a screenshot of the "tiny bar". :)
Attachment #261573 - Flags: ui-review?(beltzner) → ui-review-
(Assignee)

Comment 7

10 years ago
Created attachment 264661 [details]
Screenshots

Two screenshots; the one on top shows the tiny bar. I put it there so users would have a clue of where to look should they want the chrome back, but mostly to handle the events without even more spaghetti code. I could get rid of it but it would require more code (the last thing we need), and I'd need to sacrifice the ability to turn chrome on when the user drags something to the top of the screen (because of the way things are).

Underneath that is a shot of the context menu on the main FS toolbar. I added a separator because I don't want the user accidentally exiting full-screen when they just wanted to lock the toolbars. We already have two avenues of exiting FS mode (F11 and the middle button on the window controls) but per review request I added that option to the context menu.
(Assignee)

Comment 8

10 years ago
Created attachment 264662 [details] [diff] [review]
Patch 2

(In reply to comment #6)
> (From update of attachment 261573 [details] [diff] [review])
> The only things I'm concerned with are:
> 
>  1. ensuring the user knows how to get chrome back
>  2. ensuring the user knows how to get out of fullscreen mode
>  3. making it possible to lock the chrome in place
> 
> To solve 1, I'd suggest that the order of drawing operations be architected
> such that the chrome is originally drawn and then hidden (unless the mouse
> focus is in the area that brings it back), hopefully sliding gracefully away
> much like the Windows Task Bar auto-hide does things.

Done. I decided to make this so that animation would only happen once each time you enter FS mode (after a brief delay keeping the chrome on-screen). After that the chrome will toggle instantaneously. My reasoning is that the animation's purpose is to show the user where the chrome is going. After doing so, the user knows where to go, so there is no need for more distracting CPU-spiking animation. It looks nice though. And it decelerates :)

> Solving point 2 means, I think, having both an obvious control on the toolbar
> and also supporting actions like ESC or having a right-click entry (I'm
> embarrassed to say that I haven't got a Windows build in front of me atm, and
> can't remember if we do this already)

We do, the middle button of the window controls on the toolbar exit FS mode, as does F11 (standard FS key). But, I added an option to the new context menu (see screenshots).

> The final point is currently serviced by the pref, I guess, but we might want
> to have an "Auto-hide" context menu item added when hovering over the toolbar.

Done, through the new context menu.
Attachment #261573 - Attachment is obsolete: true
Attachment #264662 - Flags: ui-review?(beltzner)
(Assignee)

Comment 9

10 years ago
(In reply to comment #8)
> After doing so, the user knows where to go, so there is no need for more distracting
> CPU-spiking animation. 

OK, I lied a bit back then. It doesn't spike my CPU, but I do find it unnecessary every time the toolbar needs to go away.

Beltzner, when will you be able to ui-r this? Its been a month since your last comments.
(Assignee)

Comment 10

10 years ago
Created attachment 277831 [details] [diff] [review]
New patch

I would love to get this in for M8, considering how long its been and how much this feature has been requested.
Attachment #264662 - Attachment is obsolete: true
Attachment #277831 - Flags: ui-review?(mconnor)
Attachment #264662 - Flags: ui-review?(beltzner)
(Assignee)

Comment 11

10 years ago
Created attachment 278849 [details] [diff] [review]
Updated to trunk
Attachment #277831 - Attachment is obsolete: true
Attachment #278849 - Flags: ui-review?(mconnor)
Attachment #277831 - Flags: ui-review?(mconnor)
(Assignee)

Comment 12

10 years ago
Created attachment 279425 [details] [diff] [review]
Updated again
Attachment #278849 - Attachment is obsolete: true
Attachment #279425 - Flags: ui-review?(mconnor)
Attachment #279425 - Flags: review?(mconnor)
Attachment #278849 - Flags: ui-review?(mconnor)
(Assignee)

Comment 13

10 years ago
Created attachment 279644 [details] [diff] [review]
Updated yet again
Attachment #279425 - Attachment is obsolete: true
Attachment #279644 - Flags: ui-review?(mconnor)
Attachment #279644 - Flags: review?(mconnor)
Attachment #279425 - Flags: ui-review?(mconnor)
Attachment #279425 - Flags: review?(mconnor)
Flags: in-litmus?
(Assignee)

Comment 14

10 years ago
Created attachment 287816 [details] [diff] [review]
Updated patch

This functionality was listed during the FLOSS usability sprint: http://wiki.mozilla.org/FLOSSUsabilitySprint1107/IE7

*gets on knees* Please, please accept this for Beta 2. This bug has been sitting waiting for review for ages. It is very desirable functionality and doesn't significantly change the chrome to do so. It has been tested through the backside and I haven't had any problems with it so far.
Attachment #279644 - Attachment is obsolete: true
Attachment #287816 - Flags: superreview?(mconnor)
Attachment #287816 - Flags: review?(mconnor)
Attachment #279644 - Flags: ui-review?(mconnor)
Attachment #279644 - Flags: review?(mconnor)
(Assignee)

Comment 15

10 years ago
Comment on attachment 287816 [details] [diff] [review]
Updated patch

Er, ui-r not sr...
Attachment #287816 - Flags: superreview?(mconnor) → ui-review?(mconnor)

Comment 16

10 years ago
+    // F6 is another shortcut to the address bar, but its not covered in OpenLocation()

it would be "it's", but you should write "it isn't" in cases where you try to misspell "its not".

this is an else before return, which is just as bad (in my book) as an else after return:
+      if (forceHide)
+        // hidden textboxes that still have focus are bad bad bad
+        document.commandDispatcher.focusedElement.blur();
+      else
+        return false;

if (!forceHide)
  return false;

// hidden ...
...

these are drive by comments, i don't expect to comment again on this bug. cool feature though. i suppose i might comment if i test the feature :) -- I used the IE version for years...
(Assignee)

Comment 17

10 years ago
Created attachment 298766 [details] [diff] [review]
String changes only

These are just to get the strings in before the freeze. I really wish mconnor would find some time to do this as the patch has been sitting here for ages and is an oft-requested feature (the extension that provides it gets huge publicity in mainstream press!)
Attachment #298766 - Flags: ui-review?(beltzner)
Attachment #298766 - Flags: review?(beltzner)
Comment on attachment 287816 [details] [diff] [review]
Updated patch

Tiny bar looks great, thanks for doing the animation. We'll see what people say - I think it might be nice to always have it, but that's probably an easy switch later if we want to make it.
Attachment #287816 - Flags: ui-review?(mconnor) → ui-review+
Comment on attachment 298766 [details] [diff] [review]
String changes only

nit: "Hide toolbars" instead of "Auto-hide the toolbars" is a little easier to parse - I know the first was my suggestion. :)
Attachment #298766 - Flags: ui-review?(beltzner) → ui-review+
(Assignee)

Comment 20

10 years ago
(In reply to comment #18)
> (From update of attachment 287816 [details] [diff] [review])
> Tiny bar looks great, thanks for doing the animation. We'll see what people say
> - I think it might be nice to always have it, but that's probably an easy
> switch later if we want to make it.
> 

Its a very easy switch, just change the animateUp pref to 2. But this patch was written when I still had my very old computer (puts it into perspective how long this patch has awaited review ;) ) so I want to sympathize with those who this animation might chug with.

As I said, the purpose of the animation is to know where the toolbars go. When it happens once, the user knows where they are so it doesn't come as a real shock when they instantly disappear yet still notice the tiny bar that they recognize from when the animation did happen. Perf and all that, y'know. :)
(Assignee)

Comment 21

10 years ago
Created attachment 298772 [details] [diff] [review]
String changes 2 (checked in)

String nit fixed. This will only add strings (not change any) to enable them to be used by the real patch later if it gets reviewed before next Christmas.
Attachment #298766 - Attachment is obsolete: true
Attachment #298772 - Flags: approval1.9?
Attachment #298766 - Flags: review?(beltzner)
Comment on attachment 298772 [details] [diff] [review]
String changes 2 (checked in)

a=beltzner for 1.9
Attachment #298772 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Strings landed.

Checking in browser/locales/en-US/chrome/browser/browser.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/browser.dtd,v  <--  browser.dtd
new revision: 1.89; previous revision: 1.88
done
Keywords: checkin-needed
OS: Windows XP → All
Hardware: PC → All
Version: unspecified → Trunk
(Assignee)

Comment 24

10 years ago
Created attachment 300207 [details] [diff] [review]
Really updated patch

OK mconnor, you promised you'd review this tomorrow ;)

Changes:
* It is a lot more tolerant to popups now. Previous patch was written before Larry or the Awesomebar, so I had to tweak the code's perspective on popups.
* The blank pseudo-toolbar that takes events (the "fullscreen toggler") is now added dynamically instead of put in browser.xul, this should mean a far, far smaller Txul hit.
* Unbitrotted.

Since I'm now in this office of Mac Monoculturalism, I can show you the patch first-hand. :)
Attachment #287816 - Attachment is obsolete: true
Attachment #300207 - Flags: review?(mconnor)
Attachment #287816 - Flags: review?(mconnor)
(Assignee)

Updated

10 years ago
Attachment #298772 - Attachment description: String changes 2 → String changes 2 (checked in)
(Assignee)

Comment 25

10 years ago
Created attachment 300411 [details] [diff] [review]
Another patch

Updated to accommodate the renamed toolbars.
Attachment #300207 - Attachment is obsolete: true
Attachment #300411 - Flags: review?(mconnor)
Attachment #300207 - Flags: review?(mconnor)
(Assignee)

Comment 26

10 years ago
Created attachment 300801 [details] [diff] [review]
Another patch

Update changes and fix possible leak.
Attachment #300411 - Attachment is obsolete: true
Attachment #300801 - Flags: review?(mconnor)
Attachment #300411 - Flags: review?(mconnor)
(Assignee)

Comment 27

10 years ago
Created attachment 301613 [details] [diff] [review]
Another patch because the browser folks can't make up their minds on IDs

If you're still too busy to do this, mconnor, I suppose I can ask gavin.
Attachment #300801 - Attachment is obsolete: true
Attachment #301613 - Flags: review?(mconnor)
Attachment #300801 - Flags: review?(mconnor)
Comment on attachment 301613 [details] [diff] [review]
Another patch because the browser folks can't make up their minds on IDs


> 
> function BrowserTryToCloseWindow()
> {
>-  if (WindowIsClosing())
>+  if (WindowIsClosing()) {
>+    if (window.fullScreen) {
>+      gBrowser.mPanelContainer
>+        .removeEventListener("mousemove", FullScreen._collapseCallback, false);

this alignment is kinda broken, if you need to linebreak the args, do that, don't just align oddly

>+#ifdef XP_MACOSX
>+// XXX Change me when fullscreen is working on OSX
>+pref("browser.fullscreen.autohide", false);
>+#else
>+pref("browser.fullscreen.autohide", true);
>+#endif

er, we have autohide totally disabled on Mac, why do this?

general comments given over IRC:

using mousemove is going to be spammy, remove the listener once collapse happens
don't cache/observe autohide pref, just check it when needed, only add the collapse listeners when autohide is true and we're showing the toolbar.
Attachment #301613 - Flags: review?(mconnor) → review-
(Assignee)

Comment 29

10 years ago
Created attachment 305090 [details] [diff] [review]
Address review
Attachment #301613 - Attachment is obsolete: true
Attachment #305090 - Flags: review?(mconnor)
(Assignee)

Comment 30

10 years ago
Created attachment 305130 [details] [diff] [review]
Fix one last bitrot
Attachment #305090 - Attachment is obsolete: true
Attachment #305130 - Flags: review?(mconnor)
Attachment #305090 - Flags: review?(mconnor)
Comment on attachment 305130 [details] [diff] [review]
Fix one last bitrot

last nit:

}
else {

not 

} else {

I know there's a mix of styles, but the former is preferred
Attachment #305130 - Flags: review?(mconnor) → review+
(Assignee)

Comment 32

10 years ago
Created attachment 306197 [details] [diff] [review]
Address nits

Asking for spproval after B4 freeze.

The basic principles and logic of this patch haven't changed since it was first made in April(!), so it has undergone a hell of a lot of testing and refining both for bugs (of which I can no longer find any) and user experience. This patch was designed for 1.9 to be a great companion for another fullscreen bug I fixed (bug 127013) to make it easy for web apps to detect whether the user is in fullscreen (great for Powerpoint-like apps!) and to provide a "full" full screen out of the box.

Based on the amount of attention Fullerscreen has gotten, this patch is definitely wanted by a large proportion of users. The risk is very minimal as this is entirely implemented in javascript & CSS, had a lot of testing, and is designed to cater for as many edge cases as I can fathom.
Attachment #305130 - Attachment is obsolete: true
Attachment #306197 - Flags: approval1.9?
(Assignee)

Comment 33

10 years ago
Created attachment 306255 [details] [diff] [review]
Address nits 2

Sorry, missed something small.
Attachment #306197 - Attachment is obsolete: true
Attachment #306255 - Flags: approval1.9?
Attachment #306197 - Flags: approval1.9?
Comment on attachment 306255 [details] [diff] [review]
Address nits 2

a1.9=beltzner
Attachment #306255 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Checking in browser/base/content/browser.css;
/cvsroot/mozilla/browser/base/content/browser.css,v  <--  browser.css
new revision: 1.60; previous revision: 1.59
done
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.996; previous revision: 1.995
done
Checking in browser/base/content/browser.xul;
/cvsroot/mozilla/browser/base/content/browser.xul,v  <--  browser.xul
new revision: 1.444; previous revision: 1.443
done
Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.305; previous revision: 1.304
done
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 beta5

Comment 36

10 years ago
This is great!

But usability could be enhanced a little more if the toolbar+tabbar would not be hidden immediately when the mouse leaves it.  The mouse could be allowed to move about 50 pixels below the tab bar before the tab bar is hidden.  IE7 does this too.

It is easy to "throw" the mouse cursor to the top edge of the screen in one quick move, and show the tab bar.  (This is good.)  But to switch tabs, one has to move the cursor down immediately, and if this is done too quickly, one might easily overshoot.  (I do it all the time and it is a bit frustrating.)  The tab bar is quite narrow and it takes some effort to position the mouse exactly over it (and never go below it), especially with today's standard "accelerating" mouse cursors.

So, to summarize, the tabbar+toolbar should only appear when the mouse is exactly over the thin grey bar (and not any sooner).  But it should not disappear until the distance of the bottom of the tab bar and the cursor becomes greater than some value (e.g. 50 pixels).

This is just an opinion :-) (but it's based on practical experience).


(In reply to comment #20)
> (In reply to comment #18)
> > (From update of attachment 287816 [details] [diff] [review] [details])
> > Tiny bar looks great, thanks for doing the animation. We'll see what people say
> > - I think it might be nice to always have it, but that's probably an easy
> > switch later if we want to make it.
> > 
> 
> Its a very easy switch, just change the animateUp pref to 2. But this patch was
> written when I still had my very old computer (puts it into perspective how
> long this patch has awaited review ;) ) so I want to sympathize with those who
> this animation might chug with.

You might want to do something similar to bug 430925 and bug 355965.
https://litmus.mozilla.org/show_test.cgi?id=5308 added to Litmus.
Flags: in-litmus? → in-litmus+

Updated

9 years ago
Depends on: 435666

Updated

9 years ago
Depends on: 439774

Updated

9 years ago
Depends on: 450859

Updated

9 years ago
Blocks: 126718
Depends on: 417812

Updated

8 years ago
Depends on: 507586

Updated

8 years ago
Depends on: 535767

Updated

7 years ago
Depends on: 490621

Updated

7 years ago
No longer depends on: 435666

Updated

7 years ago
No longer depends on: 535767

Updated

7 years ago
Depends on: 500991

Updated

7 years ago
No longer depends on: 500991

Updated

7 years ago
Depends on: 430687

Comment 39

7 years ago
The animation should be replaced with the new css transition, reducing the custom animation code for this fullscreen autohide function.

Updated

7 years ago
Blocks: 589114

Comment 40

7 years ago
(In reply to comment #39)
> The animation should be replaced with the new css transition, reducing the
> custom animation code for this fullscreen autohide function.

Filed bug 589114 to track that suggestion.
You need to log in before you can comment on or make changes to this bug.