Closed Bug 1358958 Opened 2 years ago Closed 2 years ago

Mac FireFox requires typing Enter twice to line breaking when typing Hangul

Categories

(Core :: Widget: Cocoa, defect)

52 Branch
x86_64
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox-esr52 --- affected
firefox54 --- wontfix
firefox55 --- verified

People

(Reporter: dapsdh, Assigned: masayuki)

References

Details

(Keywords: inputmethod, Whiteboard: tpi:+)

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.81 Safari/537.36

Steps to reproduce:

1. Access to http://www.danilatos.com/event-test/ExperimentTest.html
2. Type Hangul in the Editing area.
3. Press the Enter key.
4. Press the Enter key again.


Actual results:

The first enter key does not change the line. Enter key once more to change the line.


Expected results:

If you type Enter in Hangul input, the line should be changed immediately.
Because I created this issue on Windows, the above UserAgent is different from the test environment.
The UserAgent of the test browser is as follows.
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:52.0) Gecko/20100101 Firefox/52.0
Component: Untriaged → Widget: Cocoa
Product: Firefox → Core
Well, when I type Enter key with Korean 2-Set IME on Sierra, I got following log:

> TextInputHandler::HandleKeyDownEvent, aNativeEvent=0x11e37f100, type=NSKeyDown, keyCode=36 (0x24), modifierFlags=0x100, characters="(U+000D)", charactersIgnoringModifiers="(U+000D)"
> TISInputSourceWrapper::InitKeyEvent, aNativeKeyEvent=0x11e37f100, aKeyEvent.mMessage=eKeyDown, aInsertString=0x0, IsOpenedIMEMode()=TRUE
> TISInputSourceWrapper::ComputeGeckoKeyCode, aNativeKeyCode=0x24, aKbType=0x2D, aCmdIsPressed=FALSE, IsOpenedIMEMode()=TRUE, IsASCIICapable()=FALSE
> TISInputSourceWrapper::InitKeyEvent, shift=off, ctrl=off, alt=off, meta=off
> TextInputHandler::HandleKeyDownEvent, calling interpretKeyEvents
> IMEInputHandler::SelectedRange, Destroyed()=FALSE, mSelectedRange={ location=4, length=0 }
> IMEInputHandler::HasMarkedText, mMarkedRange={ location=3, length=1 }
> IMEInputHandler::GetValidAttributesForMarkedText
> IMEInputHandler::GetValidAttributesForMarkedText
> IMEInputHandler::GetValidAttributesForMarkedText
> IMEInputHandler::GetValidAttributesForMarkedText
> IMEInputHandler::GetValidAttributesForMarkedText
> IMEInputHandler::SetMarkedText, aAttrString="ㅏ(U+314F)", aSelectedRange={ location=1, length=0 }, aReplacementRange=0x7fff5d7e68f0 { location=9223372036854775807, length=0 }, Destroyed()=FALSE, IgnoreIMEComposition()=FALSE, IsIMEComposing()=TRUE, mMarkedRange={ location=3, length=1 }, keyevent=0x11e37f100, keydownHandled=FALSE, keypressDispatched=FALSE, causedOtherKeyEvents=FALSE, compositionDispatched=FALSE
> IMEInputHandler::DispatchCompositionChangeEvent, aText="ㅏ", aAttrString="ㅏ(U+314F)", aSelectedRange={ location=1, length=0 }, Destroyed()=FALSE, mView=0x11e7cce10, mWidget=0x113332000, inputContext=0x11d779b20, mIsIMEComposing=TRUE
> IMEInputHandler::GetRangeCount, aAttrString="ㅏ(U+314F)", count=1
> IMEInputHandler::ConvertToTextRangeType, aUnderlineStyle=2, aSelectedRange.length=0,
> IMEInputHandler::CreateTextRangeArray, range={ mStartOffset=0, mEndOffset=1, mRangeType=TextRangeType::eSelectedRawClause }
> IMEInputHandler::CreateTextRangeArray, range={ mStartOffset=1, mEndOffset=1, mRangeType=TextRangeType::eCaret }
> IMEInputHandler::HasMarkedText, mMarkedRange={ location=3, length=1 }
> IMEInputHandler::GetValidAttributesForMarkedText
> IMEInputHandler::GetValidAttributesForMarkedText
> IMEInputHandler::GetValidAttributesForMarkedText
> IMEInputHandler::GetValidAttributesForMarkedText
> TextInputHandler::InsertText, aAttrString="ㅏ(U+314F)", aReplacementRange=0x7fff5d7e6b38 { location=9223372036854775807, length=0 }, IsIMEComposing()=TRUE, IgnoreIMEComposition()=FALSE, keyevent=0x11e37f100, keydownHandled=FALSE, keypressDispatched=FALSE, causedOtherKeyEvents=FALSE, compositionDispatched=TRUE
> IMEInputHandler::SelectedRange, Destroyed()=FALSE, mSelectedRange={ location=4, length=0 }
> IMEInputHandler::InsertTextAsCommittingComposition, aAttrString="ㅏ(U+314F)", aReplacementRange=0x7fff5d7e6b38 { location=9223372036854775807, length=0 }, Destroyed()=FALSE, IsIMEComposing()=TRUE, mMarkedRange={ location=3, length=1 }
> IMEInputHandler::DispatchCompositionCommitEvent, aCommitString=0x0x7fff5d7e67d0 ("ㅏ"), Destroyed()=FALSE, mView=0x11e7cce10, mWidget=0x113332000, inputContext=0x11d779b20, mIsIMEComposing=TRUE
> IMEInputHandler::HasMarkedText, mMarkedRange={ location=9223372036854775807, length=0 }
> IMEInputHandler::HasMarkedText, mMarkedRange={ location=9223372036854775807, length=0 }
> IMEInputHandler::SelectedRange, Destroyed()=FALSE, mSelectedRange={ location=4, length=0 }
> TextInputHandler::DoCommandBySelector, aSelector="insertNewline:", Destroyed()=FALSE, keydownHandled=FALSE, keypressHandled=FALSE, causedOtherKeyEvents=FALSE
> TextInputHandler::HandleKeyDownEvent, called interpretKeyEvents
> 0x11e606f30 TextInputHandler::HandleKeyDownEvent, wasComposing=TRUE, IsIMEComposing()=FALSE
> TextInputHandler::HandleKeyDownEvent, keydown handled=FALSE, keypress handled=FALSE, causedOtherKeyEvents=FALSE, compositionDispatched=TRUE

