Closed Bug 1317906 Opened 8 years ago Closed 8 years ago

Fails to input a new character when there is a composition (ASCII char after a Hangul with 2-set Korean or incorrect base char after a dead char with Western keyboard layouts)

Categories

(Core :: Widget: Cocoa, defect)

Product:
Core
Component:
Widget: Cocoa
Version:
50 Branch
Platform:
All
macOS
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox-esr45 --- unaffected
firefox50 + fixed
firefox51 + fixed
firefox52 + fixed
firefox53 + verified

People

(Reporter: kimatg, Assigned: masayuki)

References

Details

(Keywords: inputmethod, regression)

Attachments

(3 files)

Log recorded when typing (a -> . -> . -> a -> space)
61.41 KB, text/plain
Details
Bug 1317906 When a key press causes a call of InsertText(), it shouldn't mark keypress as consumed but instead, should mark InsertText() caused composition
58 bytes, text/x-review-board-request
m_kato
: review+
gchang
: approval-mozilla-aurora+
gchang
: approval-mozilla-beta+
Details
Patch for Release branch
6.97 KB, patch
gchang
: approval-mozilla-beta-
ritu
: approval-mozilla-release+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161104212021

Steps to reproduce:

Updated to Firefox 50 from 49.


Actual results:

Korean IME is broken in that after typing a Hangul Syllable, typing Space does not automatically end conversion mode (Unlike Japanese or Chinese, Korean input does not require conversion before confirming input) and input space character. So basically you need to either hit spacebar twice or right arrow key before hitting space bar to insert spaces inbetween words.


Expected results:

Hitting spacebar when Korean syllable conversion is not completed should end conversion mode and insert a space character.

Problem is reproducible on Mac, default Korean IME (2-Set Korean).
Severity: normal → critical
Keywords: inputmethod
OS: Unspecified → Mac OS X
dup of bug 1233998?  Hansol, does this occur on 2-set IM only?
Flags: needinfo?(kimatg)
Component: Untriaged → Widget: Cocoa
Product: Firefox → Core
I've looked at bug 1233988 before posting but I think it's a bit different issue. I did not have that issue in Firefox 49 and on 50 neither. 

I'm not a 3-set user so I can't really say, but I tried testing with macOS default 3-set but seems to insert spacekey fine. Default 2-set Korean is affected as well as popular third-party Korean IME "Gureum", but strangely an older third-party Korean IME "Baram" seems to input fine. 

I also noticed that it's not only space key I can't enter while Korean syllable is still in conversion mode. Any non-korean characters including numerals (1,2,3), puctuations also require manually exiting the conversion block by pressing any key.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(kimatg)
I just did some more thorough testing, and it seems like behavior is all a bit different by IMEs:

[Default macOS 2-Set Korean]
Spacebar entered during conversion block exits conversion mode and inserts space as expected.
However, puctuation, numerals do not input correctly while in conversion block. (Instead of character being inputted, it just exits conversion mode.)

