The undo and redo buttons are missing from the Firefox menu button. Iteration 6 of the Firefox menu button shows these on the Edit line of the menu. i6 is https://bugzilla.mozilla.org/attachment.cgi?id=465479

Blocking nomination.

Slightly morphing this bug to cover not just adding the undo and redo commands, but a few of the other edit menu commands (and exposing keyboard shortcuts for everything as well). Changes: -Make the "Edit" label normal black text, instead of italicized greytext -Add a submenu that contains the three top level commands, in addition to undo, redo, select all and delete.

Here's a mockup, the redundancy isn't great but it allows us to expose keyboard shortcuts.

(In reply to comment #4) > Created attachment 489405 [details] > Mockup of the proposed sub-menu > > Here's a mockup, the redundancy isn't great but it allows us to expose keyboard > shortcuts. Shouldn't the order be consistent with the Edit menu?

I went back and forth on that, this order is consistent with Windows Explorer (vista, 7), so I figured we should just leverage external consistency in the new interface. Also we aren't expecting users to rely on both the traditional menu bar and the Firefox menu, so the fact that the app is now internally inconsistent is less of a worry. I only think it makes sense to make the change in a new UI though, people who want the traditional menu bar on modern windows are clearly indicating that they don't want us to change too much from previous versions.

I can make a patch, but I will need an updated version of appmenu-icons.png that includes the undo/redo icons.

This still needs a new icon image.

actually I decided to pull those icons out since the usage of the commands isn't very high (related bug 611570)

So no icons at all, or just no undo/redo icons?

just icons for cut/copy/paste. Updated mockup here: http://people.mozilla.com/~faaborg/files/firefox4Mockups/polishFirefoxMenu-i1/polishFirefoxMenu-i1.htm

Ok, no icons for undo/redo.

Comment on attachment 490766 [details] [diff] [review] patch v2 Clearing review request because this patch is broken on trunk now.

Updated patch to work on trunk.

Comment on attachment 506576 [details] [diff] [review] patch v3 Also flagging Dolske, since I know his review queue has been short recently ;)

Comment on attachment 506576 [details] [diff] [review] patch v3 >+ <menu class="splitmenu-menu"> Kind of ugly. splitmenu-menu is supposed to be anonymous content... >+ <menupopup id="appmenu-editmenu-menupopup" >+ onpopupshowing="updateEditUIVisibility()" >+ onpopuphidden="updateEditUIVisibility()"> >+ <menuitem id="appmenu-editmenu-cut" >+ class="menuitem-iconic" >+ label="&cutCmd.label;" >+ key="key_cut" >+ accesskey="&cutCmd.accesskey;" You just failed browser_bug616836.js. > #appmenu-edit-label { > -moz-appearance: none; > background: transparent; >- font-style: italic; >+ color: #000; > } Hardcoded color in a non-"windows-default-theme" area should ring a bell.

Comment on attachment 506609 [details] [diff] [review] patch v4 - the updateEditUIVisibility calls seem bogus - styling on Linux is broken

