All users were logged out of Bugzilla on October 13th, 2018

Leverage MozInputContext#SendKey(dict) for CompositeTargets

RESOLVED FIXED

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: timdream, Assigned: timdream)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

The Emoji keys and all composite keys currently generate keydown/keypress/keyup for every character. We should leverage bug 1137557 and emit the correct key events (keydown/keypress x n/keyup).

As a bonus, that would make the undesired b/w-character flash go away. I can see a white square appear before the real Emoji show up (especially on Flame and on rocket bar where the input is the slowest) and this will remove it.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #0)
> The Emoji keys and all composite keys currently generate
> keydown/keypress/keyup for every character.

Every UCS-2 code point, to be exact. 

====

I am actually in doubt of how many events an !BMP Emoji should trigger. After my proposed patch here, with

navigator.mozInputMethod.inputcontext.sendKey({ key: '\u{1F61A}' });

I see these events:

1. keydown, key=\u{d83d}\u{de1a}
2. keypress charCode = 55357, key=\u{d83d}
3. input 
4. keypress charCode = 56858, key=\u{de1a}
5. input
6. keyup, key=\u{d83d}\u{de1a}

Is it really correct to trigger keypress event for each of the surrogate code point? Masayuki, could you confirm if this is the desired behavior? Thanks.
Flags: needinfo?(masayuki)
Under current design of Gecko, it's correct behavior. Although, I don't think that this is good behavior. E.g., web apps can consume only one keypress event.
Flags: needinfo?(masayuki)
Created attachment 8676698 [details] [review]
[gaia] timdream:keyboard-imengine-handlekey > mozilla-b2g:master
Comment on attachment 8676698 [details] [review]
[gaia] timdream:keyboard-imengine-handlekey > mozilla-b2g:master

Reza, as there is no longer any keyboard developers in the project (except me) and this is related to Emoji, I would need to ask you for a review.

Let me explain what this patch do:

1. An instance of CompositeTargetHandler is created when there is a touch happen on a Emoji key.
2. CompositeTargetHandler#commit is called when the touch ends.
3. I change that function to look for engine#handleKey -- if that function is implemented, we would send the string as-is in a dict.
4. I implemented that function in the DefaultIMEngine (but not other engines), which is the one Emoji layout uses.
5. DefaultIMEngine.handleKey will call InputMethodGlue#sendKey with the dict directly.
6. InputMethodGlue#sendKey will send the dict directly to MozInputMethod API, bypassing the switch..case we previously used for a keyCode.

Eventually we should move all IMEngines and all target handlers to implement and use handleKey(), however that require massive layout definition change first so I don't think we should implement everything here.

Thanks!
Attachment #8676698 - Flags: review?(rakhavan)
Comment on attachment 8676698 [details] [review]
[gaia] timdream:keyboard-imengine-handlekey > mozilla-b2g:master

Tim, First I was reproduced the symptom you mentioned (seeing white squares before seeing the actual emoji character). Then I cherry-picked your commit on top of the latest PR from bug 993899 to test it out. The white squares are indeed gone and the emoji characters show up in good time. I tested this on a Flame.

This is the first time I'm reviewing keyboard code so some things are new to me. Besides that, the code looks good, new tests were added and treeherder is all green [1]. Nice work.

[1] https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=23addee865cf9eac5f82ff3afbfb0a0d00e44546
Attachment #8676698 - Flags: review?(rakhavan) → review+
Thanks!

master: https://github.com/mozilla-b2g/gaia/commit/54972537aa2d8173c3bda8aa0329e5fa5ade1d1b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.