Closed Bug 245984 Opened 17 years ago Closed 17 years ago

Bookmarks context menu (Personal Bar & Bookmarks Manager) lacks mnemonics (access-keys/underlined keyboard shortcuts)

Categories

(Firefox :: Menus, defect, P3)

x86
All
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: bugzillamozilla, Assigned: mano)

References

Details

(Keywords: access, fixed-aviary1.0, polish, Whiteboard: [have patch])

Attachments

(2 files, 4 obsolete files)

This is the Firefox equivalent of Bug 214881 (for Seamonkey).

Fixing this bug would not only make Firefox more accessible, but it would also
minimize the effect of bugs like Bug 172675 (by making it almost as quick to
open a folder in new tabs).

Prog.
If parity with IE is still sought after, I suggest to follow the same mnemonics
(where possible):

  &Open
  Cu&t
  &Copy
  &Delete
  P&roperties <- not sure about this one, perhaps &Properties would be better.

The rest are not available in IE and should probably follow other Firefox
menus.

Prog.
P&roperties is what Windows Explorer uses, but IE uses &Properties.  We use
&Properties elsewhere, like in a link context menu so we should probably stay
with that.
Flags: blocking1.0?
Flags: blocking1.0? → blocking1.0-
Prog, what about doing up a patch that uses &Properties?  We'll at least be
consistent throughout the app.  Maybe with an actual patch, we can work with
Mike to drive it in for 1.0.
access fixes are always a good thing, I'll buy that for a dollar

and &Properties would be my preference.
Keywords: access
Ok, so we're settled on &Properties. There's a slight problem though - it's
already defined as "i" and used by the the new Bookmarks Manager:

http://lxr.mozilla.org/aviarybranch/source/browser/locales/en-US/chrome/browser/bookmarks/bookmarks.dtd#47
http://lxr.mozilla.org/aviarybranch/source/browser/components/bookmarks/content/bookmarksManager.xul

Prog.
Keywords: polish
Probably done only because Edit > Paste uses 'P' already.  Can we have Edit >
Properties still use 'i' as an access key and the context menu and toolbar use
'P'?  Or would that be too confusing?
*** Bug 253106 has been marked as a duplicate of this bug. ***
As we are not dealing with a xul+dtd file (bookmarks.properties), we need a way
to define and get access keys.

After landing this patch, we can add lines like the following to
bookmarks.propeties:
accesskey_bm_properties = i

(that's for cmd_bm_properties)
Assignee: firefox → bugs.mano
Attachment #154491 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: blocking-aviary1.0PR?
*** Bug 176359 has been marked as a duplicate of this bug. ***
Summary: Bookmarks context menu lacks mnemonics (access-keys/underlined keyboard shortcuts) → Bookmarks context menu (Personal Bar & Bookmarks Manager) lacks mnemonics (access-keys/underlined keyboard shortcuts)
Prog, about what bug have you talked in comment 0? bug 214881 is not the one...
It was supposed to be Bug 176359, not 214881. My clipboard was probably holding
that value for the this-bug-blocks field... ;-)

It seems that 'P' and 'R' are already taken (for Paste and Rename), but 'i' for
Properties is just too narrow. How about 'm' for Rename, 'r' for Properties, and
'g' for Manage Folder? At least all three are inline with Windows Explorer
mnemonics (if anyone was wondering, 'g' is used in My Computer context menu for
Manage).

Prog.


Prog, we use "M" as the mnemonic for "Manage" in the Bookamrks menu (main window).
 cmd_bm_openfolder = Open in Tabs
-cmd_bm_openfolder_accesskey = O
+cmd_bm_openfolder_accesskey = p

How about 'T' for this one? Better to use the first letter of a major word for
mnemonics. Or maybe even continue to use "O" here. Not many people use the
regular open in a context menu (they just do regular click), so give that the
crummy 'p'. In fact, there's a bug filed to have 'Open' removed altogether.
Perhaps I wasn't clear enough. The idea is to use 'g' for Manage, so that 'm'
becomes free for Rename, so that 'r' becomes free for Properties. Get it?

Prog.
(In reply to comment #14)
I agree for "T".

...(As we use this for "Open in new Tab" (obviously, they are not available at
the same time...))


(In reply to comment #15)
I just said we already use "m" for "Manage" in other places.
:-) We can safely use "r" without cahnging anything... The rename command
doesn't appear on the relevant menus.
We can'tm use "T" for "Open in Tabs" and not for "Open in New Tab", that's
cu&t...sorry, my mistkae
Attached patch Some changes (obsolete) — Splinter Review
"P&roperties": but not in Bookmarks Manager ("&Redo").
I restored "&Open in tabs" ("&Open" and "&Open in tabs" items don't appear in
the same time).
Comment on attachment 154593 [details] [diff] [review]
Some changes

..and I forgot to mention "Open in Ne&w Tab", this is not so good...but morst
of us ctrl+click the bookmark.
(that's because of cu&t).
Attachment #154593 - Flags: review?(mconnor)
l10n impact -> +, please get mconnor's review and land within 7 days otherwise
this will be -. 
Flags: blocking-aviary1.0PR? → blocking-aviary1.0PR+
Attachment #154593 - Flags: review?(mconnor) → review?(vladimir)
Attached patch Up-to-date patch (obsolete) — Splinter Review
I hope i won't need to create it another time...
Attachment #154593 - Attachment is obsolete: true
Attachment #154593 - Flags: review?(vladimir)
mconnor, can you please take a look at this fairly simple patch? Ben says it has
to be in by Sunday/Monday or it will be -'d due to l10n freeze.
Comment on attachment 155335 [details] [diff] [review]
Up-to-date patch

looks good to me, had to update the patch to current CVS (vlad bitrotted you). 
Will attach a current patch in a moment
Attachment #155335 - Flags: review?(vladimir) → review+
Comment on attachment 155467 [details] [diff] [review]
CVS tip version of patch for aviary, just needs approval-aviary.

moving review from mconnor, asking approval
Attachment #155467 - Flags: review+
Attachment #155467 - Flags: approval-aviary?
Whiteboard: [have patch]
Asa, can you grant it? thanks!
Sairuh, or Aaron, can you take a quick look at this change? If there are no
obvious problems/conflicts, I'd like to see it land soon.
QA Contact: bugzilla → bugzilla
the mnemonics in the patch look fine to me, but AaronL would be the best person
for a thorough r=.
Comment on attachment 155467 [details] [diff] [review]
CVS tip version of patch for aviary, just needs approval-aviary.

a=ben@mozilla.org
Attachment #155467 - Flags: approval-aviary? → approval-aviary+
Looks good.
now, can someone check it in (Trunk & Aviary)? Thanks.
Done. 
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
setting fixed-aviary1.0 for bugfixes checked into branch, for searching purposes.
sorry for bugspam.
Keywords: fixed-aviary1.0
in bookmarks toolbar and bookmarks manager context menus, these two accesskeys
conflict (redundant S):

New _Separator
_Sort by Name

tested with 2004100609-0.9+ (linux fc2). has another bug been filed on that yet?
*** Bug 276950 has been marked as a duplicate of this bug. ***
Status: RESOLVED → VERIFIED
QA Contact: bugzilla → menus
You need to log in before you can comment on or make changes to this bug.