Ilya Zakharevich Reporter
	Description • 11 years ago

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0 (Beta/Release)
Build ID: 20130409194949

Steps to reproduce:

On Windows, activate a keyboard where AltGr means something else than Ctrl-Alt (k.ilyaz.org/iz), press AltGr-a


Actual results:

The character is input.  This character is what is bound to Ctrl-Alt-a (α)


Expected results:

The character is input.  This character is what is bound to AltGr-a (æ)

	Ilya Zakharevich Reporter
	Comment 1 • 11 years ago

This bug is a split from https://bugzilla.mozilla.org/show_bug.cgi?id=880971#c6.  It is due to the following problem in KeyboardLayout.cpp:


http://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout.cpp#349

349 VirtualKey::FillKbdState(PBYTE aKbdState,
350                          const ShiftState aShiftState)

If asked to find which character a key with Control&Alt-modifiers is producing: the most probably intent is to find what AltGr-key is producing.  So when both STATE_CONTROL and STATE_ALT are set, one should also do
     aKbdState[VK_RMENU] |= 0x80;

This omission causes the 7th bug in
 http://search.cpan.org/~ilyaz/UI-KeyboardLayout/lib/UI/KeyboardLayout.pm#Firefox_misinterprets_keypresses

	Ilya Zakharevich Reporter
	Comment 2 • 11 years ago

Background: how this happens.

The Windows keyboard driver may define a flag KLLF_ALTGR. If present, the keyboard subsystem will emulate a LCTRL press/release around the RightAlt (VK_RMENU) press/release.  So, in presence of this flag, when physical AltGr is pressed, what an application sees is that LCTRL and RightAlt are pressed.

This flag is automatically enabled on all keyboards produced by MSKLC's (*THE* producer of Windows keyboard drivers; probably 99.9% of keyboards in use are produced by MSKLC) GUI when AltGr bindings are present (this may be avoided when using MSLKC's CLI).  So for most keyboards, if a character is produced when CTRL+ALT are active, it is produced when LCTRL and RightAlt are (logically) down.

As explained in https://bugzilla.mozilla.org/show_bug.cgi?id=880971, there is no way to predict which character is going to be produced based only on CapsLock/Shift/Alt/Control model.  One must know exactly ALL the keys which are now (logically) down. So when one investigates CTRL+ALT, one should choose which keys are down; one must use some heuristics.

It is natural to choose the most frequent combination: LCTRL+RightAlt.

	Ilya Zakharevich Reporter
	Comment 3 • 11 years ago

Untested.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 4 • 11 years ago

Could you attach a patch with "-p -U 8"?

# Or change your .hgrc
https://developer.mozilla.org/en-US/docs/Mercurial_Queues#Introduction
# Then, you can attach a file in (top-src-dir)/.hg/patches/ directly after |hg qrefresh -m "summary" && hg qbackup -m "backup"|.

And please read our coding style:
https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style

one line must be <= 80 characters.

https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Line_Length

Unfortunately, I'm going to go out today. I will review your new patch(es) next week, sorry.

> Untested.

Why not?

And could you try to make automated test in widget/tests/test_keycode.xul?

You should add test around here:
http://mxr.mozilla.org/mozilla-central/source/widget/tests/test_keycodes.xul?rev=ab40003ed2dc#1916

you can run the test with
./mach mochitest-chrome widget/tests/test_keycode.xul

https://developer.mozilla.org/en-US/docs/Mochitest

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 6 • 11 years ago

Comment on attachment 785152 [details] [diff] [review]
diff_AltGr_as_CtrlAlt_U - as above, with -U 12, shorter lines

