Closed Bug 477291 Opened 15 years ago Closed 13 years ago

Should not send keypress event before calling interpretKeyEvents

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

(Keywords: inputmethod, Whiteboard: [inbound])

Attachments

(1 file, 1 obsolete file)

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.
This fix is risky, we should take this ASAP and let's look for regressions.
Attachment #360962 - Attachment is obsolete: true
Attachment #562994 - Flags: review?(smichaud)
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.
Attachment #562994 - Flags: review?(smichaud) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/10298b25af4e
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla10
backedout due to random crash of mochitest-chrome.
https://hg.mozilla.org/integration/mozilla-inbound/rev/86a786598f35
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
merged backout to central.
https://hg.mozilla.org/mozilla-central/rev/308f307cabab
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.