610902 - Keyboard shortcuts should appear in grey text

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

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

To help reduce the visual complexity of sub-menus in the Firefox menu, keyboard shortcuts should appear in grey text.

We may also decide to apply this styling to the traditional menu on Vista and 7 as well.  On XP, there is more of a precedent for the visual design of traditional menu bars, since they are found everywhere on the platform.  So on XP we should just continue using a native appearance.

Comment 1

[:Margaret Leibovic]

Attachment: patch

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

FYI: This patch still applies cleanly.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 610902

I have changed the patch to use the child selector and added a new class, |appmenu_keyboard-shortcut|, to menuitems that have keys associated with them.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 572079 [details] [diff] [review]
Patch for bug 610902

Extensions' menu items are going to lack the appmenu_keyboard-shortcut class, making the styling inconsistent.

I'm not sure I understand the rationale behind this bug; these menus don't seem visually more complex than other menus... So we should probably do nothing about this or do it for all menus.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 610902 v1.1

This patch uses Margaret's approach but extends the graytext to the "alt" menus.

How concerned should we be about the performance of the descendant selectors here?

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 572178 [details] [diff] [review]
Patch for bug 610902 v1.1

>+  #appmenu-popup .menu-accel,
>+  #appmenu-popup .menu-iconic-accel,
>+  #main-menubar .menu-accel,
>+  #main-menubar .menu-iconic-accel {
>+    color: graytext;
>+  }

At this point, it looks like #appmenu-popup and #main-menubar are redundant in these selectors.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 610902 v1.2

Thanks for the review Dao. I have updated the patch to only have the class selectors.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 577347 [details] [diff] [review]
Patch for bug 610902 v1.2

Apparently the section where you're adding this affects Windows XP, which wasn't part of the plan. Please move this to the windows-default-theme section in browser-aero.css.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 610902 v1.3

I've moved the styles to the windows-default-theme portion of browser-aero.css.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 577482 [details] [diff] [review]
Patch for bug 610902 v1.3

Move it to the end of this section:
http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser-aero.css#40

r=me with that

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 610902 v1.4 (for check-in)

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 610902 v1.4 (for check-in)

Moved to the end of the section.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/e35dfafeee42

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/e35dfafeee42