Closed Bug 1343451 Opened 7 years ago Closed 6 years ago

[UI Events] Widget should set keyCode value to 229 (VK_PROCESS of Windows) and key to "Process" if key event is handled by IME before dispatching keydown or keyup event

Categories

(Core :: Widget, enhancement, P3)

Product:
Core
Component:
Widget
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod, Whiteboard: tpi:+)

Attachments

(6 files)

Bug 1343451 - part 1: Declare (DOM|NS)_VK_PROCESSKEY for keyCode value during composition
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1343451 - part 2: KeyboardLayout and NativeKey should use native key code value to check if the key event was handled by IME
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1343451 - part 3-1: Make KeymapWrapper::InitKeyEvent() mark given key event as "processed by IME" if it has been handled by IME
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1343451 - part 3-2: Make IMContextWrapper dispatch eKeyDown event or eKeyUp event if IME handles native key event but we have not dispatched DOM event for it yet
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1343451 - part 4: Make TextInputHandler dispatches eKeyDown event with marking it as "processed by IME"
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1343451 - part 5: Make GeckoEditableSupport dispatch dummy eKeyDown and eKeyUp event during composition always
59 bytes, text/x-review-board-request
jchen
: review+
Details 
During composition, other browsers dispatch keydown and keyup events, that conforms to UI Events spec. We need to fix this at bug 354358.

Currently, all widgets will use TextEventDispatcher soon (remaining platform is Android, but it will be fixed soon, bug 1137567). For now, we should set keyCode value and key value same as other browsers.

(According to check each browser's behavior, only keydown is set to special values in simple case.)
Priority: -- → P3
Whiteboard: tpi:+
Summary: TextEventDispatcher should set keyCode value and key value during composition → [UI Events] Widget should set keyCode value to 229 (VK_PROCESS of Windows) and key to "Process" if key event is handled by IME before dispatching keydown or keyup event
Current scope of this bug is, conforming keyCode value and key value of keydown and keyup event to UI Events spec. I.e., keyCode should be 229 (VK_PROCESS) and key should be "Process".
https://w3c.github.io/uievents/#determine-keydown-keyup-keyCode
https://w3c.github.io/uievents-key/#keys-composition

However, currently, whether dispatching keydown and keyup event during composition behinds pref (except on Android) and on Linux, macOS and Android, we don't dispatch eKeyDown and eKeyUp during composition even if enabling the pref. This bug roughly fixes the latter issue for confirming to check if setting keyCode and key to the value.

I'll file some follow up bugs for remaining issues of each widget before working on bug 354358.
FYI: Those patches won't be landed for 60 since they're too risky and changes some keyCode and key value even with current default settings (e.g., on some platforms, keyCode of keydown event fired immediately before compositionstart will be DOM_VK_PROCESSKEY).

So, if you have some urgent work, please give higher priority to them.
Comment on attachment 8955015 [details]
Bug 1343451 - part 5: Make GeckoEditableSupport dispatch dummy eKeyDown and eKeyUp event during composition always

https://reviewboard.mozilla.org/r/223940/#review230246

::: widget/android/GeckoEditableSupport.cpp:1034
(Diff revision 1)
>          dummyChange.mStart = aStart;
>          dummyChange.mOldEnd = dummyChange.mNewEnd = aEnd;
>          AddIMETextChange(dummyChange);
>      }
>  
> +#ifndef EARLY_BETA_OR_EARLIER

Please add a comment about the purpose of this change.
Attachment #8955015 - Flags: review?(nchen) → review+
Comment on attachment 8955010 [details]
Bug 1343451 - part 1: Declare (DOM|NS)_VK_PROCESSKEY for keyCode value during composition

https://reviewboard.mozilla.org/r/223930/#review230280

ok, so this follows what others do, but is there a spec for this? If not, could you file a spec bug?
Attachment #8955010 - Flags: review?(bugs) → review+
Comment on attachment 8955011 [details]
Bug 1343451 - part 2: KeyboardLayout and NativeKey should use native key code value to check if the key event was handled by IME

https://reviewboard.mozilla.org/r/223932/#review230502
Attachment #8955011 - Flags: review?(m_kato) → review+
Comment on attachment 8955012 [details]
Bug 1343451 - part 3-1: Make KeymapWrapper::InitKeyEvent() mark given key event as "processed by IME" if it has been handled by IME

https://reviewboard.mozilla.org/r/223934/#review230504
Attachment #8955012 - Flags: review?(m_kato) → review+
Comment on attachment 8955013 [details]
Bug 1343451 - part 3-2: Make IMContextWrapper dispatch eKeyDown event or eKeyUp event if IME handles native key event but we have not dispatched DOM event for it yet