The important thing is, "insertNewline:" command is sent, but we ignore it because the keydown event is already handled. So, I think that in such case, insertNewline: should cause additional composition to insert "\n".
Assignee: nobody → masayuki
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: inputmethod
OS: Unspecified → Mac OS X
Hardware: Unspecified → x86_64
Comment on attachment 8861885 [details]
Bug 1358958 part.1 Don't consume command when neither keydown nor keypress event was consumed

Oops, not enough for rich text editor due to bug 1350541.
Attachment #8861885 - Flags: review?(m_kato) → review-
Whiteboard: tpi:+
Comment on attachment 8861885 [details]
Bug 1358958 part.1 Don't consume command when neither keydown nor keypress event was consumed

https://reviewboard.mozilla.org/r/133904/#review137540
Attachment #8861885 - Flags: review?(m_kato) → review+
Comment on attachment 8862402 [details]
Bug 1358958 part.2 Implement TextInputHandler::InsertNewline() to handle "insertNewline:" command

https://reviewboard.mozilla.org/r/134342/#review137564
Attachment #8862402 - Flags: review?(m_kato) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/5ce6f1ab7c86
part.1 Don't consume command when neither keydown nor keypress event was consumed r=m_kato
https://hg.mozilla.org/integration/autoland/rev/4ce193967aee
part.2 Implement TextInputHandler::InsertNewline() to handle "insertNewline:" command r=m_kato
https://hg.mozilla.org/mozilla-central/rev/5ce6f1ab7c86
https://hg.mozilla.org/mozilla-central/rev/4ce193967aee
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Let's watch feedback from Nightly testers before requesting to uplift them. (part.2 has some regression risk.)
Comment on attachment 8861885 [details]
Bug 1358958 part.1 Don't consume command when neither keydown nor keypress event was consumed

