568135 - IME composition string is committed unexpectedly on Gmail when editor flag was changed by some commands

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[yoann.coldefy]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre) Gecko/20100525 Minefield/3.7a5pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre) Gecko/20100525 Minefield/3.7a5pre

When trying to write in japanese, the IME is not working properly :

a char often get 'out' of the current input and can't be included in the current word when switching to kanjis. When the first char is a consonant it often stays as a letter and don't wait the vowel to form a kana.

It seems to happen in  Gmail and not in this input form.

Reproducible: Always

Steps to Reproduce:
1.go to Gmail
2.start a new mail
3.write using the Japanese IME
Actual Results:  
the reported problem happened.

Expected Results:  
the IME should work properly

Comment 1

[Alice0775 White]

I can confirmed.

It only happens when I use Rich formatting editor.

ATOK2006+Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre) Gecko/20100525 Minefield/3.7a5pre ID:20100525062103

Comment 2

[Alice0775 White]

Regression window:

Works:
http://hg.mozilla.org/mozilla-central/rev/13bcf4386e12
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre) Gecko/20100504 Minefield/3.7a5pre ID:20100504113119

Fails:
http://hg.mozilla.org/mozilla-central/rev/f7a9b2f21b09
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre) Gecko/20100504 Minefield/3.7a5pre ID:20100504180829

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=13bcf4386e12&tochange=f7a9b2f21b09

Candidate regression bug:
Bug 488420  - IME enabled state is not modified when a focused editor's readonly attribute is changed

Comment 3

[Alice0775 White]

(In reply to comment #1)
Sorry, 
s/confirmed/confirm

Comment 4

[Alice0775 White]

In addition to the comment #1

And it happens with Microsoft IME + Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre)
Gecko/20100525 Minefield/3.7a5pre ID:20100525062103

Comment 5

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

It's strange... I cannot reproduce this bug on:

Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre) Gecko/20100525 Minefield/3.7a5pre

with MS-IME and ATOK2010.

Comment 6

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

Ah, okay, I can reproduce this bug. We need to know what happens at that time.

Comment 7

[Alice0775 White]

[STR]
0.Set up Microsoft IME as default, and ローマ字入力+ ひらがな in IME property.
1. Start with Minefield with new profile.
2.Logged in GMail.
3.Go "Compose Mail".
4.You can confirm a caret is in "To"pane now.
5.Confirm IME ON (if not. Activate IME).
6.Click "Body" pane.
7.Keyinput "bagu" without quotation.

[Actual]
bあぐ
(あぐ is composing range now)

[Expected]
ばぐ
(ばぐ is composing range now)

Comment 8

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

Attachment: Patch v1.0

Comment 9

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

Attachment: Patch v2.0

Some rich text editor commands change some flags of nsHTMLEditor, e.g., eEditorNoCSSMask. If the flag change doesn't cause actual IME state change, we shouldn't call nsIMEStateManager::ChangeIMEStateTo().

Comment 10

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

Attachment: Patch v2.0.1

sorry for the spam, this fixes wrong comment.

Comment 11

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

Comment on attachment 447536 [details] [diff] [review]
Patch v2.0.1

