Closed
Bug 1343451
Opened 8 years ago
Closed 7 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)
Core
Widget
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: masayuki, Assigned: masayuki)
References
Details
(Keywords: inputmethod, Whiteboard: tpi:+)
Attachments
(6 files)
59 bytes,
text/x-review-board-request
|
smaug
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
m_kato
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
m_kato
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
m_kato
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
m_kato
:
review+
|
Details |
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.)
![]() |
||
Updated•8 years ago
|
Priority: -- → P3
Whiteboard: tpi:+
Assignee | ||
Comment 1•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=20a390438e6aef32a3df14f4acc81da0ea4fcf12
Assignee | ||
Updated•7 years ago
|
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
Assignee | ||
Comment 2•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b34c3439c95f1f710cd34221009351944812f6b1
Assignee | ||
Comment 3•7 years ago
|
||
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.
Assignee | ||
Comment 4•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5ca0394deea5eb831304a89804dfda7a02f053c
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
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 12•7 years ago
|
||
mozreview-review |
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 13•7 years ago
|
||
mozreview-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 14•7 years ago
|
||
mozreview-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 15•7 years ago
|
||
mozreview-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 16•7 years ago
|
||
mozreview-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 17•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 18•7 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 25•7 years ago
|
||
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
Comment 26•7 years ago
|
||
Backout by csabou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/32f5111c9891 Backed out 6 changesets for mochitest android perma failures on testInputConnection.
Assignee | ||
Comment 27•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=db73432405fec1759f1ed210af8c0030d41a52d8
Assignee | ||
Comment 28•7 years ago
|
||
Wow, the robocop tests won't run when I specify "-b d" to the try syntax (i.e., only debug build)...
Comment 29•7 years ago
|
||
Backed out for mochitest android perma failures on testInputConnection Treherder push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=e07105d9698efa45ed5e08e8536414e08bba5762&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-searchStr=rc4&selectedJob=167415789 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=167415789&repo=autoland&lineNumber=1679 Backout link: https://treeherder.mozilla.org/logviewer.html#?job_id=167415789&repo=autoland&lineNumber=1679
Flags: needinfo?(masayuki)
Assignee | ||
Comment 30•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9164cd76fa6f540a9d47579dd52088b52a1d0c3
Assignee | ||
Comment 31•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e42add122c0228946967c1d6d239c6064c83a6a
Assignee | ||
Comment 32•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a10fb6e526997d102c4a6d14e88001fda168a83
Assignee | ||
Comment 33•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f01b6602dc728eea972b3c60a0162a81811ca2a
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 40•7 years ago
|
||
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)
Assignee | ||
Comment 41•7 years ago
|
||
Filed bug 1445169 for the tryserver issue.
Comment 42•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 44•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0817f9fe271a5ab7b40577846feb370f234ec137
Comment 45•7 years ago
|
||
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
Comment 46•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/91585581ac42 https://hg.mozilla.org/mozilla-central/rev/73a1a183e75f https://hg.mozilla.org/mozilla-central/rev/0c24a392e924 https://hg.mozilla.org/mozilla-central/rev/5ff56d8f616c https://hg.mozilla.org/mozilla-central/rev/3747478447d8 https://hg.mozilla.org/mozilla-central/rev/00456fae72f5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
status-firefox54:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•