Closed
Bug 176359
Opened 22 years ago
Closed 17 years ago
right-click menu in bookmark manager doesn't have underlined accesskeys (no mnemonics in personal toolbar)
Categories
(SeaMonkey :: Bookmarks & History, defect, P2)
SeaMonkey
Bookmarks & History
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.0a1
People
(Reporter: rack1, Assigned: wladow)
References
(Blocks 1 open bug)
Details
(Keywords: access, polish)
Attachments
(3 files, 2 obsolete files)
3.15 KB,
patch
|
neil
:
superreview-
|
Details | Diff | Splinter Review |
6.09 KB,
patch
|
neil
:
review+
kairo
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
6.10 KB,
patch
|
Details | Diff | Splinter Review |
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.
Comment 1•22 years ago
|
||
->XP Menus
Assignee: ben → hyatt
Component: Bookmarks → XP Toolkit/Widgets: Menus
Keywords: access
QA Contact: claudius → shrir
Comment 3•22 years ago
|
||
Confirming OS->All Severity->Normal
Severity: trivial → normal
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
Summary: right-click menu in bookmark manager doesn't have underlined hotkeys → right-click menu in bookmark manager doesn't have underlined accesskeys
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.
Summary: right-click menu in bookmark manager doesn't have underlined accesskeys → right-click menu in bookmark manager doesn't have underlined accesskeys (no mnemonics in personal toolbar)
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?
Comment 8•20 years ago
|
||
Asaf, please see the last comment: http://bugzilla.mozilla.org/show_bug.cgi?id=176359#c7 Prog.
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 ***
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → DUPLICATE
Comment 11•20 years ago
|
||
Ph! it is for the browser, sorry, REOPENED
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Comment 12•20 years ago
|
||
assigning to "nobody" for now, nut i will look at this next week.
Assignee: hyatt → nobody
Status: REOPENED → NEW
Updated•20 years ago
|
Component: XP Toolkit/Widgets: Menus → Bookmarks
Priority: -- → P2
QA Contact: shrir → prognathous
Target Milestone: --- → mozilla1.8beta
Updated•20 years ago
|
Product: Browser → Seamonkey
Updated•20 years ago
|
Target Milestone: mozilla1.8beta1 → mozilla1.8beta2
Updated•20 years ago
|
Target Milestone: mozilla1.8beta2 → ---
Updated•20 years ago
|
Target Milestone: --- → Seamonkey1.0beta
Updated•18 years ago
|
Assignee: mano → nobody
Assignee | ||
Comment 14•17 years ago
|
||
This adds accesskeys to BM context menus. Easy-to-review I guess ;)
Assignee: nobody → wladow
Status: NEW → ASSIGNED
Attachment #320990 -
Flags: superreview?(neil)
Attachment #320990 -
Flags: review?(neil)
Assignee | ||
Updated•17 years ago
|
Target Milestone: seamonkey1.0beta → seamonkey2.0alpha
Assignee | ||
Comment 15•17 years ago
|
||
Oups, uploaded wrong patch. This is a correct patch, previous has accesskey conflicts.
Attachment #320990 -
Attachment is obsolete: true
Attachment #320992 -
Flags: superreview?(neil)
Attachment #320992 -
Flags: review?(neil)
Attachment #320990 -
Flags: superreview?(neil)
Attachment #320990 -
Flags: review?(neil)
Assignee | ||
Comment 16•17 years ago
|
||
I need some sleep, really :) Sorry guys for spam.
Attachment #320992 -
Attachment is obsolete: true
Attachment #320993 -
Flags: superreview?(neil)
Attachment #320993 -
Flags: review?(neil)
Attachment #320992 -
Flags: superreview?(neil)
Attachment #320992 -
Flags: review?(neil)
Comment 17•17 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?
Comment 18•17 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.
Attachment #320993 -
Flags: superreview?(neil)
Attachment #320993 -
Flags: superreview-
Attachment #320993 -
Flags: review?(neil)
Assignee | ||
Comment 19•17 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.
Attachment #321063 -
Flags: superreview?(neil)
Attachment #321063 -
Flags: review?(neil)
Assignee | ||
Updated•17 years ago
|
Attachment #321063 -
Flags: review?(kairo)
Comment 20•17 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.
Attachment #321063 -
Flags: superreview?(neil)
Attachment #321063 -
Flags: superreview+
Attachment #321063 -
Flags: review?(neil)
Attachment #321063 -
Flags: review+
Comment 21•17 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.
Attachment #321063 -
Flags: review?(kairo) → review+
Assignee | ||
Comment 22•17 years ago
|
||
Assignee | ||
Updated•17 years ago
|
Blocks: accesskey
Keywords: checkin-needed
Comment 23•17 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
Status: ASSIGNED → RESOLVED
Closed: 20 years ago → 17 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Comment 24•17 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.
Comment 25•17 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•