Last Comment Bug 452393 - Accelerators should not be affected by keyboard layout Part Deux.
: Accelerators should not be affected by keyboard layout Part Deux.
Status: VERIFIED FIXED
[key hell]
: intl
Product: Core
Classification: Components
Component: Widget: Gtk (show other bugs)
: unspecified
: x86 Linux
: -- normal with 9 votes (vote)
: mozilla13
Assigned To: Simon Montagu :smontagu
:
Mentors:
Depends on:
Blocks: 890469 359638
  Show dependency treegraph
 
Reported: 2008-08-27 04:13 PDT by Dotan Cohen
Modified: 2015-06-26 16:33 PDT (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Possible patch (1.49 KB, patch)
2008-09-03 02:53 PDT, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
Patch v.2 (1.63 KB, patch)
2008-09-03 08:16 PDT, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
Patch v.3 (2.12 KB, patch)
2012-03-08 00:12 PST, Simon Montagu :smontagu
masayuki: review+
karlt: review+
Details | Diff | Splinter Review

Description Dotan Cohen 2008-08-27 04:13:45 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.1) Gecko/2008072820 Firefox/3.0.1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.1) Gecko/2008072820 Firefox/3.0.1

Spinnoff of Bug #69230.

It appears that a new regression in Firefox 3.x prevents some, but not all, keyboard accelerators from working in specific keyboard layouts.

With a Hebrew keyboard layout, I can confirm that CTRL-Q and CTRL-W do not work, though CTRL-A, Z, X, C, V, P, F, H, B, and L do work as expected. Other users are encouraged to specify their keyboard layout here, and to specify which keyboard accelerators do work and which do not.

Reproducible: Always

Steps to Reproduce:
1. Enable non-US keyboard layout
2. Press accelerator key combination

Actual Results:  
Nothing happens

Expected Results:  
Accelerator combination should perform it's function
Comment 1 Nick Demou 2008-08-27 04:28:37 PDT
ALL accelerators mentioned (plus Ctrl-T) work fine with Greek keyb layout on  Ubuntu Firefox/3.0.1 -- Mozilla/5.0 (X11; U; Linux i686; el-GR; rv:1.9.0.1) Gecko/2008072820 
Comment 2 Eyal Rozenberg 2008-08-28 03:36:59 PDT
Ctrl+W works for me with FF 3.0.1 and Hebrew layout on Win XP. Don't remember Ctrl+Q should be doing...
Comment 3 Haggai Eran 2008-08-30 05:21:31 PDT
I'm using Firefox 3.0.1 on Ubuntu linux, and I can confirm Dotan's report that Ctrl-W and Ctrl-Q doesn't work when the current keyboard layout is Hebrew.

