477256 - Implement menubar auto-hiding in toolkit

Status: VERIFIED FIXED

(Toolkit :: Toolbars and Toolbar Customization, enhancement)

Description

[Dão Gottwald [:dao]]

Attachment: patch

This is a Windows Vista quirk that we'll probably want to adopt in Firefox. The menubar is hidden until the Alt key is pressed.

Comment 1

[[e-mail]]

What about people who've customised stuff on to their menubar?

Comment 2

[Dão Gottwald [:dao]]

We can either disable it in that case or let them deal with it (they know how to customize, after all). I think it might actually be useful to place stuff there that you don't need to see all the time. I'm also thinking of adding a hidden(?) browser(?) pref to disable auto-hiding.

Comment 3

[Dão Gottwald [:dao]]

(In reply to comment #2)
> I think it might actually be useful to place stuff
> there that you don't need to see all the time.

This won't work, since DOMMenuBarInactive is dispatched before you can use any other element. So I think we need to disallow this, but in browser code, as an app could place non-interactive elements next to the menubar.

Comment 4

[Dave Garrett]

This is bug 456535.

Comment 5

[Dão Gottwald [:dao]]

Attachment: patch v2

When the attribute is removed during runtime, margin-top needs to be 0 again. Unfortunately, the xbl destructor isn't called in that case, the binding is just detached silently.

Comment 6

[[e-mail]]

(In reply to comment #5)
> When the attribute is removed during runtime, margin-top needs to be 0 again.
> Unfortunately, the xbl destructor isn't called in that case, the binding is
> just detached silently.
Would attachment 278129 [details] [diff] [review] help out here? It shows you how to collapse a menubar without using additional script.
Unfortunately as I don't seem to have been CCd to bug 127244 I didn't notice the confusion over whether it had been checked in or not :-(

Comment 7

[Dão Gottwald [:dao]]

That would likely have helped for that particular case (unhiding the menubar without script). But I'd still have to hide the toolbar (which has a min-height, border, -moz-appearance and potentially more content) without collapsing it. Calculating the height handles all that in a straightforward way, although the setTimeout it uses is also pretty ugly.

Comment 8

[Dão Gottwald [:dao]]

This is with the current patch and autohide="true" on Firefox' main menu bar: <https://build.mozilla.org/tryserver-builds/2009-02-06_15:[e-mail]-try-71541a08d6d/>

Can be tested on Linux and Windows. Note that Alt doesn't activate the menu bar on Linux, you need to press something like Alt+F.

Comment 9

[Philip Chee]

+toolbar[type="menubar"]:not([autohide="true"]),
[....]
+  margin-top: 0 !important;

Hmm. Why is the margin-top set to 0px for non-autohide menubars?

Comment 10

[Dão Gottwald [:dao]]

(In reply to comment #9)
> +toolbar[type="menubar"]:not([autohide="true"]),
> [....]
> +  margin-top: 0 !important;
> 
> Hmm. Why is the margin-top set to 0px for non-autohide menubars?

See comment 5.

Comment 11

[Jim Jeffery not reading bug-mail 1/2/11]

Pressing Alt and clicking on a menu item causes the menu-bar to close and re-open, causing a bounce affect leaving the menu in an odd place, pinned to top of the window and the menu bar closes behind it...

Weird looking behavior and will be very annoying.

Comment 12

[Jim Jeffery not reading bug-mail 1/2/11]

Pressing the Alt key and holding it a second or so.. 
Release the Alt key the menu appears
Press and release the Alt key and the menu-bar jumps up/down (bounces) with each press of the Alt Key

Also, while typing this out and playing with the alt-key I somehow managed to select a menu item - Clear Recent History and was presented with the CRH box.

Comment 13

[Dão Gottwald [:dao]]

Hm... looks like the DOMMenuBarInactive event isn't as great as it seemed. Thanks for testing.

Comment 14

[Dão Gottwald [:dao]]

Attachment: patch v3

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 361047 [details] [diff] [review]
patch v3

This also needs to work if the toolbar isn't at the top. Working an a new patch without margin.

Comment 16

[Dão Gottwald [:dao]]

Attachment: patch v4

https://build.mozilla.org/tryserver-builds/2009-02-07_12:[e-mail]-try-dc2abfdb9f6/

Comment 17

[Dão Gottwald [:dao]]

Attachment: patch v5

Changing the binding during customization caused the changes not to be saved.

Makes more sense this way anyway...

Comment 18

[Jim Jeffery not reading bug-mail 1/2/11]

Pressing Alt and trying to right-click on the Menu-bar to choose 'customize' -
does not show / pop-up the Customize Pallet. 

Sorry if you have already fixed this with the v5 patch from comment #17 , I'm
no good at reading code. 

View-toolbars-customize still works, but that's the long way around.

Comment 19

[Jim Jeffery not reading bug-mail 1/2/11]

Any feeling for how often people use History->Recently Closed tabs list ?

Maybe we need a Nav-bar 'button' to implement that particular feature, or maybe just add a path to show Recent Close tabs to the right-click context menu ?

Comment 20

[Dão Gottwald [:dao]]

Attachment: patch v6

the menubar gets inactive on mousedown, the context menu opens on mouseup
=> huge hack :(

Comment 21

[Dão Gottwald [:dao]]

(In reply to comment #19)
> Any feeling for how often people use History->Recently Closed tabs list ?
> 
> Maybe we need a Nav-bar 'button' to implement that particular feature, or maybe
> just add a path to show Recent Close tabs to the right-click context menu ?

This belongs more to bug 456535.

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 361110 [details] [diff] [review]
patch v6

https://build.mozilla.org/tryserver-builds/2009-02-07_19:[e-mail]-try-0e1250722a0/

Comment 23

[Markus]

I like this new feature, but I think there should be an option to always show the menu bar like with the addon "personal menu"

Comment 24

[Dão Gottwald [:dao]]

(In reply to comment #23)
> I like this new feature, but I think there should be an option to always show
> the menu bar like with the addon "personal menu"

See bug 456535.

Comment 25

[Albert Brand]

Dao, have you looked at the code of the Hide Menubar addon? Perhaps it would have saved you some dev time... https://addons.mozilla.org/en-US/firefox/addon/4762

Comment 26

[Dão Gottwald [:dao]]

Yes, I looked at the code of that extension. It seemed to be too much of a hack.

Comment 27

[[e-mail]]

Comment on attachment 361110 [details] [diff] [review]
patch v6

A moving target is a bit tricky to review ;-)

>+      <field name="_inactiveTimout">null</field>
s/Tim/Time/g :-)

>+      <field name="_contextMenuListener"><![CDATA[({
Personally I wouldn't have created a separate field for this, but I know you browser/toolkit guys like that sort of thing.

>+          this.contextMenu = document.getElementById(contextMenu);
Slightly confusing ;-)

>+          this.toolbar.addEventListener("mousemove", this, false);
I managed to nudge my mouse while I was context menuing (between click and release), so the menubar disappeared. Maybe this should be mouseout? Not sure.

There did seem to be a bit of repainting involved as the menubar temporarily hides while the dialog loads. [This was more obvious to me when I removed the Mac ifdefs so I got to use the customisation sheet. In this case if you don't customise on the menubar then the sheet appears in an incorrect position.]

One thing does worry me: what happens if you customise an item that has its own context menu on to the menubar?

Oh, and by the way, 12 lines had trailing whitespace.

Comment 28

[Dão Gottwald [:dao]]

(In reply to comment #27)
> >+      <field name="_contextMenuListener"><![CDATA[({
> Personally I wouldn't have created a separate field for this, but I know you
> browser/toolkit guys like that sort of thing.

Actually I don't. But for a toolkit binding, it seems better not to implement handleEvent, so that bindings inheriting from it can implement handleEvent without cloning code.
Also in this case I think grouping the context-menu related code improves readability.

> >+          this.toolbar.addEventListener("mousemove", this, false);
> I managed to nudge my mouse while I was context menuing (between click and
> release), so the menubar disappeared. Maybe this should be mouseout? Not sure.

Yeah, this isn't pretty. I think mouseout didn't work while the right mouse button is pressed, and I'm pretty sure it wouldn't work when the mouse leaves the window to the top.

> One thing does worry me: what happens if you customise an item that has its own
> context menu on to the menubar?

I'll test that. I don't think we need to make such context menus work, as there's already the limitation that elements other than the menubar aren't interactive.

> Oh, and by the way, 12 lines had trailing whitespace.

Interesting, I don't see any trailing whitespace in the patch, except for context lines.

Comment 29

[Dão Gottwald [:dao]]

(In reply to comment #28)
> > One thing does worry me: what happens if you customise an item that has its own
> > context menu on to the menubar?
> 
> I'll test that.

What happens is that the context menu opens and, as soon as you move the mouse, the toolbar hides ;-)

Comment 30

[Dão Gottwald [:dao]]

Attachment: patch v6.1

Really the same patch, just with the typo and the var name fixed. I'm unsure what this means:

> There did seem to be a bit of repainting involved as the menubar temporarily
> hides while the dialog loads.

Comment 31

[[e-mail]]

(In reply to comment #28)
> I don't think we need to make such context menus work, as
> there's already the limitation that elements other than the menubar aren't
> interactive.
Ah, but then I fail to see the point of making the menubar customisable...

> Interesting, I don't see any trailing whitespace in the patch, except for
> context lines.
Well git-apply claims otherwise.

Comment 32

[Dão Gottwald [:dao]]

(In reply to comment #31)
> (In reply to comment #28)
> > I don't think we need to make such context menus work, as
> > there's already the limitation that elements other than the menubar aren't
> > interactive.
> Ah, but then I fail to see the point of making the menubar customisable...

Well, toolkit allows it, but it doesn't make menubars customizable by default. That's a decision on the application level that we can't just undo now. I was going to disable auto-hiding when the menubar is customized in Firefox, but given that customized interactive elements won't work for any app, maybe it's better to do this in toolkit?
Ideally, we'd fix the interactivity. I currently don't see how to implement this solidly, so I'd spin it off to another bug.

> > Interesting, I don't see any trailing whitespace in the patch, except for
> > context lines.
> Well git-apply claims otherwise.

Maybe odd line endings? I certainly don't see trailing spaces.

Comment 33

[Dão Gottwald [:dao]]

Attachment: patch v7

auto-hiding disabled for customized menubars

Comment 34

[Dão Gottwald [:dao]]

(In reply to comment #33)
> Created an attachment (id=361616) [details]
> patch v7
> 
> auto-hiding disabled for customized menubars

https://build.mozilla.org/tryserver-builds/2009-02-10_13:[e-mail]-try-abd7334c609/

Comment 35

[[e-mail]]

Comment on attachment 361616 [details] [diff] [review]
patch v7

Well, it seems to work fine, but I'd like a second opinion on a) what the expected behaviour is b) whether the code has become too sprawled.

Comment 36

[Neil Deakin]

Comment on attachment 361616 [details] [diff] [review]
patch v7


>+  </binding>
>+
>+  <binding id="toolbar-menubar-autohide"
>+           extends="chrome://global/content/bindings/toolbar.xml#toolbar">
>+    <content>
>+      <xul:hbox flex="1">
>+        <children/>
>+      </xul:hbox>
>+    </content>

Does it not work without these?

this.contextMenu.addEventListener("popupshowing", this, false);
>+          this.contextMenu.addEventListener("popuphiding", this, false);
>+          this.toolbar.addEventListener("mousemove", this, false);

I don't understand what the mousemove event is detecting. 

Also, what if an event listener prevents the popupshowing event?

>+          function normalize(aSet) {
>+            return aSet.split(",").filter(function (element) {
>+              return element != "separator" &&
>+                     element != "spacer" &&
>+                     element != "spring" &&
>+                     element != "__empty";
>+            }).sort().join(",");
>+          }
>+          if (normalize(aCurrentSet) == normalize(defaultSet)) {
>+            this.removeAttribute("customized");
>+            return;

So somebody with the same set of items but in a different order, or with extra spacers has not customized?

Comment 37

[Dão Gottwald [:dao]]

(In reply to comment #36)
> >+    <content>
> >+      <xul:hbox flex="1">
> >+        <children/>
> >+      </xul:hbox>
> >+    </content>
> 
> Does it not work without these?

Appears to be working without it (thanks to overflow:hidden, I think). I'll attach a new patch.

> I don't understand what the mousemove event is detecting.

I need some way to tell if the mouse pointer was moved away from the toolbar after the mousedown event, as the context menu won't appear then.

> Also, what if an event listener prevents the popupshowing event?

I guess I can't use event.getPreventDefault() reliably and should listen to popupshown instead...

> So somebody with the same set of items but in a different order, or with extra
> spacers has not customized?

Not in a way that's critical for auto-hiding. Maybe a better attribute name is needed...

Comment 38

[Dão Gottwald [:dao]]

Attachment: patch v8

Comment 39

[Dão Gottwald [:dao]]

(In reply to comment #38)
> Created an attachment (id=362062) [details]
> patch v8

https://build.mozilla.org/tryserver-builds/2009-02-12_12:[e-mail]-try-15593183b58/

Comment 40

[Neil Deakin]

> I need some way to tell if the mouse pointer was moved away from the toolbar
> after the mousedown event, as the context menu won't appear then.

So why aren't you using the contextmenu event?

> Not in a way that's critical for auto-hiding. Maybe a better attribute name is
> needed...

What is that code trying to actually accomplish?

Comment 41

[Dão Gottwald [:dao]]

(In reply to comment #40)
> > I need some way to tell if the mouse pointer was moved away from the toolbar
> > after the mousedown event, as the context menu won't appear then.
> 
> So why aren't you using the contextmenu event?

DOMMenuBarInactive is dispatched sometime around mousedown, whereas the context menu opens on mouseup.

> > Not in a way that's critical for auto-hiding. Maybe a better attribute name is
> > needed...
> 
> What is that code trying to actually accomplish?

The fact that the toolbar hides when the menubar becomes inactive interferes with using other elements, such as toolbarbuttons that a user could place on the toolbar. So when the user does that, we don't hide the toolbar.

Comment 42

[Neil Deakin]

Comment on attachment 362062 [details] [diff] [review]
patch v8

OK, I don't really understand it fully, but it does seem to work properly. I don't think this should be checked in though until we have a use for it.

Comment 43

[u88484]

(In reply to comment #42)
> (From update of attachment 362062 [details] [diff] [review])
> don't think this should be checked in though until we have a use for it.

Neil could you please clarify?  What do you mean have a use for it?

Comment 44

[Dão Gottwald [:dao]]

(In reply to comment #43)
> (In reply to comment #42)
> > (From update of attachment 362062 [details] [diff] [review] [details])
> > don't think this should be checked in though until we have a use for it.
> 
> Neil could you please clarify?  What do you mean have a use for it?

bug 456535

Comment 45

[u88484]

(In reply to comment #44)
> (In reply to comment #43)
> > (In reply to comment #42)
> > > (From update of attachment 362062 [details] [diff] [review] [details] [details])
> > > don't think this should be checked in though until we have a use for it.
> > 
> > Neil could you please clarify?  What do you mean have a use for it?
> 
> bug 456535

Oh ok, thanks.  I figured this was what this bug was about.  Anyways, should this bug be blocked by that one then.  Right now it is the other way around which was probably correct to begin with but now this bug is waiting on a patch from that one.

Comment 46

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/8a6c1e5f8c52

Comment 47

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Dao, do you know an application from MS which also makes use of this feature? Just to have a reference.

Comment 48

[Dão Gottwald [:dao]]

IE7 and 8 on XP, and probably a couple more apps on Vista.

Comment 49

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks. Verified fixed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090709 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090709091211

Comment 50

[Michael]

Just a thought - why intentionally hide a major part of the interface unless users know a non-obvious way of getting to it?  They did this to IE, and it greatly reduced the usability of that browser.  Why do the same to Firefox?

Comment 51

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

I don't think this patch actually hides the menu bar by default; it just makes it possible, yes?

Comment 52

[Dão Gottwald [:dao]]

Yes.

Comment 53

[Eric Shepherd [:sheppy]]

Now documented:

https://developer.mozilla.org/XUL:Attribute:autohide

Comment 54

[u88484]

(In reply to comment #53)
> Now documented:
> https://developer.mozilla.org/XUL:Attribute:autohide

Not sure if this is just because of the link here or if this is even a problem but the page that is loaded afte following the link says, "This page redirects to a page that no longer exists en/XUL/Attribute/autohide."

Comment 55

[Dão Gottwald [:dao]]

https://developer.mozilla.org/en/XUL/menubar#a-autohide is wrong. It's not <menubar autohide="true"/> but <toolbar type="menubar" autohide="true"/>.

Comment 56

[Eric Shepherd [:sheppy]]

https://developer.mozilla.org/en/XUL:Attribute:autohide

I posted the link wrong.

Dão - where do you see that? There's no sample anywhere, so I'm not sure what you're referring to.

Comment 57

[Dão Gottwald [:dao]]

The whole section on autohide is wrong... that page is about menubar elements, not toolbar elements.

Comment 58

[Eric Shepherd [:sheppy]]

This bug says it's about menubar auto-hiding. So now I'm very confused.

Comment 59

[Dão Gottwald [:dao]]

The actual implementation requires setting autohide="true" on <toolbar type="menubar"/> elements. The menubar element is a child of such a toolbar.

Comment 60

[Eric Shepherd [:sheppy]]

Moved this to the toolbar doc:

https://developer.mozilla.org/en/XUL/toolbar

Comment 61

[Dietrich Ayala (:dietrich)]

(In reply to comment #60)
> Moved this to the toolbar doc:
> 
> https://developer.mozilla.org/en/XUL/toolbar

The link there forwarded me to the MDC front page. Also, the other attributes have examples inline and this one is just a link to a different page, which looks odd.

Comment 62

[Eric Shepherd [:sheppy]]

This was a broken use of a template; fixed now.

Comment 63

[Dietrich Ayala (:dietrich)]

Thanks!