1189396 - [TSF] Some IME's composition string is committed at starting to input composition string in google.com

Status: RESOLVED FIXED

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

Description

[kilobtye]

Attachment: screenshot

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150630154324

Steps to reproduce:

1. go to www.google.com
2. make sure Google Instant predictions setting is on
3. use Microsoft IME that ships with Windows 8.1 to type something in search field. I tried with MS pinyin(Chinese) and Janpanese.






Actual results:

the first character of the input is duplicated. eg: the result of typing zhong is zzhong, kanji is kkanji. see attached image.

it only appears when typing on the google homepage and the page direct to search result page after typed the first character.


Expected results:

IE and Chrome have right results, so i guess maybe there is a bug in Firefox.

Comment 1

[Alice0775 White]

I can reproduce on Nightly42.0a1 + MS office IME2010 + Windows7
https://hg.mozilla.org/mozilla-central/rev/24f4d8e5e24b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0 ID:20150809030213

STR
1. Open https://www.google.com/ top (not necessary sign-in)
2. Focus textbox and IME on
3. Type 'm' 'o' 'j' 'i'


Actual results:
Page is redirected
And it display 'ｍおじ' in the textbox at the top of page.
'm' was committed, 'おじ' is underlined(i.e. during conversion)

Expected Results;
’もじ'  in the textbox, and ’もじ' should be underlined.

Comment 2

[Alice0775 White]

[Tracking Requested - why for this release]:

Comment 3

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

Hmm, I cannot access google.com. I redirected to google.co.jp. I'm not sure how to access .com. Although, I changed the accept language...

In .co.jp, I cannot reproduce this bug.

Comment 4

[Alice0775 White]

> STR
> 1. Open https://www.google.com/ top (not necessary sign-in)

1-0. Start Nightly with new created profile.
1-1. Open https://www.google.com/ (not necessary sign-in)
1-2. if the page was redirected to co.jp, Click "Google.com を使用" at the bottom-right of page

Comment 5

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

Oh, thanks. I've not realized the link.

Looks like that isn't this TSF mode only? Although, I cannot reproduce this bug with Win7 + MS-IME even with TSF mode. (I can reproduce this bug with other IMEs in TSF mode, but not so in IMM mode)

Comment 6

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

Or, do you reproduce this even with IMM mode?

Comment 7

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

Anyway, looks like that this is caused by redundant selection change notification. I think that we can fix this bug in IMEContentObserver.

Comment 8

[Alice0775 White]

At least, I cannot reproduce the problem on Nightly42.0a1 + MS office IME2010 + Windows7 + intl.tsf.enable=false

Comment 9

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

Thanks. I guess that this also could be reproduced on other platforms (perhaps, GTK?), but we should mark this as TSF specific bug for now.

Comment 10

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e230180a733

Comment 11

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5867e6aa43aa

Comment 12

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f7836ddb9ae

Comment 13

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=35ee489ff57c

Comment 14

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

Attachment: part.1 Implement IMENotification::SelectionChangeData::Clear() to initialize its members

With the STR, when the first character is input, google.com moves the input field to the above of the page from the center of the page. In this time, IMEContentObserver notifies IME of selection change which is not caused by composition nor selection event. Then, TSFTextStore commits composition. This is what the symptom.

However, in this situation, selection range in flat text isn't changed. So, if IMEContentObserver caches selection range and it doesn't notify selection change when selection range in flat text content isn't changed, we can *avoid* this symptom.

Yes, this approach isn't enough for more complicated cases. However, for now, we should use this approach because this approach is simpler.

With this and following patches, IMEContentObserver will store selection with IMENotification::SelectionChangeData. However, for using SelectionChangeData, we want constructors for simpler implementation. Therefore, SelectionChangeData is renamed to SelectionChangeDataBase and SelectionChangeData will have constructor, copy constructors and copy operators. The reason is that SelectionChangeDataBase cannot have constructors due to used in the union.


In this patch, first, cleans up IMENotification::mSelectionChangeData implementation.

Comment 15

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

