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

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[dapsdh]

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.

Comment 1

[dapsdh]

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

Comment 2

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

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

Comment 3

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

Attachment: Bug 1358958 part.1 Don't consume command when neither keydown nor keypress event was consumed

When typing Enter key when active keyboard layout is Korean IME and it has composition string, the composition string is committed and then, "insertNewline:" command is sent. However, TextInputHandler::DoCommandBySelector() consumes the command because the key event has already modified the composition string.

This patch makes TextInputHandler::DoCommandBySelector() consume the command if it's not handling keydown or neither dispatched keydown event nor dispatched keypress event (if it does) is consumed. Therefore, insertNewline:sender of nsChildView will be called later, then, it causes inserting a line break with a set of composition events.

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

Comment 4

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

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.

Comment 5

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

Attachment: Bug 1358958 part.2 Implement TextInputHandler::InsertNewline() to handle "insertNewline:" command

Due to bug 1358958, simply inserting a line breaker with composition events doesn't work as expected in HTML editor.  Therefore, we need to dispatch "fake" Enter keypress event even if it's not handling Enter key actually or shouldn't dispatch keypress event anymore.

The method tries to dispatch Enter keypress event.  If it's handling Enter key press actually and can dispatch keypress event normally, it dispatches Enter keypress event as-is.  Otherwise, it tries to dispatch "fake" Enter keypress.  It doesn't have Control, Option and Command key state for emulating to insert a line breaker.  Additionally, its code value is not set to "Enter" because the fake key event isn't a physical key event.

If it cannot dispatch Enter keypress event, it dispatches composition events to insert "\n" even though it won't work in HTML editor.

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

Comment 6

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

Comment on attachment 8861885 [details]
Bug 1358958 part.1 Don't consume command when neither keydown nor keypress event was consumed

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

Comment 7

[Makoto Kato [:m_kato]]

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

Comment 8

[Makoto Kato [:m_kato]]

Comment on attachment 8862402 [details]
Bug 1358958 part.2 Implement TextInputHandler::InsertNewline() to handle "insertNewline:" command

https://reviewboard.mozilla.org/r/134342/#review137564

Comment 9

[Pulsebot]

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

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5ce6f1ab7c86
https://hg.mozilla.org/mozilla-central/rev/4ce193967aee

Comment 11

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

Let's watch feedback from Nightly testers before requesting to uplift them. (part.2 has some regression risk.)

Comment 12

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

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.

Comment 13

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

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.

Comment 14

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

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.

Comment 15

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

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.

Comment 16

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

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.

Comment 18

[sanaz]

This issue is an important issue, can you apply from version 53, the release version?

Comment 19

[Gerry Chang [:gchang]]

Hi :masayuki, 
If part2 changes the behaviors, can we only uplift part1 to Beta54 and let part2 ride the train?

Comment 20

[Gerry Chang [:gchang]]

Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

Comment 21

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

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

Comment 22

[ovidiu boca[:Ovidiu]]

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.

Comment 23

[sanaz]

Will Firefox be applied to this bug beta or release?

Comment 24

[Gerry Chang [:gchang]]

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.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bf1dbe42eca0

Comment 26

[sanaz]

hi Ryan VanderMeulen.

54b6 has not been modified.

Comment 27

[Emil Ghitta, QA [:emilghitta]]

This issue is verified fixed with 54.0b8 (Build Id:20170515124824) on macOS 10.12.1.

Comment 28

[sanaz]

I tested it on 54.0b8 but it did not work. There is still a bug in entering Enter twice when typing Hangul

Comment 29

[sanaz]

But in the 55 version it is correct.

Comment 30

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

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

Comment 32

[sanaz]

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

Comment 33

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

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.

Comment 34

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

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 35

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

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.

Comment 36

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

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.

Comment 37

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

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.

Comment 38

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

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.

Comment 39

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

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.

Comment 40

[sanaz]

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.

Comment 41

[Gerry Chang [:gchang]]

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?

Comment 42

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

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

Comment 43

[Gerry Chang [: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.

Comment 44

[Gerry Chang [:gchang]]

54 RC build is released today. Mark 54 won't fix.