477291 - Should not send keypress event before calling interpretKeyEvents

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

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

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] (he/him)(JST, +0900)]

Attachment: Patch v0.1 (work in progress)

Comment 2

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

Attachment: 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)]

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] (he/him)(JST, +0900)]

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

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/10298b25af4e

try:
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=42637a15103b

Comment 6

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/10298b25af4e

Comment 7

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

backedout due to random crash of mochitest-chrome.
https://hg.mozilla.org/integration/mozilla-inbound/rev/86a786598f35

Comment 8

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

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]]

merged backout to central.

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/308f307cabab

Comment 11

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/308f307cabab