Rack Reporter
	Description • 22 years ago

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b) Gecko/20021022 Phoenix/0.3
Build Identifier: Moz 1.1

the menu which is pulled up by right-clicking a bookmark does not have it's
hotkeys underlined.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

	Chris Lyon
	Comment 1 • 22 years ago

->XP Menus

	Chris Lyon
	Comment 2 • 22 years ago

*** Bug 177844 has been marked as a duplicate of this bug. ***

	Chris Lyon
	Comment 3 • 22 years ago

Confirming
OS->All
Severity->Normal

	piers
	Comment 4 • 22 years ago

*** Bug 140919 has been marked as a duplicate of this bug. ***

	Joe Infla
	Comment 5 • 21 years ago

*** Bug 200362 has been marked as a duplicate of this bug. ***

	Prognathous
	Comment 6 • 21 years ago

I just wasted half an hour searching for this bug. The new summary should make
it easier to find.

Old summary:
"right-click menu in bookmark manager doesn't have underlined accesskeys"

New summary:
"right-click menu in bookmark manager doesn't have underlined accesskeys (no
mnemonics in personal toolbar)"

Prog.

	Jeremy M. Dolan
	Comment 7 • 20 years ago

Just spent a great deal of time applying my very limited knowledge towards fix
this, but I'm going to need some help. It seems that the context menus for
clicking on a bookmark or folder are dynamically created with JS, so it's not
the usual simple DTD entity changes to configure access keys. :(

There is in fact, one access key already set. "O" for "Open in Tabs". However,
as you can see, the O isn't underlined (but it does work, as I found out rather
catastrophically). This is how the menu item is created:

  element = document.createElementNS(XUL_NS, "menuitem");
  element.setAttribute("class", "openintabs-menuitem");
  element.setAttribute("label",
bookmarksUtils.getLocaleString("cmd_bm_openfolder"));
  element.setAttribute("accesskey",
BookmarksUtils.getLocaleString("cmd_bm_openfolder_accesskey"));
  aTarget.appendChild(element);

Is there any way while creating menus like this, to have a letter underlined? Or
should the XUL rendering already be doing that?

	Prognathous
	Comment 8 • 20 years ago

Asaf, please see the last comment:
http://bugzilla.mozilla.org/show_bug.cgi?id=176359#c7

Prog.

	Asaf Romano (gone)
	Comment 9 • 20 years ago

Prog, the patch in that bug fixes this issue too.

	Asaf Romano (gone)
	Comment 10 • 20 years ago

Marking duplicate, this is the same menu (this is why it is fixed with-the-patch...)

*** This bug has been marked as a duplicate of 245984 ***

	Asaf Romano (gone)
	Comment 11 • 20 years ago

Ph! it is for the browser, sorry, REOPENED

	Asaf Romano (gone)
	Comment 12 • 20 years ago

assigning to "nobody" for now, nut i will look at this next week.

	Asaf Romano (gone)
	Comment 13 • 20 years ago

Taking this anyway

	Vlado Valastiak [:wladow] Assignee
	Comment 14 • 16 years ago

This adds accesskeys to BM context menus. Easy-to-review I guess ;)

	Vlado Valastiak [:wladow] Assignee
	Comment 15 • 16 years ago

Oups, uploaded wrong patch. This is a correct patch, previous has accesskey conflicts.

	Vlado Valastiak [:wladow] Assignee
	Comment 16 • 16 years ago

I need some sleep, really :) Sorry guys for spam.

	neil@parkwaycc.co.uk
	Comment 17 • 16 years ago

CC'ing KaiRo regarding the l10n impact - should all the existing properties be suffixed with ".label" to match, and assuming the last to labels are unused should they be removed, or should they be given accesskeys just in case?

	neil@parkwaycc.co.uk
	Comment 18 • 16 years ago

Comment on attachment 320993 [details] [diff] [review]
v1

