Space before parenthesis of accesskey should be removable or removed.

RESOLVED FIXED in mozilla1.8beta4

Status

()

defect
P1
normal
RESOLVED FIXED
14 years ago
a month ago

People

(Reporter: bugzilla, Assigned: masayuki)

Tracking

({intl, regression})

Trunk
mozilla1.8beta4
Points:
---
Bug Flags:
blocking1.8b3 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 11 obsolete attachments)

4.42 KB, application/vnd.mozilla.xul+xml
Details
19.05 KB, patch
masayuki
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
8.10 KB, patch
masayuki
: review+
masayuki
: superreview+
Details | Diff | Splinter Review
8.33 KB, patch
mconnor
: review+
mconnor
: approval1.8b4+
Details | Diff | Splinter Review
(Reporter)

Description

14 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-aviary1.1?
Keywords: intl, regression
Posted patch Patch rv1.0 (obsolete) — Splinter Review
Assignee: aaronleventhal → masayuki
Status: NEW → ASSIGNED
Attachment #187271 - Flags: review?(aaronleventhal)
Flags: blocking-aviary1.1? → blocking1.8b3?
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta3
Posted patch Patch rv1.0 (obsolete) — Splinter Review
Attachment #187271 - Attachment is obsolete: true
Attachment #187272 - Flags: review?(aaronleventhal)

Comment 3

14 years ago
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 ")"
(Reporter)

Comment 4

14 years ago
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.
(Reporter)

Comment 5

14 years ago
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

14 years ago
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?
Posted patch Patch rv1.0 (for text.xml) (obsolete) — Splinter Review
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.
Attachment #187404 - Flags: review?(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
Attachment #187404 - Flags: review+
Posted patch Patch rv1.1(for text.xml) (obsolete) — Splinter Review
Thank you Mike, for your review!
Attachment #187404 - Attachment is obsolete: true
Attachment #187409 - Flags: review+
Attachment #187409 - Attachment description: Patch rv1.1 → Patch rv1.1(for text.xml)
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.
(Reporter)

Comment 12

14 years ago
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

14 years ago
Okay go ahead if you think that approach is best.

If you need review in the next few days please ask someone else.
Attachment #187419 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #187419 - Flags: review?(neil.parkwaycc.co.uk)
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.
Attachment #187272 - Flags: review?(aaronleventhal) → review?(pkwarren)
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.
Attachment #187419 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #187419 - Flags: superreview-
Attachment #187419 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #187419 - Flags: review-
(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.
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?
(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.
Posted patch Patch rv2.0 (obsolete) — Splinter Review
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.
Attachment #187419 - Attachment is obsolete: true
Attachment #187516 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #187516 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #187516 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #187516 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #187516 - Attachment is obsolete: true
Attachment #187517 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #187517 - Flags: review?(neil.parkwaycc.co.uk)
Posted patch Patch rv1.0 (obsolete) — Splinter Review
Attachment #187272 - Attachment is obsolete: true
Attachment #187518 - Flags: review?(pkwarren)
Posted patch Patch rv1.0 (obsolete) — Splinter Review
Attachment #187518 - Attachment is obsolete: true
Attachment #187519 - Flags: review?(pkwarren)

Comment 24

14 years ago
Not going to block on this.
Flags: blocking1.8b3? → blocking1.8b3-

Comment 25

14 years ago
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.
Attachment #187519 - Flags: review?(pkwarren) → review-

Comment 26

14 years ago
Oops - I meant to write:

gInsertSeparatorBeforeAccessKey -> Should represent value from preference
gInsertSeparatorPrefInit -> Should represent if the preference value has been
initialized.
Posted patch Patch rv1.1 (obsolete) — Splinter Review
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.
Attachment #187519 - Attachment is obsolete: true
Attachment #188518 - Flags: review?(pkwarren)
Comment on attachment 188518 [details] [diff] [review]
Patch rv1.1

Oops.. Sorry, I have a mistake.
Attachment #188518 - Flags: review?(pkwarren) → review-
Posted patch Patch rv1.2 (obsolete) — Splinter Review
Attachment #188518 - Attachment is obsolete: true
Attachment #188519 - Flags: review?(pkwarren)

Comment 30

14 years ago
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.
Attachment #188519 - Flags: review?(pkwarren) → review+
Posted patch Patch rv1.3Splinter Review
Attachment #188519 - Attachment is obsolete: true
Attachment #188539 - Flags: superreview?(dbaron)
Attachment #188539 - Flags: review+
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].
Attachment #187517 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #187517 - Flags: superreview+
Attachment #187517 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #187517 - Flags: review+
Attachment #187517 - Attachment is obsolete: true
Attachment #188841 - Flags: superreview+
Attachment #188841 - Flags: review+
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.
Attachment #187409 - Attachment is obsolete: true
Attachment #188844 - Flags: review?(mconnor)
Target Milestone: mozilla1.8beta3 → mozilla1.8beta4
Comment on attachment 188539 [details] [diff] [review]
Patch rv1.3

Boris:

Could you sr it?
Attachment #188539 - Flags: superreview?(dbaron) → superreview?(bzbarsky)
Attachment #188539 - Flags: superreview?(bzbarsky) → superreview+
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.
Attachment #188844 - Flags: review?(mconnor) → review+
Attachment #188844 - Flags: approval1.8b4+

Updated

14 years ago
Attachment #188539 - Flags: approval1.8b4? → approval1.8b4+

Updated

14 years ago
Attachment #188841 - Flags: approval1.8b4? → approval1.8b4+
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!
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Component: Keyboard: Navigation → User events and focus handling
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.