Closed Bug 452393 Opened 13 years ago Closed 6 years ago

Accelerators should not be affected by keyboard layout Part Deux.

Categories

(Core :: Widget: Gtk, defect)

x86
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla13

People

(Reporter: mozilla-bugs-2011.08, Assigned: smontagu)

References

(Blocks 1 open bug)

Details

(Keywords: intl, Whiteboard: [key hell])

Attachments

(1 file, 2 obsolete files)

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
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 
Status: UNCONFIRMED → NEW
Ever confirmed: true
Ctrl+W works for me with FF 3.0.1 and Hebrew layout on Win XP. Don't remember Ctrl+Q should be doing...
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
I am also using Ubuntu Linux with Firefox 3, so this may be a Linux-specific or Ubuntu specific issue.
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.
OS: All → Linux
Hardware: All → PC
Reproduced using CentOS 5.2, and Hebrew layout.
Everything works except Ctrl-W and Ctrl-Q.
(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?
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...
Keywords: intl
Attached patch Possible patch (obsolete) — Splinter Review
A bit hacky, but it works. It's unfortunate that mochitests for keyhandling don't cover GTK
Assignee: nobody → smontagu
Status: NEW → ASSIGNED
Attachment #336634 - Flags: superreview?(roc)
Attachment #336634 - Flags: review?
Attachment #336634 - Flags: review? → review?(masayuki)
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.
Whiteboard: [key hell]
Attached patch Patch v.2 (obsolete) — Splinter Review
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?
Attachment #336634 - Attachment is obsolete: true
Attachment #336660 - Flags: review?(masayuki)
Attachment #336634 - Flags: superreview?(roc)
Attachment #336634 - Flags: review?(masayuki)
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...
(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".
(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.
(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.)
(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')| ?
(In reply to comment #16)
> We should use |('a' <= ch && ch <= 'z') || ('A' <= ch && ch <= 'Z')| ?

I'd feel safest with that.
(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.
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?
@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
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.
(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.
(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.
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.
(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).
@Tomer: That's why it should be optional.
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
Issue still exists with Firefox 3.6, Ubuntu Linux 9.10.
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?
I'm also affected with this bug, and it's really annoying. Hope this will be fixed soon.
Attachment #336660 - Flags: review?(masayuki)
Four years, the issue is still here and is as disturbing as ever. Is there any progress?
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.
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.
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.
Attached patch Patch v.3Splinter Review
I think that Masayuki and Karl both agreed on this approach in the comments.
Attachment #336660 - Attachment is obsolete: true
Attachment #603986 - Flags: review?(masayuki)
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.
Attachment #603986 - Flags: review?(masayuki)
Attachment #603986 - Flags: review?(karlt)
Attachment #603986 - Flags: review+
Component: Keyboard Navigation → Widget: Gtk
Product: Firefox → Core
QA Contact: keyboard.navigation → gtk
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?
Attachment #603986 - Flags: review?(karlt) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/2f21548bcbc4
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Haleluya...
Blocks: 890469
We have same problem with Azerbaijani keyboard layout, hotkey Ctrl+W not works as its Ctrl+Ü in Azerbaijani layout.
Bug: 1116180
Flags: needinfo?(smontagu)
Status: RESOLVED → REOPENED
Flags: needinfo?(mak77)
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Closed: 9 years ago6 years ago
Flags: needinfo?(smontagu)
Flags: needinfo?(mak77)
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.