Regards,
Haggai
Comment 4 Dotan Cohen 2008-08-30 10:52:30 PDT
I am also using Ubuntu Linux with Firefox 3, so this may be a Linux-specific or Ubuntu specific issue.
Comment 5 Simon Montagu :smontagu 2008-08-30 12:53:24 PDT
I can only reproduce this on Linux (also Ubuntu). Note that Q and W are keys that are not mapped to alphabetical characters in the Hebrew layout.
Comment 6 Yehuda 2008-08-30 13:19:57 PDT
Reproduced using CentOS 5.2, and Hebrew layout.
Everything works except Ctrl-W and Ctrl-Q.
Comment 7 Tomer Cohen :tomer 2008-08-30 13:53:01 PDT
(In reply to comment #5)
> I can only reproduce this on Linux (also Ubuntu). Note that Q and W are keys
> that are not mapped to alphabetical characters in the Hebrew layout.

smontagu: Do you have any idea if and how we can fix it?
Comment 8 Tomer Cohen :tomer 2008-08-30 14:27:39 PDT
Adding the 'intl' keyword. Someone should do a batch run to replace every bug with the 'i18n' keyword with 'intl' since we don't have that keyword any more...
Comment 9 Simon Montagu :smontagu 2008-09-03 02:53:42 PDT
Created attachment 336634 [details] [diff] [review]
Possible patch

A bit hacky, but it works. It's unfortunate that mochitests for keyhandling don't cover GTK
Comment 10 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-09-03 04:33:53 PDT
The patch is pretty hacky.

I think that when '/' or ''' key is pressed, we should go to |if (!isLatin)| block.

It seems that one of unshifted char and shifted char is alphabet but other is not so, then, we should go to the block.
Comment 11 Simon Montagu :smontagu 2008-09-03 08:16:19 PDT
Created attachment 336660 [details] [diff] [review]
Patch v.2

Do you mean like this? This is what I wanted to do in the first place, but I didn't want to revert the change from is_latin_shortcut_key to (code <= 0xFF) in bug 359638.

I've tested that this doesn't regress bug 427932 or bug 433192. Can you suggest other scenarios I should be testing?
Comment 12 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-09-03 18:55:22 PDT
No. I think:

+ PRBool needLatinKeyCodes = !isLatin;
+ if (!needLatinKeyCodes) {
+     PRBool isAlpha1 = NS_IS_ALPHA(altCharCodes.mUnshiftedCharCode);
+     PRBool isAlpha2 = NS_IS_ALPHA(altCharCodes.mShiftedCharCode);
+     needLatinKeyCodes = (isAlpha1 && !isAlpha2) || (!isAlpha1 && isAlpha2);
+ }

- if (isLatin) {
+ if (needLatinKeyCodes) {

I believe this is safest way for other key layouts...
Comment 13 Karl Tomlinson (:karlt) 2008-09-03 20:00:11 PDT
(In reply to comment #12)
> + PRBool needLatinKeyCodes = !isLatin;
> + if (!needLatinKeyCodes) {
> +     PRBool isAlpha1 = NS_IS_ALPHA(altCharCodes.mUnshiftedCharCode);
> +     PRBool isAlpha2 = NS_IS_ALPHA(altCharCodes.mShiftedCharCode);
> +     needLatinKeyCodes = (isAlpha1 && !isAlpha2) || (!isAlpha1 && isAlpha2);
> + }
> 
> - if (isLatin) {
> + if (needLatinKeyCodes) {

NS_IS_ALPHA uses isalpha, which depends on locale, so may produce some
surprises.

We also need to be careful about changing event.charCode if there is a Latin
character is the current group.  If Ctrl-/ is a meaningful shortcut, then I
guess it should be preferred over Ctrl-Q (with Ctrl-Q tried as fallback).

Simon, Dotan, Eyal, Haggai, or Yehuda, or Tomer, tell me if a Hebrew user
would think otherwise?

I can think of two possible alternatives for needLatinKeyCodes:

a) Provide the basic Latin letter or numeral as an alternative when and only
   when that basic letter or numeral is not available on level 0 of the
   current group, or

b) Look for a basic Latin letter or numeral only when the current group does
   not look like a Latin layout.  That perhaps can be tested through looking
   for one or more specific Latin letters on level 0 of the current layout.  I
   wonder whether any layouts have an "a" but not the rest of the Latin
   alphabet.  I wonder whether any Latin layouts do not have "z".
Comment 14 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-09-03 21:16:50 PDT
(In reply to comment #13)
> NS_IS_ALPHA uses isalpha, which depends on locale, so may produce some
> surprises.

NS_IS_ALPHA also checks whether the char is ASCII. So, if the result is true, should be ASCII alphabets.

> We also need to be careful about changing event.charCode if there is a Latin
> character is the current group.  If Ctrl-/ is a meaningful shortcut, then I
> guess it should be preferred over Ctrl-Q (with Ctrl-Q tried as fallback).

We should not modify charCode. I think Ctrl+'/' should win. Because if Ctrl+'Q' wins, Ctrl+'/' cannot be accessible. Of course, all UI developers should not use non-alphabets and non-numeric keys for shortcut.

I think that win32 build behaves so, perhaps.
We should use same processing on all platforms if we can.
Comment 15 Karl Tomlinson (:karlt) 2008-09-03 21:26:31 PDT
(In reply to comment #14)
> NS_IS_ALPHA also checks whether the char is ASCII.
> So, if the result is true, should be ASCII alphabets.

It checks whether the char is <= 0x7f.  Locale also includes the charmap.  If every charmap has the same (ASCII) meaning for the first 0x7f characters, then it should be safe.  But I don't know whether that is a safe assumption.
("locale -m" gives a list of available charmaps.)
Comment 16 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-09-03 21:36:41 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > NS_IS_ALPHA also checks whether the char is ASCII.
> > So, if the result is true, should be ASCII alphabets.
> 
> It checks whether the char is <= 0x7f.  Locale also includes the charmap.  If
> every charmap has the same (ASCII) meaning for the first 0x7f characters, then
> it should be safe.  But I don't know whether that is a safe assumption.
> ("locale -m" gives a list of available charmaps.

We should use |('a' <= ch && ch <= 'z') || ('A' <= ch && ch <= 'Z')| ?
Comment 17 Karl Tomlinson (:karlt) 2008-09-03 21:48:14 PDT
(In reply to comment #16)
> We should use |('a' <= ch && ch <= 'z') || ('A' <= ch && ch <= 'Z')| ?

I'd feel safest with that.
Comment 18 Simon Montagu :smontagu 2008-09-03 21:50:28 PDT
(In reply to comment #14)
> We should not modify charCode. I think Ctrl+'/' should win. Because if Ctrl+'Q'
> wins, Ctrl+'/' cannot be accessible. Of course, all UI developers should not
> use non-alphabets and non-numeric keys for shortcut.

I was surprised to discover that Ctrl+/ is a shortcut for "Select All" in text fields on GTK. This is rather unpleasant: if a text field is focused, Ctrl+/ is meaningful but Ctrl+' is not; otherwise neither is meaningful. If we say that the non-alpha shortcut wins iff it is meaningful, the same key combination will be "Select All" when focus is in a text field and "Quit" when it is elsewhere, which is a combination that could easily cause data loss if a user enters text, unfocusses the text field without realising and then tries to select all.

On the other hand, if the non-alpha shortcut always wins (the status quo), Ctrl-W and Ctrl-Q are inaccessible in the Hebrew layout.

In the specific case of the Hebrew layout, I'm fairly sure that users expect these keys to produce Ctrl-Q and Ctrl-W, not Ctrl-/ and Ctrl-', and it's also not the end of the world if Ctrl-/ is inaccessible, since it's synonymous with Ctrl-A. We need to check whether other layouts will be affected, though.
Comment 19 Haggai Eran 2008-09-03 22:10:49 PDT
I agree with Simon, that as a Hebrew user, I expect the shortcut to stay in their places even when I change the current layout. This means that Ctrl-Q should be preffered over Ctrl-/.
I'm not sure this must mean that Ctrl-/ is inaccessible. The Hebrew keyboard layout has '.' instead of '/' , so why can't Ctrl-. mean Ctrl-/ when the Hebrew layout is selected?
Comment 20 Dotan Cohen 2008-09-03 23:58:12 PDT
@Karl:
> We also need to be careful about changing event.charCode if there
> is a Latin character is the current group.  If Ctrl-/ is a meaningful
> shortcut, then I guess it should be preferred over Ctrl-Q (with
> Ctrl-Q tried as fallback).

I'm not sure that I understand, but the Latin Ctrl-/ shortcut I have never heard of. I say that as a keyboard power user, so I feel reasonably confident that it is a rather unknown shortcut. Personally, I would have no problem with Ctrl-Q NOT working in Hebrew, as I have only ever hit it by accident and I think that an operation as deadly as closing the app should not have a keyboard shortcut, especially when that key borders three very common shortcuts: Ctrl-W, Ctrl-A and Ctrl-TAB. That said, Ctrl-W is a critical shortcut that we do need.

Would it be reasonable to suggest that we sacrifice Ctrl-Q in the Hebrew layout for these two reasons:
1) It may interfere with another shortcut (Ctrl-/)
2) It is a dangerous and seldom-used shortcut
Comment 21 Alex Anders 2008-10-11 00:06:44 PDT
When you guys try to solve this please also test with the Dvorak keyboard layout. 
Because several users reported in this ticket here: https://bugzilla.mozilla.org/show_bug.cgi?id=439815 (which turned out to be a Mac only bug) 
that they have these shortcut issues on Windows with the Dvorak keyboard layout. 

I've tested that too and *ALL* keyboard shortcuts were broken with Dvorak layout. 
The whole thing is especially annoying when you have such shortcuts as CTRL+C, CTRL+V etc. broken...

So, please test with Dvorak layout! 
That might help to solve the issue entirely.
Comment 22 Masayuki Nakano [:masayuki] (Mozilla Japan) 2008-12-12 21:20:24 PST
(In reply to comment #18)
> On the other hand, if the non-alpha shortcut always wins (the status quo),
> Ctrl-W and Ctrl-Q are inaccessible in the Hebrew layout.

In this case, Ctrl-W and Ctrl-Q should be accessible by Ctrl-Shift-W and Ctrl-Shift-Q. Doesn't work so?

> In the specific case of the Hebrew layout, I'm fairly sure that users expect
> these keys to produce Ctrl-Q and Ctrl-W, not Ctrl-/ and Ctrl-', and it's also
> not the end of the world if Ctrl-/ is inaccessible, since it's synonymous with
> Ctrl-A. We need to check whether other layouts will be affected, though.

If most Hebrew users think that alphabets should always win, we should do so. But then, we may make / and ' are accessible when Shift key is pressed.
Comment 23 Haggai Eran 2008-12-13 02:35:01 PST
(In reply to comment #22)
> (In reply to comment #18)
> > On the other hand, if the non-alpha shortcut always wins (the status quo),
> > Ctrl-W and Ctrl-Q are inaccessible in the Hebrew layout.
> 
> In this case, Ctrl-W and Ctrl-Q should be accessible by Ctrl-Shift-W and
> Ctrl-Shift-Q. Doesn't work so?

You are assuming that all users have their shift key mapped to temporarily switch back to English keyboard. Perhaps its true for most users but not all. For instance, the variant lyx of the Hebrew keyboard on Linux is used to enter diacritics when the shift key is pressed.
Comment 24 Dotan Cohen 2008-12-29 05:48:48 PST
As mentioned on this page:
http://lindesk.com/2008/12/kde-vs-gnome-a-dvorak-users-perspective/

Gnome temporarily switches to Qwerty when an accelerator key is pressed. As both Gnome and Firefox are GTK based, couldn't Firefox do the same? This does look like the ideal solution.
Comment 25 Tomer Cohen :tomer 2008-12-29 06:17:24 PST
(In reply to comment #24)
> Gnome temporarily switches to Qwerty when an accelerator key is pressed.
Why should we? When I'm pressing Ctrl-P for print or Alt-F for File menu, I want it to be the same letter as without the accelerator. In case I have Dvorak letters printed on my keyboard, I won't have a clue where is the F letter is hiding in Qwerty (well... kind of).
Comment 26 Dotan Cohen 2008-12-29 07:17:16 PST
@Tomer: That's why it should be optional.
Comment 27 Tomer Cohen :tomer 2009-06-20 06:43:25 PDT
Issue still exists with Firefox 3.5rc2, Ubuntu Linux 9.04.

Same issue reported in Ubuntu Launchpad - https://bugs.launchpad.net/ubuntu/+source/firefox-3.0/+bug/239222
Comment 28 Shimi Meraro 2010-02-03 14:24:15 PST
Issue still exists with Firefox 3.6, Ubuntu Linux 9.10.
Comment 29 Tomer Cohen :tomer 2010-04-19 16:26:22 PDT
Issue is still relevant for 3.6.x and this bug is actually has a workaround. Any chance to land it by 1.9.3?
Comment 30 Artyom 2010-04-20 17:09:03 PDT
I'm also affected with this bug, and it's really annoying. Hope this will be fixed soon.
Comment 31 Yotam Benshalom 2012-03-06 15:58:26 PST
Four years, the issue is still here and is as disturbing as ever. Is there any progress?
Comment 32 Tomer Cohen :tomer 2012-03-06 16:03:17 PST
Smontagu: The purposed patch you created is from 2008, and AFAIK never got committed into the tree. It might be good idea to invalidate it and re-create it for more recent Firefox version.
Comment 33 Simon Montagu :smontagu 2012-03-06 20:49:09 PST
Masayuki didn't approve my patch (comment 12). I can try to produce a new one based on subsequent comments, but I'm basically too chicken to fix this bug, since my experience with keyboard bugs is that every fix breaks something else.
Comment 34 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-06 21:21:25 PST
I'm going to work on reimplementing the keycode computation on Linux. If you write the patch for this, I hope you do it quickly.

FYI: the code was moved to nsGtkKeyUtils.cpp.
Comment 35 Simon Montagu :smontagu 2012-03-08 00:12:52 PST
Created attachment 603986 [details] [diff] [review]
Patch v.3

I think that Masayuki and Karl both agreed on this approach in the comments.
Comment 36 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-08 01:02:55 PST
Comment on attachment 603986 [details] [diff] [review]
Patch v.3

I believe that this improves the usability of Hebrew users at least for now, and we should land such patch ASAP because the deadline of next merge is next week.

However, I'm concerned by following situation.

If Hebrew users is using normal Hebrew layout with Dvorak, then, I think that when Ctrl+ד is pressed, the first alternative char codes are (ד, S). And second alternative char codes are (o, O). So, there is 'S' vs 'O'. I feel it's very strange. If this is not OK, it might be better to clear the 'S' from the first alternative char codes and the charCode when shift key is pressed. Anyway, this must be minor.
Comment 37 Karl Tomlinson (:karlt) 2012-03-08 19:29:20 PST
Comment on attachment 603986 [details] [diff] [review]
Patch v.3

I may have felt safer if the charCode were not overridden when ascii.  Ctrl-A is beginning-of-line in my settings with emacs key theme, but Ctrl-A Shift-Ctrl-E is available if Ctrl-/ is not available.  I never liked having Ctrl-Q as a short cut for quit.  Keyboard input does have different behaviour depending on what is focused, and I don't think we'll be able to change that.

But this is all a guessing game and my bike-shedding shouldn't get in the way of fixing Ctrl-W.  Are you able to add a comment indicating that the new block takes effect in some Hebrew layouts, please?
Comment 39 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-03-09 02:26:09 PST
(In reply to Karl Tomlinson (:karlt) from comment #37)
> I never liked having
> Ctrl-Q as a short cut for quit.  Keyboard input does have different
> behaviour depending on what is focused, and I don't think we'll be able to
> change that.

I think that it's not minor issue if a lot of Linux users want to use emacs like shortcut keys on Fx. At least, I think that it should be able to be disabled by pref like BackSpace key. And/Or it should show confirmation dialog.
Comment 40 Marco Bonardo [::mak] 2012-03-09 08:04:31 PST
https://hg.mozilla.org/mozilla-central/rev/2f21548bcbc4
Comment 41 :Ms2ger (⌚ UTC+1/+2) 2012-03-09 08:16:10 PST
PRBool?
Comment 42 Shahar Or 2012-03-09 13:15:58 PST
Haleluya...
Comment 43 Emin Mastizada 2015-03-01 09:07:46 PST
We have same problem with Azerbaijani keyboard layout, hotkey Ctrl+W not works as its Ctrl+Ü in Azerbaijani layout.
Bug: 1116180
Comment 44 Marco Bonardo [::mak] 2015-06-26 16:33:19 PDT
Please don't reopen old bugs, especially when they are older than a week. Create a new one for your case in the appropriate component.

Note You need to log in before you can comment on or make changes to this bug.