93025 - Menu keyboard shortcuts appear to be JA-style after UI language change

Status: RESOLVED FIXED

(Core :: Internationalization: Localization, defect)

Description

[Jon Rubin]

Found in 07-27-branch JA Win build on WinMe-Ja.

Steps:
1) Create a profile and set the UI language to en-US (in JA build), or change an
existing profile to en-US.
2) Launch, or relaunch browser, according to action in step 1.  The menu will
appear in English, but the shortcuts will be JA-style.  For example, "F" in
"File" should be underlined; instead, an underlined "F" appears in parentheses
after "File".

Comment 1

[Andreas Becker]

QA -> jonrubin

Comment 2

[Jon Rubin]

This also happens when applying the DE lang pack.

Comment 3

[Jon Rubin]

If I create a ja-JP UI profile in 07-27-branch US build, the shortcuts look
correct (JA style); if I switch that profile to English, the shortcuts look
correct (EN style).  So this problem happens only in JA build.

Comment 4

[rchen]

It is a corebug. The preference of the appending the accesskey is in the 
language pack (navigator.properties). But it doesn't change when the users 
switch the language packs.

Assigned to ftang.

Comment 5

[Jon Rubin]

If I create an en-US in JA build 07-27-branch, the shortcuts look JA-style.  But
if I open the same profile in US build 07-27-branch, the shortcuts look EN-style.

Comment 6

[Frank Tang]

I am confused, please state clearly what is the expected behavior and what is
the actual behavior. 
jbetak, please help to debug this one.

Comment 7

[Jon Rubin]

In en-US profile, it is expected that the letter that corresponds to the KB
shortcut will be underlined within the word itself; in ja-JP profile, the letter
should appear in parentheses after the word.  In the US build, this is working
correctly, in the JA build, en-US profiles appear in English, but the shortcuts
appear in parentheses after the word; ja-JP profiles always appear correctly.

I will attach screenshot examples.

Comment 8

[Jon Rubin]

Attachment: This is how en-US shortcuts should appear

Comment 9

[Jon Rubin]

Attachment: This is how en-US shortcuts appear on JA build; this is wrong.

Comment 10

[Jon Rubin]

When I say "ja-JP profiles always appear correctly", I mean that they appear
correctly regardless of whether they are opened in the JA or US build.

Comment 11

[rchen]

jbetak, please talk to me if you have any question. 

There is nothing wrong with you code, I think. The problem is when we switch the  
language pack, it doesn't change according to the preference value stored in 
navigator.properties:

intl.menuitems.alwaysappendacceskeys=true  for JA pack.
intl.menuitems.alwaysappendacceskeys=      for US pack.

You may talk to vishy regarding this issue.

Comment 12

[[e-mail] (away - not reading bugmail)]

rchen, 

thanks for your comments. As far as I remember the code I wrote for bug listens 
to the preference intl.menuitems.alwaysappendacceskeys value. In a Japanese 
build we might be setting it to true and not replacing its value by false in a 
language pack. I'm downloading the latest commercial bits to verify the 
contents of navigator.properties in different language packs. 

Couldn't this be fixed by updating the non-Japanese language packs?

Comment 13

[[e-mail] (away - not reading bugmail)]

still investigating due to abysmal network performance - at this point it
appears to be a problem with incorrect navigator.properties preference setting
in individual language packs. I´ll debug it as soon as I have a 6.1 Ja RTM build up.

Comment 14

[Jon Rubin]

Changing OS to All, since this happens in Linux also (tested JA 07-26-branch
build on Linux RH 7.1-Ja).

Comment 15

[[e-mail] (away - not reading bugmail)]

Jon, 

are you still seeing this? I just pulled down a fresh 6.1 JA RTM and I can't 
reproduce the problem per the steps listed above. Is this possibly platform-
dependent? I'm testing on US W2K, have you maybe seen this on JA Windows only?

Comment 16

[Jon Rubin]

Juraj,

Yes, it looks like this problem occurs only on a JA OS (note that I saw this on
JA Linux also).  I tried this on Win2K-JA, and the problem occurs; however, I
also could not reproduce it on Win2K-US, which is what you tested on.

Comment 17

[[e-mail] (away - not reading bugmail)]

cc'ing some more i18n folks. 

Roy - would you know, whether this is typical of Japanese OSes? In another 
words would you know of a possibility to show underlined instead of appended 
accelerator keys in say Japanese Windows? 

I would like to determine the priority and schedule a milestone. If this 
requires overcoming some OS limitations and is generally within the usual 
behavior for apps on that platform, I'd push it out a little. Otherwise, we 
should probably fix this ASAP.

