250058 - Accesskeys for the submenu of "View, Sidebar" are active, however not underlined.

Status: RESOLVED FIXED

(Firefox :: Menus, defect, P5)

Description

[Aaron Graves]

Click on view, and go to Sidebar (or press Alt+V+E). Notice how the two items,
Bookmarks and History have no accesskeys underlined (as do the rest of the
menus). However, pressing B (or Alt+V+E+B) or H (or Alt+V+E+H) will activate the
respective menu item. So the accesskey is active, but not visible.

Comment 1

[Gérard Talbot]

I confirm your finding while using Firefox 0.9.1 build 20040626 under XP Pro
SP1a here.

Comment 2

[Jesse Ruderman]

Pressing B activates the Bookmarks menu item even though the menu item doesn't
have an accesskey.  You can see the same thing with items in the bookmarks menu:
pressing the first letter of a menu item highlights it, and if there are no
conflits, activates it.

Comment 3

[Jesse Ruderman]

The History item also appears in the Go menu, where it has the accesskey "i". 
Should the accesskey in View > Sidebar be the same or should it be "H"?

Comment 4

[Aaron Leventhal]

We should never use a lower case i or l for an accesskey unless there's nothing
else available. Too hard to see the underline. 
http://www.mozilla.org/projects/ui/accessibility/accesskey.html

Actually that idea was taken from Microsoft's guidelines for mnemonics.

Comment 5

[Jesse Ruderman]

I'll make the accesskey for History 's' in both places.

Comment 6

[Jesse Ruderman]

Attachment: patch

Makes the accesskeys 'B' and 's'.  Also renames "manBookmarksCmd", which opens
bokomark manager in Seamoneky but opens the bookmarks sidebar in Firefox, to
"bookmarksSidebarCmd".

cvs diff -u browser/base/content/browser-menubar.inc
browser/base/content/browser-sets.inc browser/base/locale/browser.dtd

Comment 7

[Steffen Wilberg]

Comment on attachment 152466 [details] [diff] [review]
patch

Why don't you use "H" as accesskey for History? That is not used yet in
View->Sidebar.

>+    <key id="viewBookmarksSidebarKb" key="&bookmarksSidebarCmd.commandkey;" command="viewBookmarksSidebar" modifiers="accel"/>
> #ifdef XP_WIN
>+    <key id="viewBookmarksSidebarKb" key="&bookmarksSidebarWinCmd.commandkey;" command="viewBookmarksSidebar" modifiers="accel"/>
Please give the second key a unique id, like viewBookmarksSidebarWinKb. There
was a complaint in bug 243413 comment 15 and 17 about them being identical.

>+<!ENTITY bookmarksSidebarCmd.accesskey  "B">
>+<!ENTITY bookmarksSidebarCmd.commandkey "b">
>+<!ENTITY bookmarksSidebarWinCmd.commandkey "i">
>+<!ENTITY historyCmd.accesskey           "s">
>+<!ENTITY history.commandKey             "h">
You should give the history keys similar names, like
historySidebarCmd.accesskey, historySidebarCmd.commandKey.

Comment 8

[Jesse Ruderman]

Comment on attachment 152466 [details] [diff] [review]
patch

I don't use H as an accesskey for History because the History item appears in
both Go and in View > Sidebar.	In Go, H is Home.  I could make History use
different accesskeys in different menus... do you think that would be better?

I'll fix the other things you mentioned.

Comment 9

[Steffen Wilberg]

I see, and you wrote that already in comment 3. My bad.
While I think underlining the "H" in the View menu would look nicer, having
consistent keys is probably better.

Comment 10

[Jesse Ruderman]

Attachment: patch for trunk

Makes the improvements Steffen suggested in addition to fixing the bug.

Comment 11

[Jesse Ruderman]

Fixed trunk and branch.