Last Comment Bug 477291 - Should not send keypress event before calling interpretKeyEvents
: Should not send keypress event before calling interpretKeyEvents
Status: RESOLVED FIXED
[inbound]
: inputmethod
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla10
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
Mentors:
Depends on: 519972 685073
Blocks: 392254
  Show dependency treegraph
 
Reported: 2009-02-06 12:22 PST by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2011-10-05 05:10 PDT (History)
5 users (show)
masayuki: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v0.1 (work in progress) (9.36 KB, patch)
2009-02-06 12:25 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Patch Should not send keypress event before calling interpretKeyEvents (3.75 KB, patch)
2011-09-28 00:18 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
smichaud: review+
Details | Diff | Splinter Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2009-02-06 12:22:57 PST
Now, we are sending keypress event to gecko when a native keydown event is not normal letter inputting event or Control key is pressed *before* calling interpretKeyEvents. This is really wrong behavior. Because the native key event may be eaten by super class or IME.

E.g., ATOK (Japanese IME) implements "Kakutei-undo" function (Control + delete) by a tricky way. ATOK sends delete key down events to us for removing some characters which is latest committed word from ATOK. And ATOK receives the events itself via interpretKeyEvents. Then, if it needs to remove a previous character of current insertion point, it sends deleteBackward command to us. So, deleteBackward is not executed sometimes.

So, I think that we should not send keypress event before interpretKeyEvents. And we should send the keypress event from command handler instead of processKeyDownEvent.
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) 2009-02-06 12:25:47 PST
Created attachment 360962 [details] [diff] [review]
Patch v0.1 (work in progress)
Comment 2 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-09-28 00:18:41 PDT
Created attachment 562994 [details] [diff] [review]
Patch Should not send keypress event before calling interpretKeyEvents

This fix is risky, we should take this ASAP and let's look for regressions.
Comment 3 Steven Michaud [:smichaud] (Retired) 2011-09-28 14:25:51 PDT
Comment on attachment 562994 [details] [diff] [review]
Patch Should not send keypress event before calling interpretKeyEvents

This sounds fine to me, though I agree that it's risky.

Let's try it and see how it works out.
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-09-28 18:00:58 PDT
(In reply to Steven Michaud from comment #3)
> Comment on attachment 562994 [details] [diff] [review] [diff] [details] [review]
> Patch Should not send keypress event before calling interpretKeyEvents
> 
> This sounds fine to me, though I agree that it's risky.
> 
> Let's try it and see how it works out.

Thank you for the quick review. And the patch depends on bug 685073's patches, would you review them too? They are simple and not risky.
Comment 6 :Ehsan Akhgari (away Aug 1-5) 2011-09-30 07:37:06 PDT
https://hg.mozilla.org/mozilla-central/rev/10298b25af4e
Comment 7 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-09-30 18:16:11 PDT
backedout due to random crash of mochitest-chrome.
https://hg.mozilla.org/integration/mozilla-inbound/rev/86a786598f35
Comment 8 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-09-30 18:18:43 PDT
probably, the cause of random crash is bug 685073's. If I cannot find the cause soon, I'll reland only this patch with minor change for the dependency.
Comment 9 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-10-01 02:50:21 PDT
merged backout to central.
Comment 10 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-10-04 19:20:30 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/308f307cabab
Comment 11 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-10-05 05:10:24 PDT
https://hg.mozilla.org/mozilla-central/rev/308f307cabab

Note You need to log in before you can comment on or make changes to this bug.