Comment 18

[Jon Rubin]

Juraj,

I do see underlined keys on the Japanese OS when I install the US build.  This
problem occurs only for the JA build on the JA OS.

Comment 19

[[e-mail] (away - not reading bugmail)]

Thanks Jon! I'll boot up in JA Windows and try to debug it there...

Comment 20

[[e-mail] (away - not reading bugmail)]

Jon,

could we look at this together? I'm writing this from JA W2K using 6.1 JA RTM
with an English language pack and things still seem to be happy on my end...

Comment 21

[Jon Rubin]

Sure.

Comment 22

[[e-mail] (away - not reading bugmail)]

thanks Jon! I think I finally got it - it seems that both the platform is 
irrelevant. I thought I was able to repro this using an US build, but it really 
seems to affect Japanese builds only. 

One thing we discovered together was that you need to have one profile with 
with the English language pack activated and one with the Japanese. It seems 
that the profile manager somehow turns on this global preference, when of the 
the profiles has the Japanese locale. Pretty strange, when you ask me.

Still investigating.

Comment 23

[[e-mail] (away - not reading bugmail)]

seems like the preference intl.menuitems.alwaysappendacceskeys is being read 
from the wrong locale.

For now, it seems to suffice to 
change "intl.menuitems.alwaysappendacceskeys=true" 
to "intl.menuitems.alwaysappendacceskeys=" in the Japanese language pack to 
address the problem. I'd recommend this solution as a quick workaround/remedy.

I'm still trying to understand, why apparently some preference values can be 
read from the wrong locale. I believe the culprit is in the profile manger. It 
might take a while to see, how this happens, so reconfirming 0.9.5.

Comment 24

[Andreas Becker]

mass change, switching qa contact from jonrubin to ruixu.

Comment 25

[Frank Tang]

ok, I guess I know what happened. In the Ja build, the profile is bootup with JA
prefs by default. and when user select a profile, we switch to the new pref.
Somehow the global variable which control this behavior is in PRBool
nsTextBoxFrame::gAlwaysAppendAccessKey = PR_FALSE;

which read in the following code 
 nsTextBoxFrame::Init(nsIPresContext*  aPresContext,)
Notice that nsTextBoxFrame::Init is called for every TextBoxFrame, regardless it
is menu or not. So... some UI displayed in the Profile Manager actually use
nsTextFrameBox, and there for nsTextFrameBox::Init get called and the global
boolean get setup.

now user decide to choose a profile which is en-US, but the pref have already
been read in. and there are no code to obsolete the value of that global variable. 

There are two ways to fix this problem- 
1. create a user language switch observer in here to purge the global variable .
I think we already have some code like that . tao probably know what is the
observer name.
2. delay the reading of the pref till we really need it. This mean we should
isolate the pref reading code from Init out into a new method - say
InitAccessKeyPref

nsTextBoxFrame::InitAccessKeyPref()
164 if (!gAccessKeyPrefInitialized) {
165 nsCOMPtr<nsIPref> prefs = do_GetService(NS_PREF_CONTRACTID);
166 gAccessKeyPrefInitialized = PR_TRUE;
167 if (prefs) {
168 nsXPIDLString prefValue;
169 nsresult res =
prefs->GetLocalizedUnicharPref("intl.menuitems.alwaysappendacceskeys",
getter_Copies(prefValue));
170 if (NS_SUCCEEDED(res)) {
171 gAlwaysAppendAccessKey =
nsDependentString(prefValue).Equals(NS_LITERAL_STRING("true"));
172 }
173 }
174 }

and call it when we really need to access it-
which mean add
if (!gAccessKeyPrefInitialized)
  InitAccessKeyPref();
in two places- 
inside the "if (!mAccessKey.IsEmpty()) {" block of 
nsTextBoxFrame::UpdateAccessTitle()

and the "} else {" block of "nsTextBoxFrame::UpdateAccessIndex()"

Comment 26

[Frank Tang]

these two ways are not exculse. The first one will be needed if we need on the
fly language switching later. The second one is always good for delay loading
nslocale.dll and speed up startup perf.

Comment 27

[Frank Tang]

move it back to ftang

Comment 28

[Frank Tang]

mark it as assign

Comment 29

[Frank Tang]

add to my working list

Comment 30

[Frank Tang]

Attachment: testing patch.

Comment 31

[Frank Tang]

try the test patch with debugger. It prove my theroy is right. The Init function
got hit before we select the profile. And now the new code won't read in pref
untill we select the profile because the current profile code do not have menu.
ask for review.

