324159 - The accesskey label is doubled if the label ends (X)

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P1)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

On Japanese localized build, the button label of message box are broken.
Because JA build using "intl.menuitems.alwaysappendaccesskeys" is "true".
See following screenshot.
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2845&action=view

# Note that released JA build doesn't have this problem. Because the JA build's
# these buttons doesn't have accesskey. So, this problem makes serious 
# accessibility bug.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0 for layout/xul

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.1 for layout/xul

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0 for toolkit/content

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0 for xpfe/global/resources/content/bindings

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

These patches are suppress to append accesskey label if the label is "foo(X)" or "foo(X):".

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: testcase(download and install to chrome URI)

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Screenshot with these patches

Comment 8

[Boris Zbarsky [:bzbarsky]]

I don't understand.

If the build is using "intl.menuitems.alwaysappendaccesskeys", then why is it including the access key in the label?  Isn't the whole point of that preference that if you use it you do NOT need to put the string in the label?

Note that I'm not that familiar with the code you're changing.  But it seems to me like we're hacking the C++ to work around people using bad labels.  Or is there something else going on?

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

E.g., on commonDialogs.properties, the accesskey is defined by '&' in the caption.
http://lxr.mozilla.org/mozilla/source/toolkit/locales/en-US/chrome/global/commonDialogs.properties
http://lxr.mozilla.org/mozilla/source/xpfe/global/resources/locale/en-US/commonDialogs.properties

We hope that this should be following on JA build.
Yes	= \u306f\u3044(&Y)
No	= \u3044\u3044\u3048(&N)

So, the caption need accesskey in the label for accesskey definition. But "intl.menuitems.alwaysappendaccesskeys" is appending the accesskey label in this case too. We need to suppress this behavior.

Comment 10

[Boris Zbarsky [:bzbarsky]]

So the problem is that we don't have a consistent way of defining access keys?  In some cases defining them requires putting them in the string and in other cases it does not?  And in the latter cases you want to use "intl.menuitems.alwaysappendaccesskeys" to handle it?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Put another way, why is the "intl.menuitems.alwaysappendaccesskeys" preference needed at all?  Why would one set it to true?

Comment 12

[[e-mail]]

Wouldn't it be simpler to fix common dialogs to remove the /\(.\)$/ ?

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #10)
> So the problem is that we don't have a consistent way of defining access keys?
> In some cases defining them requires putting them in the string and in other
> cases it does not? And in the latter cases you want to use
> "intl.menuitems.alwaysappendaccesskeys" to handle it?

Yes.

(In reply to comment #11)
> Put another way, why is the "intl.menuitems.alwaysappendaccesskeys" preference
> needed at all?  Why would one set it to true?

I don't know why this way is implemented. But I'm using latest nightly builds, so the locale is en-US. For me (maybe, for many Japanese people), the inline style is not useful. Because the Windows applications are always append access key label in JP. So, even if the caption doesn't have non-ASCII characters. Therefore, I changed this pref on nightly builds. This pref makes happy me :-)

(In reply to comment #12)
> Wouldn't it be simpler to fix common dialogs to remove the /\(.\)$/ ?

No, See my comment 9. For JA localization, the caption doesn't have the access key character. So, we need "(X)" in its caption, but we don't need to append access key label in this case.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 209113 [details] [diff] [review]
Patch rv1.1 for layout/xul

>Index: layout/xul/base/src/nsTextBoxFrame.cpp
> nsTextBoxFrame::UpdateAccessTitle()

>+    // 3 is accessKeyLabel length.

Why?  I see no reason that should be true in general.  Please use the actual length, ok?

r+sr=bzbarsky with that fixed.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.2 for layout/xul

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 209180 [details] [diff] [review]
Patch rv1.2 for layout/xul

This patch is checked-in to Trunk.

Comment 17

[jag (Peter Annema)]

(In reply to comment #13)
> (In reply to comment #12)
> > Wouldn't it be simpler to fix common dialogs to remove the /\(.\)$/ ?
> 
> No, See my comment 9. For JA localization, the caption doesn't have the access
> key character. So, we need "(X)" in its caption, but we don't need to append
> access key label in this case.

What I think he meant was instead of putting this code in nsTextBoxFrame.cpp where everyone pays the cost of doing that extra test, change the code in function setLabelForNode() to remove the (X) or (X): at the end when "intl.menuitems.alwaysappendaccesskeys" is true.

http://lxr.mozilla.org/seamonkey/source/toolkit/content/commonDialog.js#70
http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/commonDialog.js#70

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you jag.
But I don't think that the way is better approach than mine. Because the all related code of appending access key label process is implemented in label element implementation. I think we should keep this structure for not to make these codes dispersion.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I wait to check-in the toolkit patch until neil's XPFE code review.
Because we should use same code in both components.

Comment 20

[[e-mail]]

Sorry, I still don't understand this. Why are localizers putting (X) in their labels anyway? It looks like you're going along with their hack. Common dialogs should really have a way of specifying an access key that's not in the label.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #20)
> Sorry, I still don't understand this. Why are localizers putting (X) in their
> labels anyway? It looks like you're going along with their hack. Common dialogs
> should really have a way of specifying an access key that's not in the label.

Yes, you are right. But why current implementation is using label for to specify accesskeys? And why common dialogs are not using dtd file instead of properties file? It is common sense that the label doesn't have accesskeys in non-Latin area. (This is a mechanism for localization, if the original author of this mechanism didn't know this, we are very unhappy.)

However, Windows application developers are using this way (putting (X), manually). So, I think that this way is natural for many localizers. Therefore, I think that localizers putting (X) is not 'hack'.

Don't localizers get confused if the spec is changed under current situation?

I think that the mechanism of auto accesskey rendering (underlined in its label or appended to its tail) is very epoch-making. But the mechanism has a problem. The localizers cannot specify the accesskey which should be underlined if the label has 2 or more accesskey characters. Therefore, I think that the localizers must create  the same caption as a system. In order to this, the current mechanism (putting (X), manually) is better than auto accesskey rendering mechanism.

Comment 22

[[e-mail]]

Comment on attachment 209116 [details] [diff] [review]
Patch rv1.0 for xpfe/global/resources/content/bindings

>+              var label = "(" + accessKey.toUpperCase() + ")";
>+              if (/:$/.test(labelText) &&
>+                  labelText.substr(labelText.length - 4, 3) == label) {
>+                accessKeyIndex = labelText.length - 3;
>+              } else if (labelText.substr(labelText.length - 3, 3) == label) {
>+                accessKeyIndex = labelText.length - 2;
>+              }
* You don't uppercase the labelText; all the other routines allow mismatched case.
* You should use labelText.slice(-4, -1) or labelText.slice(-3) rather than substring and length.
* Those tests look cumbersome. Please try to simplify them or consider using a regexp e.g.
if (/\((.)\):?$/.test(labelText) && RegExp.$1.toUpperCase == accessKey.toUpperCase()) accessKeyIndex = RegExp.leftContext.length + 1;

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Neil, Thank you for your review. But I have an opinion.

(In reply to comment #22)
> * You don't uppercase the labelText; all the other routines allow mismatched
> case.

I cannot agree this. Because when appending the accesskey label, the label is always uppercased. See following code:

http://lxr.mozilla.org/mozilla/source/xpfe/global/resources/content/bindings/text.xml#162
> 162               span.textContent = accessKey.toUpperCase();
> 163               afterLabel.appendChild(span);

On a common rule, the accesskey label should be uppercase when it is appended to tail of the label with parentheses.
If the label ends "(s)" like Firefox's option dialog, we should not use this for accesskey because this is not appended accesskey label.
# Fx's option dialog has "Location(s)" label.

# I'll recreate the patch tomorrow, thanks.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.1 for xpfe/global/resources/content/bindings

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.1 for toolkit/content

Comment 26

[Boris Zbarsky [:bzbarsky]]

What was the result of the discussion with Neil?  It sounded to me like he was against the C++ changes..

Comment 27

[jag (Peter Annema)]

Attachment: Handle (&X) encoding in commonDialog.js

This is what I had in mind. The code that handles underlining accesskeys, and appending "(X)" if needed is currently (by necessity) in two places: nsTextBoxFrame.cpp and text.xml. They both use a label string and a specified accessKey to determine which key to, if necessary, append, and underline.

Instead of making the code in both those places aware of when an access key has already been appended (which only needs to be done for code that uses commonDialog.{xul,js}, through the nsIPromptService), I'd rather we deal with it in commonDialog.js, which already deals with determining access keys embedded in the labels its given.

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 210197 [details] [diff] [review]
Handle (&X) encoding in commonDialog.js

jag:

Do we have only here to specify the access key by '&'?

And if we use your patch, you should not change nsTextBoxFrame.cpp. We should back out attachment 209180 [details] [diff] [review].

And we need the patch for toolkit.

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Asai-san:

Does this problem happen only on 'commonDialogs.properties'?

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I have one concern for jag's patch.
The patch has rendering logic in its RegExp.

> / *\(\&([^&])\)(:)?$/.test(aLabel)
                 ^^^^
When we change the underlining spec in future, we need to change this code too. Don't the harckers and the reviewers forget this change?

Comment 31

[[e-mail]]

(In reply to comment #30)
>I have one concern for jag's patch.
>The patch has rendering logic in its RegExp.
Would you prefer a special format such as \u306f\u3044Y& \u306f\u3044&Y& or &Y&\u306f\u3044 to indicate that the letter Y is an access key and not part of the label? That way there would be no confusion with the desired rendering.

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Neil:

I think that if we use jag's patch, we should add notification comment in nsTextBoxFrame.cpp and text.xml(both xpfe and toolkit). Because we have experienced that the appending access key label code is changed only on nsTextBoxFrame.cpp(See bug 298712, before that bug was fixed, the "intl.menuitems.alwaysappendaccesskeys" mechanism was not implemented on text.xml).

We have three access key rendering code already. And jag's patch will add two codes...

My concern is not the jag's patch's function. My concern is duplication of the same logic code.

Comment 33

[dynamis (Tomoya ASAI)]

(In reply to comment #29)
> Asai-san:
> 
> Does this problem happen only on 'commonDialogs.properties'?

I checked whole locale files for 1.5 and found some other files using &Y style:

http://lxr.mozilla.org/mozilla1.8.0/source/browser/locales/en-US/chrome/browser/shellservice.properties
  optionsLabel=%S &Options
  safeModeLabel=%S &Safe Mode
http://lxr.mozilla.org/mozilla1.8.0/source/dom/locales/en-US/chrome/printdialog.properties
  Aslaid=As &laid out on the screen
  selectedframe=The selected &frame
  Eachframe=&Each frame separately
http://lxr.mozilla.org/mozilla1.8.0/source/mail/locales/en-US/chrome/global-platform/win/nsWindowsHooks.properties
  prefsLabel=Pr&eferences
http://lxr.mozilla.org/mozilla1.8.0/source/mail/locales/en-US/chrome/messenger/mailTurboMenu.properties
  MailNews=&Mail && Newsgroups
  Addressbook=&Address Book
http://lxr.mozilla.org/mozilla1.8.0/source/toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties
  rememberButtonText = &Remember
  notNowButtonText = &Not Now
  neverForSiteButtonText = Ne&ver for This Site
http://lxr.mozilla.org/mozilla1.8.0/source/toolkit/locales/en-US/chrome/global/commonDialogs.properties
  No=&No
  Save=&Save
  Revert=&Revert
  DontSave=&Don't Save

# grep '&', exclude below and checked remainder
# &(#[0-9]+|amp|lt|gt|quot|nbsp|copy|trade|reg|mdash|Brand(Short|Full)Name
#  |vendorURL|vendorShortName|sharedLongDesc|pref\.[^;]+
#  |(accel|alt|ctrl|enter|shift)Key|images.baseURL);

Comment 34

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment 33)
http://lxr.mozilla.org/mozilla1.8.0/source/browser/locales/en-US/chrome/browser/shellservice.properties
These are registry key values, the "&" is the way windows determines which accesskey to choose for context menu options.

http://lxr.mozilla.org/mozilla1.8.0/source/dom/locales/en-US/chrome/printdialog.properties
These are passed to native windows dialogs.

http://lxr.mozilla.org/mozilla1.8.0/source/mail/locales/en-US/chrome/global-platform/win/nsWindowsHooks.properties
Windows registry, again.

http://lxr.mozilla.org/mozilla1.8.0/source/mail/locales/en-US/chrome/messenger/mailTurboMenu.properties
Windows menus, again.

http://lxr.mozilla.org/mozilla1.8.0/source/toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties
http://lxr.mozilla.org/mozilla1.8.0/source/toolkit/locales/en-US/chrome/global/commonDialogs.properties
These are passed to the prompt service.

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you, Asai-san and Gavin.

I'll test the jag's patch and if it doesn't have problem, I'll create new patch tomorrow or next day.

Comment 36

[jag (Peter Annema)]

Masayuki-san: I agree, we should put comments in all three files pointing to the other two. I'm sorry, I should have done that with my patch. I will add those comments along with any other changes I may need to make. You don't have to spend time on updating my patch when you could be working on other things.

I don't think we should back out attachment 209180 [details] [diff] [review]. I think you made the code easier to understand, and since nothing has changed functionally except for the part I'm undoing in my patch, I'd like to keep the rest of your changes.

> We have three access key rendering code already. And jag's patch will add two
> codes...

I'm not sure how I'm adding two places that deal with rendering code. I'm adding knowledge of '(' '&' accesskey ')' and how that interacts with ':' at the end of a line to commonDialog.js. Since (in this case) we get to make up the rules of what should be in commonDialogs.properties we could just say that the string "(&X)" at the end of the label gets treated as an access key which doesn't appear in the text itself. That way the matching logic can be changed to:

/( *)\(\&([^&])\)$/.test(aLabel)

and of course we won't need to append RegExp.$2 anymore. This does mean that if a label includes a ':' it'll look like "Something:(&K)" in commonDialogs.properties, and it will render like "Something (K):" (with K underlined and the space only if the pref is set, of course). That way we get rid of knowledge about how to deal with ':' in commonDialog.js.

I would like to point out that I don't consider the '(' '&' accesskey ')' in commonDialogs.properties part of the rendered label, but rather an encoding scheme. If at some point in the future we allow '(' and ')' in the rendered label to be configurable/localizable so it could show up as "Something [K]:" or "Something <K>:" I would still expect commonDialog.properties to have "Something (&K):" (or "Something:(&K)" if we go with that suggested change).

> My concern is not the jag's patch's function. My concern is duplication of the
> same logic code.

I would love to reduce the duplication if possible. I will look into this, see what we can do, besides adding those comments to make sure future updates are made in all three files. However, as far as I can tell, and comment 34 seems to confirm this, the only file that needs to know about '&X' and "(&X)" as an accesskey encoding scheme is commonDialog.js and I don't think we should add the cost of handling that encoding to code that won't need to deal with it in the other 99.9% of the cases.

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.0 for xpfe/global/resources/content

Thank you jag. Your patch works fine. Sorry for the delay.

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.0 for toolkit/content

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch 2.0 for layout/xul/layout/base/src

Comment 40

[jag (Peter Annema)]

Comment on attachment 210892 [details] [diff] [review]
Patch rv2.0 for xpfe/global/resources/content

Masayuki, since you're familiar with the code, and you've already tested this, would you mind reviewing this patch and the one for toolkit?

As for the comments you added, I think we should add a comment to commonDialog.js to also point to nsTextBoxFrame::UpdateAccessTitle, and in text.xml, please remove the two lines that are all //////////////

Comment 41

[jag (Peter Annema)]

Comment on attachment 210894 [details] [diff] [review]
Patch 2.0 for layout/xul/layout/base/src

r=jag

I like the comment. I see you didn't change NS_LITERAL_STRING(" ") to just ' '. Personally I would make that change 'coz it's a little faster.

Comment 42

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.1 for xpfe/global/resources/content

r=me, sr+branch181=neil

Comment 43

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.1 for toolkit/content

r=me.

Comment 44

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.1 for layout/xul/base/src

r=jag

Comment 45

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 211142 [details] [diff] [review]
Patch rv2.1 for layout/xul/base/src

Oops. Sorry. We don't need this patch for 1.8 branch. (Because the previous patch didn't go to 1.8 branch.)

Comment 46

[[e-mail]]

Comment on attachment 211140 [details] [diff] [review]
Patch rv2.1 for xpfe/global/resources/content

>+  // Note that if you change following code, see the comment of
>+  // nsTextBoxFrame::UpdateAccessTitle.
After all that, we overlooked a grammar nit: "the" following code.

Comment 47

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #46)
> (From update of attachment 211140 [details] [diff] [review] [edit])
> >+  // Note that if you change following code, see the comment of
> >+  // nsTextBoxFrame::UpdateAccessTitle.
> After all that, we overlooked a grammar nit: "the" following code.
> 

Thanks, I'll change it at next patch or before checking-in.

Comment 48

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.2 xpfe/global/resources/content toolkit/content

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in to Trunk, I'll check-in to 1.8 branch too if we don't have any regression reports.

Thank you, everyone.

Comment 50

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in to 1.8 branch.(only attachment 211376 [details] [diff] [review])