Looking at the code, you're handling the access key as a local while the display name is a parameter. I think it would make sense to convert the display name to be a local too. You could probably get rid of getCommandName completely.

>     var cmd = "cmd_" + aCommandName.substring(NC_NS_CMD.length);
>+    var accesskey = BookmarksUtils.getLocaleString("cmd_" + aCommandName.substring(NC_NS_CMD.length)+".accesskey");
This is the same as (cmd + ".accesskey") isn't it? I wouldn't bother getting the accesskey until just before you set the accesskey attribute.
>     xulElement.setAttribute("command", cmd);

>       aDisplayName =  shouldCollapse? BookmarksUtils.getLocaleString("cmd_bm_collapsefolder") : aDisplayName;
>+      accesskey =  shouldCollapse? BookmarksUtils.getLocaleString("cmd_bm_collapsefolder.accesskey") : accesskey;
You could then change this to
if (shouldCollapse)
  cmd = "cmd_bm_collapsefolder";
Then later on you would fetch the correct label and accesskey.

	Vlado Valastiak [:wladow] Assignee
	Comment 19 • 16 years ago

Updated patch according to neil's comments:
1) get menuitem label locally -> get rid of getCommandName
2) add suffix ".label" for used l10n properties
3) remove unused l10n properties 

If I read the code correctly, unused l10n properties are:
cmd_bm_find
cmd_bm_selectAll
cmd_bm_renamebookmark2
cmd_bm_newseparator
cmd_bm_setnewbookmarkfolder
cmd_bm_setpersonaltoolbarfolder
cmd_bm_setnewsearchfolder

Robert, pls take a look at this, if I am right.

	neil@parkwaycc.co.uk
	Comment 20 • 16 years ago

Comment on attachment 321063 [details] [diff] [review]
v2

>-      aDisplayName =  shouldCollapse? BookmarksUtils.getLocaleString("cmd_bm_collapsefolder") : aDisplayName;
>+        if (shouldCollapse)
>+          cmd = "cmd_bm_collapsefolder";
Indentation is wrong here, the "if" should match the removed line.

>+   
Spurious extra blank line.

sr=me with those fixed.

	Robert Kaiser
	Comment 21 • 16 years ago

Comment on attachment 321063 [details] [diff] [review]
v2

If you and Neil couldn't find the deleted strings being used, I'm happy with this approach.

	Frank Wein [:mcsmurf]
	Comment 23 • 16 years ago

Checking in suite/common/bookmarks/bookmarks.js;
/cvsroot/mozilla/suite/common/bookmarks/bookmarks.js,v  <--  bookmarks.js
new revision: 1.144; previous revision: 1.143
done
Checking in suite/locales/en-US/chrome/common/bookmarks/bookmarks.properties;
/cvsroot/mozilla/suite/locales/en-US/chrome/common/bookmarks/bookmarks.properties,v  <--  bookmarks.properties
new revision: 1.12; previous revision: 1.11
done

	Stefan [:stefanh]
	Comment 24 • 16 years ago

(In reply to comment #23)
> Checking in suite/common/bookmarks/bookmarks.js;
> /cvsroot/mozilla/suite/common/bookmarks/bookmarks.js,v  <--  bookmarks.js
> new revision: 1.144; previous revision: 1.143
> done
> Checking in suite/locales/en-US/chrome/common/bookmarks/bookmarks.properties;
> /cvsroot/mozilla/suite/locales/en-US/chrome/common/bookmarks/bookmarks.properties,v
>  <--  bookmarks.properties
> new revision: 1.12; previous revision: 1.11
> done

You forgot to credit Vlado in the check-in comment.

	Frank Wein [:mcsmurf]
	Comment 25 • 16 years ago

Ooh my bad :|.
Vlado: Do you want credits in the CVS log? I can check in a dummy change so your name will appear there.

	Vlado Valastiak [:wladow] Assignee
	Comment 26 • 16 years ago

No, it's OK for now.