Last Comment Bug 610902 - Keyboard shortcuts should appear in grey text
: Keyboard shortcuts should appear in grey text
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Theme (show other bugs)
: Trunk
: All Windows Vista
: -- normal (vote)
: Firefox 11
Assigned To: Jared Wein [:jaws] (please needinfo? me)
:
Mentors:
Depends on: 724457
Blocks:
  Show dependency treegraph
 
Reported: 2010-11-09 17:56 PST by Alex Faaborg [:faaborg] (Firefox UX)
Modified: 2013-11-13 03:07 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (698 bytes, patch)
2010-11-10 15:06 PST, :Margaret Leibovic
no flags Details | Diff | Splinter Review
Patch for bug 610902 (13.83 KB, patch)
2011-11-04 14:35 PDT, Jared Wein [:jaws] (please needinfo? me)
dao+bmo: review-
Details | Diff | Splinter Review
Patch for bug 610902 v1.1 (959 bytes, patch)
2011-11-04 23:20 PDT, Jared Wein [:jaws] (please needinfo? me)
dao+bmo: review-
Details | Diff | Splinter Review
Patch for bug 610902 v1.2 (829 bytes, patch)
2011-11-28 13:21 PST, Jared Wein [:jaws] (please needinfo? me)
dao+bmo: review-
Details | Diff | Splinter Review
Patch for bug 610902 v1.3 (728 bytes, patch)
2011-11-28 21:21 PST, Jared Wein [:jaws] (please needinfo? me)
dao+bmo: review+
Details | Diff | Splinter Review
Patch for bug 610902 v1.4 (for check-in) (894 bytes, patch)
2011-11-29 07:13 PST, Jared Wein [:jaws] (please needinfo? me)
jaws: review+
Details | Diff | Splinter Review
Patch for bug 610902 v1.4 (for check-in) (856 bytes, patch)
2011-11-29 07:17 PST, Jared Wein [:jaws] (please needinfo? me)
jaws: review+
Details | Diff | Splinter Review

Description Alex Faaborg [:faaborg] (Firefox UX) 2010-11-09 17:56:01 PST
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 2010-11-10 15:06:50 PST
Created attachment 489644 [details] [diff] [review]
patch
Comment 2 Jared Wein [:jaws] (please needinfo? me) 2011-11-04 01:35:56 PDT
FYI: This patch still applies cleanly.
Comment 3 Jared Wein [:jaws] (please needinfo? me) 2011-11-04 14:35:27 PDT
Created attachment 572079 [details] [diff] [review]
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] 2011-11-04 14:44:00 PDT
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) 2011-11-04 23:20:12 PDT
Created attachment 572178 [details] [diff] [review]
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] 2011-11-26 04:53:31 PST
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) 2011-11-28 13:21:05 PST
Created attachment 577347 [details] [diff] [review]
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] 2011-11-28 15:08:39 PST
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) 2011-11-28 21:21:29 PST
Created attachment 577482 [details] [diff] [review]
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] 2011-11-29 00:40:41 PST
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) 2011-11-29 07:13:51 PST
Created attachment 577603 [details] [diff] [review]
Patch for bug 610902 v1.4 (for check-in)
Comment 12 Jared Wein [:jaws] (please needinfo? me) 2011-11-29 07:17:21 PST
Created attachment 577604 [details] [diff] [review]
Patch for bug 610902 v1.4 (for check-in)

Moved to the end of the section.
Comment 13 Jared Wein [:jaws] (please needinfo? me) 2011-11-29 07:20:09 PST
https://hg.mozilla.org/integration/fx-team/rev/e35dfafeee42
Comment 14 Tim Taubert [:ttaubert] 2011-11-29 22:04:01 PST
https://hg.mozilla.org/mozilla-central/rev/e35dfafeee42

Note You need to log in before you can comment on or make changes to this bug.