Closed Bug 1341960 Opened 6 years ago Closed 6 years ago

InsertText() should dispatch keypress event even if it's already been called for replacing previous character

Categories

(Core :: Widget: Cocoa, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox51 --- wontfix
firefox52 --- wontfix
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod, regression)

Attachments

(1 file)

Starting bug 1312649 fix, InsertText() always dispatches composition events when aReplacementRange is specified. At this time, handling NSKeyDown event is marked as keypress event was dispatched (after bug 1317906 fix, marked as composition event was dispatched). This flag causes blocking to dispatch keypress event even if InsertText() is called again without aReplacementRange.

This may be a problem for some simple IMEs. According to bug 1329474, Korean IME on Sierra may not use marked text at inputting Hangul character. In such case, Korean IME inserts a Hangul character as committed text. When user types another key to modify inputting Hangul character, Korean IME replaces previous character with a call of InsertText() with aReplacementRange. Finally, when user starts to input next Hangul character, Korean IME "commits" previous character with InsertText() with aReplacementRange and then, inserts new Hangul character at cursor position with a call of InsertText() *without* aReplacementRange.

Currently, the last InsertText() (without aReplacementRange) is ignored because the NSKeyDown has already caused dispatching composition events and preventing bug 420502.

However, we should dispatch keypress event in this case but shouldn't dispatch it when inserting text is a control character (for bug 420502).
I think that we should not uplift the fix to 52 because it's too late and *might* be risky. Additionally, nobody can check if this improves something in the real world.

On the other hand, if some users have trouble and it's fixed by this fix, it's worthwhile to take it to ESR52 later. So, I think that we should uplift it only to 53 and after release of 53, it might be a good timing to uplift to 52ESR (if it'll be allowed).
Comment on attachment 8840338 [details]
Bug 1341960 TextInputHandler shouldn't ignore InsertText() calls even if TextInputHandler has already dispatched keypress events and/or composition events for the key down but InsertText() is called again for inserting printable text

https://reviewboard.mozilla.org/r/114798/#review116596
Attachment #8840338 - Flags: review?(m_kato) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/0a0ad749ef25
TextInputHandler shouldn't ignore InsertText() calls even if TextInputHandler has already dispatched keypress events and/or composition events for the key down but InsertText() is called again for inserting printable text r=m_kato
https://hg.mozilla.org/mozilla-central/rev/0a0ad749ef25
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8840338 [details]
Bug 1341960 TextInputHandler shouldn't ignore InsertText() calls even if TextInputHandler has already dispatched keypress events and/or composition events for the key down but InsertText() is called again for inserting printable text

Approval Request Comment
[Feature/Bug causing the regression]:
Perhaps, regression of bug 1312649.

[User impact if declined]:
May be not be able to input expected character.

[Is this code covered by automated tests?]:
No.

[Has the fix been verified in Nightly?]:
No because nobody can reproduce this bug now.

[Needs manual test from QE? If yes, steps to reproduce]:
No.

[List of other uplifts needed for the feature/fix]:
No.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
Because it's very rare case to meet new adding condition of this patch.

The original bug report is bug 1329474. The reporter meet the bug, cannot input proper Hangul character with Korean IME. According to the log when the reporter reproduced the bug, oddly, Korean IME doesn't use marked text (we call composition string, which is underlined text) to input a Hangul character with multiple key press.

Then, I found this bug from the code. That is, we ignore inserting text request from IME after we already handle same request from IME (the reason to ignore same request is bug 420502). Therefore, this patch accepts the request when inserting text isn't a control character.

So, only a few user can meet this bug:
1. IME works without marked text.
2. IME requests to insert text twice or more at a keydown.

Currently, #1 is very rare and odd condition. The reporter cannot reproduce the bug anymore. I guess that the reporter met a bug of Sierra.

Although, this is rare, but I think that we should take this patch for 53 because most keyboard/IME bugs are reported after Beta stage and if user meets this bug, it's very serious bug.

[String changes made/needed]: No.
Attachment #8840338 - Flags: approval-mozilla-aurora?
Comment on attachment 8840338 [details]
Bug 1341960 TextInputHandler shouldn't ignore InsertText() calls even if TextInputHandler has already dispatched keypress events and/or composition events for the key down but InsertText() is called again for inserting printable text

Fix an insert text regression. Aurora53+.
Attachment #8840338 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I'm a little torn on whether this is worth considering for ESR52 consideration or not based on comment 7. On one hand, it seems like a pretty rare issue, but on the other hand, it seems pretty severe if it gets hit? What are your thoughts, Masayuki-san?
Flags: needinfo?(masayuki)
Yes, right. This must hit only few uses but if they hit this bug, it's too serious to use Firefox.

I have reported about this issue to Apple at fixing Vietnamese IME's odd behavior for investigating why IME may behave differently between applications and/or environments, but I have not gotten good answer from Apple yet. Since macOS Sierra, I feel some IMEs and/or the framework became really unstable.
Flags: needinfo?(masayuki)
Comment on attachment 8840338 [details]
Bug 1341960 TextInputHandler shouldn't ignore InsertText() calls even if TextInputHandler has already dispatched keypress events and/or composition events for the key down but InsertText() is called again for inserting printable text

Thanks for the reply. Given the severity for users who hit this bug, I think we should consider this for ESR52 as well.
Attachment #8840338 - Flags: approval-mozilla-esr52?
Comment on attachment 8840338 [details]
Bug 1341960 TextInputHandler shouldn't ignore InsertText() calls even if TextInputHandler has already dispatched keypress events and/or composition events for the key down but InsertText() is called again for inserting printable text

fix an issue with IME on mac, esr52+
Attachment #8840338 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Setting qe-verify- based on Masayuki Nakano's assessment on manual testing needs (see Comment 7).
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.