589146 - Firefox menu items should display sub-menu on a slightly delayed hover

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Josh Tumath]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b5pre) Gecko/20100820 Minefield/4.0b5pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b5pre) Gecko/20100820 Minefield/4.0b5pre

Open a ribbon application like Paint or Windows Live Movie Maker and click on the app button. In those applications, when you hover over *any* part of the menu item, the sub-menu will open.

However, in Firefox's app menu, when you hover over the left side of the split menu item, nothing happens. The sub-menu should appear.

Another related inconsistency is that when you hover over either menu item, the blue selection background should appear on both sides, but it only appears on the side that is being hovered over.

Sorry for the complicated explanation. Would it help if I made an image to illustrate the problem?

Reproducible: Always

Comment 1

[Josh Tumath]

Another thing that will need to be changed is to have a vertical line between the left and right menu items, like in Office.

Comment 2

[David Naylor]

I agree that hovering needs to open the sub menus. It is currently too "difficult" to get there.

Comment 3

[Tiago Sá]

I think it should basically work like Windows 7's start menu. With its animation thingies, the start menu is a big pain to navigate with the mouse, but since we don't have that, it shouldn't be a problem in Firefox menu.

Comment 4

[Alex Faaborg [:faaborg] (Firefox UX)]

Yes, this is the intended behavior for the Firefox menu items.  The only caveat is that the submenu should be on a slight delay, so that it doesn't show if you are planning on clicking on the main item.

We really need to get this fixed, needing to target the expansion arrow independently makes the menu considerably harder to use (both conceptually and physically, since there is a much smaller target).

Comment 5

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Intended Behavior

This displays the intended behavior, note that the sub-menu is being displayed even though the mouse cursor is still over the main item.

Comment 6

[u88484]