Comment 32

[Frank Tang]

It looks like the origional code is check in by jbeak (loadrunner ). jbetak,
please r= this change .

Comment 33

[[e-mail] (away - not reading bugmail)]

this is a great find Frank! The patch seems reasonable, as it's only delaying 
the pref read - it was not clear to me back when I worked on this that putting 
the initialization into nsTextBoxFrame::Init will cause the pref to be read too 
early. Just pointing this out so you don't catch fire from whoever is doing the 
sr: tab-width: 4; indent-tabs-mode: nil.

r=jbetak

Comment 34

[Frank Tang]

>tab-width: 4; indent-tabs-mode: nil.
???

Comment 35

[kinmoz]

Comment on attachment 53858 [details] [diff] [review]
testing patch.

This is just a suggestion, I know this patch probably works fine ...

Wouldn't it be better to just add an access method for
gAlwaysAppendAccessKey which included the pref init code:

    PRBool nsTextBoxFrame::AlwaysAppendAccessKey()
    {
      < pref init code goes here >

      return gAlwaysAppendAccessKey;
    }

And then just modify the points that used to access
gAlwaysAppendAccessKey directly in the following manner:


- if ((mTitle.Find(mAccessKey, PR_TRUE) == kNotFound) || gAlwaysAppendAccessKey) {
+ if ((mTitle.Find(mAccessKey, PR_TRUE) == kNotFound) || AlwaysAppendAccessKey()) {


This insures that the pref is always init'd before the first access
of gAlwaysAppendAccessKey's value, and it makes it so that you don't
have to worry about adding calls to an InitAccessKeyPref() method
everywhere, especially if in the future you needed to retrieve the value
in other parts of the code?


Why would you want to make this protected? The globals themselves are
only accessible from within nsTextBoxFrame?

>+protected:
>+  void  InitAccessKeyPref();
> private:
> 
>   CroppingStyle mCropType;

Comment 36

[Frank Tang]

good point. Let me make a better patch

Comment 37

[Frank Tang]

Attachment: v2 of the patch

Comment 38

[Frank Tang]

jbeta: can you r= the v2

Comment 39

[Frank Tang]

tao:
I think I need to register a observer to observer the "switch UI language" event
to fix this. Could you point me to some sample code? What is the topic I should
observe ?

Currently, the patch is good enough to cover the case that user shutdown the app
and relaunch. 

Comment 40

[tao]

frank: since the language switching won't take effect until relaunch, observing
the event won't help. Juraj worked on this area a while ago to set locale selection
in the prefs for next run. He might know what event to observe.

Juraj?

Comment 41

[Frank Tang]

ok. file another bug for that language switching issue. bug 105489. 
since we don't need to switch locale on the fly right now. Leave that bug there
for now. jbetak, can you focus on review of this patch. Thanks :)

Comment 42

[[e-mail] (away - not reading bugmail)]

looks good to me, r=jbetak

the only nit I had was this:

+      if (NS_SUCCEEDED(res)) 
+      {
+        gAlwaysAppendAccessKey = nsDependentString(prefValue).Equals(
+                                             NS_LITERAL_STRING("true"));
+        gAccessKeyPrefInitialized = PR_TRUE;
+      }

Wouldn't that be cleaner in some borderline cases, where we don't get to read
the pref value? We'd be forced to come back and try again next time around.

Comment 43

[[e-mail] (away - not reading bugmail)]

tao, ftang,

this is hust FYI: turbo mode needs to be exited when switching the app laguage
or region and that to the implementation of an event broadcaster - as spearheded
by ccarlen:

http://lxr.mozilla.org/seamonkey/search?string=locale-selected

This might be another way to set gAccessKeyPrefInitialized to PR_FALSE, but I'm
not sure how relevant this is for what Frank is trying to accomplish.

Comment 44

[Frank Tang]

>Wouldn't that be cleaner in some borderline cases, where we don't get to read
>the pref value? We'd be forced to come back and try again next time around.

It think that dramatically slow down the performance if somehow it always fail.

kin can you sr= the new patch ?

Comment 45

[kinmoz]

Comment on attachment 54003 [details] [diff] [review]
v2 of the patch

sr=[e-mail]

Just put an empty line between these 2 lines:

+  PRBool  AlwaysAppendAccessKey();
   CroppingStyle mCropType;

Comment 46

[Frank Tang]

fixed and check in

Comment 47

[Rui Xu]

Need some matchable language packs for current trunk build. This bug has to be 
verified when next JA build and language pack are available.