452393 - Accelerators should not be affected by keyboard layout Part Deux.

Status: VERIFIED FIXED

(Core :: Widget: Gtk, defect)

Description

[Dotan Cohen]

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]

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]

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]

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]

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]

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]

Reproduced using CentOS 5.2, and Hebrew layout.
Everything works except Ctrl-W and Ctrl-Q.

Comment 7

[Tomer Cohen :tomer]

(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]

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]

Attachment: Possible patch

A bit hacky, but it works. It's unfortunate that mochitests for keyhandling don't cover GTK

Comment 10

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

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]

Attachment: 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] (he/him)(JST, +0900)]

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)]

(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] (he/him)(JST, +0900)]

(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)]

(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] (he/him)(JST, +0900)]

(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)]

(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]

(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]

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]

@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]

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] (he/him)(JST, +0900)]

(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]

(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]

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]

(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]

@Tomer: That's why it should be optional.

Comment 27

[Tomer Cohen :tomer]

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]

Issue still exists with Firefox 3.6, Ubuntu Linux 9.10.

Comment 29

[Tomer Cohen :tomer]

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]

I'm also affected with this bug, and it's really annoying. Hope this will be fixed soon.

Comment 31

[Yotam Benshalom]

Four years, the issue is still here and is as disturbing as ever. Is there any progress?

Comment 32

[Tomer Cohen :tomer]

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]

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] (he/him)(JST, +0900)]

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]

Attachment: Patch v.3

I think that Masayuki and Karl both agreed on this approach in the comments.

Comment 36

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

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)]

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 38

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f21548bcbc4

Comment 39

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

(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]]

https://hg.mozilla.org/mozilla-central/rev/2f21548bcbc4

Comment 41

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

PRBool?

Comment 42

[Shahar Or]

Haleluya...

Comment 43

[Emin Mastizada]

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]]

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.