Attachment: part.2 Implement IMENotification::SelectionChangeData::Assign() to copy its members

This patch cleans up of IMENotification::mSelectionChangeData with a copy method. And in IMENotification::Assign(), we shouldn't recreate mString instance when it's not necessary. That reduces runtime cost and prevents memory fragmentation.

Comment 16

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

Attachment: part.3 Make IMENotification::SelectionChangeData useful even outside of IMENotification

This patch renames SelectionChangeData to SelectionChangeData and create SelectionChangeData with constructors.

Additionally, SelectionChangeData has nsString instance and initializes mString with the pointer to the instance. This must prevent memory fragmentation which is not necessary.

Comment 17

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

Attachment: part.4 IMEContentObserver should cache the selection data at notifying IME of selection change

Now, IMEContentObserver can store selection range when it notifies IME of selection change. However, mCausedByComposition and mCausedBySelectionEvent are not necessary in this case. Therefore, for keeping simpler implementation, IMEContentObserver always set them false.

Comment 18

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

Attachment: part.5 IMEContentObserver should cache selection at gets focus and every selection change

However, it's not enough for fixing this bug.

It needs selection range when IME gets focus and selection is changed even when it's caused by composition but IME doesn't want the notification.

So, we need to post SelectionChangeEvent even if it's not necessary to notify IME due to caused by composition. After it modifies the stored selection, it checks if it should notify IME actually.

Comment 19

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

Attachment: part.6 IMEContentObserver shouldn't notify IME of selection change when the range isn't actually changed

Finally, IMEContentObserver shouldn't notify IME of selection change when selection isn't actually changed.

Comment 20

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

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

Comment 21

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

Comment on attachment 8646364 [details] [diff] [review]
part.1 Implement IMENotification::SelectionChangeData::Clear() to initialize its members

