Tim Abraldes [:TimAbraldes] [:tabraldes] Assignee
	Description • 12 years ago

Using the "United States-International keyboard", holding the right Alt key and pressing these characters:
  aeiouyzd
Results in the following characters:
  áéíóúüæð

Immersive mode Firefox is currently failing to respond to characters produced by using the right Alt key.

	Asa Dotzler [:asa]
	Comment 2 • 12 years ago

Given that we're building touch first here, I think we can push this case out past v1.

	Matt Brubeck (:mbrubeck)
	Comment 5 • 12 years ago

Hardware keyboards are a first-class feature of most Windows 8 touch devices, and this is becoming a top reported issue.  We should reconsider it as a launch blocker.  It makes it impossible for many users to write emails, log into web sites, or fill in even simple forms using their devices' keyboards.

	Asa Dotzler [:asa]
	Comment 6 • 12 years ago

(In reply to Matt Brubeck (:mbrubeck) from comment #5)
> Hardware keyboards are a first-class feature of most Windows 8 touch
> devices, and this is becoming a top reported issue.  We should reconsider it
> as a launch blocker.  It makes it impossible for many users to write emails,
> log into web sites, or fill in even simple forms using their devices'
> keyboards.

OK. Sounds like we should try to solve this for v1.

	Tim Abraldes [:TimAbraldes] [:tabraldes] Assignee
	Comment 8 • 11 years ago

Sadly I'm not currently working on this, so I'm unassigning myself.  I hope this makes it into an iteration soon as it is a serious issue for any user on a non-US keyboard!

	Asa Dotzler [:asa]
	Comment 9 • 11 years ago

Yes. We need this for v1. Hooking story up to the content Epic.

	Marco Mucci [:MarcoM]
	Comment 10 • 11 years ago

Hi Asa, thanks for adding the Epic.  Just needs a story then I can move it to the Story Backlog.

	Asa Dotzler [:asa]
	Comment 13 • 11 years ago

Story: As a Metro FIrefox user with a non-English keyboard, my keyboard works.

	Tim Abraldes [:TimAbraldes] [:tabraldes] Assignee
	Comment 14 • 11 years ago

Since I was working on bug 879562, I decided to take another look at this and I had a flash of insight:
  Widget was sending keyPress events, but some code higher up the stack was deciding not to forward them to content.
  The reason is pretty simple; the altGr button is equivalent to "control+alt" so our keyPress events looked like "control+alt+some_character" which would be interpreted as accelerator keys, not as character input.

The fix is simple; if we receive a CharacterReceived event from Windows, and the altGr button is pressed, remove the modifier flags for control, alt, and altGr from the gecko event we send.

Sorry this took so long to fix!

	Tim Abraldes [:TimAbraldes] [:tabraldes] Assignee
	Comment 15 • 11 years ago

Gah, that patch had a bunch of changes from other work mixed in.  This is the correct one (hopefully :)

	Tim Abraldes [:TimAbraldes] [:tabraldes] Assignee
	Comment 16 • 11 years ago

Comment on attachment 762798 [details] [diff] [review]
Patch v1 (the real one)

Switching reviewer since jimm seems to be out today.  Hope you don't mind, bbondy :)

	Brian R. Bondy [:bbondy]
	Comment 17 • 11 years ago

Comment on attachment 762798 [details] [diff] [review]
Patch v1 (the real one)

Looks good, works good.

	Tim Abraldes [:TimAbraldes] [:tabraldes] Assignee
	Comment 18 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/baf0c2c1a3e2

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

(In reply to Brian R. Bondy [:bbondy] from comment #17)
> Comment on attachment 762798 [details] [diff] [review]
> Patch v1 (the real one)
> 
> Looks good, works good.

Although, I'm not sure what the patch tries to fix.

Without the fix of bug 843236, the behavior with non-English keyboard layout must not be "good".

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

Comment on attachment 762798 [details] [diff] [review]
Patch v1 (the real one)

>   nsKeyEvent keyEvent(true, NS_KEY_PRESS, mWidget.Get());
>   mModifierKeyState.Update();
>+  if (mModifierKeyState.IsAltGr()) {
>+    mModifierKeyState.Unset(MODIFIER_CONTROL
>+                          | MODIFIER_ALT
>+                          | MODIFIER_ALTGRAPH);
>+  }
>   mModifierKeyState.InitInputEvent(keyEvent);

I assume that you want the key event should cause text input since nsEditor doesn't input text if keypress event is fired with control or alt. If my guess is correct, unsetting ctrl and alt is right thing, however, don't remove altgrapsh flag. I think that you don't understand the flags is used for what. You must not touch it since web application should be able to check if AltGr key is pressed.

Did you check the desktop's code before writing or reviewing the patch?
http://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout.cpp#934

Please backout the patch.

	Ed Morley [:emorley]
	Comment 21 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/baf0c2c1a3e2

	Tim Abraldes [:TimAbraldes] [:tabraldes] Assignee
	Comment 22 • 11 years ago

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #19)
> (In reply to Brian R. Bondy [:bbondy] from comment #17)
> > Comment on attachment 762798 [details] [diff] [review]
> > Patch v1 (the real one)
> > 
> > Looks good, works good.
> 
> Although, I'm not sure what the patch tries to fix.
> 
> Without the fix of bug 843236, the behavior with non-English keyboard layout
> must not be "good".

The patch fixes the fact that altGr+key combinations weren't working at all in Firefox Metro; people with german keyboards (for example) can now successfully type their email addresses in Firefox Metro. I agree that we should fix bug 843236, but I believe that since we have an easy fix for this major usability issue, we should incorporate it until we have a fix for bug 843236.


(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #20)
> Comment on attachment 762798 [details] [diff] [review]
> Patch v1 (the real one)
> 
> >   nsKeyEvent keyEvent(true, NS_KEY_PRESS, mWidget.Get());
> >   mModifierKeyState.Update();
> >+  if (mModifierKeyState.IsAltGr()) {
> >+    mModifierKeyState.Unset(MODIFIER_CONTROL
> >+                          | MODIFIER_ALT
> >+                          | MODIFIER_ALTGRAPH);
> >+  }
> >   mModifierKeyState.InitInputEvent(keyEvent);
> 
> I assume that you want the key event should cause text input since nsEditor
> doesn't input text if keypress event is fired with control or alt. If my
> guess is correct, unsetting ctrl and alt is right thing, however, don't
> remove altgrapsh flag. I think that you don't understand the flags is used
> for what. You must not touch it since web application should be able to
> check if AltGr key is pressed.
> 
> Did you check the desktop's code before writing or reviewing the patch?
> http://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout.
> cpp#934
> 
> Please backout the patch.

I was online when this comment was posted but I wasn't reading bugmail so I didn't see it until this morning :/

It is entirely accurate to say that I don't understand what the MODIFIER_ALTGRAPH flag is used for. In my limited testing, I didn't hit any cases where that flag was relevant and I wasn't sure if characters would be successfully input with it set. I have read desktop's input code extensively but I missed the fact that we unset MODIFIER_CONTROL and MODIFIER_ALT while leaving MODIFIER_ALTGRAPH untouched. Apologies for this oversight.

Attached is a follow-up patch that does not unset MODIFIER_ALTGRAPH. Based on the comments above, it sounds like this patch will alleviate the remaining concerns with this section of code until a patch for bug 843236 is available.

	Tim Abraldes [:TimAbraldes] [:tabraldes] Assignee
	Comment 23 • 11 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/cbe3a5bb23d8

	Ed Morley [:emorley]
	Comment 24 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/cbe3a5bb23d8

	Mihaela Velimiroviciu (:mihaelav)
	Comment 25 • 11 years ago

Mozilla/5.0 (Windows NT 6.2; rv:25.0) Gecko/20130708 Firefox/25.0

Verified that right Alt and qwertyuiopasdlzcnm produces the following characters in Metro mode: äåé®þüúíóöáßðøæ©ñµ.