Add an edit sub menu to the Firefox menu

RESOLVED FIXED in Firefox 4.0b12

Status

()

Firefox
Menus
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: u88484, Assigned: Margaret)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
Firefox 4.0b12
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-litmus +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [target-betaN][softblocker])

Attachments

(4 attachments, 6 obsolete attachments)

(Reporter)

Description

7 years ago
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
(Reporter)

Updated

7 years ago
Summary: The undo and redo buttons are missing from Firefox menu button → The undo and redo buttons are missing from the Firefox menu button
Keywords: icon
Target Milestone: --- → Firefox 4.0

Comment 1

7 years ago
Blocking nomination.
Duplicate of this bug: 587202
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.
Summary: The undo and redo buttons are missing from the Firefox menu button → Add an edit sub menu to the Firefox menu
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.
(Reporter)

Comment 5

7 years ago
(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?
Flags: in-litmus?
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.
(Assignee)

Updated

7 years ago
Assignee: nobody → margaret.leibovic
(Assignee)

Comment 7

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

Comment 8

7 years ago
Created attachment 490672 [details] [diff] [review]
patch

This still needs a new icon image.
Attachment #490672 - Flags: review?(dao)
actually I decided to pull those icons out since the usage of the commands isn't very high (related bug 611570)
(Assignee)

Comment 10

7 years ago
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
(Assignee)

Comment 12

7 years ago
Created attachment 490766 [details] [diff] [review]
patch v2

Ok, no icons for undo/redo.
Attachment #490672 - Attachment is obsolete: true
Attachment #490766 - Flags: review?(dao)
Attachment #490672 - Flags: review?(dao)
Whiteboard: [target-betaN]
blocking2.0: --- → betaN+
Whiteboard: [target-betaN] → [target-betaN][softblocker]
(Assignee)

Comment 13

7 years ago
Comment on attachment 490766 [details] [diff] [review]
patch v2

Clearing review request because this patch is broken on trunk now.
Attachment #490766 - Flags: review?(dao)
(Assignee)

Comment 14

7 years ago
Created attachment 506576 [details] [diff] [review]
patch v3

Updated patch to work on trunk.
Attachment #490766 - Attachment is obsolete: true
Attachment #506576 - Flags: review?(dao)
(Assignee)

Comment 15

7 years ago
Comment on attachment 506576 [details] [diff] [review]
patch v3

Also flagging Dolske, since I know his review queue has been short recently ;)
Attachment #506576 - Flags: review?(dolske)
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.
Attachment #506576 - Flags: review?(dao) → review-

Updated

7 years ago
Component: Theme → Menus
QA Contact: theme → menus
(Assignee)

Comment 17

7 years ago
Created attachment 506609 [details] [diff] [review]
patch v4
Attachment #506576 - Attachment is obsolete: true
Attachment #506609 - Flags: review?(dao)
Attachment #506576 - Flags: review?(dolske)
Comment on attachment 506609 [details] [diff] [review]
patch v4

- the updateEditUIVisibility calls seem bogus
- styling on Linux is broken
Attachment #506609 - Flags: review?(dao) → review-
(Assignee)

Comment 19

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

Comment 20

7 years ago
Created attachment 506959 [details] [diff] [review]
patch v5
Attachment #506609 - Attachment is obsolete: true
Attachment #506959 - Flags: review?(dao)
(Assignee)

Updated

7 years ago
Duplicate of this bug: 629409
Whiteboard: [target-betaN][softblocker] → [target-betaN][softblocker][has patch][needs review 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.
(Assignee)

Comment 23

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

Comment 24

7 years ago
Created attachment 509214 [details] [diff] [review]
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.
Attachment #506959 - Attachment is obsolete: true
Attachment #509214 - Flags: review?(dao)
Attachment #506959 - Flags: review?(dao)
Created attachment 510123 [details]
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.
(Assignee)

Comment 26

7 years ago
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?
Keywords: icon → uiwanted
>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.
Keywords: uiwanted
(Assignee)

Comment 28

7 years ago
Created attachment 510672 [details] [diff] [review]
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.
>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.
(Assignee)

Comment 30

7 years ago
Created attachment 511201 [details] [diff] [review]
patch v7

This patch leaves the edit label alone. We can file a follow-up to improve it after Firefox 4.
Attachment #509214 - Attachment is obsolete: true
Attachment #511201 - Flags: review?(dao)
Attachment #509214 - Flags: review?(dao)

Updated

7 years ago
Attachment #511201 - Flags: review?(dao) → review+
(Assignee)

Updated

7 years ago
Blocks: 632998
(Assignee)

Comment 31

7 years ago
I filed bug 632998 as a follow-up.
Whiteboard: [target-betaN][softblocker][has patch][needs review dao] → [target-betaN][softblocker]
Is this ready to land?
(Assignee)

Comment 33

7 years ago
Yes, I was planning on landing it tomorrow morning.
(Assignee)

Comment 34

6 years ago
http://hg.mozilla.org/mozilla-central/rev/75663a5ad23e
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 4.0 → Firefox 4.0b12
https://litmus.mozilla.org/show_test.cgi?id=15086 added to Litmus.
Flags: in-litmus? → in-litmus+

Comment 36

6 years ago
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
(Assignee)

Comment 37

6 years ago
(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

6 years ago
(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

Updated

6 years ago
Depends on: 635674
You need to log in before you can comment on or make changes to this bug.