>--- KeyboardLayout.cpp	2013-07-25 12:01:28.000000000 -0700
>+++ KeyboardLayout.cpp-AltGr-as_CtrlAlt	2013-08-02 13:02:22.751135700 -0700
>@@ -360,24 +360,27 @@ VirtualKey::FillKbdState(PBYTE aKbdState
>   if (aShiftState & STATE_ALT) {
>     aKbdState[VK_MENU] |= 0x80;
>+    if (aShiftState & STATE_CONTROL) {	// Bug 900750: the most frequent
>+      aKbdState[VK_RMENU] |= 0x80;	// character-producing CTRL+ALT is when
>+    }					// LCTRL+RMENU are (logically) down
>   } else {

Sorry, I should point at previous review:

Could you rewrite the patch as:

>   if (aShiftState & STATE_ALT) {
>     aKbdState[VK_MENU] |= 0x80;
>+    // the most frequent character-producing CTRL+ALT is when LCTRL+RMENU are
>+    // (logically) down
>+    if (aShiftState & STATE_CONTROL) {
>+      aKbdState[VK_RMENU] |= 0x80;
>+    }
>   } else {

Long comment should be above of the code mentioned by it.

And don't use tab character for indentation. We use only ASCII whitespace.

Additionally, I do same question again. Cannot you write automated test for this? If this bug can be reproduced with a keyboard layout which is included in Windows (i.e., not customized keyboard layout), you can write the testcase in test_keycodes.xul.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 7 • 11 years ago

And why don't we need similar code for Ctrl?

	Ilya Zakharevich Reporter
	Comment 8 • 11 years ago

Masayuki wrote:
> And why don't we need similar code for Ctrl?

;-) When you add extra flags in case of A&B, it is enough to put the check inside the "if (A)" branch.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 9 • 11 years ago

The patch almost looks good to me. But as I mentioned in previous review, the patch should include automated test in test_keycodes.xul if this bug can cause wrong behavior with pre-install keyboard layout. Does this bug occur with only custom keyboard layout which is created by MSKLC?

Note: Please remove bug number from the comment and you don't need the additional indentation at the second line. The cause of change can be found by annotation tool.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 10 • 11 years ago

Comment on attachment 791042 [details] [diff] [review]
as previous attachment, with -U 8, shorter lines

I already reviewed this. And this patch needs to include automated test.

	Henrik Pauli
	Comment 11 • 10 years ago

Sorry to burst in here almost a year later, but I see this is still open...  Could this be the reason why JS on some sites (Google Spreadsheets, Yahoo Mail) sees Ctrl+Alt when I press AltGr on my Windows machine?  Makes it genuinely hard to type things in these — eg. Ctrl+U supposedly turns on Underline, and they see the modifier keys and capture it instead of letting me type my AltGr+U (they shouldn't see any modkeys when I'm pressing AltGr, in my opinion — it's my (or my OS's) business entirely that I'm using the 3rd level of my keyboard layout).

	Simon Lydell
	Comment 12 • 9 years ago

I think I experience the same problem as Henrik Pauli, and I also think that it is related to this bug.

I’m a developer of the [VimFx][1] extension, and we’ve got a Windows-specific problem when you try to activate VimFx keyboard shortcuts involving a character that requires AltGr on some keyboard layouts, such as $.

Quoting the [VimFx issue in question][2]:

> Tested on Windows 7 with the sv-SE keyboard layout in Firefox 42. When pressing AltGr+4 to type a $, the 'keydown' event for $ reports that both alt and ctrl are held.
>
> (In Ubuntu and Trisquel with the sv-SE keyboard layout and Firefox 42, the modifiers are not reported as held, which is the expected behavior. Don't know about OSX.)
>
> This makes it impossible to trigger VimFx shortcuts with a $ in them (or any character that requires AltGr). The workaround is to customize all such shortcuts, replacing $ with <a-c-$>.
>
> In my opinion, it is AltGr that is broken on Windows, not VimFx. Or possibly
> it is a bug in Firefox.

[1]: https://github.com/akhodakivskiy/VimFx/
[2]: https://github.com/akhodakivskiy/VimFx/issues/629

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 13 • 9 years ago

(In reply to Henrik Pauli from comment #11)
> Could this be the reason why JS on some sites (Google Spreadsheets, Yahoo
> Mail) sees Ctrl+Alt when I press AltGr on my Windows machine?

Because AltGr on Windows is same state as Ctrl+Alt. When you use a keyboard layout which have AltGr but your keyboard doesn't have right Alt key, you can emulate AltGr key with left Ctrl + left Alt or right Ctrl + left Alt.

> Makes it
> genuinely hard to type things in these — eg. Ctrl+U supposedly turns on
> Underline, and they see the modifier keys and capture it instead of letting
> me type my AltGr+U (they shouldn't see any modkeys when I'm pressing AltGr,
> in my opinion — it's my (or my OS's) business entirely that I'm using the
> 3rd level of my keyboard layout).

It's a bug of the web apps. They also should check if altKey is false at handling shortcut keys only with Ctrl key. I see same keydown/keyup events on both Google Chrome and Edge (.ctrlKey and .altKey are true). And also I see same keypress event on Google Chrome and Edge, the keypress event's .ctrlKey and .altKey are true. On the other hand, Gecko lies modifier states to you. If AltGr + foo causes inputting text, its .altKey and .ctrlKey are false. This is internal reason of Gecko.

(In reply to Simon Lydell from comment #12)
> I think I experience the same problem as Henrik Pauli, and I also think that
> it is related to this bug.

As I explained above, if the shortcut key is handled by JS, it's not related to this bug. It may be a bug of VimFx. Anyway, Ctrl + AltGr + foo isn't available on Windows. It's a platform specific limitation. Any native applications cannot handle such shortcut key.

	Simon Lydell
	Comment 14 • 9 years ago

Thanks for the explanation. So, the conclusion for me is that Windows is broken (in my opinion), not Firefox. When searching the Internet a bit more about the issue, I found the recommendation not to use Ctrl+Alt keyboard shortcuts in Windows for this reason. I’ll add an option (enabled by default on Windows) to ignore Ctrl+Alt in VimFx.

	Henrik Pauli
	Comment 15 • 7 years ago

Masayuki Nakano [:masayuki] (JST, +0900) from comment #13)
> (In reply to Henrik Pauli from comment #11)
> > Could this be the reason why JS on some sites (Google Spreadsheets, Yahoo
> > Mail) sees Ctrl+Alt when I press AltGr on my Windows machine?
> 
> Because AltGr on Windows is same state as Ctrl+Alt. When you use a keyboard
> layout which have AltGr but your keyboard doesn't have right Alt key, you
> can emulate AltGr key with left Ctrl + left Alt or right Ctrl + left Alt.
> 

There are no PC compatible keyboards on the market without a right Alt key since the PC-AT era, and if there are, they're much rarer than the people affected by being unable to type in a website because the web developer didn't check an edge case introduced by a faulty API that Firefox doesn't abstract away.

I would say the amount of people wanting to press Left Ctrl and Right Alt at the same time, and type a letter, is also about the same.

If Firefox detects an AltGr on Windows and Windows only (either by being able to see the flag ilyaz mentioned, or simply seeing a simultaneous key-down on LCtrl and RAlt), Firefox shouldn't pass the key mods through to the webpage, because it is an AltGr and almost definitely not an intent of Ctrl+Alt from the user.  People only ever press Ctrl and Alt at the same side of a keyboard.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 16 • 7 years ago

(In reply to Henrik Pauli from comment #15)
> Masayuki Nakano [:masayuki] (JST, +0900) from comment #13)
> > (In reply to Henrik Pauli from comment #11)
> > > Could this be the reason why JS on some sites (Google Spreadsheets, Yahoo
> > > Mail) sees Ctrl+Alt when I press AltGr on my Windows machine?
> > 
> > Because AltGr on Windows is same state as Ctrl+Alt. When you use a keyboard
> > layout which have AltGr but your keyboard doesn't have right Alt key, you
> > can emulate AltGr key with left Ctrl + left Alt or right Ctrl + left Alt.
> > 
> 
> There are no PC compatible keyboards on the market without a right Alt key
> since the PC-AT era, and if there are, they're much rarer than the people
> affected by being unable to type in a website because the web developer
> didn't check an edge case introduced by a faulty API that Firefox doesn't
> abstract away.

My notebook doesn't have right Alt key actually.

> If Firefox detects an AltGr on Windows and Windows only (either by being
> able to see the flag ilyaz mentioned, or simply seeing a simultaneous
> key-down on LCtrl and RAlt), Firefox shouldn't pass the key mods through to
> the webpage, because it is an AltGr and almost definitely not an intent of
> Ctrl+Alt from the user.  People only ever press Ctrl and Alt at the same
> side of a keyboard.

I don't think that platforms like browser engines shouldn't assume such things. It should be handled by each web app.

Currently, Gecko sets enough modifier states for web apps, see:
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/getModifierState#Modifier_keys_on_Gecko
So, web apps can respect either Ctrl and Alt or AltGr with using KeyboardEvent.getModifierState("AltGraph").

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 17 • 7 years ago

We discussed this issue in W3C. Chromium team suggests that AltGraph state is only set when Right-Alt key is pressed when active keyboard layout has AltGraph key or a character is being inputted by Ctrl+Alt key.

See the Chromium's draft change for the detail:
https://chromium-review.googlesource.com/c/chromium/src/+/558584

After they landed this change, I'll work on this for taking same behavior.

	Eugene Savitsky
	Comment 18 • 7 years ago

It really seems, that English is not affected, but Estonian and Russian are.

	Henrik Pauli
	Comment 19 • 7 years ago

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #16)
> My notebook doesn't have right Alt key actually.

I'm curious about this one.  I guess it's due to the Japanese keyboards having multiple extra keys around the space bar and so they save space with omitting the right alt key?

If it's a laptop and it has a Fn key, can you check if Fn+Alt generates a Right Alt keyboard code?  On an old Dell I had I had only one Windows key and Fn+Windows generated the missing Right Windows key — might not be universal across various vendors, but it would be interesting to know.

> I don't think that platforms like browser engines shouldn't assume such
> things. It should be handled by each web app.
> 

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #17)
> We discussed this issue in W3C. Chromium team suggests that AltGraph state
> is only set when Right-Alt key is pressed when active keyboard layout has
> AltGraph key or a character is being inputted by Ctrl+Alt key.
> 
> See the Chromium's draft change for the detail:
> https://chromium-review.googlesource.com/c/chromium/src/+/558584
> 
> After they landed this change, I'll work on this for taking same behavior.

It actually seems like that the Chromium implementation to clear Ctrl+Alt if AltGr is detected is exactly the same effect that I explained (admittedly I do not see the point of anyone needing to latch onto the AltGraph state, but I'm sure the people involved in the decision have seen more weird uses of the user input than I have), and I'm glad this is the route the web will take.

And, this is indeed a case of the browser, as a platform, abstracting away the differences between various hosting OSes for the web site (which it does in pretty much every other case already; and that's its single forte).  We all know how badly written web pages and web apps can be (or indeed, this is true to all software), so the fewer pitfalls they're exposed to, the happier the users will be.

Thank you for the feedback and the links though, looking forward to seeing this finally fixed :)

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 21 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d03416027c100e5e20b4692d62d3a655065fe79b

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 22 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6352e4bb70e0ab3f0c8221a0747515beb8e80214

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 23 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ccd2f11a85ae4c102061c1304c5ef33016e1d97

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 24 • 6 years ago

Coming patches will make Gecko behave same as AltGr key handling on Windows of Chromium.

I'd like Makoto-san to review widget part and smaug to review the automated test result (although, the latter is really hard to read).

First, AltGr key is mapped to AltRight key on Windows only when some keyboard layout is active. Windows treats AltGraph state with both Control and Alt state.  I.e., pressing both Control key and Alt key can emulate AltGraph state with any keyboard even if keyboard omits AltRight key like some Japanese keyboards for notebook.

When user presses AltRight key with keyboard layout which maps AltGr key to it, following native events and DOM key events are fired now:

1. WM_KEYDOWN for ControlLeft, this causes ControlLeft "keydown" event whose ctrlKey is true.
2. WM_KEYDOWN (not WM_SYSKEYDOWN) for AltRight, this causes AltRight "keydown" event whose ctrlKey and altKey are true and getModifierState("AltGraph") returns true.
3. (sets of keyboard events whose ctrlKey and altKey are true and getModifierState("AltGraph"), but only keypress events if the key combinations produce some character(s), ctrlKey and altKey are false for TextEditor)
4. WM_SYSKEYUP for ControlLeft, this causes ControlLeft "keyup" event whose altKey is true.
5. WM_KEYUP for AltRight event, this causes AltRight "keyup" event.

Google's suggestion and current Chrome behavior is:

at #2, browsers should set ctrlKey and altKey to false, but keep getModifierState("AltGraph") returning true.
at #3, browsers should set ctrlKey and altKey to false, but keep getModifierState("AltGraph") returning true.
at #4, browsers should set ctrlKey and altKey to false, but keep getModifierState("AltGraph") returning true (even though Ctrl key is released).

Additionally, they suggested and Chrome implements following behavior behind commandline parameter, |--enable-features=FixAltGraph| (coming patch (part 5) makes this behind pref):

at #2 and #5, browsers should set KeyboardEvent.key value to "AltGraph" instead of "Alt".

Additionally, when user emulates AltGr key with pressing both Ctrl and Alt keys:

* browsers should keep current behavior for both Ctrl/Alt key events.
* browsers should set ctrlKey and altKey to false instead make getModifierState("AltGraph") return true of:
  - all of keydown, keypress and keyup events.
  - but only when key combination produces some character(s).


So, with those rules, web apps won't see printable keyboard events whose ctrlKey and altKey are true, but they can distinguish if AltGr key is pressed with getModifierState("AltGraph").

Note that this does not change any behavior when active keyboard layout does not have AltGr key.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 31 • 6 years ago

> Additionally, they suggested and Chrome implements following behavior behind commandline parameter, |--enable-features=FixAltGraph| (coming patch (part 5) makes this behind pref):

Oops, I tested finally with Chrome again. Then, they have already stopped making it behind commandline parameter. So, I'll remove the pref from the part 5. Additionally, when I tested on different clean environment, they still set altKey and ctrlKey to true when getModifierState("AltGraph") returns true. I'll ask them again about current status (A/B testing now?) and perhaps, we should land the patches when they make the new behavior true on release channel. So, anyway, please review them when you have much time.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 35 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=52b0a90e81a385b2b75a81b55b1d848daedc2315

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 36 • 6 years ago

Okay, I got information. Chrome released the new behavior at 67 and will start to make it enabled permanently at 68. So, after getting r+ for them, we can land them and keep watching 68's behavior for backing out.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 37 • 6 years ago

Posted to dev-platform:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/YVh04PSDcz4

	Makoto Kato [:m_kato]
	Comment 38 • 6 years ago
mozreview-review

Comment on attachment 8983292 [details]
Bug 900750 - part 1: Make KeyboardLayout store the information if current keyboard layout has AltGr key

https://reviewboard.mozilla.org/r/249190/#review255668

	Makoto Kato [:m_kato]
	Comment 39 • 6 years ago
mozreview-review

Comment on attachment 8983293 [details]
Bug 900750 - part 2: Make ModifierKeyState and VirtualKey treat AltGraph as new modifier and won't set Control and Alt state while AltGraph is active

https://reviewboard.mozilla.org/r/249192/#review255700

	Makoto Kato [:m_kato]
	Comment 40 • 6 years ago
mozreview-review

Comment on attachment 8983294 [details]
Bug 900750 - part 3: Remove unnecessary ModifierKeyState argument from some methods of NativeKey and KeyboardLayout

https://reviewboard.mozilla.org/r/249194/#review255710

	Makoto Kato [:m_kato]
	Comment 41 • 6 years ago
mozreview-review

Comment on attachment 8983295 [details]
Bug 900750 - part 4: Make NativeKey replaces MODIFIER_CONTROL and MODIFIER_ALT of mModKeyState with MODIFIER_ALTGRAPH if user emulates AltGr key press with pressing both Ctrl and Alt keys and current keydown produces character(s)

https://reviewboard.mozilla.org/r/249196/#review255724

	Makoto Kato [:m_kato]
	Comment 42 • 6 years ago
mozreview-review

Comment on attachment 8983296 [details]
Bug 900750 - part 5: Make NativeKey set KeyboardEvent.key value of AltRight key to "AltGraph" when active keyboard layout has AltGr key

https://reviewboard.mozilla.org/r/249198/#review255740

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 43 • 6 years ago
mozreview-review

Comment on attachment 8983293 [details]
Bug 900750 - part 2: Make ModifierKeyState and VirtualKey treat AltGraph as new modifier and won't set Control and Alt state while AltGraph is active

https://reviewboard.mozilla.org/r/249192/#review256676

looks error prone code, so would be nice to land early in a cycle

::: widget/windows/KeyboardLayout.h:260
(Diff revision 1)
>    };
>  
>    KeyShiftState mShiftStates[16];
>    uint16_t mIsDeadKey;
>  
> +  static uint8_t ToIndex(ShiftState aShiftState)

this method name doesn't really tell what it returns. Index of what?

::: widget/windows/KeyboardLayout.cpp:708
(Diff revision 1)
>    }
> +  // If AltGr key is pressed, only MODIFIER_ALTGRAPH should be set.
> +  // Otherwise, i.e., if both Ctrl and Alt keys are pressed to emulate
> +  // AltGr key, MODIFIER_CONTROL and MODIFIER_ALT keys should be set
> +  // separately.
> +  if (KeyboardLayout::GetInstance()->HasAltGr() && IS_VK_DOWN(VK_RMENU)) {

use of VK_RMENU could be explained in the comment.

::: widget/windows/KeyboardLayout.cpp:1114
(Diff revision 1)
>    }
>  
> +  if (aShiftState & STATE_ALTGRAPH) {
> +    aKbdState[VK_CONTROL]  |= 0x80;
> +    aKbdState[VK_LCONTROL] |= 0x80;
> +    aKbdState[VK_RCONTROL] &= ~0x80;

why RCONTROL is unset and LCONTROL set? I would have expected RCONTROL set and LCONTROL unset.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 44 • 6 years ago
mozreview-review

Comment on attachment 8983295 [details]
Bug 900750 - part 4: Make NativeKey replaces MODIFIER_CONTROL and MODIFIER_ALT of mModKeyState with MODIFIER_ALTGRAPH if user emulates AltGr key press with pressing both Ctrl and Alt keys and current keydown produces character(s)

https://reviewboard.mozilla.org/r/249196/#review256680

::: commit-message-6b906:17
(Diff revision 2)
> +2. AltLeft keydown event should make altKey true (not AltGraph state).
> +3. ctrlKey and altKey of printable keydown, keypress and keyup events should be
> +   set to false, but getModifierState("AltGraph") should return true.
> +4. AltLeft keyup event should make altKey false.
> +5. CtrlLeft keyup event should make ctrlKey false.
> +

what happens if alt keydown happens before ctrl keydown?

::: widget/windows/KeyboardLayout.cpp:1433
(Diff revision 2)
>  
> +  // Now, we can know if the key produces character(s) or a dead key with
> +  // AltGraph modifier.  When user emulates AltGr key press with pressing
> +  // both Ctrl and Alt and the key produces character(s) or a dead key, we
> +  // need to replace Control and Alt state with AltGraph if the keyboard
> +  // layout has AltGr key actually.

some repetition of word "actually" here.
I guess both 'actually' could be just dropped

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 45 • 6 years ago
mozreview-review

Comment on attachment 8983296 [details]
Bug 900750 - part 5: Make NativeKey set KeyboardEvent.key value of AltRight key to "AltGraph" when active keyboard layout has AltGr key

https://reviewboard.mozilla.org/r/249198/#review256686

::: widget/tests/test_keycodes.xul:5378
(Diff revision 2)
> +           kDescription + "AltRight should fire 2 pairs of keydown and keyup events");
> +        is(events[0].type, "keydown",
> +           kDescription + "First event should be keydown of ControlLeft");
> +        is(events[0].key, "Control",
> +           kDescription + "First event should be keydown of ControlLeft whose key should be Control");
> +        is(events[0].code, "ControlLeft",

I'm surprised about this, but ok, if this is what other browsers do too, fine. I mean, I would have expected ControlRight

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 46 • 6 years ago
mozreview-review

Comment on attachment 8983297 [details]
Bug 900750 - part 6: Rename |name| in test_keycodes.xul to |currentTestName|

https://reviewboard.mozilla.org/r/249200/#review256702

::: widget/tests/test_keycodes.xul:294
(Diff revision 2)
> -             name + ", AltGraph of AltGraph " + e.type + " event mismatch");
> +             currentTestName + ", AltGraph of AltGraph " + e.type + " event mismatch");
>            return IS_WIN && testingEvent.modifiers.altGrKey &&
>                   removeFlag(e, kAltGraphFlag) && expectedDOMKeyCode != e.keyCode;
>          case "Meta":
> -          is(e.ctrlKey, (flags & kCtrlFlag) != 0, name + ", Ctrl of Command " + e.type + " evnet mismatch");
> -          is(e.metaKey, e.type == "keydown", name + ", Command of Command " + e.type + " evnet mismatch");
> +          is(e.ctrlKey, (flags & kCtrlFlag) != 0,
> +             currentTestName + ", Ctrl of Command " + e.type + " evnet mismatch");

s/evnet/event/ here and elsewhere

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 47 • 6 years ago
mozreview-review-reply

Comment on attachment 8983293 [details]
Bug 900750 - part 2: Make ModifierKeyState and VirtualKey treat AltGraph as new modifier and won't set Control and Alt state while AltGraph is active

https://reviewboard.mozilla.org/r/249192/#review256676

Okay, I'll wait 63, the new behavior is not still implemented by Edge. So, must not be so urgent even after Chrome changes their behavior.

> this method name doesn't really tell what it returns. Index of what?

Okay, I rename it to ToShiftStateIndex()

> why RCONTROL is unset and LCONTROL set? I would have expected RCONTROL set and LCONTROL unset.

No, as I explain in commit messages and some other comments, Windows represents AltGr state (when AltRight is pressed) with AltRight and ControlLeft.

Specifying STATE_ALTGRAPH means that AltRight key is pressed (i.e., usual case of Western language users). If test needs to emulate to emulate AltGr key press with Ctrl and Alt, STATE_CONTROL and STATE_ALT should be set instead of STATE_ALTGRAPH.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 48 • 6 years ago
mozreview-review-reply

Comment on attachment 8983295 [details]
Bug 900750 - part 4: Make NativeKey replaces MODIFIER_CONTROL and MODIFIER_ALT of mModKeyState with MODIFIER_ALTGRAPH if user emulates AltGr key press with pressing both Ctrl and Alt keys and current keydown produces character(s)

https://reviewboard.mozilla.org/r/249196/#review256680

> what happens if alt keydown happens before ctrl keydown?

Ah, occurs usual sequence. So, Alt key down has only altKey=true, following Contrl key down has both altKey=true and ctrlKey=true.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 55 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad1b8d3c634efe2526abcca44e0c2a934895d9b3

	Pulsebot
	Comment 56 • 6 years ago

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/3f2ed470f406
part 1: Make KeyboardLayout store the information if current keyboard layout has AltGr key r=m_kato
https://hg.mozilla.org/integration/autoland/rev/c2e17337f8b8
part 2: Make ModifierKeyState and VirtualKey treat AltGraph as new modifier and won't set Control and Alt state while AltGraph is active r=m_kato,smaug
https://hg.mozilla.org/integration/autoland/rev/2e6a8268778e
part 3: Remove unnecessary ModifierKeyState argument from some methods of NativeKey and KeyboardLayout r=m_kato
https://hg.mozilla.org/integration/autoland/rev/311827011b11
part 4: Make NativeKey replaces MODIFIER_CONTROL and MODIFIER_ALT of mModKeyState with MODIFIER_ALTGRAPH if user emulates AltGr key press with pressing both Ctrl and Alt keys and current keydown produces character(s) r=m_kato,smaug
https://hg.mozilla.org/integration/autoland/rev/44cb540ef2fc
part 5: Make NativeKey set KeyboardEvent.key value of AltRight key to "AltGraph" when active keyboard layout has AltGr key r=m_kato,smaug
https://hg.mozilla.org/integration/autoland/rev/f15f24bf1131
part 6: Rename |name| in test_keycodes.xul to |currentTestName| r=smaug

	Eliza Balazs [:ebalazs_]
	Comment 57 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/3f2ed470f406
https://hg.mozilla.org/mozilla-central/rev/c2e17337f8b8
https://hg.mozilla.org/mozilla-central/rev/2e6a8268778e
https://hg.mozilla.org/mozilla-central/rev/311827011b11
https://hg.mozilla.org/mozilla-central/rev/44cb540ef2fc
https://hg.mozilla.org/mozilla-central/rev/f15f24bf1131

	Eric Shepherd [:sheppy]
	Comment 58 • 6 years ago

Updated the table at https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key/Key_Values#Modifier_keys to add a footnote to the Alt and AltGr key entries describing this change.

I've also added the following text to Firefox 63 for developers:

"Handling of the Alt key on the right side of the keyboard has been improved on Windows. If the user's current keyboard layout maps the Alt key to the AltGr modifier key, the value of KeyboardEvent.key is now reported as "AltGraph". This behavior matches that recently introduced into Chrome (bug 900760)."

Please let me know if I've misinterpreted any information so I can make needed corrections (or feel free to make them yourself; that's fine too!). Otherwise, this is considered doc-complete.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 59 • 6 years ago

(In reply to Eric Shepherd [:sheppy] from comment #58)
> Updated the table at
> https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key/
> Key_Values#Modifier_keys to add a footnote to the Alt and AltGr key entries
> describing this change.
> 
> I've also added the following text to Firefox 63 for developers:
> 
> "Handling of the Alt key on the right side of the keyboard has been improved
> on Windows. If the user's current keyboard layout maps the Alt key to the
> AltGr modifier key, the value of KeyboardEvent.key is now reported as
> "AltGraph". This behavior matches that recently introduced into Chrome (bug
> 900760)."
> 
> Please let me know if I've misinterpreted any information so I can make
> needed corrections (or feel free to make them yourself; that's fine too!).
> Otherwise, this is considered doc-complete.

Thank you. Looks good the document for Firefox 63 for developers.

How about to write it into the cell of AltGraph of Windows directly instead of footnode in https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key/Key_Values#Modifier_keys ? Then, it becomes clearer that it's talking about Firefox and Chrome for Windows and the value may be set even on Windows.

	h404744
	Comment 60 • 6 years ago

Hello everyone,

Thank you for your contributions.

Unfortunately, it seems that a recent change related to this bug has made it so AltGR cannot be used anymore in the way it was used for some users.

Old version of Firefox: type URL in URL bar -> press AltGr -> ".com" is attached to URL and is opened in new tab. Example: type "nyt" in URL bar plus AltGR = opens "www.nyt.com"
New version of Firefox: type URL in URL bar -> press AltGr -> URL is opened in current tab, without attaching ".com"

This breaks the behavior for many users, including myself. Already there are complaints in various forums around the web.

I am aware that there are alternative shortcuts, like Ctrl+Alt, but these are more uncomfortable for example with some keyboards.

Please, enable AltGr+Enter again. It would be welcome by many users. Users should not needlessly be forced to change their routines. I don't see how the change is possibly positive in this context -- AltGr now simply does nothing, so an useful option is eliminated.

It would seem sensible to, at least, add an option so that Ctrl+Alt can behave as AltGr again -- or an option that allows AltGr to both canonize URL and open in new tab in the URL bar.

	h404744
	Comment 61 • 6 years ago

Perhaps the option could be added to about:config.

	Marco Bonardo [:mak] (Away Apr 25 - May 5)
	Comment 62 • 6 years ago

(In reply to h404744 from comment #60)
> Hello everyone,
> 
> Thank you for your contributions.
> 
> Unfortunately, it seems that a recent change related to this bug has made it
> so AltGR cannot be used anymore in the way it was used for some users.
> 
> Old version of Firefox: type URL in URL bar -> press AltGr -> ".com" is
> attached to URL and is opened in new tab. Example: type "nyt" in URL bar
> plus AltGR = opens "www.nyt.com"
> New version of Firefox: type URL in URL bar -> press AltGr -> URL is opened
> in current tab, without attaching ".com"

It's unrelated to this bug, we changed canonization to CTRL in bug 237027, alt will always open in a new tab. AltGr has a few issues atm, in particular bug 1389739.

> This breaks the behavior for many users, including myself. Already there are
> complaints in various forums around the web.

We are aware of that, we think the changes make sense for most users, it's unfortunate some will have to adapt their habits, but we can't satisfy everyone.

	roger21
	Comment 63 • 6 years ago

so is there a KeyboardEvent property like altKey ctrlKey etc. that says if the altGrKey is pressed ? it is not documented 

https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent

	roger21
	Comment 64 • 6 years ago

(In reply to roger21 from comment #63)
> so is there a KeyboardEvent property like altKey ctrlKey etc. that says if
> the altGrKey is pressed ? it is not documented 
> 
> https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent

ok, the getModifierState("AltGraph") thingy

	roger21
	Comment 65 • 6 years ago

(In reply to roger21 from comment #64)
> (In reply to roger21 from comment #63)
> > so is there a KeyboardEvent property like altKey ctrlKey etc. that says if
> > the altGrKey is pressed ? it is not documented 
> > 
> > https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent
> 
> ok, the getModifierState("AltGraph") thingy

OMG it's a disaster, it doesn't work at all WHAT HAVE YOU GUYS DONE AGAIN OMG

	roger21
	Comment 66 • 6 years ago

ok so

previously i used

if(event.altKey && event.ctrlKey && event.key == myKey)

and it works with altGr an ctrl+alt, ok all fine

and now what ?

if(event.getModifierState("AltGraph") &&  event.key == myKey)

it does not work with ctrl+alt and it makes a mess with altgr: it's not a shortcut for my 'myKey' anymore it does write the myKey like whatever your modifiers dude you pressed myKey you get myKey ... wot?

	roger21
	Comment 67 • 6 years ago

ok the preventDefault can deal with the second part apparently and i guess a full test would deal with the first part:

if(((event.altKey && event.ctrlKey) || event.getModifierState("AltGraph")) && event.key == myKey)

...

	roger21
	Comment 68 • 6 years ago

and so the doc is wrong here https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/getModifierState#Modifier_keys_on_Gecko

event.getModifierState("AltGraph") does not return true when "Both Alt and Ctrl keys are pressed"