Approval Request Comment
[Feature/Bug causing the regression]: Not sure, perhaps, it's regression (this affects only some IME users)
[User impact if declined]: As far as I tested, Korean and Vietnamese IME users need to type Enter key twice to break the line when there is composing text. I don't see same symptom with Japanese nor Chinese IME.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: Yes.
1. Install 2Set Korean IME.
2. Load data:text/html,<textarea></textarea>
3. Type "a" or something with Korean IME, then, you see underlined Hangul character.
4. Type Enter.
Then, caret should be next line.
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Our code accidentally consumed "insertNewline:" command after changing (committing) existing composition. Therefore, inserting new line (\n) was ignored.
[String changes made/needed]: No.
Attachment #8861885 - Flags: approval-mozilla-beta?
Comment on attachment 8861885 [details]
Bug 1358958 part.1 Don't consume command when neither keydown nor keypress event was consumed

[Approval Request Comment]
User impact if declined: As far as I tested, Korean and Vietnamese users need to type twice to break current line when there is composition.
Fix Landed on Version: 55.
Risk to taking this patch (and alternatives if risky): Not risky.
String or UUID changes made by this patch: No.
Attachment #8861885 - Flags: approval-mozilla-esr52?
Comment on attachment 8862402 [details]
Bug 1358958 part.2 Implement TextInputHandler::InsertNewline() to handle "insertNewline:" command

Approval Request Comment
[User impact if declined]: Only with the previous patch, <textarea> case is fixed but not so with <div contenteditable> and designMode.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: Yes.
STR is same as above, but load data:text/html,<div contenteditable><p><br></p></div>
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]: Honestly, this may have some risk.
[Why is the change risky/not risky?]: Our HTML editor doesn't support line breaks in composition string nor commit string. Therefore, sending "\n" doesn't cause line break (displayed as a whitespace). Therefore, this patch dispatches dummy Enter keypress event instead. So, it *might* cause different result at using Enter key in some web apps. On the other hand, this new behavior is more similar to Chrome's behavior. So, it *might* be safe enough even though changing the event behavior.
[String changes made/needed]: No.
Attachment #8862402 - Flags: approval-mozilla-beta?
Comment on attachment 8862402 [details]
Bug 1358958 part.2 Implement TextInputHandler::InsertNewline() to handle "insertNewline:" command

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is very annoying bug for some language users especially for Korean people.
User impact if declined: Only with the previous patch, Korean and Vietnamese users need to type Enter twice to break current line if there is composition in HTML editor like Gmail, Twitter, Facebook etc.
Fix Landed on Version: 55.
Risk to taking this patch (and alternatives if risky): This has some risk due to changing firing event at receiving "insertNewline:" command but there was composition or it's not caused by Enter keypress.  However, the new behavior is more similar to Chrome. Form this point of view, it might be safe, though.
String or UUID changes made by this patch: No.
Attachment #8862402 - Flags: approval-mozilla-esr52?
So, part 1 is safe and it can save the victims at least when they use <textarea>. On the other hand, part 2 has some risk but it may be worthwhile to uplift because major SNS web services use HTML editor as plaintext editor in these days.

