1317906 - 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)

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Hansol Kim (zvuc)]

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).

Comment 1

[Makoto Kato [:m_kato]]

dup of bug 1233998?  Hansol, does this occur on 2-set IM only?

Comment 2

[Hansol Kim (zvuc)]

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.

Comment 3

[Hansol Kim (zvuc)]

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.

Comment 4

[Sangwon Yi]

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.

Comment 5

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

Hmm, regression of bug 1309515? I don't find other bugs which changes cocoa specific code except logger changes.
https://bugzilla.mozilla.org/buglist.cgi?keywords=inputmethod%2C%20&keywords_type=allwords&f1=OP&list_id=13312294&f0=OP&j2=OR&resolution=FIXED&f4=CP&query_format=advanced&f3=CP&target_milestone=mozilla50&target_milestone=mozilla51&target_milestone=mozilla52&product=Core&product=Firefox&product=Firefox%20for%20Android&product=Other%20Applications&product=SeaMonkey&product=Thunderbird&product=Toolkit

Comment 6

[Taeho Kim]

I'm experiencing this as well. (macOS Sierra / Default IME / 2-Set Korean / FF 50.0)

Repro screencast: https://giphy.com/gifs/oOqAe2aT3qrzq

Comment 7

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

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.

Comment 8

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

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.

Comment 9

[Makoto Kato [:m_kato]]

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

Comment 10

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

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.

Comment 11

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

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

Comment 12

[Taeho Kim]

Attachment: Log recorded when typing (a -> . -> . -> a -> space)

Space is working fine. Only punctuations (",", ".", etc.) are problematic in my case.

Comment 13

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

And I'd like somebody to fill this table...

                     +----+----+----+----+
                     | 50 | 51 | 52 | 53 |
+--------------------+----+----+----+----+
| macOS 2-Set Korean | BAD|  ? |  ? |  ? |
+--------------------+----+----+----+----+
| Gureum             | BAD| BAD|  ? |  ? |
+--------------------+----+----+----+----+
| Baram              | OK | OK |  ? |  ? |
+--------------------+----+----+----+----+

Comment 14

[Taeho Kim]

Still buggy with punctuations in 51.0a2 / 53.0a1. (with macOS 2-Set Korean)

But Space is also working fine in those versions.

Comment 15

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

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.

Comment 16

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

[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.

Comment 17

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=785ee133a752

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 50. This might be lead to a fix in 50.1.0 or sooner.

Comment 19

[Marcia Knous [:marcia]]

Tracking 53+ so we can address this issue for our Korean user base.

Comment 20

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

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

Currently, when InsertText() which is caused by a key press causes committing composition, it consumes keypress event.  However, Korean 2-set IME calls InsertText() two times when there is composition and key press causes inserting another character next to the composition.  In this case, current design ignores second InsertText() becuase keypress event is already consumed by the first InsertText() call.

For solving this issue safely, InsertText() should mark current key event as "dispatched composition event". Then, following InsertText() calls should cause composition events instead of keypress events since following event order is too odd:

1. keydown (currently not dispatched by TextEventDisaptcher)
2. compositionupdate
3. compositionend
4. keypress
5. keyup

with the new design this becomes:

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

This is similar to Chromium, although, Chromium includes the second InsertText() call into the first composition, we need to fix it later due to risky.

Review commit: https://reviewboard.mozilla.org/r/93648/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93648/

Comment 21

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

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93648/diff/1-2/

Comment 22

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

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/93648/diff/2-3/

Comment 23

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

Sorry for the review request spams. I forgot to add new state into the log.

Comment 24

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

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 25

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0db0a2757aa4

Comment 26

[Makoto Kato [:m_kato]]

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.

Comment 27

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

(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).

Comment 28

[Makoto Kato [: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.

Comment 29

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

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?

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

But anyhow, the new approach doesn't feel too bad. We just have more accurate events for inputting than chromium.

Comment 32

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

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

Comment 33

[Pulsebot]

Pushed by [e-mail]:
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

Comment 35

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

Attachment: Patch for Release branch

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.

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b9a918aca10c

Comment 37

[areya]

https://hg.mozilla.org/mozilla-central/rev/b9a918aca10c

Comment 38

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

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/[e-mail]-b3c9eb6b89455fb72dd4e67fedf0baf79c87c99b/try-macosx64/firefox-50.1.0.en-US.mac.dmg

Comment 39

[Hansol Kim (zvuc)]

(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/[e-mail]-
> 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?

Comment 40

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

(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/[e-mail]-
> > 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.

Comment 41

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

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.

Comment 42

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

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.

Comment 43

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

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

Comment 44

[Hansol Kim (zvuc)]

(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! :)

Comment 45

[Julien Cristau [:jcristau]]

tracking for 51 and 52

Comment 46

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Florin, Andrei, could you please help verify this fix? Not sure if you guys can repro it.

Comment 47

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Moving this to Ovidiu who's been investigating lately these sort of issues on Mac.

Comment 48

[ovidiu boca[:Ovidiu]]

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.

Comment 49

[Gerry Chang [:gchang]]

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.

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ef9b2a0ba51b

Comment 51

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 52

[Carsten Book [:Tomcat]]

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')

Comment 53

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

I guess that it's Beta, did you merge the depending bugs before the patch?

Comment 54

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

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.

Comment 55

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/434ad784075d

Comment 56

[Carsten Book [:Tomcat]]

(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 :)

Comment 57

[Young]

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

Comment 58

[Sangwon Yi]

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

Comment 60

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Comment 61

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/b6520a9a6d19