Closed Bug 1339331 Opened 3 years ago Closed 3 years ago

[TSF] new line is ignored when use ATOK's template input function

Categories

(Core :: Widget: Win32, defect, P2)

41 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- wontfix
firefox51 --- wontfix
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: ayakawa.m, Assigned: masayuki)

References

Details

(Keywords: inputmethod, regression, Whiteboard: tpi:+)

Attachments

(2 files)

Attached image result.jpg
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170213030206

Steps to reproduce:

1.enter Editable area(for example, bugzilla comment field)
2.atok on( I use atok2017)
3.push Ctrl+Shift+F11(default assigned template input:定型文章入力)
4.select sentence with new line



Actual results:

Line breaks in fixed form sentences are ignored


Expected results:

Line breaks in fixed form sentences are reflected properly
What happens if you make dom.compositionevent.allow_control_characters false? (I guess you need to restart Firefox to enable the pref change.)
Flags: needinfo?(ayakawa.m)
Keywords: inputmethod
Oops, I meant making it true.
change dom.compositionevent.allow_control_characters to TRUE, and I tried again.
Same result. New line is ignored.
(In reply to Takeshi Ichimaru(a.k.a. Ayakawa) from comment #3)
> change dom.compositionevent.allow_control_characters to TRUE, and I tried
> again.
> Same result. New line is ignored.

Thank you. I'll check it in my envrionment later, but I have some urgent bugs, so, I cannot work on this soon, sorry.
Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3ae3c45db961&tochange=0a7ace2ffb9b

Regressed by: 6f16a41f2318	Masayuki Nakano — Bug 1187579 Enable TSF in e10s mode r=m_kato



When intl.tsf.enable=false (and restart browser), it fixes the problem with/without e10s.

And it triggered to TSF on in Bug 478029 non-e10s.
Blocks: 478029, 1187579
Status: UNCONFIRMED → NEW
Component: Untriaged → Widget: Win32
Ever confirmed: true
Keywords: regression
Version: Trunk → 41 Branch
Summary: new line is ignored when use ATOK's template input function → [TSF] new line is ignored when use ATOK's template input function
Alice-san: You don't need to (and shouldn't) mark like this bug as a blocker of bug 478029 because it just enabled the pref. So, it doesn't point actual regression cause and it spreads bug spam to the watchers. Only adding "[TSF]" into the summary is enough.
Flags: needinfo?(ayakawa.m)
Assignee: nobody → masayuki
Priority: -- → P2
Whiteboard: tpi:+
Comment on attachment 8847977 [details]
Bug 1339331 TextEventDispatcher should replace \r in composition string with \n and TextComposition should allow to input \n with composition events

https://reviewboard.mozilla.org/r/120934/#review123250
Attachment #8847977 - Flags: review?(m_kato) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/8e72178c3893
TextEventDispatcher should replace \r in composition string with \n and TextComposition should allow to input \n with composition events r=m_kato
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/8e72178c3893
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Should we consider this for uplift?
Flags: needinfo?(masayuki)
Honestly, I have no idea whether this should be uplifted or not.

If we'll uplift this, the patches for bug 1347433 are also necessary to be uplifted. Fortunately, they and this have automated tests. So, the risk must not be so high.

On the other hand, we've never gotten this report since TSF is enabled. So, I'm not sure how many users will be helped by the uplift.

I know, initial TSF implementation already handled \r\n in composition string. IIRC, that was for tablet PC released with WinXP. However, we've already stopped supporting TSF on WinXP due to unstable and if such users still use Firefox on such devices, we must have gotten this reports earlier.

So, it seems that only a few users have trouble with this bug but they don't think that this is not so inconvenience because nobody hasn't reported this.

Makoto-san, do you have some ideas about this?
Flags: needinfo?(masayuki) → needinfo?(m_kato)
humm, change isn't small, so it is too late to uplift to beta.  And we have a workaround not to use TSF.
If change isn't big, we should uplift this to aurora.  And, if critical, some people already alerts this regression after changing to TSF.  But no reported...   So I think that we shouldn't uplift to aurora, too.
Flags: needinfo?(m_kato)
This should be uplifted for fixing bug 1358958 which is critical for Korean IME users on macOS.
Comment on attachment 8847977 [details]
Bug 1339331 TextEventDispatcher should replace \r in composition string with \n and TextComposition should allow to input \n with composition events

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

This is required to fix bug 1358958 which is really annoying bug for Korean IME users on macOS.

User impact if declined:

A few ATOK (Japanese IME) users cannot use its template function if template includes line breakers.

Korean IME users of macOS needs to type Enter key twice to insert a line break during inputting Hangul characters.

Fix Landed on Version:

55

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

Not risky because this makes TextComposition which manages IME composition in content allow to include \n in composition/commit string.

String or UUID changes made by this patch:

No.

Approval Request Comment
[Feature/Bug causing the regression]:

Perhaps, regression of bug 1158456.

[User impact if declined]:

See above. This is required to fix bug 1358958.

[Is this code covered by automated tests?]:

Yes. The patch includes automated tests.

[Has the fix been verified in Nightly?]:

Yes.

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

ATOK isn't free to charge and it has only Japanese UI to setup and use. So, it's difficult to check the original report's symptom. However, after landing this and the patch for bug 1358958, you can check this with manual QA with Korean IME.

1. Focus to a <textarea>, e.g., in bugzilla.
2. Activate 2-Set Korean IME on macOS.
3. Type 'a' and type 'Enter'.

Then, it should cause breaking the line.

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

No.

[Is the change risky?]:

No.

[Why is the change risky/not risky?]:

See above. And this patch has automated tests.

[String changes made/needed]:

No.
Attachment #8847977 - Flags: approval-mozilla-esr52?
Attachment #8847977 - Flags: approval-mozilla-beta?
Comment on attachment 8847977 [details]
Bug 1339331 TextEventDispatcher should replace \r in composition string with \n and TextComposition should allow to input \n with composition events

Sorry for the spam. This patch cannot graft from mozilla-central.
Attachment #8847977 - Flags: approval-mozilla-esr52?
Attachment #8847977 - Flags: approval-mozilla-beta?
Sigh, this cannot be uplifted without patches for bug 1347433. And they shouldn't be uplifted due to the risk management...
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/26) from comment #21)
> Sigh, this cannot be uplifted without patches for bug 1347433. And they
> shouldn't be uplifted due to the risk management...

What do you think is so risky about the patches from that bug?  Are you worried about webcompat issues?
Flags: needinfo?(masayuki)
Like :masayuki mentioned in comment #21, it's too risky given we are in the very late Beta54 cycle. I prefer to let this ride the train and won't fix in 54.
Not the webcompat issue. However, basically, like the patches for bug 1347433 which redesign the initializer of events before dispatching shouldn't be uplifted to anything, especially, ESR.
Flags: needinfo?(masayuki)
You need to log in before you can comment on or make changes to this bug.