The user base of the language users may be not big since this bug hasn't been reported quickly. So, due to the risk management, it's okay not to uplift them. However, in such case, this must be very annoying bug for them.
See Also: → 1361556
Duplicate of this bug: 1361556
This issue is an important issue, can you apply from version 53, the release version?
Hi :masayuki, 
If part2 changes the behaviors, can we only uplift part1 to Beta54 and let part2 ride the train?
Flags: needinfo?(masayuki)
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: qe-verify+
Flags: needinfo?(brindusa.tot)
(In reply to Gerry Chang [:gchang] from comment #19)
> Hi :masayuki, 
> If part2 changes the behaviors, can we only uplift part1 to Beta54 and let
> part2 ride the train?

Yes, that's a possible scenario (that's why I separated the patches) even if this bug is reproducible in some major web services, it must be worthwhile to uplift for some usual websites (which uses <textarea>).
Flags: needinfo?(masayuki)
I tested this bug on Mac OS X 10.10 with Nightly 55.0a1(2017-05-04) using the scenario from comment 14. I can confirm the fix and I will mark this as a verified fix.
Status: RESOLVED → VERIFIED
Flags: needinfo?(brindusa.tot)
Will Firefox be applied to this bug beta or release?
Comment on attachment 8861885 [details]
Bug 1358958 part.1 Don't consume command when neither keydown nor keypress event was consumed

Per comment #21, let's only uplift patch1 and let patch2 ride the train. Beta54+. Should be in 54 beta 6.
Attachment #8861885 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8862402 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
hi Ryan VanderMeulen.

