Russian, Greek, and Hebrew localizations (for example) use non-latin accesskey assignments. These worked fine in Firefox 2 (when the selected keyboard layout is in the corresponding mode) on Windows and Linux. (I don't know how accesskeys work on Mac.) It looks like this behaviour has changed (bug 287179 comment 83) so that only latin Alt-key combinations are available. (I haven't tested - i don't know of localizations for trunk and i use a latin keyboard.) The new behaviour is actually consistent with documentation at http://www.mozilla.org/projects/l10n/mlp_what2.html: "Both AccessKeys and CommandKeys work with Latin (occidental) letters only." but it is a change and I'm not sure its for the better. This article seems to think that localized menu access keys are normal: http://blogs.msdn.com/michkap/archive/2004/12/24/331661.aspx What proportion of people with Cyrillic/Greek/Hebrew keyboards have the latin letters on the keyboard? Is this an intentional change agreed on after consideration of all side-effects?

Requesting blocking1.9 because this is a regression. If this is the new desired behavior, then other platforms need to be modified to be consistent. The best answer is probably to have a flag that localizers can set to say whether menu access keys characters are in Latin or local charsets. That way the latin access keys on non-localized installs would work while in local (non-latin) layout, but non-latin characters could be used in localized installs.

CCing Simon and our Polish crew for some feedback here. Regarding the quote from mlp_what2.html, I guess that comment is more targetting at languages that don't have their glyphs on keys, like Japanese. That doesn't say that we're not having an ongoing problem with the fact that we just don't know if a user has a native keyboard or not.

Russian Windows users generally have Russian and EN-US layouts and switch between them with Alt-Shift. In Firefox 2 RU menu access keys are localized and only work in Russian layout. Personally, I don't use this way to activate the menu and don't remember any Russian user complaining about non-working menu access keys in EN-US layout in Fx 2.

Since bug #69230 fixed I've got the same problem in FreeBSD and in Linux. Maybe this bug can be resolved in some crossplatform manner?

Same problem with Greek builds and Alt+ accesskeys. It is an annoying bug for many users, since menu is not accessible anymore via the keyboard in localised builds

First I've seen something like that somewhere on the mozilla-russia.com. I don't know why the author hasn't created the patch. It's very very similar and intuitive, so I'm lazy to find him.

Comment on attachment 301363 [details] [diff] [review] Try1 Let's only do the is_latin_shortcut_key stuff when event.isControl. We shouldn't need to do this when event.isMeta. In my opinion, putting the isControl test before the is_latin_shortcut_key (rather than after) would look better because the simpler test is done first (even though it probably doesn't make much difference given the function is inline).

(In reply to comment #7) > (From update of attachment 301363 [details] [diff] [review]) > Let's only do the is_latin_shortcut_key stuff when event.isControl. > We shouldn't need to do this when event.isMeta. Meta is this: http://en.wikipedia.org/wiki/Meta_key ? > In my opinion, putting the isControl test before the is_latin_shortcut_key > (rather than after) would look better because the simpler test is done first > (even though it probably doesn't make much difference given the function is > inline). Ok, I will change it. You're right. And also I will add !isMeta.

Karl, I did in an another way. >if (event.isControl && !is_latin_shortcut_key(event.charCode) ) This thing should do the same as before. But it doesn't touch Meta keys.

Comment on attachment 302109 [details] [diff] [review] for Linux This is good, thanks. Before landing, we'll need a similar fix for Windows and ui-review. http://groups.google.com/group/mozilla.dev.planning/browse_thread/thread/0d87e087c5b8b2f9

(In reply to comment #10) > This is good, thanks. Before landing, we'll need a similar fix for Windows and > ui-review. Mozilla is welcome :) It's simple. But I will be able to work with it only next week. > http://groups.google.com/group/mozilla.dev.planning/browse_thread/thread/0d87e087c5b8b2f9 >

Comment on attachment 302109 [details] [diff] [review] for Linux I don't know whom I should select as ui-reviewer. I have tested it, so can I mark ui-review as "+"?

For ui-review: http://rapidshare.com/files/91588156/firefox-3.0b4pre.en-US.linux-i686.tar.bz2 http://rapidshare.com/files/91528025/firefox-3.0b2.en-US.win32.zip (from CVS too, don't know why it is b2, maybe I used just cvs up).

Comment on attachment 303030 [details] [diff] [review] for Windows Note that mIsControlDown ^ mIsAltDown excluded the mIsControlDown && mIsAltDown case which apparently is equivalent to AltGr on some MS Windows keyboards (bug 341308 comment 1). So we need "mIsControlDown && !mIsAltDown" here. The comments in the paragraph below also need updating to exclude the references to Alt+*. I don't know whether or not we should have a similar test here: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/windows/nsWindow.cpp&rev=3.726&mark=3241#3235 We did before attachment 215299 [details] [diff] [review] changed the behavior. Ere, do you know what the appropriate test here should be?

As soon as you will checkin this patch there will be new crowd of people complaining that standard Alt+f shortcuts stopped working to access File menu if active keyboard layout is Russian. We already seen all this... The only solution that possibly will please everyone is first to lookup all language specific accelerator keys. If those are not consumed then lookup all English ones.

(In reply to comment #15) > (From update of attachment 303030 [details] [diff] [review]) > So we need "mIsControlDown && !mIsAltDown" here. > The comments in the paragraph below also need updating to exclude the > references to Alt+*. Ok. I will do. > I don't know whether or not we should have a similar test here: > http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/windows/nsWindow.cpp&rev=3.726&mark=3241#3235 > Guess no, everything works without it. And this should be something else. (In reply to comment #16) > As soon as you will checkin this patch there will be new crowd of people > complaining that standard Alt+f shortcuts stopped working to access File menu > if active keyboard layout is Russian. We already seen all this... Menu shorcuts should depend on localization. There is special keyword in localization for it (something like accesskey). So the question is why it doesn't work. > The only solution that possibly will please everyone is first to lookup all > language specific accelerator keys. If those are not consumed then lookup all > English ones. For menu shortcuts we don't need any special checking (only in "hack" code, like my). I think we should fix the reason, why it doesn't work. This weekend I will try to do linux code which would work correctly (it's very simple) and then during the week or two I will play with xul to understand what's broken.

Oops, to tired to think in a right way. The accesskey system is ok - was mistaken :) As I told it's simple to fix linux code. And windows code I will hack during next week.

alt+everything works. Localized ui: <alt+latin> || <alt+non-Latin> Eng ui and <alt+non-latin> is ok too. I tested only by hands. Will run automated tests after your reviewing.

Comment on attachment 303893 [details] [diff] [review] Linux patch 2 +nsWindow::try_to_dispatch_OnKeyPressEvent(nsKeyEvent &event, nsEventStatus &status) + +{ + // before we dispatch a key, check if it's the context menu key. + // If so, send a context menu key event instead. + if (is_context_menu_key(event)) { + nsMouseEvent contextMenuEvent(PR_TRUE, 0, nsnull, nsMouseEvent::eReal); + key_event_to_context_menu_event(&event, &contextMenuEvent); + DispatchEvent(&contextMenuEvent, status); + } Note that is_context_menu_key() won't be true for Alt or Ctrl key combinations and doesn't use characters from any particular language so context menu keys handling may be able to remain in OnKeyPressEvent. And maybe we wouldn't need the try_to_dispatch_OnKeyPressEvent function. See if this is possible/reasonable with the changes I'm suggesting below. + else { + // send the key press event + DispatchEvent(&event, status); + } + + // If the event was consumed, return. + LOGIM(("status %d\n", status)); + if (status == nsEventStatus_eConsumeNoDefault) { + LOGIM(("key press consumed\n")); + return TRUE; + } After speaking with roc, we're a bit concerned about what event listeners will think of two events being sent. It would be safer to return from OnKeyPressEvent also in the case of nsEventStatus_eConsumeDoDefault. i.e. return if status != nsEventStatus_eIgnore, but (to be consistent I guess) return FALSE on nsEventStatus_eConsumeDoDefault. // Fix for bug 69230: // if modifier key is pressed and key pressed is not latin character, // we should try other keyboard layouts to find out correct latin // character corresponding to pressed key; I think this and the code that follows it should only be done if (event.isControl || event.isAlt). + //Assume UI is localized and we have <alt+Latin>, let's convert to Native + if (event.isAlt && is_latin_shortcut_key(event.charCode)) { + // We have a latin char, try other keyboard groups + GdkKeymapKey *keys; + guint *keyvals; + gint n_entries; + PRUint32 nonLatinCharCode; + gint level; + if (gdk_keymap_translate_keyboard_state(NULL, + tmpEvent.hardware_keycode, + (GdkModifierType)tmpEvent.state, + tmpEvent.group, + NULL, NULL, &level, NULL) + && gdk_keymap_get_entries_for_keycode(NULL, + tmpEvent.hardware_keycode, + &keys, &keyvals, + &n_entries)) { + gint n; + for (n=0; n<n_entries; n++) { + if (keys[n].group == tmpEvent.group) { + // Skip keys from the same group + continue; + } + if (keys[n].level != level) { + // Allow only same level keys + continue; + } Can this be merged with the similar "if (!is_latin_shortcut_key(event.charCode))" block? The idea is to test the current group first (where you had the first try_to_dispatch_OnKeyPressEvent call) then try other groups. I guess some is_latin_shortcut_key logic should be used so as not to surprise Dvorak users by converting Latin codes to different Latin codes. + if (nonLatinCharCode) { + event.charCode = nonLatinCharCode; + break; + } There may be more than one non-Latin group. Rather than "break"ing I think this is the place to DispatchEvent and check the status. + } + } + return try_to_dispatch_OnKeyPressEvent(event, status); //references! What does "references" refer to? It looks like this will sometimes dispatch the same event again. We should only dispatch again if a different code was found. Something is wrong with the indentation here. } - - return FALSE; } What happens if !event.charCode?

(In reply to comment #21) > (From update of attachment 303893 [details] [diff] [review]) > Note that is_context_menu_key() won't be true for Alt or Ctrl key combinations > and doesn't use characters from any particular language so context menu keys > handling may be able to remain in OnKeyPressEvent. And maybe we wouldn't need > the try_to_dispatch_OnKeyPressEvent function. See if this is > possible/reasonable with the changes I'm suggesting below. Ok, done. > + else { > + // send the key press event > + DispatchEvent(&event, status); > + } > + > + // If the event was consumed, return. > + LOGIM(("status %d\n", status)); > + if (status == nsEventStatus_eConsumeNoDefault) { > + LOGIM(("key press consumed\n")); > + return TRUE; > + } > > After speaking with roc, we're a bit concerned about what event listeners will > think of two events being sent. It would be safer to return from > OnKeyPressEvent also in the case of nsEventStatus_eConsumeDoDefault. > i.e. return if status != nsEventStatus_eIgnore, but (to be consistent I guess) > return FALSE on nsEventStatus_eConsumeDoDefault. Hm... Didn't get you. Do you mean such thing: if (status == nsEventStatus_eConsumeNoDefault) { LOGIM(("key press consumed\n")); return FALSE; } if (status != nsEventStatus_eIgnore) return TRUE; In the trunc it returns TRUE for nsEventStatus_eConsumeNoDefault, don't know if change to FALSE needed. > // Fix for bug 69230: > // if modifier key is pressed and key pressed is not latin > character, > // we should try other keyboard layouts to find out correct latin > // character corresponding to pressed key; > > I think this and the code that follows it should only be done if > (event.isControl || event.isAlt). In order yes. Other things where dispatched before this code. > + //Assume UI is localized and we have <alt+Latin>, let's convert to > Native > + if (event.isAlt && is_latin_shortcut_key(event.charCode)) { > + // We have a latin char, try other keyboard groups > + GdkKeymapKey *keys; > + guint *keyvals; > + gint n_entries; > + PRUint32 nonLatinCharCode; > + gint level; > + if (gdk_keymap_translate_keyboard_state(NULL, > + > tmpEvent.hardware_keycode, > + > (GdkModifierType)tmpEvent.state, > + tmpEvent.group, > + NULL, NULL, &level, > NULL) > + && gdk_keymap_get_entries_for_keycode(NULL, > + > tmpEvent.hardware_keycode, > + &keys, &keyvals, > + &n_entries)) { > + gint n; > + for (n=0; n<n_entries; n++) { > + if (keys[n].group == tmpEvent.group) { > + // Skip keys from the same group > + continue; > + } > + if (keys[n].level != level) { > + // Allow only same level keys > + continue; > + } > > Can this be merged with the similar > "if (!is_latin_shortcut_key(event.charCode))" block? I think no. These blocks make different conversions. 1 is for non-Latin, another is for Latin. Also the situation is so, that we woul go only through 1 block. Thus I suggest to leave it as it is. > The idea is to test the current group first (where you had the first > try_to_dispatch_OnKeyPressEvent call) then try other groups. > I guess some is_latin_shortcut_key logic should be used so as not to > surprise Dvorak users by converting Latin codes to different Latin codes. I don't think this might be. Dvorak and Qwerty keys have the same keycodes (GDK_a-GDK_z). > > + if (nonLatinCharCode) { > + event.charCode = nonLatinCharCode; > + break; > + } > > There may be more than one non-Latin group. > Rather than "break"ing I think this is the place to DispatchEvent and check > the status. You're right, didn't think about it. > + } > + } > + return try_to_dispatch_OnKeyPressEvent(event, status); //references! > > What does "references" refer to? Arguments. Stupid comment, I will remove it. > } > > What happens if !event.charCode? Nothing. if(event.charCode) is used for some conversions as I see.

(In reply to comment #22) > > // Fix for bug 69230: > > // if modifier key is pressed and key pressed is not latin > > character, > > // we should try other keyboard layouts to find out correct latin > > // character corresponding to pressed key; > > > > I think this and the code that follows it should only be done if > > (event.isControl || event.isAlt). > > In order yes. Other things where dispatched before this code. > > > + if (gdk_keymap_translate_keyboard_state(NULL, > > + > > tmpEvent.hardware_keycode, > > + > > (GdkModifierType)tmpEvent.state, > > + tmpEvent.group, > > + NULL, NULL, &level, > > NULL) > > + && gdk_keymap_get_entries_for_keycode(NULL, > > + > > tmpEvent.hardware_keycode, > > + &keys, &keyvals, > > + &n_entries)) { > > + gint n; > > + for (n=0; n<n_entries; n++) { > > + if (keys[n].group == tmpEvent.group) { > > + // Skip keys from the same group > > + continue; > > + } > > + if (keys[n].level != level) { > > + // Allow only same level keys > > + continue; > > + } > > > > Can this be merged with the similar > > "if (!is_latin_shortcut_key(event.charCode))" block? > > I think no. These blocks make different conversions. 1 is for non-Latin, > another is for Latin. Also the situation is so, that we woul go only through 1 > block. Thus I suggest to leave it as it is. I think it could be tidier (less code) to do the is_latin_shortcut_key(event.charCode) test within the loop. > > > The idea is to test the current group first (where you had the first > > try_to_dispatch_OnKeyPressEvent call) then try other groups. > > I guess some is_latin_shortcut_key logic should be used so as not to > > surprise Dvorak users by converting Latin codes to different Latin codes. > > I don't think this might be. Dvorak and Qwerty keys have the same keycodes > (GDK_a-GDK_z). > > } > > > > What happens if !event.charCode? > > Nothing. if(event.charCode) is used for some conversions as I see. Maybe this will become clearer when the indentation is corrected. But before your patch an event was still sent when !event.charCode.

(In reply to comment #22) Sorry, some of my comments seemed to get lost somewhere. > Hm... Didn't get you. Do you mean such thing: > if (status == nsEventStatus_eConsumeNoDefault) { > LOGIM(("key press consumed\n")); > return FALSE; > } > if (status != nsEventStatus_eIgnore) > return TRUE; Note that nsEventStatus_eConsumeNoDefault and nsEventStatus_eConsumeDoDefault are two distinct status values. So something like: if (status != nsEventStatus_eIgnore) LOGIM(("key press consumed\n")); return status == nsEventStatus_eConsumeNoDefault; } > > The idea is to test the current group first (where you had the first > > try_to_dispatch_OnKeyPressEvent call) then try other groups. > > I guess some is_latin_shortcut_key logic should be used so as not to > > surprise Dvorak users by converting Latin codes to different Latin codes. > > I don't think this might be. Dvorak and Qwerty keys have the same keycodes > (GDK_a-GDK_z). Yes, your current patch prevents this from happening for Latin keycodes (but it is difficult to do the right thing for punctuation). I was meaning that, with my suggested changes, the is_latin_shortcut_key(keyvals[n]) checks would need to be retained in some form.

I fixed all things about you told, but didn't find any good way to remove try_to_dispatch_OnKeyPressEvent() (this routine still needs attention). Also there were some bugs with isShidt operations (I moved the code to try_to_dispatch_OnKeyPressEvent() ) Karl, thanks for your advices, explanations and patient.

Comment on attachment 306727 [details] [diff] [review] Linux patch. Fixed problems found by Karl + if (event.isShift) + tmpEvent.keyval = gdk_keyval_to_upper(keyvals[n]); + else + tmpEvent.keyval = gdk_keyval_to_lower(keyvals[n]); I don't know if this code needed. There is very similar code in the try_to_dispatch_OnKeyPressEvent (before the patch it was after such code too). Oops, I also have to remove if (event.isControl || event.isAlt || event.isMeta) from try_to_dispatch_OnKeyPressEvent.

The question above are still opened. In this patch I made some "clean" job.

Comment on attachment 306853 [details] [diff] [review] Linux patch 4 (the same as previous, but with little fixes) --- mozilla/widget/src/gtk2/nsWindow.cpp 10 Feb 2008 06:08:58 -0000 1.258 Update your source code as there have been a couple of changes near here. +inline PRBool +nsWindow::try_to_dispatch_OnKeyPressEvent(nsKeyEvent &event, nsEventStatus &status,GdkEventKey &tmpEvent) I think you are right that keeping this method is helpful, with all the case shifting code. Change the name to use CamelCase consistently instead of underlines_between_words. It would be helpful to indicate that this is only intended for shortcut key combinations. Perhaps change the name to TryShortcutKeyPressEvent()? + if (status != nsEventStatus_eIgnore) + return status == nsEventStatus_eConsumeNoDefault; + + return FALSE; When I suggested this, I was expecting "return" to mean "return from OnKeyPressEvent". Here you just need return status != nsEventStatus_eIgnore // if the control, meta, or alt key is down, then we should leave // the isShift flag alone (probably not a printable character) // if none of the other modifier keys are pressed then we need to // clear isShift so the character can be inserted in the editor + // It's done in the try_to_dispatch_OnKeyPressEvent try_to_dispatch_OnKeyPressEvent is correctly leaving isShift alone, but I find it confusing to say that this is "done in the try_to_dispatch_OnKeyPressEvent". Best to remove the added line I think. Leave the old comment there as it looks like the current code is not clearing isShift and this may need further investigation. + if ((event.isControl || event.isAlt || event.isMeta) && + !(event_consumed=try_to_dispatch_OnKeyPressEvent(event, status,tmpEvent)) ) { It's easier to read if the function call is on a separate line before the "if", with spaces around the "=" operator. + //It seems that if user typed non-Latin character with control/alt and he has + //DVORAK keyboard he would have problems. Different layouts for the same language are very + //dangerous for the code below. + if (curentCharCode) { Thanks for merging these. (Dvorak is a person's name if I recall correctly and so is just Capitalized. QWERTY is letters on a keyboard and so is UPPERCASE.) To work-around this problem I suggest: // Don't convert Latin characters to different Latin characters: that can // confuse people using multiple Latin layouts (notably Dvorak users). if (curentCharCode && !(is_latin_shortcut_key(event.charCode) && is_latin_shortcut_key(currentCharCode)) { + if (!event_consumed) + // before we dispatch a key, check if it's the context menu key. Indentation should be 4 spaces. + else { + // send the key press event + DispatchEvent(&event, status); The event is still being sent twice if not consumed and this shouldn't be happening. + } // If the event was consumed, return. Indentation issue. +inline PRBool //making it a friend may cause runtime error (I don't know the reason) +try_to_dispatch_OnKeyPressEvent(nsKeyEvent &event, nsEventStatus &status,GdkEventKey &tmpEvent); + Indent this. I don't understand the comment. Probably just remove the comment unless you want to explain. It would be best to move this declaration to just after DispatchCommandEvent. (In reply to comment #26) > (From update of attachment 306727 [details] [diff] [review]) > + if (event.isShift) > + tmpEvent.keyval = > gdk_keyval_to_upper(keyvals[n]); > + else > + tmpEvent.keyval = > gdk_keyval_to_lower(keyvals[n]); > > I don't know if this code needed. There is very similar code in the > try_to_dispatch_OnKeyPressEvent (before the patch it was after such code too). This is a more language generic (not Latin-specific) version of the code for removing the effects of CapsLock on shortcut combinations. Looks like this could be moved to replace the first block in try_to_dispatch_OnKeyPressEvent. The second block in try_to_dispatch_OnKeyPressEvent (if (!event.isControl && event.isShift) is for Alt-Shift document access keys (http://en.wikipedia.org/wiki/Access_keys). Using shift to indicate a different kind of shortcut seems dangerous to me and causes problems with punctuation (bug 359638), but best just leave this alone for now.

(In reply to comment #28) All required things are done. But I still have some problems. > + //It seems that if user typed non-Latin character with > control/alt and he has > + //DVORAK keyboard he would have problems. Different > layouts for the same language are very > + //dangerous for the code below. > > + if (curentCharCode) { > > Thanks for merging these. (Dvorak is a person's name if I recall correctly and > so is just Capitalized. QWERTY is letters on a keyboard and so is UPPERCASE.) > To work-around this problem I suggest: > > // Don't convert Latin characters to different Latin characters: that can > // confuse people using multiple Latin layouts (notably Dvorak users). > if (curentCharCode && > !(is_latin_shortcut_key(event.charCode) && > is_latin_shortcut_key(currentCharCode)) { For Latin everything ok and such code isn't required (Latin keys are consumed in event_consumed = TryShortcutKeyPressEvent(event, status,tmpEvent); But for non-Latin it may be a problem (most problems are with Cyrillic and Greek), so I did such thing: if (curentCharCode && !KeysSameLanguage(event.charCode,curentCharCode)){ KeysSameLanguage is from nsGtkKeyUtils (It's my function). It is flexible, so if some other languages will have bugs with layouts (keyboard groups) it's easy to fix it. > + if (!event_consumed) > + // before we dispatch a key, check if it's the context menu key. > > + else { > + // send the key press event > + DispatchEvent(&event, status); > > The event is still being sent twice if not consumed and this shouldn't be > happening. > > + } > > // If the event was consumed, return. Sorry, but I still not familar with this problem. Why twice? If you mean DispatchEvent it is send always twice (ot even more) for non-Latin shortcuts. And I don't see anyway to fix it there (becouse we may tries). For example, I type ctrl+Russian_u (Eng_e) - there is no shortcut for it. event_consumed = TryShortcutKeyPressEvent(event, status,tmpEvent); //first time //Then every time we a have a new key group we send this event... if (curentCharCode && !KeysSameLanguage(event.charCode,curentCharCode)){ event.charCode = curentCharCode; event_consumed = TryShortcutKeyPressEvent(event, status,tmpEvent); I don't see the way to prevent it. But evеrything works (in ui-review). And why is it bad? If we send badkey than nothing happens. So eventlistener may think we say different events, but it doesn't make any case... I'm not strong in it, so IMHO. > > The second block in try_to_dispatch_OnKeyPressEvent (if (!event.isControl && > event.isShift) is for Alt-Shift document access keys > (http://en.wikipedia.org/wiki/Access_keys). Using shift to indicate a > different kind of shortcut seems dangerous to me and causes problems with > punctuation (bug 359638), but best just leave this alone for now. > Ok, thanks for the link. I will leave it as it is.

(In reply to comment #29) > For Latin everything ok and such code isn't required (Latin keys are consumed > in event_consumed = TryShortcutKeyPressEvent(event, status,tmpEvent); Only consumed if there is a binding (associated shortcut) I assume? But this should be fixed by your function below. > But for non-Latin it may be a problem (most problems are with Cyrillic and > Greek), so I did such thing: > if (curentCharCode && !KeysSameLanguage(event.charCode,curentCharCode)){ > KeysSameLanguage is from nsGtkKeyUtils (It's my function). It is flexible, so > if some other languages will have bugs with layouts (keyboard groups) it's > easy to fix it. Sounds good. Have you seen g_unichar_get_script()? If you use this, KeysSameScript might be a better name. It might be a good idea to consider G_UNICODE_SCRIPT_COMMON the same as any other script too (i.e. KeysSameLanguage return true if either are common). That could prevent punctuation from being picked up from other layouts (and I think that would be good).

(In reply to comment #29) > Sorry, but I still not familar with this problem. Why twice? If you mean > DispatchEvent it is send always twice (ot even more) for non-Latin shortcuts. > And I don't see anyway to fix it there (becouse we may tries). For example, I > type ctrl+Russian_u (Eng_e) - there is no shortcut for it. > event_consumed = TryShortcutKeyPressEvent(event, status,tmpEvent); //first time > > //Then every time we a have a new key group we send this event... > if (curentCharCode && !KeysSameLanguage(event.charCode,curentCharCode)){ > event.charCode = curentCharCode; > event_consumed = > TryShortcutKeyPressEvent(event, status,tmpEvent); Yes, that's fine. There is no way to avoid that. But the event is sent once more (if not consumed) here: + if (!event_consumed) + // before we dispatch a key, check if it's the context menu key. + else { + // send the key press event + DispatchEvent(&event, status); If this event has already been through TryShortcutKeyPressEvent, then it doesn't need to be sent again here. Perhaps rename event_consumed to eventProcessed and set it to true at the end of the block with does TryShortcutKeyPressEvent.

Please coordinate with bug 359638 -- looks to me these are closely related.

(In reply to comment #30) > Have you seen g_unichar_get_script()? If you use this, KeysSameScript might > be a better name. It might be a good idea to consider G_UNICODE_SCRIPT_COMMON > the same as any other script too (i.e. KeysSameLanguage return true if either > are common). That could prevent punctuation from being picked up from other > layouts (and I think that would be good). I had another implementation, but it's better and I updated the function as you told, thanks. (In reply to comment #31) > If this event has already been through TryShortcutKeyPressEvent, then it > doesn't need to be sent again here. > > Perhaps rename event_consumed to eventProcessed and set it to true at the end > of the block with does TryShortcutKeyPressEvent. > Oh, this. Thanks for the explanation. I put the code to the else of if (event.charCode). So problem should be fixed. I hope nothing else is incorrect, so ask for review.

I think the latest patch of this bug is not needed. Because my posted patch in bug 359638 fixes similar bugs in XP level. And the patch doesn't depend on keyboard layout.

Comment on attachment 307444 [details] [diff] [review] Linux, try5 I'll comment on this patch because it looks very close, but we still need to decide whether to do this or use an alternative as suggested in bug 359638. + if (is_context_menu_key(event)) { + nsMouseEvent contextMenuEvent(PR_TRUE, NS_CONTEXTMENU, this, + nsMouseEvent::eReal, + nsMouseEvent::eContextMenuKey); + key_event_to_context_menu_event(contextMenuEvent, aEvent); + DispatchEvent(&contextMenuEvent, status); + } + if (event.charCode) { I think you want "else if (event.charCode)" here, so that the event is not dispatched twice. If so remove the blank line. + event_consumed = TryShortcutKeyPressEvent(event, status,tmpEvent); + if ((event.isControl || event.isAlt || event.isMeta) && !event_consumed ) { I see that my suggestion has made things complicated here, sorry. TryShortcutKeyPressEvent is currently only intended for shortcut keys. i.e. when (event.isControl || event.isAlt || event.isMeta) It looks like CapsLock won't work for Latin characters. Maybe its best to revert to your previous code here (including the function in the condition). But put a space between "," and "tmpEvent". Also, if you do that, then the event still needs dispatching when (event.charCode) but not (event.isControl || event.isAlt || event.isMeta). + if (curentCharCode && !KeysSameScript(event.charCode,curentCharCode)){ Follow file style, with spaces between ",currentCharCode" and "){" +KeysSameScript(guint key1, guint key2) This function looks good, but the arguments are really character codes rather than keys and it would be good to make this clear. I think this is probably better: CharCodesSameScript(guint code1, guint code2)

There are currently two proposed solutions to this issue. 1. dispatch the event using the current layout first, then try other layouts. (As suggested in comment 16. The patch here that is almost complete for Linux but we don't have a Windows patch yet.) 2. Include all the information in the event so that event handling code can decide which information to use. (This is the approach used in attachment 307497 [review]. I don't think that patch is complete enough for a solution to this bug yet but I'll discuss that in bug 359638.) The nsEventStatus_eIgnore/eConsumeNoDefault/eConsumeDoDefault seems suitable for implementing Option 1. The risk is that some code may not return the correct code. nsEventStateManager seems to return the correct code. Do we know of any code that doesn't? Option 2 is less risky but I don't think it can be a complete solution. With this option: * It would be feasible to access Latin command keys (Ctrl) from non-Latin layouts (by including a Latin code in the event). * To access localized menu-access keys from other layouts (including Latin), it would be necessary to look for a key with a code that corresponded to the language of the localization. I assume this is possible, but we don't have code for this yet. * For a document with access keys in a language that is neither Latin nor the localization, the shortcut would only be available from the layout with that key (unless we were to include codes for all layouts in the event). * Native keybindings for editable text areas would need to decide which events to try. This is possibly the biggest hindrance to this approach. If a code from a non-selected layout is tried here, then that will take precedence over a XUL (or maybe other?) binding in the selected layout. Instead all types of bindings in the selected layout should be tried first, and Latin or other codes should only be tried if there was no binding for the selected layout. This was the problem in bug 411005 and is still a problem in GTK bug http://bugzilla.gnome.org/show_bug.cgi?id=162726.

Karl: Bug 400858 is a problem for me. «View» translated into Russian has first letter Russian_D. So it's not activated when I type En_D (and works with Rus_D). It's out of this scope, but it blocks us.

Karl, do we mean we may check for alt+non-Latin characters first to fix Bug 400858? The code will be ugly (and it would have several blocks like first version), but it will fix those bug.

Don't my comment #39, it's my fault.

please test on this html file. you can see two or more alerts for one shortcut/access key inputting with your patch. it's very bad thing for the compatibility.

Masayuki: thanks. Sad... We should to use another approach...

(In reply to comment #42) > Masayuki: thanks. Sad... We should to use another approach... Yes. Therefore I proposed |nsKeyEvent::localizedCharCodes|. I think we should fix bug 359638. And next, we should fix this bug. Because my bug 359638 is only for *single* keyboard layout environment. But this is for multiple. So, my bug is easier than this bug.

And note that I'm not a specialist for GTK2 code. Therefore, I hope that you improve the my patch of gtk2 part after bug 359638.

Comment on attachment 302109 [details] [diff] [review] for Linux Masayuki: ok. Since my job is stopped (I have to start from zero), let's wait for you results. I'm not very confident with gtk2, but I will do everything to help you if you need and to improve anything required to be improved. So in this case I ask for approval of my first patch.

(In reply to comment #41) > testcase Thank you, Masayuki for this.

Comment on attachment 302109 [details] [diff] [review] for Linux This patch doesn't need approval1.9 because this bug is marked blocking1.9. However we need to touch up attachment 303030 [details] [diff] [review] for Windows. With those, then at least people will be able to access menus, even though it requires changing layout sometimes. The ru localization hasn't changed to using Latin menu access keys. Have any localizations changed yet?

(In reply to comment #47) > (From update of attachment 302109 [details] [diff] [review]) > This patch doesn't need approval1.9 because this bug is marked blocking1.9. Ok. I thought every patch should be approved, thanks for information. > However we need to touch up attachment 303030 [details] [diff] [review] for Windows. Ok, the new version is coming soon. > With those, then at least people will be able to access menus, even though it > requires changing layout sometimes. Yes, but the bug would still be opened. > The ru localization hasn't changed to using Latin menu access keys. Have any > localizations changed yet? Do you mean changing from Localized to Latin access keys? I don't think it may help. In this case we should start convert all non-Latin shortcuts to Latin and this may be bad for non-menu access keys (if they may be localized).

(In reply to comment #15) > (From update of attachment 303030 [details] [diff] [review]) > Note that mIsControlDown ^ mIsAltDown excluded the mIsControlDown && > mIsAltDown case which apparently is equivalent to AltGr on some MS Windows > keyboards (bug 341308 comment 1). > > So we need "mIsControlDown && !mIsAltDown" here. > The comments in the paragraph below also need updating to exclude the > references to Alt+*. Done. > I don't know whether or not we should have a similar test here: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/windows/nsWindow.cpp&rev=3.726&mark=3241#3235 > We did before attachment 215299 [details] [diff] [review] changed the behavior. > Ere, do you know what the appropriate test here should be? I don't know too. But if everything works with one check let's leave it alone.

Evgeniy: Please try after my patch. We must not modify the charCode even when the ctrl or alt is pressed. So, the our current code is wrong. First, I correct it in bug 359638.

Masayuki: don't worry, I did it just for review. Nobody would commit before you.

(In reply to comment #48) > Do you mean changing from Localized to Latin access keys? No. > I don't think it may help. In this case we should start convert > all non-Latin shortcuts to Latin and > this may be bad for non-menu access keys (if they may be localized). I agree. I was just concerned that some localizations may have already made this change (because that is currently the only way menu access keys can work).

(In reply to comment #52) > I agree. I was just concerned that some localizations may have already made > this change (because that is currently the only way menu access keys can work). I hope before ff3 released they will see they were wrong :)

Comment on attachment 307925 [details] [diff] [review] Windows, try2 This would improve the current situation (and be good enough to fix this bug). A fix for bug 414130 (perhaps in 359638) might also fix this bug. Getting Latin access keys activated from non-Latin layouts is bug 177508. The approach of storing the Latin code separately as used in bug 359638 would enable this bug to be fixed while keeping bug 177508 (mostly?) fixed. But I'd like to get something in soon so that localizers can test their keys. Getting localized menu access keys activated from Latin layouts can be another bug.

Changing priority from P2 to P1, because I think localizers need this ASAP, and because Beta testing would be helpful, and we have enough here for at least a partial fix, if we don't get a more complete fix in time for Beta 5.

(In reply to comment #50) > Please try after my patch. We must not modify the charCode even when the ctrl > or alt is pressed. So, the our current code is wrong. First, I correct it in > bug 359638. Note that Evgeniy's patch would mean that the charCode is modified less, so the situation would be better than now. But Masayuki's approach of storing the Latin code separately is a better solution (that we can use if that is ready in time).

I agree with Karl. It will give localizers the ability to test and also Masayuki can then remove it with his patch (as I saw Linux part fully). Also as soon as I can I will try to fix Latin_key+Localized_menu like Masayuki fixed his (use the same approach).

Is the plan to do bug 359638 between beta5 and rc1? (Is the end result of fixing that the ideal state that we want to be in?) If so, in what ways does this patch get us closer to that state, and in what ways does it get us farther?

David: these patches just prevent keycodes from converting to Latin (only for alt shortcuts). So alt+nonLatin menu (for localized UI) and web accesskeys will work. Also as Karl tell it's good to give localizators the ability to test their localizations. But it's a little problem with Bug 359638: it fixes many things in a very good way (the job Masayuki did is great), I don't follow those bug, but it seems it's not ready. But it will modify the code from my patches. Farther direction is to fix the problem with alt+Latin and localized menus (we have the same like had with ctrl+latin).

(In reply to comment #58) > Is the plan to do bug 359638 between beta5 and rc1? I hope so. > (Is the end result of fixing that the ideal state that we want to be in?) Not quite but closer. > If so, in what ways does this patch get us closer to that state, Evgeniy is right. Currently non-Latin access keys are inaccessible. This patch makes them accessible (from the layout that provides the characters). > and in what ways does it get us farther? Currently Latin access keys are accessible from non-Latin layouts. With this patch, users will need to be in Latin layout to access Latin access keys. Although not ideal, I doubt this is a big problem. Users with non-Latin scripts on their keyboard need to switch to Latin layout often (or might usually be there) because not many URLs are in native scripts. The patch in bug 359638 would make non-Latin access keys accessible from a particular layout while keeping Latin access keys accessible from all layouts. The ideal situation would be to have access keys in the language of the localization accessible from all layouts.

(In reply to comment #60) > Currently Latin access keys are accessible from non-Latin layouts. > With this patch, users will need to be in Latin layout to access Latin access > keys. Although not ideal, I doubt this is a big problem. Users with non-Latin > scripts on their keyboard need to switch to Latin layout often (or might > usually be there) because not many URLs are in native scripts. I don't agree with this argument. With the awesome bar users can get to previously visited URLs without switching layout, and anyway average users don't often type in URLs, they click on links.

(In reply to comment #61) Thank you for your comments, Simon. You and Evgeniy know the issues re multiple layouts much better than I. I just want to make it clear that this is changing access keys (Alt) not accel keys (Ctrl). > (In reply to comment #60) > > > Currently Latin access keys are accessible from non-Latin layouts. With > > this patch, users will need to be in Latin layout to access Latin access > > keys. Although not ideal, I doubt this is a big problem. Users with > > non-Latin scripts on their keyboard need to switch to Latin layout often > > (or might usually be there) because not many URLs are in native scripts. > > I don't agree with this argument. With the awesome bar users can get to > previously visited URLs without switching layout, and anyway average users > don't often type in URLs, they click on links. I wonder how many such average users use access keys. Maybe it is a bigger problem than I imagined. But I guess the main point I was trying to make was that having access keys in some localizations completely inaccessible seems a bigger problem than having access keys in Latin localizations only accessible from a Latin layout. I thought the patches here were a step forward (if we must view the world as a one-dimensional line). Do you disagree with that?

Since we're talking about taking bug 359638 after beta5, which is at least as invasive as this patch, why does this patch have to block beta5?

Comment on attachment 302109 [details] [diff] [review] for Linux Requesting ui-review, and feedback on whether this should land for Beta 5. Really this deserves a late-l10n keyword but we are l10n-frozen for Beta 5. Please see comment 60 to 62.

(In reply to comment #63) > Since we're talking about taking bug 359638 after beta5, which is at least as > invasive as this patch, why does this patch have to block beta5? I changed target to Beta 5 so that localizers could get the change ASAP. As they now can't make changes, the Beta 5 milestone is not so important.

Per comment 65, moving this off the beta 5 blocking queue. Localizers would not be able to act on this change until *after* the beta 5 milestone, at which point (if I'm understanding things correctly) we're hoping that the issue will be resolved by bug 359638. I removed myself as a ui-reviewer as I do not believe that I am the best person to make this decision without more information. Axel knows better than I do what people in various locales will find most natural. The proposed solutions in comment 36 both seem sensible to me, with option 1 seeming like the easiest and most reasonable. From a UI perspective, it is my belief that keyboard accelerators for menuItems should work, by default, in Firefox as they do in other localized applications for that locale - the goal should be that the same key combination used to get to "quit" and "new" in Windows Explorer or the Finder are the ones used in Firefox. (Though I do agree that the absolute ideal is such that's proposed in comment 36 option 1, a situation where the default for a locale is used and the english is used as a fallback) Axel: your thoughts on the right thing to do here?

If the milestone the same as for bug 359638 we should wait for it. If I have time these weeks I will try to follow its discussion and code to start solve the problem as those patch is checked in.

Sadly, I know jack about keyboards myself. I always was on a latin keyboard, though I find keyboard layouts heck-confusing when going between US, DE and FR. I really lack any background in making a call on this beyond: - local accesskeys must work. - if one can reliably correlate a key to a latin char, I don't mind trying to send that, too. I'd be heck-confused though to see Alt-z triggering _Y_ankee from a German keyboard though. Or to get other funkynesses on some dvorak layout. I.E., if I see only latin keys on my keyboard, I really don't want to see keyboard events sent for an imaginary qwerty keyboard. Does that help? I've been reading through http://en.wikipedia.org/wiki/Keyboard_layout, and it didn't really help me in finding out if there is a reliable mapping or not.

Axel, I saw some code, which ckecks the menu accesskeys (and if I'm not mistaken returns something like widget). I have shown it to Karl, but can't find text file with mxr links. In this case it may be safe to check menus twice: every menu has only localized or Latin accesskey. P.S. to Karl: if you have IRC private history, please send me it (the messages with links about alternative approach fixing the bug). I guess I lost the txt with it... I always loose something (((

Comment on attachment 307925 [details] [diff] [review] Windows, try2 Is this ready to go?

Comment on attachment 307925 [details] [diff] [review] Windows, try2 No, it's not ready. This code depends on Bug 359638 (the code I patch is removed in that patch). Also Bug 359638 fixes the most part of this patch. Only alt+Latin for non-Latin layouts (keyboards) doesn't work (and we need to decide if it's a bug or expected behavior).

From changes committed yesterday for Bug 359638, accesskeys requiring localized characters should now accessible from layouts that provide those characters. So the regression from the change in behavior originally reported should now be fixed. (Please reopen if not.) (In reply to comment #71) > Only alt+Latin for non-Latin layouts (keyboards) doesn't work (and we need to > decide if it's a bug or expected behavior). If the only issue now is that localized access keys are not accessible from Latin layouts, then that is best filed as a separate bug. (I think that we could and probably should make that work, but that would be a new feature.)

(In reply to comment #72) > If the only issue now is that localized access keys are not accessible from > Latin layouts, then that is best filed as a separate bug. (I think that we > could and probably should make that work, but that would be a new feature.) Unfortunately I haven't time before summer (much study + from May GSoC (as I hope) ). If nobody would I will return to it later (in a separate Bug).

(In reply to comment #72) > If the only issue now is that localized access keys are not accessible from > Latin layouts, then that is best filed as a separate bug. (I think that we > could and probably should make that work, but that would be a new feature.) Bug 87253.