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

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

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

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.

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

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

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

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.

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.

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

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

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.

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?

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.

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

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

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

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.

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; }

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

(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 on attachment 491740 [details] [diff] [review] alternative approach This seems to lack the delay? That's a rather central piece of this widget...

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.

(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

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

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

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.

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.

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

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

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

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

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

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

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

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

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

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

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

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.

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

Ok, taking.

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

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.

New Tab -> no New Tab again New Window separator Open File The user will have to click New Tab. Twice-listing is not minimalistic.

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 49 wasn't supposed to be here

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.

updated to tip

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

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

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

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?

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

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

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

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

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

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

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.

Verified OK in beta11

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)

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?

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

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

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

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

*ok, but two sry

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

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

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

and the built-in menu editor would have to be even better like an other patent improvement

because I think both are patented

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

Could you pleas stop making noise here? Thanks.

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