Dão Gottwald [:dao] Assignee
	Description • 15 years ago

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.

	neil@parkwaycc.co.uk
	Comment 1 • 15 years ago

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

	Dão Gottwald [:dao] Assignee
	Comment 2 • 15 years ago

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.

	Dão Gottwald [:dao] Assignee
	Comment 3 • 15 years ago

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

	Dave Garrett
	Comment 4 • 15 years ago

This is bug 456535.

	Dão Gottwald [:dao] Assignee
	Comment 5 • 15 years ago

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.

	neil@parkwaycc.co.uk
	Comment 6 • 15 years ago

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

	Dão Gottwald [:dao] Assignee
	Comment 7 • 15 years ago

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.

	Dão Gottwald [:dao] Assignee
	Comment 8 • 15 years ago

This is with the current patch and autohide="true" on Firefox' main menu bar: <https://build.mozilla.org/tryserver-builds/2009-02-06_15:26-dgottwald@mozilla.com-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.

	Philip Chee
	Comment 9 • 15 years ago

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

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

	Dão Gottwald [:dao] Assignee
	Comment 10 • 15 years ago

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

	Jim Jeffery not reading bug-mail 1/2/11
	Comment 11 • 15 years ago

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.

	Jim Jeffery not reading bug-mail 1/2/11
	Comment 12 • 15 years ago

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.

	Dão Gottwald [:dao] Assignee
	Comment 13 • 15 years ago

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

	Dão Gottwald [:dao] Assignee
	Comment 15 • 15 years ago

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.

	Dão Gottwald [:dao] Assignee
	Comment 16 • 15 years ago

https://build.mozilla.org/tryserver-builds/2009-02-07_12:19-dgottwald@mozilla.com-try-dc2abfdb9f6/

	Dão Gottwald [:dao] Assignee
	Comment 17 • 15 years ago

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

Makes more sense this way anyway...

	Jim Jeffery not reading bug-mail 1/2/11
	Comment 18 • 15 years ago

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.

	Jim Jeffery not reading bug-mail 1/2/11
	Comment 19 • 15 years ago

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 ?

	Dão Gottwald [:dao] Assignee
	Comment 20 • 15 years ago

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

	Dão Gottwald [:dao] Assignee
	Comment 21 • 15 years ago

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

	Dão Gottwald [:dao] Assignee
	Comment 22 • 15 years ago

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

https://build.mozilla.org/tryserver-builds/2009-02-07_19:44-dgottwald@mozilla.com-try-0e1250722a0/

	Markus
	Comment 23 • 15 years ago

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"

	Dão Gottwald [:dao] Assignee
	Comment 24 • 15 years ago

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

	Albert Brand
	Comment 25 • 15 years ago

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

	Dão Gottwald [:dao] Assignee
	Comment 26 • 15 years ago

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

	neil@parkwaycc.co.uk
	Comment 27 • 15 years ago

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.

	Dão Gottwald [:dao] Assignee
	Comment 28 • 15 years ago

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

	Dão Gottwald [:dao] Assignee
	Comment 29 • 15 years ago

(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 ;-)

	Dão Gottwald [:dao] Assignee
	Comment 30 • 15 years ago

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.

	neil@parkwaycc.co.uk
	Comment 31 • 15 years ago

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

	Dão Gottwald [:dao] Assignee
	Comment 32 • 15 years ago

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

	Dão Gottwald [:dao] Assignee
	Comment 33 • 15 years ago

auto-hiding disabled for customized menubars

	Dão Gottwald [:dao] Assignee
	Comment 34 • 15 years ago

(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:42-dgottwald@mozilla.com-try-abd7334c609/

	neil@parkwaycc.co.uk
	Comment 35 • 15 years ago

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.

	Neil Deakin
	Comment 36 • 15 years ago

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?

	Dão Gottwald [:dao] Assignee
	Comment 37 • 15 years ago

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

	Dão Gottwald [:dao] Assignee
	Comment 39 • 15 years ago

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

https://build.mozilla.org/tryserver-builds/2009-02-12_12:01-dgottwald@mozilla.com-try-15593183b58/

	Neil Deakin
	Comment 40 • 15 years ago

> 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?

	Dão Gottwald [:dao] Assignee
	Comment 41 • 15 years ago

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

	Neil Deakin
	Comment 42 • 15 years ago

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.

	u88484
	Comment 43 • 15 years ago

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

	Dão Gottwald [:dao] Assignee
	Comment 44 • 15 years ago

(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

	u88484
	Comment 45 • 15 years ago

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

	Dão Gottwald [:dao] Assignee
	Comment 46 • 15 years ago

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

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 47 • 15 years ago

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

	Dão Gottwald [:dao] Assignee
	Comment 48 • 15 years ago

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

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 49 • 15 years ago

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

	Michael
	Comment 50 • 15 years ago

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?

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 51 • 15 years ago

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

	Dão Gottwald [:dao] Assignee
	Comment 52 • 15 years ago

Yes.

	Eric Shepherd [:sheppy]
	Comment 53 • 15 years ago

Now documented:

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

	u88484
	Comment 54 • 15 years ago

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

	Dão Gottwald [:dao] Assignee
	Comment 55 • 15 years ago

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

	Eric Shepherd [:sheppy]
	Comment 56 • 15 years ago

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.

	Dão Gottwald [:dao] Assignee
	Comment 57 • 15 years ago

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

	Eric Shepherd [:sheppy]
	Comment 58 • 15 years ago

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

	Dão Gottwald [:dao] Assignee
	Comment 59 • 15 years ago

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

	Eric Shepherd [:sheppy]
	Comment 60 • 15 years ago

Moved this to the toolbar doc:

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

	Dietrich Ayala (:dietrich)
	Comment 61 • 14 years ago

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

	Eric Shepherd [:sheppy]
	Comment 62 • 14 years ago

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

	Dietrich Ayala (:dietrich)
	Comment 63 • 14 years ago

Thanks!