Implement menubar auto-hiding in toolkit

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Toolkit
Toolbars and Toolbar Customization
--
enhancement
VERIFIED FIXED
9 years ago
6 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

(Depends on: 5 bugs, {dev-doc-complete})

Trunk
mozilla1.9.2a1
x86
Windows Vista
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 360918 [details] [diff] [review]
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.
Attachment #360918 - Flags: review?(neil)

Comment 1

9 years ago
What about people who've customised stuff on to their menubar?
(Assignee)

Comment 2

9 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.
(Assignee)

Comment 3

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

Comment 4

9 years ago
This is bug 456535.
(Assignee)

Updated

9 years ago
Blocks: 456535
(Assignee)

Comment 5

9 years ago
Created attachment 360980 [details] [diff] [review]
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.
Attachment #360918 - Attachment is obsolete: true
Attachment #360980 - Flags: review?(neil)
Attachment #360918 - Flags: review?(neil)

Comment 6

9 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 :-(
(Assignee)

Comment 7

9 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.
(Assignee)

Comment 8

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

Comment 9

9 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?
(Assignee)

Comment 10

9 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.
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.
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.
(Assignee)

Comment 13

9 years ago
Hm... looks like the DOMMenuBarInactive event isn't as great as it seemed. Thanks for testing.
(Assignee)

Comment 14

9 years ago
Created attachment 361047 [details] [diff] [review]
patch v3
Attachment #360980 - Attachment is obsolete: true
Attachment #361047 - Flags: review?(neil)
Attachment #360980 - Flags: review?(neil)
(Assignee)

Comment 15

9 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.
Attachment #361047 - Attachment is obsolete: true
Attachment #361047 - Flags: review?(neil)
(Assignee)

Comment 16

9 years ago
Created attachment 361101 [details] [diff] [review]
patch v4

https://build.mozilla.org/tryserver-builds/2009-02-07_12:19-dgottwald@mozilla.com-try-dc2abfdb9f6/
Attachment #361101 - Flags: review?(neil)
(Assignee)

Comment 17

9 years ago
Created attachment 361104 [details] [diff] [review]
patch v5

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

Makes more sense this way anyway...
Attachment #361101 - Attachment is obsolete: true
Attachment #361104 - Flags: review?(neil)
Attachment #361101 - Flags: review?(neil)
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.
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 ?
(Assignee)

Comment 20

9 years ago
Created attachment 361110 [details] [diff] [review]
patch v6

the menubar gets inactive on mousedown, the context menu opens on mouseup
=> huge hack :(
Attachment #361104 - Attachment is obsolete: true
Attachment #361110 - Flags: review?(neil)
Attachment #361104 - Flags: review?(neil)
(Assignee)

Comment 21

9 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.
(Assignee)

Updated

9 years ago
Severity: normal → enhancement
(Assignee)

Comment 22

9 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/

Comment 23

9 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"
(Assignee)

Comment 24

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

Comment 25

9 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
(Assignee)

Comment 26

9 years ago
Yes, I looked at the code of that extension. It seemed to be too much of a hack.
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.
(Assignee)

Comment 28

9 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.
(Assignee)

Comment 29

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

Comment 30

9 years ago
Created attachment 361372 [details] [diff] [review]
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.
Attachment #361110 - Attachment is obsolete: true
Attachment #361372 - Flags: review?(neil)
Attachment #361110 - Flags: review?(neil)
(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.
(Assignee)

Comment 32

9 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.
(Assignee)

Comment 33

9 years ago
Created attachment 361616 [details] [diff] [review]
patch v7

auto-hiding disabled for customized menubars
Attachment #361372 - Attachment is obsolete: true
Attachment #361616 - Flags: review?(neil)
Attachment #361372 - Flags: review?(neil)
(Assignee)

Comment 34

9 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/

Updated

9 years ago
Attachment #361616 - Flags: review?(neil)
Attachment #361616 - Flags: review?(gavin.sharp)
Attachment #361616 - Flags: review+
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.
(Assignee)

Updated

9 years ago
Attachment #361616 - Flags: review?(gavin.sharp) → review?(enndeakin)
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?
(Assignee)

Comment 37

9 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...
(Assignee)

Comment 38

9 years ago
Created attachment 362062 [details] [diff] [review]
patch v8
Attachment #361616 - Attachment is obsolete: true
Attachment #361616 - Flags: review?(enndeakin)
(Assignee)

Updated

9 years ago
Attachment #362062 - Flags: review?(enndeakin)
(Assignee)

Comment 39

9 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/
> 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?
(Assignee)

Comment 41

9 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.
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.
Attachment #362062 - Flags: review?(enndeakin) → review+

Comment 43

9 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?
(Assignee)

Comment 44

9 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

Comment 45

9 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.
(Assignee)

Comment 46

8 years ago
http://hg.mozilla.org/mozilla-central/rev/8a6c1e5f8c52
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
(Assignee)

Updated

8 years ago
Keywords: dev-doc-needed
Summary: Implement menubar auto-hiding → Implement menubar auto-hiding in toolkit
Depends on: 498102

Updated

8 years ago
Depends on: 498100

Updated

8 years ago
Depends on: 498108

Updated

8 years ago
Depends on: 498112
Depends on: 498176
(Assignee)

Updated

8 years ago
Depends on: 498628
(Assignee)

Updated

8 years ago
Depends on: 499298

Updated

8 years ago
Depends on: 501950
Dao, do you know an application from MS which also makes use of this feature? Just to have a reference.
(Assignee)

Comment 48

8 years ago
IE7 and 8 on XP, and probably a couple more apps on Vista.
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
Status: RESOLVED → VERIFIED

Comment 50

8 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?
I don't think this patch actually hides the menu bar by default; it just makes it possible, yes?
(Assignee)

Comment 52

8 years ago
Yes.
Now documented:

https://developer.mozilla.org/XUL:Attribute:autohide
Keywords: dev-doc-needed → dev-doc-complete

Comment 54

8 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."
(Assignee)

Comment 55

8 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"/>.
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.
(Assignee)

Comment 57

8 years ago
The whole section on autohide is wrong... that page is about menubar elements, not toolbar elements.
This bug says it's about menubar auto-hiding. So now I'm very confused.
(Assignee)

Comment 59

8 years ago
The actual implementation requires setting autohide="true" on <toolbar type="menubar"/> elements. The menubar element is a child of such a toolbar.
Moved this to the toolbar doc:

https://developer.mozilla.org/en/XUL/toolbar
(Assignee)

Updated

8 years ago
Depends on: 525762

Updated

8 years ago
Depends on: 526461

Updated

8 years ago
Blocks: 540629

Updated

8 years ago
Depends on: 542115
(Assignee)

Updated

8 years ago
No longer depends on: 542115
(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.
This was a broken use of a template; fixed now.
Thanks!

Updated

8 years ago
No longer depends on: 525762

Updated

8 years ago
Blocks: 555576

Updated

7 years ago
Depends on: 78414
(Assignee)

Updated

7 years ago
Depends on: 541844
Depends on: 643919
Depends on: 648550
You need to log in before you can comment on or make changes to this bug.