54b6 has not been modified.
This issue is verified fixed with 54.0b8 (Build Id:20170515124824) on macOS 10.12.1.
Flags: qe-verify+
I tested it on 54.0b8 but it did not work. There is still a bug in entering Enter twice when typing Hangul
But in the 55 version it is correct.
Flags: needinfo?(masayuki)
(In reply to sanaz from comment #28)
> I tested it on 54.0b8 but it did not work. There is still a bug in entering
> Enter twice when typing Hangul

(In reply to sanaz from comment #29)
> But in the 55 version it is correct.


Did you test it with <textarea>? We won't fix this bug in richtext editor until 55 (see comment 21 and comment 24). You'll see inserting a whitespace in richtext editor in 54 instead of a line break.
Flags: needinfo?(masayuki) → needinfo?(sanaz)
Duplicate of this bug: 1361556
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #30)
> (In reply to sanaz from comment #28)
> > I tested it on 54.0b8 but it did not work. There is still a bug in entering
> > Enter twice when typing Hangul
> 
> (In reply to sanaz from comment #29)
> > But in the 55 version it is correct.
> 
> 
> Did you test it with <textarea>? We won't fix this bug in richtext editor
> until 55 (see comment 21 and comment 24). You'll see inserting a whitespace
> in richtext editor in 54 instead of a line break.



The environment I tested was

MacOS 10.12.5
Firefox 54.0b8 ~ 9,
Hangul

In the address bar and website textarea, bugs occur.

Enter it twice or hit Hangul and put space once to proceed.

It works fine on the 55 nightly yet fixed.

Will not fix in release 55?
Flags: needinfo?(sanaz) → needinfo?(masayuki)
Indeed, I confirmed that this hasn't been fixed on 54 yet. The patch might depend on something other patch(es). I'll check the Beta branch's code.
Flags: needinfo?(masayuki)
Unfortunately, the patch works as expected. The reason why it's not been fixed yet is, the patch depends on bug 1339331.

Currently, committing composition with \n in Beta is ignored by TextComposition class. I'll request to uplift bug 1339331.
Comment on attachment 8862402 [details]
Bug 1358958 part.2 Implement TextInputHandler::InsertNewline() to handle "insertNewline:" command

This part.2 is pretty risky for now. So, we shouldn't uplift this for now.

However, after releasing 55, we should try to uplift this if there will be no regression reports.
Attachment #8862402 - Flags: approval-mozilla-esr52?
Comment on attachment 8861885 [details]
Bug 1358958 part.1 Don't consume command when neither keydown nor keypress event was consumed

Now, I realized that this requires the patch for bug 1339331. I'll request this again soon.
Attachment #8861885 - Flags: approval-mozilla-esr52?
Comment on attachment 8861885 [details]
Bug 1358958 part.1 Don't consume command when neither keydown nor keypress event was consumed

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:

This must be really annoying bugs for Korean IME and some other simple IME users of macOS.

User impact if declined:

They need to type Enter key twice when they want to break a line during inputting characters with IME.

Fix Landed on Version:

55

Risk to taking this patch (and alternatives if risky):

This is not so risky patch because the behavior is changed only when a keydown event causes composition string change and after that insertNewline: comes.

String or UUID changes made by this patch: 

No.



This fix requires the patch for bug 1339331.
Attachment #8861885 - Flags: approval-mozilla-esr52?
Comment on attachment 8861885 [details]
Bug 1358958 part.1 Don't consume command when neither keydown nor keypress event was consumed

Sorry for the spam. The patch for bug 1339331. However, it depends on patches for bug 1347433 and they shouldn't be uplifted because of too big for uplifting.

So, we shouldn't take this to ESR52 for now.
Attachment #8861885 - Flags: approval-mozilla-esr52?
gchang:

Unfortunately, we cannot fix this bug only with the part.1 patch on Beta because it depends on the patch for bug 1339331, additionally, the patch depends on the big patches for bug 1347433.

However, if we can take the part.2, we can fix this bug on Beta. If you don't mind, I hope that part.2 is uplifted to Beta too. It has some risk, but for Korean users, I think we should take it because this bug must be really annoying.

And I think that these patches should be uplifted after the patches are included into release builds. Then, we won't have any regression reports, we should uplift them to ESR52.  Therefore, uplifting part.2 to Beta helps quicker release.

So, my questions are:

1. Do you agree with uplifting part.2 to Beta?
2. If not agreed, should we backout part.1 from Beta?

I know that remaining Beta cycle is short. But if it's possible, I recommend to uplift it because the victims must be really hate this bug and that may cause our users switching to the other browsers.
Flags: needinfo?(gchang)
Thank you for your consideration of Korean users. I think that this part contributes to the development of the web in conclusion as much as consideration.
Hi :masayuki,
1. These patches including this one & bug 1339331 are too huge and risky. Like you said, it's very late in Beta54 and these patches are not baked long enough. I'm afraid it will introduce other regressions with IME. I prefer not to take part 2 to Beta.
2. What's the cost if we leave part 1 in beta?
Flags: needinfo?(gchang) → needinfo?(masayuki)
(In reply to Gerry Chang [:gchang] from comment #41)
> Hi :masayuki,
> 1. These patches including this one & bug 1339331 are too huge and risky.
> Like you said, it's very late in Beta54 and these patches are not baked long
> enough. I'm afraid it will introduce other regressions with IME. I prefer
> not to take part 2 to Beta.

Okay, I agree.

> 2. What's the cost if we leave part 1 in beta?

If we keep landing part.1 in Beta, Beta dispatches a set of compositionstart, compositionupdate and compositionend event for trying to commit composition whose composition/commit string is "\n" after committing a Hangul character.

Probably, it won't cause any problem on actual web pages. However, the behavior is specific behavior only 54. Starting 55, the set of the events is replaced with an 'Enter' keypress event (by part.2).

So, for better consistency with 53/ESR52 and 55, 54 shouldn't dispatch the events. However, if you and QA team thinks it's risky to backout the patch from Beta. I think it's okay.

So, I recommend to back it out from Beta, but up to you.

Finally, I apologize about this problem.
Flags: needinfo?(masayuki) → needinfo?(gchang)
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #42)

> > 2. What's the cost if we leave part 1 in beta?
> 
> If we keep landing part.1 in Beta, Beta dispatches a set of
> compositionstart, compositionupdate and compositionend event for trying to
> commit composition whose composition/commit string is "\n" after committing
> a Hangul character.
> 
> Probably, it won't cause any problem on actual web pages. However, the
> behavior is specific behavior only 54. Starting 55, the set of the events is
> replaced with an 'Enter' keypress event (by part.2).
> 
> So, for better consistency with 53/ESR52 and 55, 54 shouldn't dispatch the
> events. However, if you and QA team thinks it's risky to backout the patch
> from Beta. I think it's okay.
> 
> So, I recommend to back it out from Beta, but up to you.
> 
> Finally, I apologize about this problem.

It's late in Beta54. I prefer not to change this. Thanks for the information.
Flags: needinfo?(gchang)
54 RC build is released today. Mark 54 won't fix.
You need to log in before you can comment on or make changes to this bug.