(In reply to comment #4)
> Yes, this is the intended behavior for the Firefox menu items.  The only caveat
> is that the submenu should be on a slight delay, so that it doesn't show if you
> are planning on clicking on the main item.
> 
> We really need to get this fixed, needing to target the expansion arrow
> independently makes the menu considerably harder to use (both conceptually and
> physically, since there is a much smaller target).

Requesting blocking based on this.

Comment 7

[Darren Kalck [:D-Kalck]]

Attachment: Detailed mockup of the Intended behavior

Comment 8

[[not reading bugmail]]

This bug is really awkward. I for one keep hitting the menu option to open the menu, and its annoying because I want a sub menu option, not the default option.  So if I click the wrong thing, I also have to close it too and go back and do it all over again.

Comment 9

[Tiago Sá]

This (In reply to comment #8)
> This bug is really awkward. I for one keep hitting the menu option to open the
> menu, and its annoying because I want a sub menu option, not the default
> option.  So if I click the wrong thing, I also have to close it too and go back
> and do it all over again.
What I'm about to say is offtopic, but you should know that, when hovering a menu entry with a sub menu, your mouse should ALWAYS, be near the right hand side of the menu, simply because it's easier to access the sub menu without accidentally closing it. If you do that, you should have no problems with this behavior.

Comment 10

[[not reading bugmail]]

Yeah, I mean to say if this bug can fix the underlining issue that is annoying, it would be nice to get this fixed with the suggested single hover/delay button, because its too easy to do the wrong behavior because of habitual behavior having expectations of menu systems already in place.  It should be like the start menu windows 7 flyout menu items, those make more sense then what is in place.

Comment 11

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 468633 [details]
Detailed mockup of the Intended behavior

This is fine (matches the visual design of the windows 7 start menu).  Alternatively if we just draw both items hovered with a small gap between them, that's fine as well.  The main issue here is making sure the menu displays on a delayed hover.

Comment 12

[imradyurrad]

(In reply to comment #11)
> Comment on attachment 468633 [details]
> Detailed mockup of the Intended behavior

Is there a bug for getting the menu to look that way?

Comment 13

[Dão Gottwald [:dao]]

Attachment: patch that doesn't work

I did this very quickly on top of bug 613156. The reason this doesn't work is that only the item or the menu can be active at a time, so when the menu opens, the item appears inactive, and when moving the cursor over the item again, the menu closes.

Comment 14

[:Margaret Leibovic]

Thanks for making the binding in another bug. I have a similar patch in my queue that has this same problem. I also made a new binding, but I was having trouble figuring out how to deal with the active states. Do you have any ideas how to fix this?

Comment 15

[:Margaret Leibovic]

Attachment: alternative approach

After talking with Dolske, I decided to try this different approach.

This patch makes a split-menu binding that extends the menu binding, so that we can take advantage of the native menupopup behavior that comes with the menu widget. It does a pretty basic job of styling a separator between the text and the arrow to indicate that it is a split menu instead of a regular menu. The click handler that triggers the menu command also needs some work. For some reason it won't hide the "Options..." menupopup, but it seems to work for the rest of the menus.

Comment 16

[Bill Gianopoulos [:WG9s]]

(In reply to comment #15)
> Created attachment 491740 [details] [diff] [review]
> alternative approach

> the arrow to indicate that it is a split menu instead of a regular menu. The
> click handler that triggers the menu command also needs some work. For some
> reason it won't hide the "Options..." menupopup, but it seems to work for the
> rest of the menus.

I created a Linux build including this patch and my pending patch for bug 585370.  Oddly, under Linux, I do not have the issue with the "Preferences" menupopup.  However it does occur with the "Print..." menupopup.

I'm not sure if that helps any in figuring out what the issue is.

Comment 17

[Bill Gianopoulos [:WG9s]]

(In reply to comment #15)
> Created attachment 491740 [details] [diff] [review]
> alternative approach 

> the arrow to indicate that it is a split menu instead of a regular menu. The
> click handler that triggers the menu command also needs some work. For some
> reason it won't hide the "Options..." menupopup, but it seems to work for the
> rest of the menus.

Simply re-ordering the code as follows seems to avoid the issue:

      <handler event="click"><![CDATA[
        if (event.originalTarget == this) {
          let node = this;
          while (node) {
            if (node.tagName == "menupopup")
              node.hidePopup();
            node = node.parentNode;
          }
          this.doCommand();
        }
      ]]></handler>

Comment 18

[Bill Gianopoulos [:WG9s]]

(In reply to comment #17)
> (In reply to comment #15)
> > Created attachment 491740 [details] [diff] [review] [details]
> > alternative approach 
> 
> > the arrow to indicate that it is a split menu instead of a regular menu. The
> > click handler that triggers the menu command also needs some work. For some
> > reason it won't hide the "Options..." menupopup, but it seems to work for the
> > rest of the menus.
> 
> Simply re-ordering the code as follows seems to avoid the issue:
> 
>       <handler event="click"><![CDATA[
>         if (event.originalTarget == this) {
>           let node = this;
>           while (node) {
>             if (node.tagName == "menupopup")
>               node.hidePopup();
>             node = node.parentNode;
>           }
>           this.doCommand();
>         }
>       ]]></handler>

I am now including this patch (with the above fix for the on click binding) as well as Firefox button under Linux and Tabs-in-titlebar for maximized windows in my daily builds available at http://www.wg9s.com/mozilla/firefox/ for the benefit of those playing along at home.

Comment 19

[:Margaret Leibovic]

Thanks for the fix, Bill! Another issue is that clicking on the arrow triggers the command, when we only want the left part of the menu to trigger the command. I tried checking event.originalTarget, but that only gives me the menu element, not it's children, so that will require more investigation.

Comment 20

[Bill Gianopoulos [:WG9s]]

Another issue with this patch is that because of adding the border on hover, split menu items end up being 1 pixel wider when hovered.  This does not currently cause any issue under Windows with the current default menu items, as on both the Primary and Secondary panes, the widest labels are on non split-menu items.  However, under Linux, the widest item on label on the secondary pane is preferences.  Hovering over preferences makes the right hand border of the menu panel shift 1 pixel to the right.

This can be fixed by doing the following:

1.  Change the splitter code in browser/base/content/browser.css to

.split-menu-splitter {
  color: transparent;
  margin-right: 1px;
}

.split-menu-splitter[_moz-menuactive="true"]:not([disabled]) {
  border-right: 1px solid -moz-menuhovertext;
  margin-right: 0px;
}

2. change the Winstripe splitter code to

  .split-menu-splitter[_moz-menuactive="true"]:not([disabled]) {
    border-right: 1px solid #9ac8fd;
    margin-right: 0px;
  }

Comment 21

[Bill Gianopoulos [:WG9s]]

(In reply to comment #19)
> Thanks for the fix, Bill! Another issue is that clicking on the arrow triggers
> the command, when we only want the left part of the menu to trigger the
> command. I tried checking event.originalTarget, but that only gives me the menu
> element, not it's children, so that will require more investigation.

There is a similar issue with the tooltips.

Would it be possible to place a transparent element over the arrow portion that disables the click and tooltip, but permits the hover to pass through?

Comment 22

[Bill Gianopoulos [:WG9s]]

(In reply to comment #20)
> This can be fixed by doing the following:
> 
> 1.  Change the splitter code in browser/base/content/browser.css to
> 
> .split-menu-splitter {
>   color: transparent;
>   margin-right: 1px;
> }
> 
> .split-menu-splitter[_moz-menuactive="true"]:not([disabled]) {
>   border-right: 1px solid -moz-menuhovertext;
>   margin-right: 0px;
> }
> 
> 2. change the Winstripe splitter code to
> 
>   .split-menu-splitter[_moz-menuactive="true"]:not([disabled]) {
>     border-right: 1px solid #9ac8fd;
>     margin-right: 0px;
>   }

OK.  Although that fixed the problem it was stupid.  It is much simpler and should perform better on the transition to just have a transparent border in the default case rather than adding and removing a margin.  So the change becomes to just redefine the non-hover case in browser/.base/content/browser.css as follows:

.split-menu-splitter {
  color: transparent;
  border-right: 1px solid transparent;
}

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 491740 [details] [diff] [review]
alternative approach

This seems to lack the delay? That's a rather central piece of this widget...

Comment 24

[Bill Gianopoulos [:WG9s]]

I am also thinking that as clicking on the left and clicking on the right during the delay period would perform different actions, perhaps a visible splitter is required in the non-default case.

Comment 25

[Bill Gianopoulos [:WG9s]]

(In reply to comment #24)
> I am also thinking that as clicking on the left and clicking on the right
> during the delay period would perform different actions, perhaps a visible
> splitter is required in the non-default case.
                                  ^^^^^^^
                                  hover

Comment 26

[Alex Faaborg [:faaborg] (Firefox UX)]

>perhaps a visible splitter is required in the non-[hover]case.

The white space should hopefully be enough, also a visible splitter might imply that you have to hover to the arrow to trigger the submenu (more splitting than is actually the case).  As soon as people interact with it a little they should pick up the behavior.  The other downside is that visible splitters would add a lot of visual complexity.

Comment 27

[Bill Gianopoulos [:WG9s]]

(In reply to comment #26)
> >perhaps a visible splitter is required in the non-[hover]case.
> 
> The white space should hopefully be enough, also a visible splitter might imply
> that you have to hover to the arrow to trigger the submenu (more splitting than
> is actually the case).  As soon as people interact with it a little they should
> pick up the behavior.  The other downside is that visible splitters would add a
> lot of visual complexity.

I think you misunderstood my point.  If nothing is displayed on hover for a delay period, then there is nothing visible to differentiate between clicking on the label, which performs one action and clicking on the arrow which performs a different action.  What I was suggesting is that if the highlight with the vertical separator between the regions where clicking triggers 2 different behaviors is going to display on a delay, then a vertical separator between the regions where clicking performs 2 different behaviors should probably be displayed during the delay period.  If not, then there should just be no delay and the behavior of this patch is fine as it is.

Comment 28

[Bill Gianopoulos [:WG9s]]

Upon re-reading your comment, I think you actually did understand my point.  The problem is that if I see an item in a menu that has an arrow on the right, if hovering over it does not produce any immediate visual effect, I would expect clicking on it to open a sub-menu.  Any other action is no intuitive.  The fact that I can later learn how a non-intuitive interface works over time does not really alter the fact that it is a bad user interface, in my opinion.

Comment 29

[Bill Gianopoulos [:WG9s]]

Just to be clear this is NOT an issue with the patch as it stands.  My comment was based on if a delay is introduced then the interface is confusing.  The user is presented with 7 emnu choices with arrows.  Six of which work in a non-obvious manner and 1 of which works exactly as would be expected.  I am not at all sure how experience with this is going to make it any less confusing.

The original idea made sense with the delayed hover change because the idea was that hoveing over the print label would highlight just the word print, but would expand to later highlight the entire region.  The patch you have commented on suggests losing the original partial item highlight, which is the only visual cue that clicking on print does anything different that clicking on the arrow.  The space does not work because on the "Web Developer" menu it visually looks identical yet clicking on the label works exactly the same as clicking on the arrow.

So my point was that if we go with the alternative approach, then either we need to lose the delay idea or provide a visual cue during the delay period on the split menu items that something different happens depending on where you click.

Comment 30

[Alex Faaborg [:faaborg] (Firefox UX)]

We're hoping to address that with the separation between the two hover effects (displaying that there are two click targets):

https://bug589146.bugzilla.mozilla.org/attachment.cgi?id=468522

Comment 31

[Bill Gianopoulos [:WG9s]]

(In reply to comment #30)
> We're hoping to address that with the separation between the two hover effects
> (displaying that there are two click targets):
> 
> https://bug589146.bugzilla.mozilla.org/attachment.cgi?id=468522

I understood that.  But the patch here seemed to be suggesting an alternative approach that did not do that.  That is why I said if we do not have a separation between the 2 hover effects until the delay period is up, then we should have a separation in the non-hovered state.  So, it sounds like we really agree here.

Comment 32

[Dão Gottwald [:dao]]

Attachment: patch that mostly works

Biggest issue here is that the tooltips don't display because the acceltext attribute isn't being set.

Comment 33

[Dão Gottwald [:dao]]

(In reply to comment #32)
> Created attachment 493270 [details] [diff] [review]
> patch that mostly works
> 
> Biggest issue here is that the tooltips don't display because the acceltext
> attribute isn't being set.

I should note that I only tested this on Linux.

Comment 34

[Dão Gottwald [:dao]]

I guess the loss of the tooltips isn't really a problem for these items, since the keyboard shortcut is repeated in the sub menu...

Comment 35

[Bill Gianopoulos [:WG9s]]

(In reply to comment #32)
> Created attachment 493270 [details] [diff] [review]
> patch that mostly works

Just to help out anyone else trying to build with this patch, this patch requires, and applies on top of, the pending patch in bug 613156.

Comment 36

[Bill Gianopoulos [:WG9s]]

(In reply to comment #34)
> I guess the loss of the tooltips isn't really a problem for these items, since
> the keyboard shortcut is repeated in the sub menu...

This is especially true as the duration of the delay before the sub-menu appears seems to be the same as that for displaying the tooltip on the non-split menu items.  Having the tooltip appear at the same time that the sub-menu opens and having both display the shortcut seems a bit redundant.

Comment 37

[Bill Gianopoulos [:WG9s]]

(In reply to comment #33)
> (In reply to comment #32)
> > Created attachment 493270 [details] [diff] [review] [details]
> > patch that mostly works
> > 
> > Biggest issue here is that the tooltips don't display because the acceltext
> > attribute isn't being set.
> 
> I should note that I only tested this on Linux.

I have tested this under Windows/XP, Vista and Windows 7.
In all three cases it works exactly as you have described.

Comment 38

[Alex Faaborg [:faaborg] (Firefox UX)]

>That is why I said if we do not have a
>separation between the 2 hover effects until the delay period is up

ah, sorry haven't tested the patch yet.  Yeah, it sounds like we agree that both areas should display a hover state immediately (with a small separation between them).

Comment 39

[Bill Gianopoulos [:WG9s]]

(In reply to comment #32)
> Created attachment 493270 [details] [diff] [review]
> patch that mostly works

I think I have found an issue that I am fairly certain is caused by this patch.  Some of the sub-menu items, particularly those on the right hand panel now seem to perform 2 actions, at least on Vista.

For example, selecting options and re-enabling the Menubar results in both the menubar being enabled and the Options dialog box opening.  Similarly, doing a help -> about results in the about dialog and the help page being displayed.  I will do more testing on this when I get home and have all of my test builds and machines available and can verify that this does not occur without the patch and also what other operating systems are affected by this.

Comment 40

[Bill Gianopoulos [:WG9s]]

(In reply to comment #39)
> (In reply to comment #32)
> > Created attachment 493270 [details] [diff] [review] [details]
> > patch that mostly works
> 
> I think I have found an issue that I am fairly certain is caused by this patch.
>  Some of the sub-menu items, particularly those on the right hand panel now
> seem to perform 2 actions, at least on Vista.
> 
> For example, selecting options and re-enabling the Menubar results in both the
> menubar being enabled and the Options dialog box opening.  Similarly, doing a
> help -> about results in the about dialog and the help page being displayed.  I
> will do more testing on this when I get home and have all of my test builds and
> machines available and can verify that this does not occur without the patch
> and also what other operating systems are affected by this.

I was wrong here.  This is actually caused by the binding changes in bug 613156.  This is NOT OS specific.  The same issue occurs under Linux as well with just the patch for bug 613156 and without the patch here.

Comment 41

[:Margaret Leibovic]

I agree that the tooltips seem redundant (and a bit noisy) if we're opening the submenu when hovering over the menuitem.

Dão, do you just want to take this bug? I tested your patch, and it seems really close to being done. I definitely think your patch is the way to go, since the highlight styling of the left and right side of the split menuitem is much better when they're separate elements.

Comment 42

[Alex Faaborg [:faaborg] (Firefox UX)]

>I agree that the tooltips seem redundant (and a bit noisy) if we're opening the
>submenu when hovering over the menuitem.

Also, we may want to use those tooltips later for keyboard access (try alt-f with one of the ribbon interfaces to see what I mean, they all display simultaneously and give you the next key that you can press).  That's of course assuming we can't just create multiple tooltips for an item.  Either way, we don't need mouse hover tooltips anymore.

Comment 43

[Dão Gottwald [:dao]]

Ok, taking.

Comment 44

[Dão Gottwald [:dao]]

Attachment: updated patch

Comment 45

[Dão Gottwald [:dao]]

Attachment: patch

Comment 46

[:Gavin Sharp [email: [e-mail]]]

So I guess the idea behind avoiding using an actual <menuitem> is to avoid the nsMenuFrame::HandleEvent logic that interferes with the menu state? It really seems like it would be cleaner to instead add some way to avoid the problematic parts of that logic, e.g. using an attribute. That is, have all of the relevant logic continue to be handled in the back-end rather than just bypassing it and re-implementing the behavior ourselves. That would also avoid the need to add the splitmenu classes to toolkit themes.

That being said, I'm not sure whether there are any actual downsides to avoiding the use of nsMenuFrame (keyboard access? accessibility?), and I guess messing with menuframe code at this point in the cycle may be best avoided...

Comment 47

[Dão Gottwald [:dao]]

This is kind of the easy way which I think works okay for our current needs. I agree that ideally and eventually this should be a proper menu frame.

Comment 49

[Pedro Bessa]

New Tab -> no New Tab again
           New Window
           separator
           Open File

The user will have to click New Tab.


Twice-listing is not minimalistic.

Comment 50

[Pedro Bessa]

You hover a bookmark folder to see its bookmark items and you have discovered
things accidentally by exploration in the past, so you click something that's
supposed to be hovered and discover a feature! The bookmark folder, already
selected, in the bookmarks manager. Tagging, anyone?

I'd go clicking everything supposed to be hovered...


To show a hoverable item is a clickable item:

- I don't like much: underline
- I don't like much: keep the arrow out of the highlight

New Tab -> no New Tab again
           New Window
           separator
           Open File

The user will have to click New Tab, so the user will discover hoverable items are clickable items.

Twice-listing is not minimalistic.

Comment 51

[Pedro Bessa]

comment 49 wasn't supposed to be here

Comment 53

[Pedro Bessa]

If the user clicks, the menu doesn't open.
If the user doesn't click, the menu opens.

Is that it? I hope it works.

Comment 54

[Dão Gottwald [:dao]]

Attachment: patch

updated to tip

Comment 55

[:Gavin Sharp [email: [e-mail]]]

These new menus don't interact with "normal" menus in the standard way:
http://grab.by/8ljE

Comment 56

[Dão Gottwald [:dao]]

(In reply to comment #55)
> These new menus don't interact with "normal" menus in the standard way:
> http://grab.by/8ljE

WFM when moving the mouse cursor up from Web Developer to Print, but fails when moving from Web Developer up to the right pane. Probably not worth working around for the given limited scope... I can do it, but I suspect it's going to be ugly.

Comment 57

[Justin Dolske [:Dolske]]

I can't reproduce comment 55/56 at all on a current build. :/

I'd be somewhat hesitant to take the brokenness unless it's very tricky to reproduce (which it seems to be?). But maybe we could just make Web Developer into a splitmenu as well (action going to opening the Web Console). It's not a natural fit, but neither is it being the only normal submenu (and it's slightly annoying to get to the web console through the submenu). Although, hmm, I guess addons could always add a non-splitmenu submenu item to the app menu so just wallpapering Web Developer doesn't completely avoid the problem...

Comment 58

[:Gavin Sharp [email: [e-mail]]]

I think when I discussed this with beltzner that the main reason this was considered a hardblocker was the actual appearance of the hover/secondary state, rather than the delayed-hover behavior itself. How much simpler would the patch be if we didn't do the hovering and focused only on styling changes?

Comment 59

[Dão Gottwald [:dao]]

Attachment: patch v2

Comment 60

[Dão Gottwald [:dao]]

(In reply to comment #58)
> I think when I discussed this with beltzner that the main reason this was
> considered a hardblocker was the actual appearance of the hover/secondary
> state, rather than the delayed-hover behavior itself. How much simpler would
> the patch be if we didn't do the hovering and focused only on styling changes?

I'm not sure what styling we'd want without hovering over the items opening the sub menus, so I don't know how much simpler it would be. Either way it's extra work and doesn't sound like a useful investment.

Comment 61

[Bill Gianopoulos [:WG9s]]

(In reply to comment #59)
> Created attachment 505063 [details] [diff] [review]
> patch v2

This patch fixes the issue from comment 55 for me.  Tested both in Linux and Windows.

Comment 62

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 505063 [details] [diff] [review]
patch v2

>diff --git a/toolkit/content/xul.css b/toolkit/content/xul.css

> @media not all and (-moz-images-in-menus) {
>-  menuitem:not([type]):not(.menuitem-with-favicon) > .menu-iconic-left {
>+  .menu-iconic-left {
>     visibility: hidden;
>   }
>-  menu > .menu-iconic-left {
>-    visibility: hidden;
>+  :-moz-any(menuitem[type], .menuitem-with-favicon) > .menu-iconic-left {
>+    visibility: visible;
>   }

Why is this change needed? (It would be generally helpful for you to assume that the reasoning for changes like these will be non-intuitive to me, and explain as you post the patch).

>diff --git a/toolkit/themes/gnomestripe/global/menu.css b/toolkit/themes/gnomestripe/global/menu.css
>diff --git a/toolkit/themes/winstripe/global/menu.css b/toolkit/themes/winstripe/global/menu.css

I take it you audited all of the "menuitem" style rules, and only added equivalents for .splitmenu-menuitem for the cases we currently hit in the Firefox menu? It's really quite unpleasant to need to modify toolkit rules for our specific use case. I'd almost prefer just copying the relevant rules to browser/... though potential maintenance burden of doing that (and then having to update two places to adjust menu appearance) is obviously also unpleasant.

I wonder whether we can override the frame type by using display="xul:hbox" on a binding applied to the menu item, to avoid needing these theme changes?

>+.splitmenu-menu {
>+  -moz-box-pack: end;
>+}

There doesn't really seem to be much benefit to moving this rule to toolkit/ (from /browser)... I don't think we want to encourage others to depend on these rules, and maintenance burden concerns don't apply here, right?

> .menu-iconic-left {
>   min-width: 1.45em;
>+  -moz-appearance: menuimage;
>+  padding-top: 2px;
> }

>-menu.menu-iconic > .menu-iconic-left,
>-menuitem.menuitem-iconic > .menu-iconic-left {
>-  -moz-appearance: menuimage;
>-  padding-top: 2px;
>-}

Why this change?

Comment 63

[Dão Gottwald [:dao]]

(In reply to comment #62)
> Comment on attachment 505063 [details] [diff] [review]
> patch v2
> 
> >diff --git a/toolkit/content/xul.css b/toolkit/content/xul.css
> 
> > @media not all and (-moz-images-in-menus) {
> >-  menuitem:not([type]):not(.menuitem-with-favicon) > .menu-iconic-left {
> >+  .menu-iconic-left {
> >     visibility: hidden;
> >   }
> >-  menu > .menu-iconic-left {
> >-    visibility: hidden;
> >+  :-moz-any(menuitem[type], .menuitem-with-favicon) > .menu-iconic-left {
> >+    visibility: visible;
> >   }
> 
> Why is this change needed?

To let splitmenus obey the images-in-menus setting.

> I take it you audited all of the "menuitem" style rules, and only added
> equivalents for .splitmenu-menuitem for the cases we currently hit in the
> Firefox menu?

The stylesheet supports disabled splitmenus, of which we currently have none.

> I wonder whether we can override the frame type by using display="xul:hbox" on
> a binding applied to the menu item, to avoid needing these theme changes?

I tried that, it didn't work.

> >+.splitmenu-menu {
> >+  -moz-box-pack: end;
> >+}
> 
> There doesn't really seem to be much benefit to moving this rule to toolkit/
> (from /browser)... I don't think we want to encourage others to depend on these
> rules, and maintenance burden concerns don't apply here, right?

I can change it, I don't care either way.

> > .menu-iconic-left {
> >   min-width: 1.45em;
> >+  -moz-appearance: menuimage;
> >+  padding-top: 2px;
> > }
> 
> >-menu.menu-iconic > .menu-iconic-left,
> >-menuitem.menuitem-iconic > .menu-iconic-left {
> >-  -moz-appearance: menuimage;
> >-  padding-top: 2px;
> >-}
> 
> Why this change?

To make this work for .splitmenu-menuitem nodes.

Comment 64

[Dão Gottwald [:dao]]

Attachment: patch v3

Comment 65

[Alice0775 White]

I think that
the delay time a menu takes to open a sub-menu in Firefox Button should follow ui.submenuDelay, should not be a constant value(600ms).

Comment 66

[Valerio]

With this patch the arrows of the .splitmenu-menuitem are not alligned with the arrow of the menuitem, looks for example Print and Web Developer menu

Comment 67

[Bill Gianopoulos [:WG9s]]

(In reply to comment #66)
> With this patch the arrows of the .splitmenu-menuitem are not alligned with the
> arrow of the menuitem, looks for example Print and Web Developer menu

The followup patch fixed this.

Comment 68

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/dc22e591c184
http://hg.mozilla.org/mozilla-central/rev/8c7931233204

This will be in beta11, I think. Can't set the target milestone.

Comment 69

[Ben]

Verified OK in beta11

Comment 70

[Pedro Bessa]

in firefox app button

hover an arrow
when the sub-menu opens
hover an arrow below it

(for a small time, the above item is still highlighted, but the above arrow isn't still highlighted)

hover the item by the left of the arrow
when the sub-menu opens
hover the item by the left of the arrow below it

(for a small time, the above item and arrow are still highlighted)

Comment 71

[Pedro Bessa]

Firefox App Button has to be minimal, but New Tab appears in Menu AND Sub-Menu, Print, Preferences and Help are appearing twice too. Is this really intended?

Comment 72

[Pedro Bessa]

When I move the pointer from Bookmarks arrow to New Tab item, the New Tab highlighting behavior is the old (first the item, later the arrow).

Comment 73

[Pedro Bessa]

Minimalistic design is the least number of things to view, click...

Comment 74

[Josh Tumath]

(In reply to comment #71)
> Firefox App Button has to be minimal, but New Tab appears in Menu AND Sub-Menu,
> Print, Preferences and Help are appearing twice too. Is this really intended?

Yes, for the same reasons that it is intended in other software, such as Office 2007.

However, this bug is verified, and there is no need to continue discussion here. Please file a new bug if there are any issues with the implantation. :-)

Comment 75

[Pedro Bessa]

ok, two of them don't have shortcut keys lol nm

Comment 76

[Pedro Bessa]

*ok, but two

sry

Comment 77

[Bill Gianopoulos [:WG9s]]

Bugzilla IS NOT a discussion forum.  Please take this discussion someplace else.

Comment 78

[Pedro Bessa]

in the app button

the only separator used in the left list is a line
the only separator used in the right list is a space

use space instead of line in the left list
the right list is so small with less than half items than the left list
put the right list on top, put the left list below

separate lists by line
separate items within lists by space

that would be ideal

Comment 79

[Pedro Bessa]

AMO has https://addons.mozilla.org/pt-BR/firefox/addon/menu-editor/
https://addons.mozilla.org/pt-BR/firefox/addon/personal-menu/
https://addons.mozilla.org/pt-BR/firefox/addon/compact-menu-2/
https://addons.mozilla.org/pt-BR/firefox/addon/tiny-menu/ but a build-in menu editor would be better

like a patent improvement

Comment 80

[Pedro Bessa]

and the built-in menu editor would have to be even better

like an other patent improvement

Comment 81

[Pedro Bessa]

because I think both are patented

Comment 82

[Pedro Bessa]

LET ME DRAG-AND-DROP MENU ITEMS IN THE APP BUTTON! thx

Comment 83

[redsign]

Could you pleas stop making noise here? Thanks.

Comment 84

[barrett.m.john]

This bug appears to be back in the desktop version of Firefox 57 on Windows 7