>diff --git a/content/events/src/nsIMEStateManager.cpp b/content/events/src/nsIMEStateManager.cpp
>--- a/content/events/src/nsIMEStateManager.cpp
>+++ b/content/events/src/nsIMEStateManager.cpp
>@@ -132,24 +132,18 @@ nsIMEStateManager::OnChangeFocus(nsPresC
>   if (aPresContext == sPresContext && aContent == sContent) {
>     // actual focus isn't changing, but if IME enabled state is changing,
>     // we should do it.
>     PRUint32 newEnabledState = newState & nsIContent::IME_STATUS_MASK_ENABLED;
>     if (newEnabledState == 0) {
>       // the enabled state isn't changing, we should do nothing.
>       return NS_OK;
>     }
>-    PRUint32 enabled;
>-    if (NS_FAILED(widget->GetIMEEnabled(&enabled))) {
>-      // this platform doesn't support IME controlling
>-      return NS_OK;
>-    }
>-    if (enabled ==
>-        nsContentUtils::GetWidgetStatusFromIMEStatus(newEnabledState)) {
>-      // the enabled state isn't changing.
>+    // If IME enabled state isn't changing, we shouldn't change open state.
>+    if (!IsDifferentIMEEnabledState(newState)) {
>       return NS_OK;
>     }
>   }
> 
>   // Current IME transaction should commit
>   if (sPresContext) {
>     nsCOMPtr<nsIWidget> oldWidget;
>     if (sPresContext == aPresContext)
>@@ -193,16 +187,35 @@ nsIMEStateManager::ChangeIMEStateTo(PRUi
>   }
> 
>   // commit current composition
>   widget->ResetInputState();
> 
>   SetIMEState(aNewIMEState, widget);
> }
> 
>+PRBool
>+nsIMEStateManager::IsDifferentIMEEnabledState(PRUint32 aIMEState)
>+{
>+  NS_ENSURE_TRUE(sPresContext, PR_TRUE);
>+  nsCOMPtr<nsIWidget> widget = GetWidget(sPresContext);
>+  NS_ENSURE_TRUE(widget, PR_TRUE);
>+
>+  aIMEState &= nsIContent::IME_STATUS_MASK_ENABLED;
>+
>+  PRUint32 currentWidgetEnabledState;
>+  nsresult rv = widget->GetIMEEnabled(&currentWidgetEnabledState);
>+  if (rv == NS_ERROR_NOT_IMPLEMENTED) {
>+    return NS_OK; // Don't output the warning.
>+  }
>+  NS_ENSURE_SUCCESS(rv, PR_TRUE);
>+  return currentWidgetEnabledState !=
>+    nsContentUtils::GetWidgetStatusFromIMEStatus(aIMEState);
>+}
>+
> PRUint32
> nsIMEStateManager::GetNewIMEState(nsPresContext* aPresContext,
>                                   nsIContent*    aContent)
> {
>   // On Printing or Print Preview, we don't need IME.
>   if (aPresContext->Type() == nsPresContext::eContext_PrintPreview ||
>       aPresContext->Type() == nsPresContext::eContext_Print) {
>     return nsIContent::IME_STATUS_DISABLE;
>diff --git a/content/events/src/nsIMEStateManager.h b/content/events/src/nsIMEStateManager.h
>--- a/content/events/src/nsIMEStateManager.h
>+++ b/content/events/src/nsIMEStateManager.h
>@@ -77,17 +77,28 @@ public:
>   // aContent is the nsIContent receiving focus
>   static nsresult OnTextStateFocus(nsPresContext* aPresContext,
>                                    nsIContent* aContent);
>   // Get the focused editor's selection and root
>   static nsresult GetFocusSelectionAndRoot(nsISelection** aSel,
>                                            nsIContent** aRoot);
>   // This method changes the current IME state forcedly.
>   // So, the caller should check whether you're focused or not.
>+  // And also the caller should check whether the change is actually needed,
>+  // e.g., even when the new state is same as current state, this method
>+  // resets the current composition.
>+  // aNewIMEState must have an enabled state of nsIContent::IME_STATUS_*.
>+  // And optionally, it can have an open state of nsIContent::IME_STATUS_*.
>   static void ChangeIMEStateTo(PRUint32 aNewIMEState);
>+
>+  // Check whether aIMEState's enabled state is different with current state.
Checks whether aIMEState's state is different than the current state.

(or something like that.)

>+  // aIMEState must include an enabled state of nsIContent::IME_STATUS_*.
>+  // The open state of nsIContent::IME_STATUS_* is ignored.
>+  static PRBool IsDifferentIMEEnabledState(PRUint32 aIMEState);
>+
But actually, could you merge ChangeIMEStateTo and IsDifferentIMEEnabledState
and call it perhaps
UpdateIMEState(PRUint32 aNewIMEState) or MaybeChangeIMEState(PRUint32 aNewIMEState)
and document that in which cases the state is updated.

Comment 12

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

Attachment: Patch v3.0

Comment 13

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

Comment on attachment 447553 [details] [diff] [review]
Patch v3.0


>+  // Don't update IME state when enabled state isn't changed actually.
...isn't actually changed.
or
... has not been changed.

Comment 14

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

Attachment: Patch v3.0.1

Thank you smaug.

Comment 15

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

http://hg.mozilla.org/mozilla-central/rev/f367856ca514

Thank you the reporter and Alice-san.