There is no way to specify non-latin style accesskey. This issue is same as Bug 324159, so this can be fixed by porting toolkit part of Bug 324159 to ChatZilla. Steps to Reproduce: Try to localize ChatZilla to Japanese. Actual Results: There is no way to specify Japanese-style accesskey correctly. Expected Results: Can localize correctly to Japanese.

function getLabel (str) { if (/ *\(\&([^&])\)(:|\u2026|\.\.\.)?$/.test(str)) return RegExp.leftContext + RegExp.$2; return str.replace("&", ""); } could be function getLabel (str) { match = / *\(\&(?:[^&])\)(:|\u2026|\.\.\.)?$/.exec(str) if (match) return match.input.substr(0,match.index) + match[1]; return str.replace("&", ""); } And suddenly is much less evil :)

So, because the bug reported hasn't made this remotely clear, I'm going to guess what the real issue here is: you can't specify an access key that isn't part of the label text. That regexp is hideous, too. I would much prefer to match and replace only / *\(\&([^&])\)/ and not have the special exceptions and the end anchor. The neatest code would be something like:

function getLabel(str) { if (/ *\(\&([^&])\)/.test(str)) return str.replace(/ *\(\&([^&])\)/, ""); return str.replace("&", ""); }

(In reply to comment #2) > That regexp is hideous, too. I would much prefer to match and replace only / > *\(\&([^&])\)/ and not have the special exceptions and the end anchor. Nakano-san, shouldn't we make this consistent with nsTextBoxFrame?

Comment on attachment 357472 [details] [diff] [review] Patch v1.0 r-, per comment #1 and #3

(In reply to comment #4) > (In reply to comment #2) > > That regexp is hideous, too. I would much prefer to match and replace only / > > *\(\&([^&])\)/ and not have the special exceptions and the end anchor. > > Nakano-san, shouldn't we make this consistent with nsTextBoxFrame? If so, we need to block 3.1 by this bug. But we already froze its features. If XUL members allow to change the label generation, I'll post the patch. Basically, Chatzilla is using wrong way in its l10n. Chatzilla should have both items per menu: an accesskey character and a label text. But Chatzilla specifies an accesskey with '&' in label text. Therefore, Chatzilla developers cannot specify accesskey without "(&X)" *hack* when the label doesn't have the accesskey character, e.g., at Japanese language. So, if I fixes this bug on nsTextBoxFrame, it's really new feature... (Yes, I also believe its risk should not be high.)

drivers: oops, see my previous comment for the blocking request reason. If we should fix this bug on XUL implementation level, we need to add a new feature :-(

I don't think you really want to port toolkit's hack with displaying labels containing accesskeys but, instead, just have an "accesskey" property that can be set independently of the "label". Is that right?

Yes. E.g., currently chatzilla has following resource: cmd.cmd-copy.label = &Copy This should be: cmd.cmd-copy.label = Copy cmd.cmd-copy.accesskey = c And current Japanese resource is: cmd.cmd-copy.label = コピー(&C) but this should be: cmd.cmd-copy.label = コピー cmd.cmd-copy.accesskey = c I think if very many add-ons and XUL apps using same way for l10n, we should improve the localizability in XUL level. But I think that most add-ons use .dtd instead of .properties. If so, we don't need to change the XUL specification.

First, James, Nakano-san, would this approach work for both of you? Then, I need someone else to verify that this patch actually works, because I am on Mac and can't actually see the access keys (they are not underlined, and the menus just use the first letter to navigate to them on Mac... go figure). Anyway, I don't get any errors, but I am also obviously not able to see if everything works as it should. Finally, I am not completely happy with the hack for the main menu labels, but was not sure how to make that nicer. I'll be happy to take suggestions.

(In reply to comment #7) > drivers: > > oops, see my previous comment for the blocking request reason. > > If we should fix this bug on XUL implementation level, we need to add a new > feature :-( I'm not clear what feature you're proposing is needed. Accesskeys are already supported through the accesskey attribute. The patch here seems to suggest that Chatzilla is using some Windows-like way of using & to delineate accesskeys instead. That's a bug in itself, but I don't understand what general XUL issue this is.

(In reply to comment #10) > Then, I need someone else to verify that this patch actually works, because I > am on Mac and can't actually see the access keys (they are not underlined, and > the menus just use the first letter to navigate to them on Mac... go figure). Is this a general Mac thing, or are accesskeys actually broken on mac?

Comment on attachment 357536 [details] [diff] [review] Patch to enable .accesskey string support Gijs: I have a few minor nits with the patch, but the approach and .properties changes are just what I wanted.

(In reply to comment #12) > (In reply to comment #10) > > Then, I need someone else to verify that this patch actually works, because I > > am on Mac and can't actually see the access keys (they are not underlined, and > > the menus just use the first letter to navigate to them on Mac... go figure). > > Is this a general Mac thing, or are accesskeys actually broken on mac? They kind of work on buttons, I *think*. But in menus, it just goes by first letter, afaict, and if you want the second item that starts with a C you have to press C twice. Also, there is no way to keyboard invoke a specific menu in one go, so those access keys are def. useless. For buttons, ctrl+key works, I believe. Haven't tested it extensively. (In reply to comment #13) > (From update of attachment 357536 [details] [diff] [review]) > Gijs: I have a few minor nits with the patch, but the approach and .properties > changes are just what I wanted. Excellent. Nakano-san?

(In reply to comment #12) > Is this a general Mac thing, or are accesskeys actually broken on mac? The Mac doesn't have the access key concept at all. Technically, chrome code on Mac shouldn't even support access keys. That said, Mozilla will handle pressing control+key (or whatever the preferences are set to use) as if it was an accesskey.

(In reply to comment #11) > (In reply to comment #7) > > drivers: > > > > oops, see my previous comment for the blocking request reason. > > > > If we should fix this bug on XUL implementation level, we need to add a new > > feature :-( > > I'm not clear what feature you're proposing is needed. Accesskeys are already > supported through the accesskey attribute. The patch here seems to suggest that > Chatzilla is using some Windows-like way of using & to delineate accesskeys > instead. That's a bug in itself, but I don't understand what general XUL issue > this is. Sorry. Currently, we are appending "(X)" to menu/button/label captions automatically when "intl.menuitems.alwaysappendaccesskeys" is true or the specified accesskey is not in the caption text. At that time, XUL implementation checks whether the caption ends with ellipsis. I.e., if the captions is "Open...", the generated caption will not be "Open...(O)", it will be "Open(O)...". However, chatzilla is using '&' for specifying accesskeys in the caption. E.g., "&Copy" means: the caption should be "Copy" and its accesskey is 'C'. And Japanese language doesn't contain alphabets in most captions, therefore, Chatzilla needs "コピー(&C)" in its resource. By the hack, the generated caption will be "コピー(C)(C)". So, the "(C)" is doubled. So, if nsTextBoxFrame checks whether the caption ends with "(C)" or not, this bug is fixed in XUL implementation level. And Sakai-san thinks XUL implementation should be so. But I think that basically, this is a bug of XUL apps and add-ons under current XUL specification. So, the bug should be fixed on them. However, *if* there are many applications which has such accesskey specifying way, we should change XUL specification for them. Of course, this is a feature change, therefore, I don't think we should change the XUL spec at this time. Fortunately, Gijs creates a patch which fixes this bug on chatzilla level. Therefore, I think we don't need to change the XUL spec now. Thank you, Gijs! # I'm building Seamonkey for testing your patch.

(In reply to comment #16) > Sorry. Currently, we are appending "(X)" to menu/button/label captions > automatically when "intl.menuitems.alwaysappendaccesskeys" is true or the > specified accesskey is not in the caption text. At that time, XUL > implementation checks whether the caption ends with ellipsis. I.e., if the > captions is "Open...", the generated caption will not be "Open...(O)", it will > be "Open(O)...". > > However, chatzilla is using '&' for specifying accesskeys in the caption. Which is a bug is Chatzilla and nothing to do with XUL. > However, *if* there are many applications which has such accesskey > specifying way, we should change XUL specification for them. No, those are bugs in the application. Accesskeys in XUL are done with the accesskey attribute. Access keys are not and have never been done with & and should not be supported.

First, the patch of Gijs is works fine for me. Excellent! But the IRC window is broken on my build, I'm not sure that is a bug of the patch or another bug or my build's issue. (In reply to comment #17) > (In reply to comment #16) > > Sorry. Currently, we are appending "(X)" to menu/button/label captions > > automatically when "intl.menuitems.alwaysappendaccesskeys" is true or the > > specified accesskey is not in the caption text. At that time, XUL > > implementation checks whether the caption ends with ellipsis. I.e., if the > > captions is "Open...", the generated caption will not be "Open...(O)", it will > > be "Open(O)...". > > > > However, chatzilla is using '&' for specifying accesskeys in the caption. > > Which is a bug is Chatzilla and nothing to do with XUL. Chatzilla sets '&' removed caption. I.e., at the example, Chatzilla sets "Open(O)..." to the menu. Therefore, XUL textbox generates "Open(O)(O)...". > > However, *if* there are many applications which has such accesskey > > specifying way, we should change XUL specification for them. > > No, those are bugs in the application. Accesskeys in XUL are done with the > accesskey attribute. Access keys are not and have never been done with & and > should not be supported. Yeah, your comment is really correct. I think so too. However, if very very many add-ons have same bug, XUL implementation change is best way for such add-on users. Because the evangelism may be very hard... I posted an entry for this bug to my blog, some add-on localizers may tell me whether such add-ons is rare or not. # I should document this bug in MDC for add-on developers...

(In reply to comment #18) > But the IRC window is broken on my build Probably bug 474193.

And note that I think my work for common dialog in bug 324159 used wrong approach. # https://bugzilla.mozilla.org/attachment.cgi?id=211376&action=diff The correct approach should be: * adding accesskey resource to commonDialog.properties > http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/commonDialogs.properties * commonDialog.js should not remove (X) automatically. I think that this my mistake leads the mistake of XUL developers. I should fix this in 3.2.

(In reply to comment #20) > The correct approach should be: > * adding accesskey resource to commonDialog.properties Unfortunately we're limited by the nsIPrompt* interfaces to approaches that encode the access key into the label.

(In reply to comment #21) > (In reply to comment #20) > > The correct approach should be: > > * adding accesskey resource to commonDialog.properties > Unfortunately we're limited by the nsIPrompt* interfaces to approaches that > encode the access key into the label. I don't think so. Because we are using nsIDialogParamBlock for setting button caption from nsPromptService. So, looks like we only need to add new params to nsIDialogParamBlock instance.

(In reply to comment #22) > (In reply to comment #21) > > (In reply to comment #20) > > > The correct approach should be: > > > * adding accesskey resource to commonDialog.properties > > Unfortunately we're limited by the nsIPrompt* interfaces to approaches that > > encode the access key into the label. > I don't think so. Because we are using nsIDialogParamBlock for setting button > caption from nsPromptService. Actually it's the methods exposed by nsPromptService that I was thinking of. In particular nsIPromptService is frozen and cannot be changed.

The patch works fine for me, too. Thanks a lot!! I've found that accesskey of menu element (ChatZilla, IRC, Edit, ...) is not displayed (not underlined, or (X) is not appended) on Windows or Linux. Though I guess this issue is backend's bug, not app bug. (In reply to comment #23) > > > > The correct approach should be: > > > > * adding accesskey resource to commonDialog.properties > > > Unfortunately we're limited by the nsIPrompt* interfaces to approaches that > > > encode the access key into the label. > > I don't think so. Because we are using nsIDialogParamBlock for setting button > > caption from nsPromptService. > Actually it's the methods exposed by nsPromptService that I was thinking of. > In particular nsIPromptService is frozen and cannot be changed. In other words, "adding accesskey resource to commonDialog.properties" can be fixed by not using nsIPromptService, but "commonDialog.js should not remove (X) automatically" is limited. Right?

(In reply to comment #24) > (In reply to comment #23) > > > > > The correct approach should be: > > > > > * adding accesskey resource to commonDialog.properties > > > > Unfortunately we're limited by the nsIPrompt* interfaces to approaches that > > > > encode the access key into the label. > > > I don't think so. Because we are using nsIDialogParamBlock for setting button > > > caption from nsPromptService. > > Actually it's the methods exposed by nsPromptService that I was thinking of. > > In particular nsIPromptService is frozen and cannot be changed. > > In other words, "adding accesskey resource to commonDialog.properties" can be > fixed by not using nsIPromptService, but "commonDialog.js should not remove (X) > automatically" is limited. > Right? But the actual interface between nsPromptService and commonDialog.js is nsPIPromptService that is not frozen... http://mxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/public/nsPIPromptService.idl

(In reply to comment #24) > I've found that accesskey of menu element (ChatZilla, IRC, Edit, ...) is not > displayed (not underlined, or (X) is not appended) on Windows or Linux. > Though I guess this issue is backend's bug, not app bug. This is strange... I cannot reproduce this bug on simple XUL file which changes accessKey property from JS code. I filed bug 474270 for this bug.

(In reply to comment #26) > But the actual interface between nsPromptService and commonDialog.js is > nsPIPromptService that is not frozen... So, I guess "adding accesskey resource to commonDialog.properties" can be fixed. But "should not remove (X) automatically" is limited; for example: var ps = Components.classes["@mozilla.org/embedcomp/prompt-service;1"] .getService(Components.interfaces.nsIPromptService); ps.confirmEx(null, null, "ここで設定することもできます。\n設定しますか？それともこのまま続行しますか？", ps.BUTTON_TITLE_IS_STRING * ps.BUTTON_POS_0 + ps.BUTTON_TITLE_IS_STRING * ps.BUTTON_POS_1, "設定(&O)...", "続行(&C)", null, null, {}); In this case, (X) should be removed automatically.

Ah, ok, it's reasonable...

I created a document in MDC, please check it: https://developer.mozilla.org/En/XUL_Tutorial/Accesskey_display_rules # the document is not linked from other documents now.

(In reply to comment #30) > I created a document in MDC, please check it: > https://developer.mozilla.org/En/XUL_Tutorial/Accesskey_display_rules Looks good, I'll take a look tomorrow and improve it (fix up English grammar for example)

Not blocking, would take a patch.

(In reply to comment #13) > (From update of attachment 357536 [details] [diff] [review]) > Gijs: I have a few minor nits with the patch, but the approach and .properties > changes are just what I wanted. Cool. I think this is what we want here, too. If there is some necessity for a toolkit change, that should be a separate bug. Could you review it? Thanks! :-)

Comment on attachment 357536 [details] [diff] [review] Patch to enable .accesskey string support >Index: mozilla/extensions/irc/js/lib/command-manager.js >@@ -235,19 +236,21 @@ function cmdmgr_defcmd (name, func, flag >- var command = new CommandRecord (name, func, usage, help, label, flags, >- keystr, tip, format); >+ var command = new CommandRecord (name, func, usage, help, label, accesskey, >+ flags, keystr, tip, format); Nit: remove the space before the open parenthesis. >Index: mozilla/extensions/irc/js/lib/menu-manager.js >@@ -529,13 +529,14 @@ function mmgr_menucmd(event) > * @param id ID of the sub-menu to add. >-function mmgr_addsmenu (parentNode, beforeNode, menuName, domId, label, attribs) >+function mmgr_addsmenu (parentNode, beforeNode, menuName, domId, label, >+ accesskey, attribs) Nit: remove the space before the open parenthesis. Docnit: add @param accesskey documentation (you can fix the id one to be domId too). >@@ -554,13 +555,13 @@ function mmgr_addsmenu (parentNode, befo >- menu.setAttribute ("accesskey", getAccessKey(label)); >+ menu.setAttribute ("accesskey", accesskey); Nit: remove the space before the open parenthesis. >@@ -640,13 +641,13 @@ function mmgr_addmenu (parentNode, befor >- menuitem.setAttribute ("accesskey", getAccessKey(command.label)); >+ menuitem.setAttribute ("accesskey", command.accesskey); Nit: remove the space before the open parenthesis. >@@ -775,14 +776,15 @@ function mmgr_newmenu (parentNode, befor > var menuSpec = this.menuSpecs[menuName]; > >- var subMenu = this.appendSubMenu (parentNode, beforeNode, menuName, domId, >- menuSpec.label, attribs); >+ var subMenu = this.appendSubMenu(parentNode, beforeNode, menuName, domId, >+ menuSpec.label, menuSpec.accesskey, >+ attribs); This needs to account for menuSpec not having an accesskey (plugins can add their own menus). My preferred solution is to, just after 'var menuSpec', set it to getAccessKey(menuSpec.label) if it doesn't exist (only check existence, not contents, or we'll clobber explicitly empty values). >@@ -827,8 +829,15 @@ function mmgr_newitems (parentNode, befo >+function getAccessKeyForMenu(labelString) >+{ >+ var rv = getAccessKey(window[labelString]); >+ if (!rv) >+ rv = window[labelString + "_ACCESSKEY"]; >+ return rv; > } So, this function is in /js/lib but relies on front-end stuff ('window'). Either use 'this' instead of 'window' or move the function into front-end code (it's only used in /xul/content/menus.js anyway) - the latter is preferred (messages.js or menus.js seem good candidates). It should have a comment in it to indicate that it's checking for an embedded access key first, and then falling back to a separate string. >Index: mozilla/extensions/irc/locales/en-US/chrome/chatzilla.properties >@@ -93,12 +93,24 @@ msg.confirm = Confirm >+# Note also that for every command, an accesskey may be specified: "Note also that, for every command, an accesskey may be specified:" >+# The following are therefore equivalent: >+# cmd.foo.label = &Foo >+# vs. "and" >Index: mozilla/extensions/irc/xul/content/menus.js >@@ -132,12 +132,13 @@ function initMenus() > client.menuSpecs["mainmenu:chatzilla"] = { > label: MSG_MNU_CHATZILLA, >+ accesskey: getAccessKeyForMenu('MSG_MNU_CHATZILLA'), These don't half look strange, but I can't think of a neater way to do them. :( r=silver with all that fixed

Neil Deakin: Please review the following document: https://developer.mozilla.org/En/XUL_Tutorial/Accesskey_display_rules # see comment 31, probably, you forgot it :-)

I incorporated this into the accesskey attribute page a while ago. https://developer.mozilla.org/En/XUL/Attribute/Accesskey

It would be good to have the page use a different example though, as OK and Cancel shouldn't have access keys.

Neil, thank you. Should I remove the page (Accesskey_display_rules)?