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

RESOLVED FIXED in mozilla1.8.1

Status

()

defect
P1
major
RESOLVED FIXED
14 years ago
4 months ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({fixed1.8.1, intl, jp-critical})

Trunk
mozilla1.8.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 15 obsolete attachments)

2.20 KB, patch
masayuki
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
7.81 KB, patch
Details | Diff | Splinter Review
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.
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Posted patch Patch rv1.1 for layout/xul (obsolete) — Splinter Review
Attachment #209112 - Attachment is obsolete: true
Attachment #209113 - Flags: superreview?(bzbarsky)
Attachment #209113 - Flags: review?(bzbarsky)
Attachment #209112 - Flags: superreview?(bzbarsky)
Attachment #209112 - Flags: review?(bzbarsky)
Attachment #209116 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #209116 - Flags: superreview?
Attachment #209116 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #209116 - Flags: review?
These patches are suppress to append accesskey label if the label is "foo(X)" or "foo(X):".
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?
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.
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?
Put another way, why is the "intl.menuitems.alwaysappendaccesskeys" preference needed at all?  Why would one set it to true?
Wouldn't it be simpler to fix common dialogs to remove the /\(.\)$/ ?
(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 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.
Attachment #209113 - Flags: superreview?(bzbarsky)
Attachment #209113 - Flags: superreview+
Attachment #209113 - Flags: review?(bzbarsky)
Attachment #209113 - Flags: review+
Comment on attachment 209180 [details] [diff] [review]
Patch rv1.2 for layout/xul

This patch is checked-in to Trunk.
Attachment #209180 - Flags: approval1.8.1?
(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
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.
Whiteboard: [needs review mconnor][needs review neil]
Attachment #209115 - Flags: review?(mconnor) → review+
Whiteboard: [needs review mconnor][needs review neil] → [needs review neil]
I wait to check-in the toolkit patch until neil's XPFE code review.
Because we should use same code in both components.
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.
(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 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;
Attachment #209116 - Flags: superreview?(neil)
Attachment #209116 - Flags: superreview-
Attachment #209116 - Flags: review?(neil)
Attachment #209116 - Flags: review-
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.
What was the result of the discussion with Neil?  It sounded to me like he was against the C++ changes..
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.
Attachment #210197 - Flags: superreview?(neil)
Attachment #210197 - Attachment description: Handle & encoding in commonDialog.js → Handle (&X) encoding in commonDialog.js
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.
Asai-san:

Does this problem happen only on 'commonDialogs.properties'?
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?
(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.
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.
Attachment #209180 - Flags: approval1.8.1?
(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);
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.
Whiteboard: [needs review neil] → [needs another patch]
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.
Thank you jag. Your patch works fine. Sorry for the delay.
Attachment #210108 - Attachment is obsolete: true
Attachment #210197 - Attachment is obsolete: true
Attachment #210892 - Flags: superreview?(neil)
Attachment #210892 - Flags: review?(neil)
Attachment #210108 - Flags: superreview?(neil)
Attachment #210108 - Flags: review?(neil)
Attachment #210108 - Flags: branch-1.8.1?(neil)
Attachment #210197 - Flags: superreview?(neil)
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 //////////////
Attachment #210892 - Flags: review?(neil) → review?(masayuki)
Attachment #210893 - Flags: superreview?(neil)
Attachment #210893 - Flags: review?(masayuki)
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.
Attachment #210894 - Flags: review+
Attachment #210892 - Flags: superreview?(neil)
Attachment #210892 - Flags: superreview+
Attachment #210892 - Flags: branch-1.8.1?(neil)
Attachment #210892 - Flags: branch-1.8.1+
r=me, sr+branch181=neil
Attachment #210892 - Attachment is obsolete: true
Attachment #211140 - Flags: superreview+
Attachment #211140 - Flags: review+
Attachment #211140 - Flags: branch-1.8.1+
Attachment #210892 - Flags: review?(masayuki)
r=me.
Attachment #210893 - Attachment is obsolete: true
Attachment #211141 - Flags: superreview?(mconnor)
Attachment #211141 - Flags: review+
Attachment #210893 - Flags: superreview?(neil)
Attachment #210893 - Flags: review?(masayuki)
r=jag
Attachment #210894 - Attachment is obsolete: true
Attachment #211142 - Flags: superreview?(bzbarsky)
Attachment #211142 - Flags: review+
Attachment #211142 - Flags: branch-1.8.1?(bzbarsky)
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.)
Attachment #211142 - Flags: branch-1.8.1?(bzbarsky)
Whiteboard: [needs another patch] → [needs review mconnor][needs review bzbarsky]
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.
(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.
Attachment #211142 - Flags: superreview?(bzbarsky) → superreview+
Whiteboard: [needs review mconnor][needs review bzbarsky] → [needs review mconnor]
Attachment #211141 - Flags: superreview?(mconnor)
Attachment #211141 - Flags: superreview+
Attachment #211141 - Flags: branch-1.8.1?(mconnor)
Attachment #211141 - Flags: branch-1.8.1+
checked-in to Trunk, I'll check-in to 1.8 branch too if we don't have any regression reports.

Thank you, everyone.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: blocking1.9a1?
Resolution: --- → FIXED
Whiteboard: [needs review mconnor] → [pending to check-in to 1.8 branch]
checked-in to 1.8 branch.(only attachment 211376 [details] [diff] [review])
Keywords: fixed1.8.1
Whiteboard: [pending to check-in to 1.8 branch]
Component: Keyboard: Navigation → User events and focus handling
You need to log in before you can comment on or make changes to this bug.