1216888 - Leverage MozInputContext#SendKey(dict) for CompositeTargets

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 3

[GitHub Autolander Bot]

Attachment: [gaia] timdream:keyboard-imengine-handlekey > mozilla-b2g:master

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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!

Comment 5

[Reza Akhavan [:jedireza]]

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

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Thanks!

master: https://github.com/mozilla-b2g/gaia/commit/54972537aa2d8173c3bda8aa0329e5fa5ade1d1b