Closed Bug 1260710 Opened 9 years ago Closed 9 years ago

Test for routing hardware key events to IME

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla48
Tracking Flags:
Tracking Status
firefox48 --- fixed

People

(Reporter: chunmin, Assigned: chunmin)

References

Details

(Whiteboard: btpp-active)

Attachments

(1 file, 4 obsolete files)

MozReview Request: Bug 1260710 - Test for routing hardware key events to IME; r?masayuki
58 bytes, text/x-review-board-request
masayuki
: review+
Details
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.
Depends on: 1110030, 983015
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.
Attached patch [WIP] test (obsolete) — — Splinter Review
Depends on: 1260713
Attachment #8737035 - Attachment is obsolete: true
Attached patch Testcase (obsolete) — — Splinter Review
Attachment #8738051 - Attachment is obsolete: true
Attached patch Testcase (obsolete) — — Splinter Review
Attachment #8738384 - Attachment is obsolete: true
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: nobody → cchang
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)
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.
https://reviewboard.mozilla.org/r/44493/#review41249 > nit: insert a whitepsace before ? and :. ok
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)
(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)
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().
(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
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?
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
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/
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!
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+
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+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/922f7078062a
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox48: --- → fixed
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.

Attachment

General

Creator:
Created:
Updated:
Size: