1400189 - Space character occasionally skiped when writeing in Korean IME

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[tjtncks]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170914100122

Steps to reproduce:

write korean characters in web in some forms (like search bars in site, twitch chat, etc... and type space bar while last letter doesn't finallized (blue square cover the last letter)
P.S it happens most offen in twitch search and chat.


Actual results:

occasionally (like once in 5~10 works), the last letter doesn finnalized, but space character (U+0020, I assume) doesn't appear after it.


Expected results:

space character should appear after last latter.

Comment 1

[Makoto Kato [:m_kato]]

Although I cannot reproduce this using Microsoft IME (Korean) + Windows 10.  Do you use same IME?

Comment 2

[tjtncks]

yes I using default MS IME for korean.
I captured a video of it, How I upload a attachment?

Comment 3

[tjtncks]

Attachment: bandicam 2017-09-15 21-27-01-226.mp4

Comment 4

[Makoto Kato [:m_kato]]

Ah, I can reproduce this on Twitter.  [Space] doesn't sometimes insert space.  But I cannot reproduce address bar and simple contenteditable sample.

Comment 5

[tjtncks]

Yes, it doesn't happen in simple forms.

Comment 6

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

I reproduced this bug even on simple <input> element like:
https://w3c.github.io/uievents/tools/key-event-viewer.html

Type "KeyA" and type "Space" quickly.

Then, I see no white spaces between Hangul characters. At that time, I see the events in this order:

> 	type			charCode	keyCode	key	code	isComposing	data	<input>.value
> 88	compositionstart	-		-	-	-	-		''	'ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ ㅁㅁ '
> 87	keyup			0		32		Space	0		-	'ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ ㅁㅁ '
> 86	input			-		-	-	-	-		-	'ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ ㅁㅁ '
> 85	keypress		32		0		Space	0		-	'ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ ㅁㅁ'
> 84	keydown			0		32		Space	0		-	'ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ ㅁㅁ'
> 83	input			-		-	-	-	-		-	'ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ ㅁㅁ'
> 82	compositionend		-		-	-	-	-		'ㅁ'	'ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ ㅁㅁ'
> 81	input			-		-	-	-	-		-	'ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ ㅁㅁ'
> 80	keyup			0		65	a	KeyA	0		-	'ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ ㅁㅁ'
> 79	input			-		-	-	-	-		-	'ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ ㅁㅁ'
> 78	compositionupdate	-		-	-	-	-		'ㅁ'	'ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ'
> 77	compositionstart	-		-	-	-	-		''	'ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ'
> 76	keyup			0		32		Space	0		-	'ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ'
> 75	input			-		-	-	-	-		-	'ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ'
> 74	compositionend		-		-	-	-	-		'ㅁ'	'ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ'
> 73	input			-		-	-	-	-		-	'ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ'
> 72	keypress		32		0		Space	0		-	'ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ'
> 71	keydown			0		32		Space	0		-	'ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ'
> 70	keyup			0		65	a	KeyA	0		-	'ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ'
> 69	input			-		-	-	-	-		-	'ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ'
> 68	compositionupdate	-		-	-	-	-		'ㅁ'	'ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ '
> 67	compositionstart	-		-	-	-	-		''	'ㅁ ㅁ ㅁ ㅁ ㅁ ㅁ '

According to this log, Space key event may be fired in a composition and ignored. As far as I've tested, I cannot reproduce this bug in non-e10s window. So, perhaps, event order is broken in remote process.

Comment 7

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

Could be a regression of bug 1389314 since it doesn't give same priority to composition events and selectionset event as keyboard events, but still not sure since it's for DnD.

Comment 8

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=40760344ba15972dfa23ca217f3ac289a0e46b3b

Comment 9

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

tjtncks:

Could you check if one of these builds fixes this bug?
x86: https://queue.taskcluster.net/v1/task/UCFGr3GHQGuahhafVU4Xpw/runs/0/artifacts/public/build/target.zip
x64: https://queue.taskcluster.net/v1/task/fOGr0WNIRnKc5N5hZOLR2A/runs/0/artifacts/public/build/target.zip

If you cannot reproduce this bug with these builds, I'll request to review the patch.

Comment 11

[tjtncks]

I couldn't reproduce the bug on it.
by the way, can you make the fix landed on FF57b4~5 to? as the bug was there from the 57, the beta versions are affected, too.

Comment 12

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

(In reply to tjtncks from comment #11)
> I couldn't reproduce the bug on it.

Thank you!

> by the way, can you make the fix landed on FF57b4~5 to? as the bug was there
> from the 57, the beta versions are affected, too.

I guess it's possible because of low risk.

Comment 13

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

Attachment: Bug 1400189 - Give same priority to composition events and selectionset event as keyboard events at sending those events from the main process to a remote process

When sending keyboard events from TabParent to its remote process, TabParent
may give higher priority to them than composition events and selectionset event.

Therefore, the event order between keyboard events and composition events (and
selection set event) may be broken.  Keyboard events which should cause
inputting some characters are ignored if keyboard events are fired before
compositionend event accidentally.

This patch gives same priority to composition events and selectionset event
as keyboard event for avoiding breaking the event order.

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

Comment 14

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

[Tracking Requested - why for this release]:

This is serious regression of bug 1389314. This must be really annoying for Korean users and may be so some other IME users too.

Comment 15

[Ming-Chou Shih [:stone]]

Comment on attachment 8913942 [details]
Bug 1400189 - Give same priority to composition events and selectionset event as keyboard events at sending those events from the main process to a remote process

https://reviewboard.mozilla.org/r/185322/#review190284

LGTM, Thanks.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d9c1d98878cb
Give same priority to composition events and selectionset event as keyboard events at sending those events from the main process to a remote process r=stone

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 18

[Ethan Tseng [:ethan]]

Hi, Masayuki
Wil you request an uplift to beta 57?

Comment 19

[tjtncks]

(In reply to Ethan Tseng [:ethan] - 57 Regression Engineering Support from comment #18)
> Hi, Masayuki
> Wil you request an uplift to beta 57?

(Masayuki Nakano [:masayuki] (JST, +0900) from comment #12)
> (In reply to tjtncks from comment #11)
> > I couldn't reproduce the bug on it.
> 
> Thank you!
> 
> > by the way, can you make the fix landed on FF57b4~5 to? as the bug was there
> > from the 57, the beta versions are affected, too.
> 
> I guess it's possible because of low risk.

I think he will.

Comment 20

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

Comment on attachment 8913942 [details]
Bug 1400189 - Give same priority to composition events and selectionset event as keyboard events at sending those events from the main process to a remote process

Approval Request Comment
[Feature/Bug causing the regression]:
Regression of bug 1389314.

[User impact if declined]:
Some keyboard inputs immediately after committing IME composition may be discarded in content process.

[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]:
No.

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

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
Just giving same priority as keyboard events to composition events and selection set event when sending them from TabParent to its remote process.

[String changes made/needed]:
No.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8913942 [details]
Bug 1400189 - Give same priority to composition events and selectionset event as keyboard events at sending those events from the main process to a remote process

Recent regression, taking it.
Should be in 57b5

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/89d8e11182d2

Comment 23

[Andrei Vaida [:avaida]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #20)
> [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]:
> No.

Setting qe-verify- based on Masayuki Nakano's assessment on manual testing needs.