298712 - Space before parenthesis of accesskey should be removable or removed.

Status: RESOLVED FIXED

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

Description

[dynamis (Tomoya ASAI)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ja-JP; rv:1.7.8) Gecko/20050511
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; ja-JP; rv:1.7.8) Gecko/20050511

When an accesskey character is not in the label or when we set
intl.menuitems.alwaysappendaccesskeys=true
in the  intl.propertie file to always show accesskey, accesskey is appended in
parenthesis after the menu label.

In the bug 260563, space is added before parenthesis of accesskey.
# patch was attachment 159487 [details] [diff] [review]
But in all other softwares, no space exists between label and accesskey.
# No space inserted as far as Japanese softwares.

To match Firefox with other applications, space should not be inserted.
Probably in some country (some language), they want to insert space, but the 
other countries including Japan want no space.

So, I thisk we should add new pref "intl.menuitems.spacebeforeaccesskey" and
read this to determine add space or not.

c.f. Japanese Bugzilla's bug about this
http://bugzilla.mozilla.gr.jp/show_bug.cgi?id=4492

Reproducible: Always



Expected Results:  
We should select to add space or not between label and acceskey.

Comment 1

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

Attachment: Patch rv1.0

Comment 2

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

Attachment: Patch rv1.0

Comment 3

[Aaron Leventhal]

Comment on attachment 187272 [details] [diff] [review]
Patch rv1.0

Can't we just have 2 .properties strings?
string #1 -- what gets prepended before an appended accesskey
string #2 -- what gets appeneded after an appended accesskey

So in English, it would be " (" and ")"
For Japanes, perhaps "(" and ")"

Comment 4

[dynamis (Tomoya ASAI)]

Yes, that's good idea.

Not only parenthesis string properties can be the anser of this, but also make
possible to specify "[" and "]" or multibite parenthesis characters if we want.

Comment 5

[dynamis (Tomoya ASAI)]

Preparing 2 string can solve this bug but I don't know how to define " (" in
.properties files.