(In reply to comment #18) > Comment on attachment 506609 [details] [diff] [review] > patch v4 > > - the updateEditUIVisibility calls seem bogus I forget why I did that. It looks like I must have just copied it from browser-menubar.inc (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-menubar.inc#140). The menupopup works without it, though, so you're right, and I'll get rid of it. > - styling on Linux is broken Oops, old patch from before Firefox button landed on Linux :)

Comment on attachment 506959 [details] [diff] [review] patch v5 >--- a/browser/themes/gnomestripe/browser/browser.css >+++ b/browser/themes/gnomestripe/browser/browser.css >+#appmenu-editmenu { >+ -moz-box-pack: end; >+} Have you verified that this is needed on Linux? >+#appmenu-editmenu > .menu-text, >+#appmenu-editmenu > .menu-accel-container { >+ display: none; >+} Maybe this should be in content/browser.css along with .splitmenu-menu > .menu-text? >+#appmenu-edit-label { >+ -moz-appearance: none; >+ background: transparent; >+ color: MenuText; >+} I'm pretty sure -moz-appearance: none; and background: transparent; aren't needed.

(In reply to comment #22) > Comment on attachment 506959 [details] [diff] [review] > patch v5 > > >--- a/browser/themes/gnomestripe/browser/browser.css > >+++ b/browser/themes/gnomestripe/browser/browser.css > > >+#appmenu-editmenu { > >+ -moz-box-pack: end; > >+} > > Have you verified that this is needed on Linux? I just tested it, and it isn't needed. I'll get rid of it. > >+#appmenu-editmenu > .menu-text, > >+#appmenu-editmenu > .menu-accel-container { > >+ display: none; > >+} > > Maybe this should be in content/browser.css along with .splitmenu-menu > > .menu-text? Sounds good to me. > >+#appmenu-edit-label { > >+ -moz-appearance: none; > >+ background: transparent; > >+ color: MenuText; > >+} > > I'm pretty sure -moz-appearance: none; and background: transparent; aren't > needed. Without them, the hover style is broken (the label looks like a normal disabled menuitem), but I could move the styles to an #appmenu-edit-label:hover rule if you think that's better.

Updated patch to address first two comments. I left the #appmenu-edit-label styles as-is, since they already include -moz-appearance: none; and background: transparent; in winstripe.

I don't think I understand the proposed design. "Edit" looks like it should be interactive but it isn't. Maybe the label should be "Edit:", or the text styling shouldn't be changed after all.

Yeah, I don't think it's supposed to be interactive because it seems like it would be strange to open the submenu from that label, since there are toolbar buttons in between. I'm in favor of keeping the text styling the same. Alex, what do you think?

>Alex, what do you think? Is there any way that we can get a hover on edit to act like split-menu, where the sub menu opens on a delay? This is strange since the main command does nothing (to provide quick access to cut/copy/paste), but would be more consistent than having the hover do nothing, now that we are adding the sub-menu.

This is my quick attempt at implementing Alex's idea from comment 27. The two main problems I'm having is that sometimes the hover effect hangs around on the "Edit" label when it shouldn't, and clicking on the "Edit" label will dismiss the app menu. Dão, do you have ideas about the best way to resolve these two issues? Even if we can resolve these problems, I'm a little worried that it looks kind of strange to have toolbar buttons in the middle of a split menu. Right now the menu becomes active when you hover over the toolbar buttons, but they're not included in the active hover style on the "Edit" label, which makes the behavior slightly confusing. However, including them in the hover style would be strange looking because we would also want them to have individual hover styles. For the sake of simplicity, I'm in favor of keeping the edit label the way it is currently. The blocking issue is that there is no access to the additional edit actions from the app menu, and the behavior of the label does not affect that.

>For the sake of simplicity, I'm in favor of keeping the edit label the way it >is currently. The blocking issue is that there is no access to the additional >edit actions from the app menu, and the behavior of the label does not affect >that. that's fine, the non-interactive edit label is a bit strange, but I think people will be able to figure it out.

This patch leaves the edit label alone. We can file a follow-up to improve it after Firefox 4.

I filed bug 632998 as a follow-up.

Is this ready to land?

Yes, I was planning on landing it tomorrow morning.

https://litmus.mozilla.org/show_test.cgi?id=15086 added to Litmus.

Items that are disabled in the edit sub menu do not appear as disabled, though they do act it (no highlight on mouseover). Mozilla/5.0 (X11; Linux x86_64; rv:2.0b12pre) Gecko/20110212 Firefox/4.0b12pre

(In reply to comment #36) > Items that are disabled in the edit sub menu do not appear as disabled, though > they do act it (no highlight on mouseover). I can't reproduce this, although I noticed that the icons in the disabled menuitems are not desaturated like I would expect them to be. Is that what you noticed? We could file a follow-up bug about that.

(In reply to comment #37) > (In reply to comment #36) > > Items that are disabled in the edit sub menu do not appear as disabled, though > > they do act it (no highlight on mouseover). > > I can't reproduce this, although I noticed that the icons in the disabled > menuitems are not desaturated like I would expect them to be. Is that what you > noticed? We could file a follow-up bug about that. My comment relates to my KDE install. On closer inspection, the text looks *maybe* *slightly* disabled. :D So, yes, they should probably be more desaturated, though now that I look at other menus with disabled items, it looks pretty close to the others. And in KDE, disabled items don't get a mouseover highlight. I'm new to playing with KDE. It looks fine in WinXP and Win7 to me. So, I'm closing my report as WFM. :D