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

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Core
Editor
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: yoann.coldefy, Assigned: masayuki)

Tracking

({inputmethod, regression})

Trunk
mozilla1.9.3a5
inputmethod, regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

8 years ago
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

8 years ago
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

8 years ago
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
Blocks: 488420
Status: UNCONFIRMED → NEW
Component: General → DOM: Events
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
QA Contact: general → events
Version: unspecified → Trunk

Comment 3

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

Comment 4

8 years ago
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
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.
Ah, okay, I can reproduce this bug. We need to know what happens at that time.

Comment 7

8 years ago
[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)
Created attachment 447478 [details] [diff] [review]
Patch v1.0
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Component: DOM: Events → Editor
OS: Windows 7 → All
QA Contact: events → editor
Hardware: x86_64 → All
Summary: The Japanese IME is not working properly on Gmail → IME composition string is committed unexpectedly on Gmail when editor flag was changed by some commands
Created attachment 447531 [details] [diff] [review]
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().
Attachment #447478 - Attachment is obsolete: true
Attachment #447531 - Flags: review?(Olli.Pettay)
Created attachment 447536 [details] [diff] [review]
Patch v2.0.1

sorry for the spam, this fixes wrong comment.
Attachment #447531 - Attachment is obsolete: true
Attachment #447536 - Flags: review?(Olli.Pettay)
Attachment #447531 - Flags: review?(Olli.Pettay)
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.
Attachment #447536 - Flags: review?(Olli.Pettay) → review-
Created attachment 447553 [details] [diff] [review]
Patch v3.0
Attachment #447536 - Attachment is obsolete: true
Attachment #447553 - Flags: review?(Olli.Pettay)
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.
Attachment #447553 - Flags: review?(Olli.Pettay) → review+
Created attachment 447678 [details] [diff] [review]
Patch v3.0.1

Thank you smaug.
http://hg.mozilla.org/mozilla-central/rev/f367856ca514

Thank you the reporter and Alice-san.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Flags: in-testsuite+
Keywords: inputmethod
You need to log in before you can comment on or make changes to this bug.