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

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: kilobtye, Assigned: masayuki)

Tracking

({inputmethod})

Trunk
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 wontfix, firefox40+ wontfix, firefox41+ wontfix, firefox42+ wontfix, firefox43 fixed)

Details

Attachments

(7 attachments, 1 obsolete attachment)

14.05 KB, image/png
Details
1.97 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.18 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.35 KB, patch
smaug
: review+
Details | Diff | Splinter Review
8.26 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.08 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.81 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Posted image 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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: inputmethod
Flags: needinfo?(masayuki)
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 'mおじ' 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.
[Tracking Requested - why for this release]:
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.
Flags: needinfo?(masayuki)
> 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
Flags: needinfo?(masayuki)
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)
Flags: needinfo?(masayuki)
Or, do you reproduce this even with IMM mode?
Flags: needinfo?(alice0775)
Anyway, looks like that this is caused by redundant selection change notification. I think that we can fix this bug in IMEContentObserver.
Assignee: nobody → masayuki
Blocks: 1184890
Status: NEW → ASSIGNED
Component: Untriaged → Event Handling
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → All
Version: 39 Branch → Trunk
At least, I cannot reproduce the problem on Nightly42.0a1 + MS office IME2010 + Windows7 + intl.tsf.enable=false
Flags: needinfo?(alice0775)
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.
Summary: MS IME Shows Redundent Character → [TSF] Some IME's composition string is committed at starting to input composition string in google.com
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.
Attachment #8646364 - Flags: review?(bugs)
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.
Attachment #8646369 - Flags: review?(bugs)
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.
Attachment #8646375 - Flags: review?(bugs)
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.
Attachment #8646376 - Flags: review?(bugs)
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.
Attachment #8646377 - Flags: review?(bugs)
Finally, IMEContentObserver shouldn't notify IME of selection change when selection isn't actually changed.
Attachment #8646378 - Flags: review?(bugs)
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?
Attachment #8646364 - Flags: review?(bugs) → review+
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.
Attachment #8646369 - Flags: review?(bugs) → review+
Attachment #8646375 - Flags: review?(bugs) → review+
more reviewing tomorrow
(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.
Attachment #8646376 - Flags: review?(bugs) → review+
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() ?
Attachment #8646377 - Flags: review?(bugs) → review-
Attachment #8646378 - Flags: review?(bugs) → review+
Oh, yes. Thank you.

I think that I should improve nsITextInputProcessor for being able to test NOTIFY_IME_OF_* in automated tests.
Attachment #8646377 - Attachment is obsolete: true
Attachment #8651026 - Flags: review?(bugs)
Attachment #8651026 - Flags: review?(bugs) → review+
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/1f6713554675c6a655b0229b1e44decaf33ae0d2
changeset:  1f6713554675c6a655b0229b1e44decaf33ae0d2
user:       Masayuki Nakano <masayuki@d-toybox.com>
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 <masayuki@d-toybox.com>
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 <masayuki@d-toybox.com>
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 <masayuki@d-toybox.com>
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 <masayuki@d-toybox.com>
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 <masayuki@d-toybox.com>
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
Too late for 40.
Masayuki, are you planning to request uplift that to 41 or 42? Thanks
(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.
Flags: needinfo?(masayuki)
(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.
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.