>+    void Clear()
>+    {
>+      mOffset = UINT32_MAX;
>+      mString->Truncate();
Can mString ever be null here, even in the future patches?

Comment 22

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

Comment on attachment 8646369 [details] [diff] [review]
part.2 Implement IMENotification::SelectionChangeData::Assign() to copy its members

>-        if (!mSelectionChangeData.mCausedByComposition) {
>-          mSelectionChangeData.mCausedByComposition =
>-            aNotification.mSelectionChangeData.mCausedByComposition;
>-        } else {
>-          mSelectionChangeData.mCausedByComposition =
>-            mSelectionChangeData.mCausedByComposition &&
>-              aNotification.mSelectionChangeData.mCausedByComposition;
>-        }
>-        if (!mSelectionChangeData.mCausedBySelectionEvent) {
>-          mSelectionChangeData.mCausedBySelectionEvent =
>-            aNotification.mSelectionChangeData.mCausedBySelectionEvent;
>-        } else {
>-          mSelectionChangeData.mCausedBySelectionEvent =
>-            mSelectionChangeData.mCausedBySelectionEvent &&
>-              aNotification.mSelectionChangeData.mCausedBySelectionEvent;
>-        }
>+        mSelectionChangeData.Assign(aNotification.mSelectionChangeData);

Oh, how did I miss those rather useless if-elses before.

Comment 23

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

more reviewing tomorrow

Comment 24

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

(In reply to Olli Pettay [:smaug] from comment #21)
> Comment on attachment 8646364 [details] [diff] [review]
> part.1 Implement IMENotification::SelectionChangeData::Clear() to initialize
> its members
> 
> >+    void Clear()
> >+    {
> >+      mOffset = UINT32_MAX;
> >+      mString->Truncate();
> Can mString ever be null here, even in the future patches?

Yes. In (new) SelectionChangeData, mString is initialized in the constructor. In SelectionChangeDataBase (current SelectionChangeData), mString is initialized when IMENotification::mMessage becomes NS_NOTIFY_IME_OF_SELECTION_CHANGE. So, when Clear() is called, each initializing code must have already been run.

Comment 25

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

Comment on attachment 8646377 [details] [diff] [review]
part.5 IMEContentObserver should cache selection at gets focus and every selection change


>+  // If the IME doesn't want selection change notifications caused by
>+  // composition, we should do nothing anymore.
>+  if (mCausedByComposition &&
>+      mIMEContentObserver->mUpdatePreference.WantChangesCausedByComposition()) {
>+    return NS_OK;
>+  }
Aren't you missing '!' before mIMEContentObserver->mUpdatePreference.WantChangesCausedByComposition() ?

Comment 26

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

Attachment: part.5 IMEContentObserver should cache selection at gets focus and every selection change

Oh, yes. Thank you.

I think that I should improve nsITextInputProcessor for being able to test NOTIFY_IME_OF_* in automated tests.

Comment 27

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

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/1f6713554675c6a655b0229b1e44decaf33ae0d2
changeset:  1f6713554675c6a655b0229b1e44decaf33ae0d2
user:       Masayuki Nakano <[e-mail]>
date:       Sat Aug 22 01:43:41 2015 +0900
description:
Bug 1189396 part.1 Implement IMENotification::SelectionChangeData::Clear() to initialize its members r=smaug

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/f99e7dec5a4f5ca54d0562431bc67bdbff8a7695
changeset:  f99e7dec5a4f5ca54d0562431bc67bdbff8a7695
user:       Masayuki Nakano <[e-mail]>
date:       Sat Aug 22 01:43:41 2015 +0900
description:
Bug 1189396 part.2 Implement IMENotification::SelectionChangeData::Assign() to copy its members r=smaug

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/c5f78184538a0ce83186948a8983344318755556
changeset:  c5f78184538a0ce83186948a8983344318755556
user:       Masayuki Nakano <[e-mail]>
date:       Sat Aug 22 01:43:41 2015 +0900
description:
Bug 1189396 part.3 Make IMENotification::SelectionChangeData useful even outside of IMENotification r=smaug

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/f124971f48a6d519bf79a0f360e6b5acdff673fb
changeset:  f124971f48a6d519bf79a0f360e6b5acdff673fb
user:       Masayuki Nakano <[e-mail]>
date:       Sat Aug 22 01:43:41 2015 +0900
description:
Bug 1189396 part.4 IMEContentObserver should cache the selection data at notifying IME of selection change r=smaug

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/174cb5e95ffd0544fb67d735fe4c003b53344e07
changeset:  174cb5e95ffd0544fb67d735fe4c003b53344e07
user:       Masayuki Nakano <[e-mail]>
date:       Sat Aug 22 01:43:42 2015 +0900
description:
Bug 1189396 part.5 IMEContentObserver should cache selection at gets focus and every selection change r=smaug

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/792d790f6bee08d4808563c09c2b3dc585a5a302
changeset:  792d790f6bee08d4808563c09c2b3dc585a5a302
user:       Masayuki Nakano <[e-mail]>
date:       Sat Aug 22 01:43:42 2015 +0900
description:
Bug 1189396 part.6 IMEContentObserver shouldn't notify IME of selection change when the range isn't actually changed r=smaug

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1f6713554675
https://hg.mozilla.org/mozilla-central/rev/f99e7dec5a4f
https://hg.mozilla.org/mozilla-central/rev/c5f78184538a
https://hg.mozilla.org/mozilla-central/rev/f124971f48a6
https://hg.mozilla.org/mozilla-central/rev/174cb5e95ffd
https://hg.mozilla.org/mozilla-central/rev/792d790f6bee

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Too late for 40.
Masayuki, are you planning to request uplift that to 41 or 42? Thanks

Comment 30

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

(In reply to Sylvestre Ledru [:sylvestre] from comment #29)
> Too late for 40.
> Masayuki, are you planning to request uplift that to 41 or 42? Thanks

Ah, yes.

Comment 31

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #30)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #29)
> > Too late for 40.
> > Masayuki, are you planning to request uplift that to 41 or 42? Thanks
> 
> Ah, yes.

Er, no. The requests are not mine. This fix is pretty risky. We should not uplift them.

Comment 32

[Sylvestre Ledru [:Sylvestre]]

OK, let it ride the train then. Thanks