Test for routing hardware key events to IME

RESOLVED FIXED in Firefox 48

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years 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)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

2 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

2 years ago
Depends on: 1110030, 983015
(Assignee)

Comment 1

2 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

2 years ago
Created attachment 8737035 [details] [diff] [review]
[WIP] test
(Assignee)

Updated

2 years ago
Depends on: 1260713
(Assignee)

Comment 3

2 years ago
Created attachment 8738051 [details] [diff] [review]
[WIP] test for forwarding hardware key events to IME
Attachment #8737035 - Attachment is obsolete: true
(Assignee)

Comment 4

2 years ago
Created attachment 8738384 [details] [diff] [review]
Testcase
Attachment #8738051 - Attachment is obsolete: true
(Assignee)

Comment 5

2 years ago
Created attachment 8738385 [details] [diff] [review]
Testcase
Attachment #8738384 - Attachment is obsolete: true
(Assignee)

Comment 6

2 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

2 years ago
Assignee: nobody → cchang
(Assignee)

Comment 7

2 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)
(Assignee)

Comment 8

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

Review commit: https://reviewboard.mozilla.org/r/44493/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44493/
Attachment #8738389 - 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

2 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

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

> nit: insert a whitepsace before ? and :.

ok
(Assignee)

Comment 13

2 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

2 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

2 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

2 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

2 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

2 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

2 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

2 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!
Attachment #8738389 - Flags: review?(masayuki) → review+
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

2 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

2 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

2 years ago
Keywords: checkin-needed

Comment 27

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/922f7078062a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.