in .properties file, leading and tailing space will be removed.
  foo=(
  foo= (
  foo = (
All of these are same, foo property contains only "(".

We need define them in .dtd (I'm not sure wether we can get string from .dtd)
or use some tricky way (like used in venkman debugger).
# In dtd, leading and tailing spcae are important.

That is, if we can get parenthesis string from .dtd or we can define " (" in
.properties, we should add and use them.
But if we cannot define " (", we should use masayuki's patch (attachment 187272 [details] [diff] [review]).

Comment 6

[Aaron Leventhal]

How hard is it to use the DTD instead?

Or just surround these strings with a before and after character -- is that what
Venkman does?

Comment 7

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

Attachment: Patch rv1.0 (for text.xml)

text.xml ignores "intl.menuitems.insertseparatorbeforeaccesskeys" alway. That
is another bug. And this algorithm is not same as nsTextBoxFrame.cpp.
I fix these bugs too in this time.

Comment 8

[Mike Connor [:mconnor]]

Comment on attachment 187404 [details] [diff] [review]
Patch rv1.0 (for text.xml)

>+            // If accesskey is not in string, append in parentheses
>+            if (this.mAlwaysAppendAccessKey || accessKeyIndex < 0) {

if this.mAlwaysAppendAccessKey is true, accessKeyIndex can only be -1 here, so
you can simplify the check here.

>+              // If end is colon, we should insert before colon.
>+              // i.e., "label:" -> "label(X):"
>+              var colonRemoved = false;
>+              if (labelText.length > 0) {
>+                if (labelText.charAt(labelText.length - 1) == ":") {
>+                  this.textContent = labelText.substr(0, labelText.length - 1);
>+                  labelText = this.textContent;
>+                  colonRemoved = true;
>+                }
>+              }

why are we nesting a second if statement here? Do something like this instead

if (labelText.length > 0 &&
    labelText.charAt(labelText.length - 1) == ":") {

>+              if (labelText.length > 0) {
>+                if (labelText.charAt(labelText.length - 1) == " ") {
>+                  endIsSpace = true;
>+                }
>+              }

same comment applies here too

otherwise, looks ok

Comment 9

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

Attachment: Patch rv1.1(for text.xml)

Thank you Mike, for your review!

Comment 10

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

Aaron:

If we use my patch, *we MUST fix before 1.1b*. Becuase my patch changes
intl.properties that is intl resource. It is frozen by 1.1b release.

Comment 11

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

Attachment: Patch rv1.1(for XPFE's text.xml)

Comment 12

[dynamis (Tomoya ASAI)]

Sorry disturbing.
As a result, I think Masayuki's patch is good and enough. Go ahead please.

For japanese locale, both adding one bool like masayuki's patch and adding two
parenthesis string as Aaron say will be the answer. Former is more flexible
as I wrote but I'm not sure if that flexibility is really needed or not.

Since files for i18n are only intl.css and intl.properties (no .dtd file),
in case we define 2 properties of parenthesis string, we should define them in
intl.properties with quotes like this.
  intl.menuitems.accesskeyparenthesis.pre  = " ("
  intl.menuitems.accesskeyparenthesis.post =  ")"
But this tricky way (using quotes) is not standard.
# No properties file in locale do like this so far.
Using these will not be so simpler or smarter.
Unless we really need flexibility to use like "[" or multibyte parenthsis,
Masayuki's patch is enough for us.
We japanese locale is ok to go ahead with his patch.

FYI: about venkman, see this for more information:
http://lxr.mozilla.org/mozilla/source/extensions/venkman/resources/locale/en-US/venkman.properties#39
This is done with venkman specific property loading function, not with mozilla
standard features.

Comment 13

[Aaron Leventhal]

Okay go ahead if you think that approach is best.

If you need review in the next few days please ask someone else.

Comment 14

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

Comment on attachment 187272 [details] [diff] [review]
Patch rv1.0

I change the reviewer to pkwarren.
Aaron, if you can review before him, please review it.

Comment 15

[[e-mail]]

Comment on attachment 187419 [details] [diff] [review]
Patch rv1.1(for XPFE's text.xml)

>+            var val =
>+              prefs.getComplexValue("intl.menuitems.insertseparatorbeforeaccesskeys",
No point wrapping this, it's still over 80 characters.

>+              if (labelText.length > 0 &&
>+                  labelText.charAt(labelText.length - 1) == ":") {
You should use /:$/.test(labelText) instead of this complex condition.

>+                this.textContent = labelText.substr(0, labelText.length - 1);
>+                labelText = this.textContent;
You should set labelText first, then the text content. You should use slice(0,
-1) or replace(/:$/, "") instead of substr. Also nowhere do you restore the
colon if the access key is changed by script.

>+              if (labelText.length > 0 &&
>+                  labelText.charAt(labelText.length - 1) == " ") {
You should use / $/.test(labelText) instead of this complex condition.

Comment 16

[[e-mail]]

(In reply to comment #15)
>nowhere do you restore the colon if the access key is changed by script.
Actually you shouldn't just delete the colon, you should wrap it in a hidden
span. That way the value of this.textContent doesn't change. You'd have to use a
tree walker to locate the last text node, split off the colon if necessary etc.
Then write some test cases based on attachment 159098 [details] to make sure it works.

Comment 17

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

Neil:

Thank you for your review.
But I cannot understand following text.
> Actually you shouldn't just delete the colon, you should wrap it in a hidden
> span. That way the value of this.textContent doesn't change. You'd have to use
> a tree walker to locate the last text node, split off the colon if necessary
> etc.

Did you say I should create following tree?

labelText<span style="display:none;">:</span>(<span class="accesskey">X</span>):
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

If so, I don't know how to do it. tree walker? what is this?
Aren't there sample code?

Comment 18

[[e-mail]]

(In reply to comment #17)
>Did you say I should create following tree?
>
>labelText<span style="display:none;">:</span>(<span class="accesskey">X</span>):
Basically, yes: of course the (<span class="accesskey">X</span>) part needs to
be attached to the afterLabel element.

>If so, I don't know how to do it.
Hmm... I'll see if I can find someone to come up with a way of doing it.

Comment 19

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

Attachment: testcase of label ends ':'

Comment 20

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

Attachment: Patch rv2.0

O.K. I recreated with tree walker.
I set

> +	 <field name="mAlwaysAppendAccessKey">true</field>

Because on the testcase, always using default field values.
So, it's for testcase, for check-in I will change it to false.

Comment 21

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

Attachment: Patch rv2.0(for XPFE's text.xml)

Comment 22

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

Attachment: Patch rv1.0

Comment 23

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

Attachment: Patch rv1.0

Comment 24

[Benjamin Smedberg]

Not going to block on this.

Comment 25

[Philip K. Warren]

Comment on attachment 187519 [details] [diff] [review]
Patch rv1.0

>--- layout/xul/base/src/nsTextBoxFrame.cpp	28 Jun 2005 17:21:21 -0000	1.79
>+++ layout/xul/base/src/nsTextBoxFrame.cpp	28 Jun 2005 17:52:41 -0000
>@@ -199,17 +201,30 @@ nsTextBoxFrame::AlwaysAppendAccessKey()
>+PRBool
>+nsTextBoxFrame::InsertSeparatorBeforeAccessKey()
>+{
>+  if (!gInsertSeparatorBeforeAccessKey)
>+  {
>+    gInsertSeparatorBeforeAccessKey = PR_TRUE;
>+
>+    const char* prefName = "intl.menuitems.insertseparatorbeforeaccesskeys";
>+    nsAdoptingString val = nsContentUtils::GetLocalizedStringPref(prefName);
>+    gInsertSeparatorPrefInit = val.Equals(NS_LITERAL_STRING("true"));

Why not GetBoolPref here? Also I think the variable names are swapped here. It
sounds like:

gInsertSeparatorBeforeAccessKey -> Should represent value from preference
gInsertSeparatorPrefInit -> Show represent if the preference value has been
initialized.

These are swapped in the implementation. If you decide to keep the
GetLocalizedStringPref instead of using GetBoolPref, please change the line:

>+    gInsertSeparatorPrefInit = val.Equals(NS_LITERAL_STRING("true"));

to:

>+    gInsertSeparatorPrefInit = val.EqualsLiteral("true");

>Index: layout/xul/base/src/nsTextBoxFrame.h
>===================================================================
>RCS file: /cvsroot/mozilla/layout/xul/base/src/nsTextBoxFrame.h,v
>retrieving revision 1.17
>diff -u -8 -p -r1.17 nsTextBoxFrame.h
>--- layout/xul/base/src/nsTextBoxFrame.h	31 Dec 2004 01:13:26 -0000	1.17
>+++ layout/xul/base/src/nsTextBoxFrame.h	28 Jun 2005 17:52:41 -0000
>@@ -120,25 +120,28 @@ protected:
>                            nscoord&             aAscent);
> 
>   nsresult RegUnregAccessKey(nsPresContext* aPresContext,
>                              PRBool          aDoReg);
> 
> private:
> 
>   PRBool  AlwaysAppendAccessKey();
>+  PRBool  InsertSeparatorBeforeAccessKey();
> 
>   CroppingStyle mCropType;
>   nsString mTitle;
>   nsString mCroppedTitle;
>   nsString mAccessKey;
>   nscoord mTitleWidth;
>   nsAccessKeyInfo* mAccessKeyInfo;
>   PRBool mNeedsRecalc;
>   nsSize mTextSize;
>   nscoord mAscent;
> 
>   static PRBool gAlwaysAppendAccessKey;
>   static PRBool gAccessKeyPrefInitialized;
>+  static PRBool gInsertSeparatorBeforeAccessKey;
>+  static PRBool gInsertSeparatorPrefInit;

Nit: You may want to make these members into PRPackedBool.

Comment 26

[Philip K. Warren]

Oops - I meant to write:

gInsertSeparatorBeforeAccessKey -> Should represent value from preference
gInsertSeparatorPrefInit -> Should represent if the preference value has been
initialized.

Comment 27

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

Attachment: Patch rv1.1

Philip:
Thank you for your review.

> Why not GetBoolPref here?

We need to override the value by "intl.properties". So, we cannot use bool
value.
Therefore, we must use *Localized*String.

Comment 28

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

Comment on attachment 188518 [details] [diff] [review]
Patch rv1.1

Oops.. Sorry, I have a mistake.

Comment 29

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

Attachment: Patch rv1.2

Comment 30

[Philip K. Warren]

Comment on attachment 188519 [details] [diff] [review]
Patch rv1.2

>-PRBool nsTextBoxFrame::gAlwaysAppendAccessKey       = PR_FALSE;
>-PRBool nsTextBoxFrame::gAccessKeyPrefInitialized    = PR_FALSE;
>+PRPackedBool nsTextBoxFrame::gAlwaysAppendAccessKey          = PR_FALSE;
>+PRPackedBool nsTextBoxFrame::gAccessKeyPrefInitialized       = PR_FALSE;
>+PRPackedBool nsTextBoxFrame::gInsertSeparatorBeforeAccessKey = PR_FALSE;
>+PRPackedBool nsTextBoxFrame::gInsertSeparatorPrefInit        = PR_FALSE;

Sorry I made a mistake in the PRPackedBool recommendation - I misread the patch
initially. If they had all been bools which were class or struct members, then
PRPackedBool would have made sense. I think you can just keep it as PRBool. r+
if you change them back to PRBool.

Comment 31

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

Attachment: Patch rv1.3

Comment 32

[[e-mail]]

Comment on attachment 187517 [details] [diff] [review]
Patch rv2.0(for XPFE's text.xml)

r+sr=me if a) this code does not relate to test case 7 (which did not seem to
work), and b) you change WrapChar and MergeElement into <method>s, so that
WrapChar(accesskeyIndex, span, root); becomes this.wrapChar(span,
accesskeyIndex); [it makes more sense to have the element first, and also use
camelCase for the method names].

Comment 33

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

Attachment: Patch rv2.1(for XPFE's text.xml)

Comment 34

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

Attachment: Patch rv2.1(for toolkit's text.xml)

Mike:

Please re-review it.
I changed the patch from attachment 187409 [details] [diff] [review](previous patch that is reviewed
you).
This patch based attachment 188841 [details] [diff] [review] that is a patch of XPFE's text.xml.
Previous patch is not support the changing accesskey by Javascript. But it is
supported on this.

Comment 35

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

Comment on attachment 188539 [details] [diff] [review]
Patch rv1.3

Boris:

Could you sr it?

Comment 36

[Mike Connor [:mconnor]]

Comment on attachment 188844 [details] [diff] [review]
Patch rv2.1(for toolkit's text.xml)

sorry for the delay on this, I've looked at this multiple times and thought I'd
marked this.

Comment 37

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

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.62; previous revision: 1.61
done
Checking in calendar/sunbird/app/profile/sunbird.js;
/cvsroot/mozilla/calendar/sunbird/app/profile/sunbird.js,v  <--  sunbird.js
new revision: 1.17; previous revision: 1.16
done
Checking in embedding/minimo/all.js;
/cvsroot/mozilla/embedding/minimo/all.js,v  <--  all.js
new revision: 1.16; previous revision: 1.15
done
Checking in embedding/minimo/wince/all.js;
/cvsroot/mozilla/embedding/minimo/wince/all.js,v  <--  all.js
new revision: 1.8; previous revision: 1.7
done
Checking in layout/xul/base/src/nsTextBoxFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsTextBoxFrame.cpp,v  <--  nsTextBoxFrame.cpp
new revision: 1.80; previous revision: 1.79
done
Checking in layout/xul/base/src/nsTextBoxFrame.h;
/cvsroot/mozilla/layout/xul/base/src/nsTextBoxFrame.h,v  <--  nsTextBoxFrame.h
new revision: 1.18; previous revision: 1.17
done
Checking in mail/app/profile/all-thunderbird.js;
/cvsroot/mozilla/mail/app/profile/all-thunderbird.js,v  <--  all-thunderbird.js
new revision: 1.38; previous revision: 1.37
done
Checking in mail/locales/en-US/chrome/navigator/navigator.properties;
/cvsroot/mozilla/mail/locales/en-US/chrome/navigator/navigator.properties,v  <--
 navigator.properties
new revision: 1.4; previous revision: 1.3
done
Checking in modules/libpref/src/init/all.js;
/cvsroot/mozilla/modules/libpref/src/init/all.js,v  <--  all.js
new revision: 3.583; previous revision: 3.582
done
Checking in toolkit/locales/en-US/chrome/global/intl.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/global/intl.properties,v  <-- 
intl.properties
new revision: 1.4; previous revision: 1.3
done
Checking in xpfe/browser/resources/locale/en-US/navigator.properties;
/cvsroot/mozilla/xpfe/browser/resources/locale/en-US/navigator.properties,v  <--
 navigator.properties
new revision: 1.95; previous revision: 1.94
done
Checking in xulrunner/app/xulrunner.js;
/cvsroot/mozilla/xulrunner/app/xulrunner.js,v  <--  xulrunner.js
new revision: 1.3; previous revision: 1.2
done
Checking in xpfe/global/resources/content/bindings/text.xml;
/cvsroot/mozilla/xpfe/global/resources/content/bindings/text.xml,v  <--  text.xml
new revision: 1.16; previous revision: 1.15
done
Checking in toolkit/content/widgets/text.xml;
/cvsroot/mozilla/toolkit/content/widgets/text.xml,v  <--  text.xml
new revision: 1.15; previous revision: 1.14
done


checked-in. Thank you!