240859 - Full Screen Navigation Bar Should Have Auto Hide

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, enhancement)

Description

[Stephan T. Lavavej]

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:

Comment 1

[Stephan T. Lavavej]

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

[José Jeria]

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

[Asa Dotzler [:asa]]

Pretty sure we'd get it from them. 

*** This bug has been marked as a duplicate of 126718 ***

Comment 4

[Michael Ventnor]

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.

Comment 5

[Michael Ventnor]

Attachment: 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.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

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". :)

Comment 7

[Michael Ventnor]

Attachment: 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.

Comment 8

[Michael Ventnor]

Attachment: 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.

Comment 9

[Michael Ventnor]

(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.

Comment 10

[Michael Ventnor]

Attachment: New patch

I would love to get this in for M8, considering how long its been and how much this feature has been requested.

Comment 11

[Michael Ventnor]

Attachment: Updated to trunk

Comment 12

[Michael Ventnor]

Attachment: Updated again

Comment 13

[Michael Ventnor]

Attachment: Updated yet again

Comment 14

[Michael Ventnor]

Attachment: 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.

Comment 15

[Michael Ventnor]

Comment on attachment 287816 [details] [diff] [review]
Updated patch

Er, ui-r not sr...

Comment 16

[timeless]

+    // 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...

Comment 17

[Michael Ventnor]

Attachment: 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!)

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

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. :)

Comment 20

[Michael Ventnor]

(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. :)

Comment 21

[Michael Ventnor]

Attachment: 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.

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 298772 [details] [diff] [review]
String changes 2 (checked in)

a=beltzner for 1.9

Comment 23

[Reed Loden [:reed]]

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

Comment 24

[Michael Ventnor]

Attachment: 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. :)

Comment 25

[Michael Ventnor]

Attachment: Another patch

Updated to accommodate the renamed toolbars.

Comment 26

[Michael Ventnor]

Attachment: Another patch

Update changes and fix possible leak.

Comment 27

[Michael Ventnor]

Attachment: 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.

Comment 28

[Mike Connor [: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.

Comment 29

[Michael Ventnor]

Attachment: Address review

Comment 30

[Michael Ventnor]

Attachment: Fix one last bitrot

Comment 31

[Mike Connor [: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

Comment 32

[Michael Ventnor]

Attachment: 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.

Comment 33

[Michael Ventnor]

Attachment: Address nits 2

Sorry, missed something small.

Comment 34

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 306255 [details] [diff] [review]
Address nits 2

a1.9=beltzner

Comment 35

[Reed Loden [:reed]]

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

Comment 36

[Szabolcs Horvát]

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).

Comment 37

[Dão Gottwald [:dao]]

(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.

Comment 38

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=5308 added to Litmus.

Comment 39

[Alfred Kayser]

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

Comment 40

[Thomas Stache]

(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.