https://reviewboard.mozilla.org/r/223936/#review230526
Attachment #8955013 - Flags: review?(m_kato) → review+
Comment on attachment 8955014 [details]
Bug 1343451 - part 4: Make TextInputHandler dispatches eKeyDown event with marking it as "processed by IME"

https://reviewboard.mozilla.org/r/223938/#review230528
Attachment #8955014 - Flags: review?(m_kato) → review+
Comment on attachment 8955010 [details]
Bug 1343451 - part 1: Declare (DOM|NS)_VK_PROCESSKEY for keyCode value during composition

https://reviewboard.mozilla.org/r/223930/#review230280

Yes, other browsers behave as almost same. And it's already declared by the specs. When we discussed them, WG decided to take other browsers' behavior as new standard. See comment 3 for the links to the specs.
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/4dce7ab14f71
part 1: Declare (DOM|NS)_VK_PROCESSKEY for keyCode value during composition r=smaug
https://hg.mozilla.org/integration/autoland/rev/b34d48936db8
part 2: KeyboardLayout and NativeKey should use native key code value to check if the key event was handled by IME r=m_kato
https://hg.mozilla.org/integration/autoland/rev/84a5ec921442
part 3-1: Make KeymapWrapper::InitKeyEvent() mark given key event as "processed by IME" if it has been handled by IME r=m_kato
https://hg.mozilla.org/integration/autoland/rev/9561ed261d04
part 3-2: Make IMContextWrapper dispatch eKeyDown event or eKeyUp event if IME handles native key event but we have not dispatched DOM event for it yet r=m_kato
https://hg.mozilla.org/integration/autoland/rev/dc4a2a5932c3
part 4: Make TextInputHandler dispatches eKeyDown event with marking it as "processed by IME" r=m_kato
https://hg.mozilla.org/integration/autoland/rev/e07105d9698e
part 5: Make GeckoEditableSupport dispatch dummy eKeyDown and eKeyUp event during composition always r=jchen
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/32f5111c9891
Backed out 6 changesets for mochitest android perma failures on testInputConnection.
Wow, the robocop tests won't run when I specify "-b d" to the try syntax (i.e., only debug build)...
Comment on attachment 8955015 [details]
Bug 1343451 - part 5: Make GeckoEditableSupport dispatch dummy eKeyDown and eKeyUp event during composition always

Could you review this patch again?

That was backed out due to permanent orange of mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testInputConnection.java (I tested trysever with "-b d -u mochitests" but robocop tests were not run even though they are grouped in "M" in treehearder).

I added a new pref to dispatch keydown/keyup events even during composition for *any* web applications and the test checks only the new behavior since I'm not sure how to check the pref in the test, the new behavior should be enabled in default settings as soon as possible and also the testing part must not be broken so frequently.
Flags: needinfo?(masayuki)
Attachment #8955015 - Flags: review+ → review?(nchen)
Filed bug 1445169 for the tryserver issue.
Comment on attachment 8955015 [details]
Bug 1343451 - part 5: Make GeckoEditableSupport dispatch dummy eKeyDown and eKeyUp event during composition always

https://reviewboard.mozilla.org/r/223940/#review233328

::: widget/android/GeckoEditableSupport.h:195
(Diff revision 3)
>  
>      // Constructor for content process GeckoEditableChild.
>      GeckoEditableSupport(java::GeckoEditableChild::Param aEditableChild)
>          : GeckoEditableSupport(nullptr, nullptr, aEditableChild)
> -    {}
> +    {
> +        ObservePrefs();

Don't need this line because `ObservePrefs` is already called through the delegate constructor.
Attachment #8955015 - Flags: review?(nchen) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/91585581ac42
part 1: Declare (DOM|NS)_VK_PROCESSKEY for keyCode value during composition r=smaug
https://hg.mozilla.org/integration/autoland/rev/73a1a183e75f
part 2: KeyboardLayout and NativeKey should use native key code value to check if the key event was handled by IME r=m_kato
https://hg.mozilla.org/integration/autoland/rev/0c24a392e924
part 3-1: Make KeymapWrapper::InitKeyEvent() mark given key event as "processed by IME" if it has been handled by IME r=m_kato
https://hg.mozilla.org/integration/autoland/rev/5ff56d8f616c
part 3-2: Make IMContextWrapper dispatch eKeyDown event or eKeyUp event if IME handles native key event but we have not dispatched DOM event for it yet r=m_kato
https://hg.mozilla.org/integration/autoland/rev/3747478447d8
part 4: Make TextInputHandler dispatches eKeyDown event with marking it as "processed by IME" r=m_kato
https://hg.mozilla.org/integration/autoland/rev/00456fae72f5
part 5: Make GeckoEditableSupport dispatch dummy eKeyDown and eKeyUp event during composition always r=jchen
Depends on: 1447866
No longer depends on: 1448282
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: