590744 - Add an edit sub menu to the Firefox menu

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[u88484]

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

Comment 1

[imradyurrad]

Blocking nomination.

Comment 3

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

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.

Comment 4

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

Attachment: Mockup of the proposed sub-menu

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

Comment 5

[u88484]

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

Comment 6

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

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.

Comment 7

[:Margaret Leibovic]

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

Comment 8

[:Margaret Leibovic]

Attachment: patch

This still needs a new icon image.

Comment 9

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

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

Comment 10

[:Margaret Leibovic]

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

Comment 11

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

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

Comment 12

[:Margaret Leibovic]

Attachment: patch v2

Ok, no icons for undo/redo.

Comment 13

[:Margaret Leibovic]

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

Clearing review request because this patch is broken on trunk now.

Comment 14

[:Margaret Leibovic]

Attachment: patch v3

Updated patch to work on trunk.

Comment 15

[:Margaret Leibovic]

Comment on attachment 506576 [details] [diff] [review]
patch v3

Also flagging Dolske, since I know his review queue has been short recently ;)

Comment 16

[Dão Gottwald [:dao]]

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 17

[:Margaret Leibovic]

Attachment: patch v4

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 506609 [details] [diff] [review]
patch v4

- the updateEditUIVisibility calls seem bogus
- styling on Linux is broken

Comment 19

[:Margaret Leibovic]

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

[:Margaret Leibovic]

Attachment: patch v5

Comment 22

[Dão Gottwald [:dao]]

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.

Comment 23

[:Margaret Leibovic]

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

Comment 24

[:Margaret Leibovic]

Attachment: patch v6

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.

Comment 25

[Dão Gottwald [:dao]]

Attachment: screenshot

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.

Comment 26

[:Margaret Leibovic]

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?

Comment 27

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

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

Comment 28

[:Margaret Leibovic]

Attachment: alternative patch (w/ splitmenu hover effect)

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.

Comment 29

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

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

Comment 30

[:Margaret Leibovic]

Attachment: patch v7

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

Comment 31

[:Margaret Leibovic]

I filed bug 632998 as a follow-up.

Comment 32

[(no longer active)]

Is this ready to land?

Comment 33

[:Margaret Leibovic]

Yes, I was planning on landing it tomorrow morning.

Comment 34

[:Margaret Leibovic]

http://hg.mozilla.org/mozilla-central/rev/75663a5ad23e

Comment 35

[Marcia Knous [:marcia]]

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

Comment 36

[Sean Smith]

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

Comment 37

[:Margaret Leibovic]

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

Comment 38

[Sean Smith]

(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