[Gureum (http://gureum.io/)]
Spacebar entered during conversion block is not printed but just exits conversion mode. (Need to press space twice to actually enter space character.)
Punctuation, numeral input during conversion block exits and enters character correctly.

[Baram (http://baramim.blogspot.kr/) - This is a once-was-popular 3rd party Korean IME for Mac, but development is halted since long so don't really take this much into account.]
Everything works as supposed to be.


All of the above IMEs behave correctly on other OS native applications and other browsers (Chrome, Safari) as well as Firefox 49.
Same problem here with Gureum 2-set with Firefox 50 & 51(aurora) on macOS Sierra. but:

[Gureum with 3-set families] Everything works as supposed to be.
I'm experiencing this as well. (macOS Sierra / Default IME / 2-Set Korean / FF 50.0)

Repro screencast: https://giphy.com/gifs/oOqAe2aT3qrzq
Hmm, I cannot reproduce this. However, looks like that this is really similar to bug 1312649.

Vietnamese Telex stops to use marked text (composition string which is underlined) in native cocoa apps. And when I test 2-Set Korean in Safari, the behavior is similar. I.e., inputting Hangul character is never underlined in Safari. (IIRC, 2-Set Korean used marked text at inputting Hangul character on El Captian or earlier, isn't it?)

On the other hand, in Google Chrome and Firefox, inputting Hangul character is underlined in my environment. But perhaps, this may depend on the internal state of IMK or 2-Set Korean like Vietnamese Telex. I'll test this again after rebooting macOS.
Sigh, even after rebooting macOS, I cannot reproduce this bug in my environment.

However, only in release (50), I don't see underline at inputting a Hangul character, but I see underline at same time on Aurora (52) and Nightly (53). So, this must be related to bug 1312649.
See Also: → 1312649
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #7)
> Hmm, I cannot reproduce this. However, looks like that this is really
> similar to bug 1312649.
> 
> Vietnamese Telex stops to use marked text (composition string which is
> underlined) in native cocoa apps. And when I test 2-Set Korean in Safari,
> the behavior is similar. I.e., inputting Hangul character is never
> underlined in Safari. (IIRC, 2-Set Korean used marked text at inputting
> Hangul character on El Captian or earlier, isn't it?)

Your issue seems to be bug 1233998.  2-set Korean doesn't sometimes call setMarkedText if it is 1st process that you use IME.
Somebody can attach log file for us?

I assume that you can reproduce this bug with following steps:

1. Type "a" key of QWERTY with 2-Set Korean.
2. Type "Space" key with 2-Set Korean.

If so, I'd like to see the log of these steps.

You can create log file with setting following env before launching Firefox:

MOZ_LOG=TextInputHandlerWidgets:5,sync
MOZ_LOG_FILE=~/fx.log (any path is okay, though)

Then, launch Firefox, then, click searchbar, do #1 and #2, finally, quit from Firefox with Quit menu (please don't use keyboard shortcut key during logging the behavior).

Then, please attach the log file to this bug (from the "Add an attachment" link above).

Be careful, during logging the behavior, all inputting text is logged. So, don't include any your privacy date like password.
(In reply to Makoto Kato [:m_kato] from comment #9)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #7)
> > Hmm, I cannot reproduce this. However, looks like that this is really
> > similar to bug 1312649.
> > 
> > Vietnamese Telex stops to use marked text (composition string which is
> > underlined) in native cocoa apps. And when I test 2-Set Korean in Safari,
> > the behavior is similar. I.e., inputting Hangul character is never
> > underlined in Safari. (IIRC, 2-Set Korean used marked text at inputting
> > Hangul character on El Captian or earlier, isn't it?)
> 
> Your issue seems to be bug 1233998.  2-set Korean doesn't sometimes call
> setMarkedText if it is 1st process that you use IME.

Ah, yes. I guess that it was fixed by bug 1312649's 2nd patch. It seems that we should take them into release build if it'd be allowed?
Space is working fine. Only punctuations (",", ".", etc.) are problematic in my case.
And I'd like somebody to fill this table...

                     +----+----+----+----+
                     | 50 | 51 | 52 | 53 |
+--------------------+----+----+----+----+
| macOS 2-Set Korean | BAD|  ? |  ? |  ? |
+--------------------+----+----+----+----+
| Gureum             | BAD| BAD|  ? |  ? |
+--------------------+----+----+----+----+
| Baram              | OK | OK |  ? |  ? |
+--------------------+----+----+----+----+
Attachment #8811178 - Attachment mime type: text/x-log → text/plain
Still buggy with punctuations in 51.0a2 / 53.0a1. (with macOS 2-Set Korean)

But Space is also working fine in those versions.
Thank you for the log file, that helps me what's going on.

Korean IME calls insertText: twice when you type something causes committing existing composition and insert new text. One is for committing existing composition. Then, next is inserting new character for the keypress.

At first insertText:, we started to consume following keypress event for other languages' IME behavior from 50:
https://dxr.mozilla.org/mozilla-central/rev/f8ba9c9b401f57b0047ddd6932cb830190865b38/widget/cocoa/TextInputHandler.mm#2203,2206-2207
Then, second insertText: is ignored for hack for legacy bug:
https://dxr.mozilla.org/mozilla-central/rev/f8ba9c9b401f57b0047ddd6932cb830190865b38/widget/cocoa/TextInputHandler.mm#2214

So, we need to change around here without regression. I'll take a look this deeper.
[Tracking Requested - why for this release]:
Serious regression of bug 1309515 and this affects a lot of Korean users of Mac.

I'm thinking that this can fix safely.
Assignee: nobody → masayuki
Blocks: 1309515
Status: NEW → ASSIGNED
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox-esr45: --- → unaffected
tracking-firefox50: --- → ?
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
Keywords: regression
Hardware: Unspecified → All
Tracked for 50. This might be lead to a fix in 50.1.0 or sooner.
tracking-firefox50: ? → +
Tracking 53+ so we can address this issue for our Korean user base.
tracking-firefox53: ? → +
Sorry for the review request spams. I forgot to add new state into the log.
The patch depends on bug 1312649, and it depends on bug 1310565, and finally, it depends on bug 1309515 (the regression cause of this).

It's risky to uplift bug 1312649 and bug 1310565 in Release. But for safer to uplift, they should be in Beta.

Only for Release branch, I'll create another patch.
Comment on attachment 8811574 [details]
Bug 1317906 When a key press causes a call of InsertText(), it shouldn't mark keypress as consumed but instead, should mark InsertText() caused composition

https://reviewboard.mozilla.org/r/93648/#review93746

Humm, this may causes breaking change on some sites.  But I cannot better workaround for this issue.  You should ask this to smuag for this envet change if needed.
Attachment #8811574 - Flags: review?(m_kato) → review+
(In reply to Makoto Kato [:m_kato] from comment #26)
> Comment on attachment 8811574 [details]
> Bug 1317906 When a key press causes a call of InsertText(), it shouldn't
> mark keypress as consumed but instead, should mark InsertText() caused
> composition
> 
> https://reviewboard.mozilla.org/r/93648/#review93746
> 
> Humm, this may causes breaking change on some sites.  But I cannot better
> workaround for this issue.  You should ask this to smuag for this envet
> change if needed.

I don't think so because Chromium works similar to new behavior:

1. keydown
2. compositionupdate (this updates composition string as existing Hangul character and new character)
3. compositionend
4. keyup

So, our behavior is redundant because there is a set of composition events between #3 and #4. However, we need a lot of work for it. We need to store all actions as pending action like TSFTextStore and flush it when it's back to interpretsKeyEvent.  Therefore, this patch just fire additional set of composition events for the inputting character of current key.

Additionally, keydown and keyup (and input) events are still useful/available for Korean IME for catching every committing composition (or changing marked text with input event).

Still does it sound bad for you?

FYI: Safari doesn't fire composition events because 2-Set Korean IME don't use marked text on Safari (even on El Capitan).
Flags: needinfo?(m_kato)
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #27)
> (In reply to Makoto Kato [:m_kato] from comment #26)
> > Comment on attachment 8811574 [details]
> > Bug 1317906 When a key press causes a call of InsertText(), it shouldn't
> > mark keypress as consumed but instead, should mark InsertText() caused
> > composition
> > 
> > https://reviewboard.mozilla.org/r/93648/#review93746
> > 
> > Humm, this may causes breaking change on some sites.  But I cannot better
> > workaround for this issue.  You should ask this to smuag for this envet
> > change if needed.
> 
> I don't think so because Chromium works similar to new behavior:
> 
> 1. keydown
> 2. compositionupdate (this updates composition string as existing Hangul
> character and new character)
> 3. compositionend
> 4. keyup
> 
> So, our behavior is redundant because there is a set of composition events
> between #3 and #4. However, we need a lot of work for it. We need to store
> all actions as pending action like TSFTextStore and flush it when it's back
> to interpretsKeyEvent.  Therefore, this patch just fire additional set of
> composition events for the inputting character of current key.
> 
> Additionally, keydown and keyup (and input) events are still
> useful/available for Korean IME for catching every committing composition
> (or changing marked text with input event).
> 
> Still does it sound bad for you?
> 
> FYI: Safari doesn't fire composition events because 2-Set Korean IME don't
> use marked text on Safari (even on El Capitan).

My concern is that some developers like UA check.  So, when order and events are changed, it may cause breaking web on Firefox only JS code.  We are no way to check whether event handling works even if we change this.
Flags: needinfo?(m_kato)
Hmm, I still have no idea of the scenarios which break the compatibility because this is changing the firing event it may not need UA check.

Smaug, do you think that changing firing keypress event to a set of composition events in this case?

2-Set Korean IME starts composition at typing a Korean syllable. Then, following syllable may be merged to the first syllable and make a Hangul character which the user wants. While composing a Hangul key, hitting some keys, e.g., ".", which is not a part of Hangul character, the IME commits composition and inserts a character for the key.

Our old behavior is, the inserting character is performed with a keypress event.  However, new behavior is, it's performed with a set of composition events after committing existing Hangul character. Chromium merges inputting the character to the existing composition.

So, our old behavior dispatches:

1. keydown (currently not dispatched by TextEventDisaptcher)
2. compositionupdate
3. compositionend
4. keypress (this is not dispatched by regression of bug 1309515)
5. keyup

our new behavior dispatches:

1. keydown (currently not dispatched by TextEventDispatcher)
2. compositionupdate
3. compositionend
4. compositionstart
5. compositionupdate
6. compositionend
7. keyup

Chromium dispatches:

1. keydown
2. compositionupdate
3. compositionend
4. keyup

The reasons why I need to change the behavior are:

1. Design of our code. using keypress event after committing composition during a key press is difficult to manage the state. So, current patch is safer than keeping the old behavior.
2. Using composition events are similar to Chromium's behavior even though separated set of composition events. So, if some web apps are not tested with Chromium, it may not have keypress event handler for this case or existing keypress event handler might break our behavior.

Therefore, I believe that this behavior change is reasonable.

Any idea?
Flags: needinfo?(bugs)
So Chromium merges basically two compositions to one, but our new proposed behavior wouldn't do that, right?
But I don't still understand what our current 'old' behavior is. There is just one compositionupdate/end. Do we also merge currently?
Maybe I'm misunderstanding what old and new mean here.
Flags: needinfo?(bugs)
But anyhow, the new approach doesn't feel too bad. We just have more accurate events for inputting than chromium.
(In reply to Olli Pettay [:smaug] (vacation Nov 19-26) from comment #30)
> So Chromium merges basically two compositions to one, but our new proposed
> behavior wouldn't do that, right?

Right.

> But I don't still understand what our current 'old' behavior is.

I meant the behavior of ~49.

> There is just one compositionupdate/end. Do we also merge currently?

No, we will start to use additional composition instead of keypress.

(In reply to Olli Pettay [:smaug] (vacation Nov 19-26) from comment #31)
> But anyhow, the new approach doesn't feel too bad. We just have more
> accurate events for inputting than chromium.

Yes. But I think that we should redesign our cocoa TextInputHandler because current design always generates composition when space key pressed with 2-Set Korean. When we send space key press event to IME, IME starts composition and commit it immediately. However, Chromium handles it as a keypress. I think that Chromium behavior prevents to expose odd IME behavior in some scenario.
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/b9a918aca10c
When a key press causes a call of InsertText(), it shouldn't mark keypress as consumed but instead, should mark InsertText() caused composition r=m_kato
Aurora can take same patch as Nightly.

Beta depends on if drivers take the patches of bug 1310565 and bug 1312649.

Release needs this patch since we shouldn't take those uplift risk in release branch.
https://hg.mozilla.org/mozilla-central/rev/b9a918aca10c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #38)
> Now, Nightly build has this fix. Could you somebody test the Korean IME
> behavior?
> https://archive.mozilla.org/pub/firefox/nightly/latest-mozilla-central/
> firefox-53.0a1.en-US.mac.dmg
> 
> And also I7d like somebody to test patched release build too:
> https://archive.mozilla.org/pub/firefox/try-builds/masayuki@d-toybox.com-
> b3c9eb6b89455fb72dd4e67fedf0baf79c87c99b/try-macosx64/firefox-50.1.0.en-US.
> mac.dmg

I tried first installing the patched release build 50.1.0, but somehow I can't open the extracted Nightly.app in Applications folder. When I launch it it says that the file is damaged and needs to be sent to trash.

The Nightly (53.0a1) installs and opens fine. I've confirmed that for all 3 korean IMEs (Default, Baram, Gureum) the space/punctuation issue have been fixed and normalized.

There is another issue that I just realized now that I hadn't mentioned when I first reported this. When you're still in conversion mode (underlined character) and you try to perform editing actions (Cmd+A, Shift+arrow keys), pressing key combinations doesn't perform actions right away as you would expect, but just exits conversion mode--which means you need to press the same keys again just like the space/punctuation issue above. 

Can you look into this?
(In reply to Hansol Kim (zvuc) from comment #39)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #38)
> > Now, Nightly build has this fix. Could you somebody test the Korean IME
> > behavior?
> > https://archive.mozilla.org/pub/firefox/nightly/latest-mozilla-central/
> > firefox-53.0a1.en-US.mac.dmg
> > 
> > And also I7d like somebody to test patched release build too:
> > https://archive.mozilla.org/pub/firefox/try-builds/masayuki@d-toybox.com-
> > b3c9eb6b89455fb72dd4e67fedf0baf79c87c99b/try-macosx64/firefox-50.1.0.en-US.
> > mac.dmg
> 
> I tried first installing the patched release build 50.1.0, but somehow I
> can't open the extracted Nightly.app in Applications folder. When I launch
> it it says that the file is damaged and needs to be sent to trash.

I think that you don't allow to run apps which are not signed nor installed from App Store.
https://www.tekrevue.com/tip/gatekeeper-macos-sierra/

> The Nightly (53.0a1) installs and opens fine. I've confirmed that for all 3
> korean IMEs (Default, Baram, Gureum) the space/punctuation issue have been
> fixed and normalized.

Thanks!

> There is another issue that I just realized now that I hadn't mentioned when
> I first reported this. When you're still in conversion mode (underlined
> character) and you try to perform editing actions (Cmd+A, Shift+arrow keys),
> pressing key combinations doesn't perform actions right away as you would
> expect, but just exits conversion mode--which means you need to press the
> same keys again just like the space/punctuation issue above. 
> 
> Can you look into this?

Hmm, we need to manage it in another bug since this bug is already closed for Nightly.
Status: RESOLVED → VERIFIED
Comment on attachment 8811574 [details]
Bug 1317906 When a key press causes a call of InsertText(), it shouldn't mark keypress as consumed but instead, should mark InsertText() caused composition

Approval Request Comment
[Feature/regressing bug #]: Regression of bug 1309515.
[User impact if declined]: Korean users cannot type non-Hangul characters as they expected (they need to commit Hangul character before inputting other characters like space, comma or period).
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: Low, this patch makes keyboard event state managed with separated flag than keypress and allow multiple inserting text of IME per keydown.
[String/UUID change made/needed]: Nothing.


FYI: If we take this patch which is being tested on Nightly into Beta, we need to uplift both the fixes of bug 1310565 and bug 1312649t too. If we should just fix this bug on Beta, we should take following patch which should be uplifted to Release.
Attachment #8811574 - Flags: approval-mozilla-beta?
Attachment #8811574 - Flags: approval-mozilla-aurora?
Comment on attachment 8812078 [details] [diff] [review]
Patch for Release branch

Approval Request Comment: See above comment.

For Beta, if we wont' take the fixes of bug 1310565 and bug 1312649, we should use this patch. However, bug 1310565 is important for Vietnamese users. So, they should be uplifted too.

For Release, for minimizing the risk, we should uplift only this patch. Although, uplifting both of bug 1310565 and bug 1312649 allows us to use same patch as Nightly.
Attachment #8812078 - Flags: approval-mozilla-release?
Attachment #8812078 - Flags: approval-mozilla-beta?
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #40)
> (In reply to Hansol Kim (zvuc) from comment #39)
> > (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #38)
> > There is another issue that I just realized now that I hadn't mentioned when
> > I first reported this. When you're still in conversion mode (underlined
> > character) and you try to perform editing actions (Cmd+A, Shift+arrow keys),
> > pressing key combinations doesn't perform actions right away as you would
> > expect, but just exits conversion mode--which means you need to press the
> > same keys again just like the space/punctuation issue above. 
> > 
> > Can you look into this?
> 
> Hmm, we need to manage it in another bug since this bug is already closed
> for Nightly.

I tested this briefly. Then, my conclusion is, current behavior is correct at least for current our design. This is the log at pressing Cmd+A during composition (some non-useful log is removed):

> [Main Thread]: I/TextInputHandlerWidgets 1214d0c80 TextInputHandler::HandleKeyDownEvent, aNativeEvent=1153d0e20, type=NSKeyDown, keyCode=0 (0x0), modifierFlags=0x100108, characters="a", charactersIgnoringModifiers="ㅁ(U+3141)"
> [Main Thread]: I/TextInputHandlerWidgets 1204bd9a0 TISInputSourceWrapper::InitKeyEvent, aNativeKeyEvent=1153d0e20, aKeyEvent.mMessage=eKeyDown, aInsertString=0, IsOpenedIMEMode()=TRUE
> [Main Thread]: I/TextInputHandlerWidgets 1204bd9a0 TISInputSourceWrapper::ComputeGeckoKeyCode, aNativeKeyCode=0x0, aKbType=0x2D, aCmdIsPressed=TRUE, IsOpenedIMEMode()=TRUE, IsASCIICapable()=FALSE
> [Main Thread]: I/TextInputHandlerWidgets 1204bd9a0 TISInputSourceWrapper::InitKeyEvent, shift=off, ctrl=off, alt=off, meta=ON
> [Main Thread]: I/TextInputHandlerWidgets 1214d0c80 TextInputHandler::HandleKeyDownEvent, calling interpretKeyEvents
> [Main Thread]: I/TextInputHandlerWidgets 1214d0c80 IMEInputHandler::SelectedRange, Destroyed()=FALSE, mSelectedRange={ location=15, length=0 }
> [Main Thread]: I/TextInputHandlerWidgets 1214d0c80 IMEInputHandler::HasMarkedText, mMarkedRange={ location=14, length=1 }
> [Main Thread]: I/TextInputHandlerWidgets 1214d0c80 IMEInputHandler::SetMarkedText, aAttrString="마(U+B9C8)", aSelectedRange={ location=1, length=0 }, aReplacementRange=7fff5a7db7d0 { location=9223372036854775807, length=0 }, Destroyed()=FALSE, IgnoreIMEComposition()=FALSE, IsIMEComposing()=TRUE, mMarkedRange={ location=14, length=1 }, keyevent=1153d0e20, keydownHandled=FALSE, keypressDispatched=FALSE, causedOtherKeyEvents=FALSE, compositionDispatched=FALSE
> [Main Thread]: I/TextInputHandlerWidgets 1214d0c80 IMEInputHandler::DispatchCompositionChangeEvent, aText="마", aAttrString="마(U+B9C8)", aSelectedRange={ location=1, length=0 }, Destroyed()=FALSE, mView=115a42580, mWidget=119c96800, inputContext=1160e8080, mIsIMEComposing=TRUE
> [Main Thread]: I/TextInputHandlerWidgets 1214d0c80 IMEInputHandler::HasMarkedText, mMarkedRange={ location=14, length=1 }
> [Main Thread]: I/TextInputHandlerWidgets 1214d0c80 TextInputHandler::InsertText, aAttrString="마(U+B9C8)", aReplacementRange=7fff5a7db9f8 { location=9223372036854775807, length=0 }, IsIMEComposing()=TRUE, IgnoreIMEComposition()=FALSE, keyevent=1153d0e20, keydownHandled=FALSE, keypressDispatched=FALSE, causedOtherKeyEvents=FALSE, compositionDispatched=TRUE
> [Main Thread]: I/TextInputHandlerWidgets 1214d0c80 IMEInputHandler::SelectedRange, Destroyed()=FALSE, mSelectedRange={ location=15, length=0 }
> [Main Thread]: I/TextInputHandlerWidgets 1214d0c80 IMEInputHandler::InsertTextAsCommittingComposition, aAttrString="마(U+B9C8)", aReplacementRange=7fff5a7db9f8 { location=9223372036854775807, length=0 }, Destroyed()=FALSE, IsIMEComposing()=TRUE, mMarkedRange={ location=14, length=1 }
> [Main Thread]: I/TextInputHandlerWidgets 1214d0c80 IMEInputHandler::DispatchCompositionCommitEvent, aCommitString=0x7fff5a7db690 ("마"), Destroyed()=FALSE, mView=115a42580, mWidget=119c96800, inputContext=1160e8080, mIsIMEComposing=TRUE
> [Main Thread]: I/TextInputHandlerWidgets 1214d0c80 IMEInputHandler::HasMarkedText, mMarkedRange={ location=9223372036854775807, length=0 }
> [Main Thread]: I/TextInputHandlerWidgets 1214d0c80 IMEInputHandler::HasMarkedText, mMarkedRange={ location=9223372036854775807, length=0 }
> [Main Thread]: I/TextInputHandlerWidgets 1214d0c80 IMEInputHandler::SelectedRange, Destroyed()=FALSE, mSelectedRange={ location=15, length=0 }
> [Main Thread]: I/TextInputHandlerWidgets 1214d0c80 TextInputHandler::DoCommandBySelector, aSelector="noop:", Destroyed()=FALSE, keydownHandled=FALSE, keypressHandled=FALSE, causedOtherKeyEvents=FALSE
> [Main Thread]: I/TextInputHandlerWidgets 1214d0c80 TextInputHandler::HandleKeyDownEvent, called interpretKeyEvents
> [Main Thread]: I/TextInputHandlerWidgets 1214d0c80 TextInputHandler::HandleKeyDownEvent, wasComposing=TRUE, IsIMEComposing()=FALSE
> [Main Thread]: I/TextInputHandlerWidgets 1214d0c80 TextInputHandler::HandleKeyDownEvent, keydown handled=FALSE, keypress handled=FALSE, causedOtherKeyEvents=FALSE, compositionDispatched=TRUE

First, Cmd+A commits composition as expected. Then, it sends command with doCommandBySelector. However, the command is "noop:", not "selectAll:".

Next, we should not treat any shortcut keys at committing composition because it might be handled as shortcut key of IME. I.e., IME might have already handled the key combination as their own shortcut key. In such case, performing something in app may cause "double action".

Finally, it's not a new regression. I also see same behavior in ESR 45.

Although, Safari and native NSTextView widget works as you claimed. So, ideally, we should follow the behavior. However, we need more time to change the behavior for investigation and shouldn't uplift such change.

So, could you file a new bug for it if you think that we should fix it in the future?
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #43)

I understand. I think this sort of issue pops up pretty often (in other applications as well) for Korean input systems because usually CJK input are thought as one for their similarities but only Korean is the odd one out that doesn't really have/need the conversion step until comitting composition compared to Chinese and Japanese.

I will do file a separate bug regarding the shortcut key issue.
Thanks for the speedy fix! :)
Hi Florin, Andrei, could you please help verify this fix? Not sure if you guys can repro it.
Flags: needinfo?(florin.mezei)
Flags: needinfo?(andrei.vaida)
Moving this to Ovidiu who's been investigating lately these sort of issues on Mac.
Flags: needinfo?(ovidiu.boca)
Flags: needinfo?(florin.mezei)
Flags: needinfo?(andrei.vaida)
Hi, 

I tested this issue on Mac OS X 10.12 with 2-set Korean input source following this steps:

1. Select 2-set Korean input source and open FF
2. In URL bar write something and hit space bar

Following this scenario the actual result is the same with expected result meaning space is inserted after the text. Tested on all FF 50 release, FF beta 51.0b2, FF aurora 52.0a2 and FF Nightly 53.0a1. All the builds are up to date (2016-11-22).

I tried another scenario:

1. Select 2-set Korean input source and open FF
2. In URL bar write some word or test and then write a number or a special character like "/.,?><"

The number or the special character is not inserted after the text from step 2.

I can reproduce this scenario on FF 50 release, FF beta 51.0b2, FF aurora 52.0a2.(all up to date) 
It works on FF Nightly 53.0a1 (2016-11-22), the number or the special character is inserted. 


I can confirm the fix on Nightly 53.0a1(2016-11-22), it works as expected if I use space bar, number or special character.
status-firefox53: fixed → verified
Flags: needinfo?(ovidiu.boca)
Comment on attachment 8811574 [details]
Bug 1317906 When a key press causes a call of InsertText(), it shouldn't mark keypress as consumed but instead, should mark InsertText() caused composition

Since I took the patches of bug 1310565 and bug 1312649 and this one was also verified, I take this patch in 51 beta, too. Beta51+ and Aurora52+. Should be in 51 beta 3.
Attachment #8811574 - Flags: approval-mozilla-beta?
Attachment #8811574 - Flags: approval-mozilla-beta+
Attachment #8811574 - Flags: approval-mozilla-aurora?
Attachment #8811574 - Flags: approval-mozilla-aurora+
Attachment #8812078 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Thanks for the verification guys. Given the late regressions we usually find in IME related fixes, I don't feel safe to take this fix in 50.0.1. We will however plan to uplift this in 50.1.0. Hoping that 3-4 weeks on Beta51 will help stabilize and catch any regressions.
has problems to apply:


merging widget/cocoa/TextInputHandler.mm
warning: conflicts while merging widget/cocoa/TextInputHandler.mm! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(masayuki)
I guess that it's Beta, did you merge the depending bugs before the patch?
Flags: needinfo?(masayuki) → needinfo?(cbook)
FYI: I confirmed that I don't see any error at importing this patch after applying the patch for bug 1310565 and the patches for bug 1312649.
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #54)
> FYI: I confirmed that I don't see any error at importing this patch after
> applying the patch for bug 1310565 and the patches for bug 1312649.

yeah this helped a lot - worked in this order also :)
Flags: needinfo?(cbook)
Is this problem fixed in beta? I tried 51b3 and it seemed okay in general.
However, I got a different problem when I typed Korean in a reply box on Facebook.
Every character that I typed is duplicated, i.e. they appear twice...
(In reply to Young from comment #57)
> Is this problem fixed in beta? I tried 51b3 and it seemed okay in general.
> However, I got a different problem when I typed Korean in a reply box on
> Facebook.
> Every character that I typed is duplicated, i.e. they appear twice...

Aurora(Developer Edition) and Beta is fixed as seen above(comment #52, comment #55). I didn't experienced Facebook input problem. Since Facebook does frequently changes their behaviors (A/B Testing), it's hard for developers to reproduce what's going on.
Summary: Broken Korean IME in Firefox 50+ (Mac) → Fails to input a new character when there is a composition (ASCII char after a Hangul with 2-set Korean or incorrect base char after a dead char with Western keyboard layouts)
Comment on attachment 8812078 [details] [diff] [review]
Patch for Release branch

Recent regression in 50 that is severe, let's include the fix in 50.1.0
Attachment #8812078 - Flags: approval-mozilla-release? → approval-mozilla-release+
See Also: → 1529466
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: