Test for routing hardware key events to IME

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: chunmin, Assigned: chunmin)

Tracking

(Depends on 1 bug)

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(1 attachment, 4 obsolete attachments)

Assignee

Description

3 years ago
The implementation of routing hardware key events to IME finishes, but we don't have a good test for it. Currently we have one test case is implemented in GIP, but GIP is replaced with GIJ. With the transition plan of B2G OS, GIJ might not have official support. The most stable way is to write a mochitest for it. However, dom/inputmethod/mochitest can not run in B2G now.
Assignee

Updated

3 years ago
Depends on: 1110030, 983015
Assignee

Comment 1

3 years ago
I found the mochitest can run on Mulet build! 

.mozconfig for Mulet
------------------------------------------------
ac_add_options --enable-optimize                                                                     
ac_add_options --enable-debug
ac_add_options --enable-application=b2g/dev

Mulet can build the code inside #MOZ_B2G, so we might not need to use gonk for our widget,
and we can use existing api |synthesizeNativeKey| to fire the hardware key events on Mac and Win, instead of creating a new api to simulate the keystroke.
Assignee

Comment 2

3 years ago
Posted patch [WIP] test (obsolete) — Splinter Review
Assignee

Updated

3 years ago
Depends on: 1260713
Assignee

Comment 3

3 years ago
Attachment #8737035 - Attachment is obsolete: true
Assignee

Comment 4

3 years ago
Posted patch Testcase (obsolete) — Splinter Review
Attachment #8738051 - Attachment is obsolete: true
Assignee

Comment 5

3 years ago
Posted patch Testcase (obsolete) — Splinter Review
Attachment #8738384 - Attachment is obsolete: true
Assignee

Comment 6

3 years ago
Comment on attachment 8738385 [details] [diff] [review]
Testcase

Hi Masayuki-san,
Could you take some time to look at this?
Attachment #8738385 - Flags: review?(masayuki)
Assignee

Updated

3 years ago
Assignee: nobody → cchang
Assignee

Comment 7

3 years ago
Comment on attachment 8738385 [details] [diff] [review]
Testcase

Switch to use mozreview to do this
Attachment #8738385 - Attachment is obsolete: true
Attachment #8738385 - Flags: review?(masayuki)
I'm still reviewing this. I hope I'd publish the review tomorrow.
Comment on attachment 8738389 [details]
MozReview Request: Bug 1260710 - Test for routing hardware key events to IME; r?masayuki

https://reviewboard.mozilla.org/r/44493/#review41249

::: dom/inputmethod/mochitest/bug1110030_embedded.html:1
(Diff revision 1)
> +<!DOCTYPE HTML>

I think that file_iframe_bug1110030.html or iframe_bug1110030.html is ordinal name for this file.

::: dom/inputmethod/mochitest/bug1110030_helper.js:46
(Diff revision 1)
> +// (Our input method for testing will handle alphabets)
> +// On the other hand, to test key events that will not be generated by IME,
> +// we use 0-9 for such case in our testing.
> +function guessNativeKeyCode(key)
> +{
> +  let NativeCodeName = (kIsWin)? 'WIN_VK_' : 'MAC_VK_ANSI_';

nit: nativeCodeName rather than NativeCodeName?

::: dom/inputmethod/mochitest/bug1110030_helper.js:66
(Diff revision 1)
> +// * Frame loader and frame scripts
> +// ***********************************
> +function frameScript()
> +{
> +  function handler(e) {
> +    sendAsyncMessage("forwardevent", { type: e.type, key: e.key });

I hope that using "window." explicitly is easier to read.

::: dom/inputmethod/mochitest/bug1110030_helper.js:68
(Diff revision 1)
> +  addEventListener('keydown', handler);
> +  addEventListener('keypress', handler);
> +  addEventListener('keyup', handler);

Same. Especially, these tests have similar name function |addKeyEventListeners|.

::: dom/inputmethod/mochitest/bug1110030_helper.js:99
(Diff revision 1)
> +// ***********************************
> +function fireEvent(callback)
> +{
> +  let key = gCurrentTest.key;
> +  synthesizeNativeKey(KEYBOARD_LAYOUT_EN_US, guessNativeKeyCode(key), {},
> +                      key, key, (callback)? callback: null);

nit: insert a whitepsace before ? and :.

::: dom/inputmethod/mochitest/test_forward_hardware_key_to_ime.html:124
(Diff revision 1)
> +function waitAndVerifyResults(aCount)
> +{
> +  if (areEventsSame(gCurrentTest) || aCount > 10) {
> +    verifyResults(gCurrentTest);
> +    startTesting();
> +  } else {
> +    setTimeout(() => waitAndVerifyResults(aCount + 1), 100);
> +  }
> +}

I don't like to use setTimeout() here. Looks like that wating the last expected key event is enough. If it's failed (i.e., timed out), it means that this test detects a regression. So, timeout isn't problem.
Attachment #8738389 - Flags: review?(masayuki)
Assignee

Comment 11

3 years ago
https://reviewboard.mozilla.org/r/44493/#review41249

> I think that file_iframe_bug1110030.html or iframe_bug1110030.html is ordinal name for this file.

How about |file_test_empty_app.html|? 
It's basically same as |file_test_app.html| except it has no default value in input text.

> nit: nativeCodeName rather than NativeCodeName?

ok!

> I hope that using "window." explicitly is easier to read.

I am afraid that I can not use 'window' in frame script because 'window' is not defined there.
ReferenceError: window is not defined

> Same. Especially, these tests have similar name function |addKeyEventListeners|.

same as above.

> I don't like to use setTimeout() here. Looks like that wating the last expected key event is enough. If it's failed (i.e., timed out), it means that this test detects a regression. So, timeout isn't problem.

I would like to propose another understanding of the delay time for receiving the expected events.

The ```synthesizeNativeKey(...)``` will trigger the ```nsIHardwareKeyHandler::ForwardKeyToInputMethodApp```, which forwards keyboard events to IME and suspends keyboard events handling. Then, we need to wait for IME's replies. I think it's the reason that the test need some time to receive events. 

If the event handling isn't suspended, then events can be received immediately upon ```synthesizeNativeKey(...)``` is called, because the events are dispatched without any interrupts. It's a continuous task. However, if we need to wait for IME's replies(and we don't know how long IME needs to process), then it's not a continuous task. Other tasks might interrupt and be scheduled to execute, and I guess that's the reason why the test need some time to receive the events.
Assignee

Comment 12

3 years ago
https://reviewboard.mozilla.org/r/44493/#review41249

> nit: insert a whitepsace before ? and :.

ok
Assignee

Comment 13

3 years ago
Comment on attachment 8738389 [details]
MozReview Request: Bug 1260710 - Test for routing hardware key events to IME; r?masayuki

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44493/diff/1-2/
Attachment #8738389 - Flags: review?(masayuki)
(In reply to Chun-Min Chang[:chunmin] from comment #11)
> > I don't like to use setTimeout() here. Looks like that wating the last expected key event is enough. If it's failed (i.e., timed out), it means that this test detects a regression. So, timeout isn't problem.
> 
> I would like to propose another understanding of the delay time for
> receiving the expected events.
> 
> The ```synthesizeNativeKey(...)``` will trigger the
> ```nsIHardwareKeyHandler::ForwardKeyToInputMethodApp```, which forwards
> keyboard events to IME and suspends keyboard events handling. Then, we need
> to wait for IME's replies. I think it's the reason that the test need some
> time to receive events. 
> 
> If the event handling isn't suspended, then events can be received
> immediately upon ```synthesizeNativeKey(...)``` is called, because the
> events are dispatched without any interrupts. It's a continuous task.
> However, if we need to wait for IME's replies(and we don't know how long IME
> needs to process), then it's not a continuous task. Other tasks might
> interrupt and be scheduled to execute, and I guess that's the reason why the
> test need some time to receive the events.

input#text-input won't receive any key events?
Flags: needinfo?(cchang)
Assignee

Comment 15

3 years ago
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #14)
> input#text-input won't receive any key events?
input#text-input will receive key events. It just need some time to wait.
Flags: needinfo?(cchang)
Assignee

Comment 16

3 years ago
https://reviewboard.mozilla.org/r/44493/#review41249

> I would like to propose another understanding of the delay time for receiving the expected events.
> 
> The ```synthesizeNativeKey(...)``` will trigger the ```nsIHardwareKeyHandler::ForwardKeyToInputMethodApp```, which forwards keyboard events to IME and suspends keyboard events handling. Then, we need to wait for IME's replies. I think it's the reason that the test need some time to receive events. 
> 
> If the event handling isn't suspended, then events can be received immediately upon ```synthesizeNativeKey(...)``` is called, because the events are dispatched without any interrupts. It's a continuous task. However, if we need to wait for IME's replies(and we don't know how long IME needs to process), then it's not a continuous task. Other tasks might interrupt and be scheduled to execute, and I guess that's the reason why the test need some time to receive the events.

I think I could explain it clearer by illustrating the code flow in Fig 1 and Fig 2. The Fig 1 is followed by Fig 2.

-------------------------------------------------------------------------------------------------------------------------------
                       PresShell::            HardwareKeyHandler::          [Keyboard.jsm]                         [MozKeyboard.js]
synthesizeNativeKey -> HandleKeyboardEvent -> ForwardKeyToInputMethodApp -> OnHardwareKey => *sendAsyncMessage* => receiveMessage 
                                                                                    |
                                                             waitAndVerifyResults() will be called here for the first time.
-------------------------------------------------------------------------------------------------------------------------------
Fig. 1


I think waitAndVerifyResults() will be fired immediately after |OnHardwareKey| in Keyboard.jsm is called because |OnHardwareKey| send an *async* message to MozKeyboard.js to ask it to handle the key. 


-------------------------------------------------------------------------------------------------------------------------------
[MozKeyboard.js]  [MozKeyboard.js]                                       HardwareKeyHandler::
receiveMessage -> forwardHardwareKeyEvent .. => *sendAsyncMessage* => .. OnHandledByInputMethodApp
                           |                                                      |
          Key events will be dispatched to hardwareinput                 do further key processing
-------------------------------------------------------------------------------------------------------------------------------
Fig. 2


Next, MozKeyboard.js will dispatch key events to hardwareinput, and let IME decide whether or not key will be handled.
If IME want to handle the key(our test case 1), then IME will generate a new key event by TIP and dispatch it to the target(input#text-input here). 

On the other hand, if IME doesn't want to handle the key(our test case 2), then the forwarded pending key event will be resumed to dispatch to its target(input#text-input here).


No matter which case, the key events will be dispatched after |waitAndVerifyResults()| is called, so I think that's the reason to use setTimeout().
Assignee

Comment 17

3 years ago
(In reply to Chun-Min Chang[:chunmin] from comment #16)
Please read this comment on reviewboard. The layout of figures here is messy.
https://reviewboard.mozilla.org/r/44493/#review41249

> How about |file_test_empty_app.html|? 
> It's basically same as |file_test_app.html| except it has no default value in input text.

Sounds good to me.

> I am afraid that I can not use 'window' in frame script because 'window' is not defined there.
> ReferenceError: window is not defined

Thanks, sorry for my incorrect review.

> I think I could explain it clearer by illustrating the code flow in Fig 1 and Fig 2. The Fig 1 is followed by Fig 2.
> 
> -------------------------------------------------------------------------------------------------------------------------------
>                        PresShell::            HardwareKeyHandler::          [Keyboard.jsm]                         [MozKeyboard.js]
> synthesizeNativeKey -> HandleKeyboardEvent -> ForwardKeyToInputMethodApp -> OnHardwareKey => *sendAsyncMessage* => receiveMessage 
>                                                                                     |
>                                                              waitAndVerifyResults() will be called here for the first time.
> -------------------------------------------------------------------------------------------------------------------------------
> Fig. 1
> 
> 
> I think waitAndVerifyResults() will be fired immediately after |OnHardwareKey| in Keyboard.jsm is called because |OnHardwareKey| send an *async* message to MozKeyboard.js to ask it to handle the key. 
> 
> 
> -------------------------------------------------------------------------------------------------------------------------------
> [MozKeyboard.js]  [MozKeyboard.js]                                       HardwareKeyHandler::
> receiveMessage -> forwardHardwareKeyEvent .. => *sendAsyncMessage* => .. OnHandledByInputMethodApp
>                            |                                                      |
>           Key events will be dispatched to hardwareinput                 do further key processing
> -------------------------------------------------------------------------------------------------------------------------------
> Fig. 2
> 
> 
> Next, MozKeyboard.js will dispatch key events to hardwareinput, and let IME decide whether or not key will be handled.
> If IME want to handle the key(our test case 1), then IME will generate a new key event by TIP and dispatch it to the target(input#text-input here). 
> 
> On the other hand, if IME doesn't want to handle the key(our test case 2), then the forwarded pending key event will be resumed to dispatch to its target(input#text-input here).
> 
> 
> No matter which case, the key events will be dispatched after |waitAndVerifyResults()| is called, so I think that's the reason to use setTimeout().

> If IME want to handle the key(our test case 1), then IME will generate a new key event by TIP and dispatch it to the target(input#text-input here). 

So, sounds like that at least test case 1, you can use a keyboard event listener of input#text-input for avoiding to use setTimeout with non-zero value.

> On the other hand, if IME doesn't want to handle the key(our test case 2), then the forwarded pending key event will be resumed to dispatch to its target(input#text-input here).

This sounds like that even in test case 2, a keyboard event will be fired on input#text-input?


So, still I think that you can add an event listener to input#text-input to kick next test. (Note that I don't mind you to use setTimout with 0 for quitting from the stack of the keyboard event listener, of course.)
Whiteboard: btpp-active
Assignee

Comment 19

3 years ago
https://reviewboard.mozilla.org/r/44493/#review41249

> > If IME want to handle the key(our test case 1), then IME will generate a new key event by TIP and dispatch it to the target(input#text-input here). 
> 
> So, sounds like that at least test case 1, you can use a keyboard event listener of input#text-input for avoiding to use setTimeout with non-zero value.
> 
> > On the other hand, if IME doesn't want to handle the key(our test case 2), then the forwarded pending key event will be resumed to dispatch to its target(input#text-input here).
> 
> This sounds like that even in test case 2, a keyboard event will be fired on input#text-input?
> 
> 
> So, still I think that you can add an event listener to input#text-input to kick next test. (Note that I don't mind you to use setTimout with 0 for quitting from the stack of the keyboard event listener, of course.)

Oh, it seems like I misunderstood you.

I've tried to do that before using setTimeout, but I don't have good way to send message from the embedded page(file_test_empty_app.html) to the host page(test_forward_hardware_key_to_ime.html). Any idea?
Assignee

Comment 20

3 years ago
https://reviewboard.mozilla.org/r/44493/#review41249

> Oh, it seems like I misunderstood you.
> 
> I've tried to do that before using setTimeout, but I don't have good way to send message from the embedded page(file_test_empty_app.html) to the host page(test_forward_hardware_key_to_ime.html). Any idea?

Oh, framescript now support |content.document.getElement*|
I think I could use it to addEventListener and use it to avoid setTimeout
Assignee

Comment 21

3 years ago
Comment on attachment 8738389 [details]
MozReview Request: Bug 1260710 - Test for routing hardware key events to IME; r?masayuki

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44493/diff/2-3/
Assignee

Comment 22

3 years ago
https://reviewboard.mozilla.org/r/44493/#review41249

> Oh, framescript now support |content.document.getElement*|
> I think I could use it to addEventListener and use it to avoid setTimeout

I've update a new patch without setTimeout. 
Thank you for your wise insight! It let me know more interesting stuff about gecko!
Comment on attachment 8738389 [details]
MozReview Request: Bug 1260710 - Test for routing hardware key events to IME; r?masayuki

https://reviewboard.mozilla.org/r/44493/#review42263

Thanks!
Assignee

Comment 24

3 years ago
Comment on attachment 8738389 [details]
MozReview Request: Bug 1260710 - Test for routing hardware key events to IME; r?masayuki

https://reviewboard.mozilla.org/r/44493/#review42863
Attachment #8738389 - Flags: review+
Assignee

Comment 25

3 years ago
Comment on attachment 8738389 [details]
MozReview Request: Bug 1260710 - Test for routing hardware key events to IME; r?masayuki

misuse from reviewboard
Attachment #8738389 - Flags: review+
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 27

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/922f7078062a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.