Story - Functioning non-English Keyboard

VERIFIED FIXED

Status

defect
P2
normal
VERIFIED FIXED
7 years ago
5 years ago

People

(Reporter: TimAbraldes, Assigned: TimAbraldes)

Tracking

Trunk
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=story c=content_features u=metro_firefox_user p=2)

Attachments

(2 attachments, 1 obsolete attachment)

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.
Whiteboard: [metro-mvp?]
Duplicate of this bug: 842368
Blocks: metrov1triage
No longer blocks: 790486
Given that we're building touch first here, I think we can push this case out past v1.
Whiteboard: [metro-mvp?] → [metro-mvp-]
Blocks: metrov2planning
No longer blocks: metrov1triage
Whiteboard: [metro-mvp-]
Duplicate of this bug: 843498
Duplicate of this bug: 847109
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.
Blocks: metrov1triage
No longer blocks: metrov2planning
(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.
Blocks: 833153
No longer blocks: metrov1triage
Whiteboard: feature=work c=content_features u=metro_firefox_user
Summary: With certain keyboard layouts, holding right Alt while pressing other keys should produce special characters → Work - With certain keyboard layouts, holding right Alt while pressing other keys should produce special characters
Whiteboard: feature=work c=content_features u=metro_firefox_user → feature=work
Whiteboard: feature=work → feature=work [needs to block new higher priority story]
Blocks: metrov1onhold
No longer blocks: 833153
Flags: needinfo?(asa)
Summary: Work - With certain keyboard layouts, holding right Alt while pressing other keys should produce special characters → Story - With certain keyboard layouts, holding right Alt while pressing other keys should produce special characters
Whiteboard: feature=work [needs to block new higher priority story] → feature=story c=tbd u=tbd p=0[needs to block new higher priority story]
Duplicate of this bug: 856414
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!
Assignee: tabraldes → nobody
Status: ASSIGNED → NEW
Yes. We need this for v1. Hooking story up to the content Epic.
Blocks: 831565
No longer blocks: metrov1onhold
Flags: needinfo?(asa)
Priority: -- → P2
Hi Asa, thanks for adding the Epic.  Just needs a story then I can move it to the Story Backlog.
Flags: needinfo?(asa)
Blocks: metrov1planning
No longer blocks: metrov1onhold
Duplicate of this bug: 859716
Duplicate of this bug: 868537
Depends on: 843236
Story: As a Metro FIrefox user with a non-English keyboard, my keyboard works.
Flags: needinfo?(asa)
Summary: Story - With certain keyboard layouts, holding right Alt while pressing other keys should produce special characters → Story - Functioning non-English Keyboard
Whiteboard: feature=story c=tbd u=tbd p=0[needs to block new higher priority story] → feature=story c=tbd u=tbd p=0
Blocks: metrov1backlog
No longer blocks: metrov1planning
Posted patch Patch v1 (obsolete) — Splinter Review
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!
Assignee: nobody → tabraldes
Status: NEW → ASSIGNED
Attachment #762797 - Flags: review?(jmathies)
Gah, that patch had a bunch of changes from other work mixed in.  This is the correct one (hopefully :)
Attachment #762797 - Attachment is obsolete: true
Attachment #762797 - Flags: review?(jmathies)
Attachment #762798 - Flags: review?(jmathies)
Blocks: metrov1it9
No longer blocks: metrov1backlog
QA Contact: jbecerra
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 :)
Attachment #762798 - Flags: review?(jmathies) → review?(netzen)
Whiteboard: feature=story c=tbd u=tbd p=0 → feature=story c=content_features u=metro_firefox_user p=2
Comment on attachment 762798 [details] [diff] [review]
Patch v1 (the real one)

Looks good, works good.
Attachment #762798 - Flags: review?(netzen) → review+
(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".
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.
https://hg.mozilla.org/mozilla-central/rev/baf0c2c1a3e2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
(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.
Attachment #764191 - Flags: review?(masayuki)
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: äåé®þüúíóöáßðøæ©ñµ.
Status: RESOLVED → VERIFIED
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.