For fixing bug 645974, we need to dispatch compositionupdate, compositionend and internal text event to compositionstart handled editor rather than focused one. For this, we need to manage composition as a transaction between a native IME context and an our editor. And also, we cannot forcibly commit composition string on Linux but our XP code assumes that we can do it on all platforms. On such platform, we should move active composition from blurred editor to focused editor. Besides, I want to remove our text event at least from widget. If editor can get some information for IME selection from a class directly, we don't need to use non-standard composition events like text event. For solving such issues, I suggest implementing "composition transaction manager". I think that it should be called mozilla::widget::CompositionTransaction. And it is defined in widget/public/CompositionTransaction.h. And also it's implemented in widget/src/xpwidgets/CompositionTransaction.cpp. The class should be refcounted and held by nsIWidget instance and nsIEditor instance. PresShell should target compositionupdate, compositionend and text events to the composing editor element.

Hmm, I forgot e10s (it still exists for mobile)... A native IME context can be shared by content and chrome. The fact blocks my plan. I need to research more.

This fixes only bug 645974. This stores event target (and its presContext) of compositionstart event and compositionupdate, compositionend and text events are dispatched to the stored target by the CompositionEventManager if the PresShell is going to dispatch the events to different target. I believe that these classes will help the blocked bugs by this. Next, we should move the rangeArray of nsTextEvent to compositionupdate event, remove nsTextEvent from widget code and make CompositionEventManager dispatch empty nsTextEvent when compositionupdate event is received in bug 700897.

Comment on attachment 662851 [details] [diff] [review] Implement CompositionEventManager and ensure a set of composition events must be fired on same element I think this patch should be reviewed by roc too.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #1) > Hmm, I forgot e10s (it still exists for mobile)... Yeah, exists very much for B2G

(In reply to Olli Pettay [:smaug] from comment #4) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #1) > > Hmm, I forgot e10s (it still exists for mobile)... > Yeah, exists very much for B2G The patch doesn't cross the process boundary. It works only in the process which includes the event target.

Comment on attachment 662851 [details] [diff] [review] Implement CompositionEventManager and ensure a set of composition events must be fired on same element >+void >+CompositionEventManager::OnRemoveContent(nsIContent* aContent) >+{ >+ if (!sCompositions) { >+ return; >+ } >+ >+ for (uint32_t i = sCompositions->Length(); i > 0; --i) { >+ TextComposition& composition = sCompositions->ElementAt(i - 1); >+ if (composition.GetContent() == aContent) { >+ // XXX Do we need to dispatch compositionend event? I think so >+ sCompositions->RemoveElementAt(i - 1); >+ break; // there shouldn't be two or more compositions on same content Maybe: // There should be only one composition per content object. >+ TextComposition* composition = GetCompositionFor(GUIEvent->widget); >+ if (!composition) { >+ NS_ASSERTION(GUIEvent->message == NS_COMPOSITION_START, >+ "There is no composition, but received composing event"); composition event. Perhaps use MOZ_ASSERT >+ TextComposition(const TextComposition &aOther) const TextComposition& aOther >+ { >+ mNativeContext = aOther.mNativeContext; >+ mPresContext = aOther.mPresContext; >+ mContent = aOther.mContent; >+ } >+ >+ nsPresContext* GetPresContext() const { return mPresContext; } >+ nsIContent* GetContent() const { return mContent; } >+ >+ bool MatchesNativeContext(nsIWidget* aWidget) const; >+ bool MatchesContent(nsPresContext* aPresContext, nsIContent* aContent); >+ >+private: >+ nsPresContext* mPresContext; Raw pointers are scary. Is it guaranteed that mPresContext never points to a deleted object? If so, add a comment here why. >+ // mNativeContext stores a opaque pointer. This works as the "ID" for this >+ // composition. Don't access the instance, it may not be available. >+ void* mNativeContext; >+ // Array should be enough. In most cases, only one composition exists at the >+ // same time. However, if two or more native IME context is available on a >+ // process, there may be two or more compositions. >+ static nsTArray<TextComposition>* sCompositions; Perhaps nsAutoTArray<TextComposition, 2> Any chance for tests which involve several active compositions? I'd like to see a new patch which has the compositionend case fixed.

(In reply to Olli Pettay [:smaug] from comment #6) > >+ TextComposition* composition = GetCompositionFor(GUIEvent->widget); > >+ if (!composition) { > >+ NS_ASSERTION(GUIEvent->message == NS_COMPOSITION_START, > >+ "There is no composition, but received composing event"); > composition event. > Perhaps use MOZ_ASSERT MOZ_ASSERT is too dangerous for this case. If widget/ has bugs for this, they cause crash. Actually, we have had such bugs because some IME behaves strange and we might lose the chance to dispatch. I'm planning to dispatch compositionstart event automatically in such cases. Then, we can make widget code simpler. > >+ // mNativeContext stores a opaque pointer. This works as the "ID" for this > >+ // composition. Don't access the instance, it may not be available. > >+ void* mNativeContext; > >+ // Array should be enough. In most cases, only one composition exists at the > >+ // same time. However, if two or more native IME context is available on a > >+ // process, there may be two or more compositions. > >+ static nsTArray<TextComposition>* sCompositions; > Perhaps nsAutoTArray<TextComposition, 2> I think that we must not use nsAuto* out of stack, isn't it? # When I defined it as non-pointer, the internal allocated members are detected as leak when debug build is closed. > Any chance for tests which involve several active compositions? If you meant that we test it by automated tests, unfortunately no. I tried creating automated tests for bug 645974, but then, I realized that nsIWidget::ResetInputState() doesn't work for synthesized composition. I'm planing to add TextComposition::NotifyIME() which wraps notify/request to IME. Then, we can emulate the API behavior for synthesized composition too. However, it needs more work, therefore, this patch doesn't include the tests.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #7) > I think that we must not use nsAuto* out of stack, isn't it? nsAuto* can be used on heap too when you want to avoid extra allocations and deallocations. > # When I defined it as non-pointer, the internal allocated members are > detected as leak when debug build is closed. Oh, I meant nsAutoTArray<TextComposition, 2>*, not nsAutoTArray<TextComposition, 2>

Comment on attachment 662851 [details] [diff] [review] Implement CompositionEventManager and ensure a set of composition events must be fired on same element Hmm, I'm trying to add the methods to dispatch composition events. However, it becomes too big and it looks too risky without automated tests. So, I'll separate the patches and I'll add new APIs for testing.

This patch doesn't include dispatching the events to removed content.

And the patch uses sContentUtils::ContentIsDescendantOf() in OnRemoveContent(). I think that this is a bug of previous patch.

This patch dispatches the compositionend event (and necessary other events) from OnRemoveContent().

This patch wraps the IME APIs of nsIWidget for making automated tests. See following patches.

ResetInputState() and CancelIMEComposition() do just notify native IME. Therefore, if the IME doesn't have composition actually, they don't cause dispatching compositionend events. This patch emulates these APIs behavior if the composition is made by synthesized event.

This patch adds automated tests for this bug and bug 645974.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #10) > Created attachment 665304 [details] [diff] [review] > part.1 Ensure a set of composition events is fired on same content > > This patch doesn't include dispatching the events to removed content. (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #11) > And the patch uses sContentUtils::ContentIsDescendantOf() in > OnRemoveContent(). I think that this is a bug of previous patch. Additionally, CompositionEventManager::PreHandleEvent() and PostHandleEvent() are removed. Instead of them, Dispatch() is added. Now, the CompositionEventManager always dispatches compositionstart, compositionupdate, compositionend and text events. The reason of this change is that setting NS_EVENT_FLAG_STOP_DISPATCH doesn't work properly. Therefore, this patch doesn't pass the events to normal path. However, this makes the code simpler. Anyway, I'd like you to review all of the part.1 again since I fixed a lot of bugs of previous patch which are detected by the new automated tests.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #7) > MOZ_ASSERT is too dangerous for this case. If widget/ has bugs for this, > they cause crash. But it is debug only.

What do you mean with NS_EVENT_FLAG_STOP_DISPATCH not working properly? If that flag is set, the listeners in the current event group aren't called. (the flag is unset before calling listeners in system event group.)

Comment on attachment 665304 [details] [diff] [review] part.1 Ensure a set of composition events is fired on same content >+ // There should be only one composition per content object. >+ TextComposition* foundComposition = nullptr; >+ for (CompositionArray::index_type i = sCompositions->Length(); i > 0; --i) { >+ nsINode* node = sCompositions->ElementAt(i -1).GetEventTargetNode(); missing space before 1 >+/** >+ * TextComposition represents a text composition. This class stores the >+ * composition event target and its presContext. Dispatching the event, >+ * the instances uses the stroed event target. stored > if (NS_IsAllowedToDispatchDOMEvent(aEvent)) { > nsPresShellEventCB eventCB(this); > if (aEvent->eventStructType == NS_TOUCH_EVENT) { > DispatchTouchEvent(aEvent, aStatus, &eventCB, touchIsNew); >- } >- else if (mCurrentEventContent) { >- nsEventDispatcher::Dispatch(mCurrentEventContent, mPresContext, >- aEvent, nullptr, aStatus, &eventCB); >- } >- else { >- nsCOMPtr<nsIContent> targetContent; >- if (mCurrentEventFrame) { >- rv = mCurrentEventFrame->GetContentForEvent(aEvent, >- getter_AddRefs(targetContent)); >+ } else { >+ nsCOMPtr<nsINode> eventTarget = >+ do_QueryInterface(mCurrentEventContent); This QI isn't needed. nsIContent inherits nINode. >+ nsPresShellEventCB* eventCBPtr = &eventCB; >+ if (!eventTarget) { >+ nsCOMPtr<nsIContent> targetContent; >+ if (mCurrentEventFrame) { >+ rv = mCurrentEventFrame-> >+ GetContentForEvent(aEvent, getter_AddRefs(targetContent)); >+ } >+ if (NS_SUCCEEDED(rv) && targetContent) { >+ eventTarget = do_QueryInterface(targetContent); >+ } else if (mDocument) { >+ eventTarget = do_QueryInterface(mDocument); >+ eventCBPtr = nullptr; // XXX why? About XXX, I think the idea has been that since if we don't have any content, the callback wouldn't probably do anything.

Comment on attachment 665307 [details] [diff] [review] part.2 Dispatch composition events on removed content for canceling the composition if widget's IME APIs don't work > TextComposition::DispatchEvent(nsGUIEvent* aEvent, > nsEventStatus* aStatus, > nsDispatchingCallback* aCallBack) > { >+ if (aEvent->message == NS_COMPOSITION_UPDATE) { >+ mLastData = static_cast<nsCompositionEvent*>(aEvent)->data; >+ } Do other composition events contain data? If not, could we just always assign mLastData to static_cast<nsCompositionEvent*>(aEvent)->data; >+TextComposition::EmulateToCommit(bool aDiscard) >+{ >+ // backup this instance and use it since this instance might be destroyed >+ // by CompositionEventManager if this is maraged by it. managed >+ /** >+ * EmulateToCommit() dispatches compositionupdate, text and compositionend >+ * events for emulating to commit on the content. >+ * >+ * @param aDiscard true when committing with empty string. Otherwise, false. >+ */ >+ void EmulateToCommit(bool aDiscard); Odd method name. Would SynthesizeCommit() work? >+ /** >+ * DispatchCompsotionEventRunnable() dispatches a composition or text event >+ * to the content. Beaware, if you use this method, nsPresShellEventCB isn't Be aware

Comment on attachment 665311 [details] [diff] [review] part.3 XP level code shouldn't call nsIWidget::ResetInputState() and nsIWidget::CancelIMEComposition() directly > ># HG changeset patch ># User Masayuki Nakano <masayuki@d-toybox.com> ># Date 1348638471 -32400 ># Node ID 39f51b9bab8ebadd3beeb59485e4e225b62e7d08 ># Parent d4a5929e24e655d9a35490e4d30f03b8645bf46b >Bug 705057 part.3 XP level code shouldn't call nsIWidget::ResetInputState() and nsIWidget::CancelIMEComposition() directly > >diff --git a/content/events/src/CompositionEventManager.cpp b/content/events/src/CompositionEventManager.cpp >--- a/content/events/src/CompositionEventManager.cpp >+++ b/content/events/src/CompositionEventManager.cpp >@@ -152,19 +152,19 @@ CompositionEventManager::OnRemoveContent > TextComposition composition = *foundComposition; > > // Try resetting the native IME state. Be aware, typically, this method is > // called during the content being removed. Then, the native composition > // events which are caused by following APIs are ignored due to unsafe to > // run script (in PresShell::HandleEvent()). > nsCOMPtr<nsIWidget> widget = aPresContext->GetNearestWidget(); > if (widget) { >- nsresult rv = widget->CancelIMEComposition(); >+ nsresult rv = composition.NotifyIME(nsIMEStateManager::REQUEST_CANCEL); > if (NS_FAILED(rv)) { >- widget->ResetInputState(); >+ composition.NotifyIME(nsIMEStateManager::REQUEST_COMMIT); > } > // By calling the APIs, the composition may have been finished normally. > foundComposition = GetCompositionFor(composition.GetPresContext(), > composition.GetEventTargetNode()); > if (!foundComposition) { > // Okay, the composition has gone. > return; > } >@@ -280,16 +280,23 @@ TextComposition::EmulateToCommit(bool aD > nsAutoString data(aDiscard ? EmptyString() : composition.mLastData); > if (composition.mLastData != data) { > composition.DispatchCompsotionEventRunnable(NS_COMPOSITION_UPDATE, data); > composition.DispatchCompsotionEventRunnable(NS_TEXT_TEXT, data); > } > composition.DispatchCompsotionEventRunnable(NS_COMPOSITION_END, data); > } > >+nsresult >+TextComposition::NotifyIME(nsIMEStateManager::Notification aNotification) >+{ >+ NS_ENSURE_TRUE(mPresContext, NS_ERROR_NOT_AVAILABLE); >+ return nsIMEStateManager::NotifyIME(aNotification, mPresContext); >+} >+ > /****************************************************************************** > * TextComposition::CompositionEventDispatcher > ******************************************************************************/ > > NS_IMETHODIMP > TextComposition::CompositionEventDispatcher::Run() > { > if (!mPresContext->GetPresShell() || >diff --git a/content/events/src/CompositionEventManager.h b/content/events/src/CompositionEventManager.h >--- a/content/events/src/CompositionEventManager.h >+++ b/content/events/src/CompositionEventManager.h >@@ -3,16 +3,17 @@ > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > #ifndef mozilla_CompositionEventManager_h > #define mozilla_CompositionEventManager_h > > #include "nsCOMPtr.h" >+#include "nsIMEStateManager.h" > #include "nsIPresShell.h" > #include "nsIWidget.h" > #include "nsINode.h" > #include "nsPresContext.h" > #include "nsString.h" > #include "nsTArray.h" > #include "nsThreadUtils.h" > #include "mozilla/Attributes.h" >@@ -65,16 +66,19 @@ public: > /** > * EmulateToCommit() dispatches compositionupdate, text and compositionend > * events for emulating to commit on the content. > * > * @param aDiscard true when committing with empty string. Otherwise, false. > */ > void EmulateToCommit(bool aDiscard); > >+ // Calls nsIMEStateManager::NotifyIME() for this composition. >+ nsresult NotifyIME(nsIMEStateManager::Notification aNotification); >+ > private: > // This class holds nsPresContext weak. This instance shouldn't block > // destroying it. When the presContext is being destroyed, it's notified to > // CompositionEventManager::OnDestroyPresContext(), and then, it destroy > // this instance. > nsPresContext* mPresContext; > nsCOMPtr<nsINode> mNode; > >@@ -162,32 +166,39 @@ public: > nsIContent* aContent); > > static void Dispatch(nsINode* aNode, > nsPresContext* aPresContext, > nsEvent* aEvent, > nsEventStatus* aStatus, > nsDispatchingCallback* aCallBack); > >+ /** >+ * GetCompositionFor() returns the TextComposition instance which is active >+ * composition in the widget, presContext and/or the node. >+ * Note that the returned instance is managed by CompositionEventManager. >+ * So, you don't need to release it yourself, but be aware, the instance >+ * may be broken if you dispatch a DOM event. >+ */ >+ static TextComposition* GetCompositionFor(nsIWidget* aWidget); >+ static TextComposition* GetCompositionFor(nsPresContext* aPresContext); >+ static TextComposition* GetCompositionFor(nsPresContext* aPresContext, >+ nsINode* aNode); >+ > private: > static void Init(); > > typedef nsAutoTArray<TextComposition, 2> CompositionArray; > > // Helper methods for accessing sCompositions. > static CompositionArray::index_type IndexOf(nsIWidget* aWidget); > static CompositionArray::index_type IndexOf(nsPresContext* aPresContext); > static CompositionArray::index_type IndexOf(nsPresContext* aPresContext, > nsINode* aNode); > >- static TextComposition* GetCompositionFor(nsIWidget* aWidget); >- static TextComposition* GetCompositionFor(nsPresContext* aPresContext); >- static TextComposition* GetCompositionFor(nsPresContext* aPresContext, >- nsINode* aNode); >- > // Array should be enough. In most cases, only one composition exists at the > // same time. However, if two or more native IME context is available on a > // process, there may be two or more compositions. > static CompositionArray* sCompositions; > }; > > } // namespace mozilla > >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 >@@ -158,18 +158,19 @@ nsIMEStateManager::OnChangeFocusInternal > > // Current IME transaction should commit > if (sPresContext) { > nsCOMPtr<nsIWidget> oldWidget; > if (sPresContext == aPresContext) > oldWidget = widget; > else > oldWidget = GetWidget(sPresContext); >- if (oldWidget) >- oldWidget->ResetInputState(); >+ if (oldWidget) { >+ NotifyIME(REQUEST_COMMIT, oldWidget); >+ } > } > > // Update IME state for new focus widget > SetIMEState(newState, aContent, widget, aAction); > > sPresContext = aPresContext; > if (sContent != aContent) { > NS_IF_RELEASE(sContent); >@@ -245,17 +246,17 @@ nsIMEStateManager::UpdateIMEState(const > > // Don't update IME state when enabled state isn't actually changed. > InputContext context = widget->GetInputContext(); > if (context.mIMEState.mEnabled == aNewIMEState.mEnabled) { > return; > } > > // commit current composition >- widget->ResetInputState(); >+ NotifyIME(REQUEST_COMMIT, widget); > > InputContextAction action(InputContextAction::CAUSE_UNKNOWN, > InputContextAction::FOCUS_NOT_CHANGED); > SetIMEState(aNewIMEState, aContent, widget, action); > } > > IMEState > nsIMEStateManager::GetNewIMEState(nsPresContext* aPresContext, >@@ -378,16 +379,52 @@ nsIMEStateManager::GetWidget(nsPresConte > if (!vm) > return nullptr; > nsCOMPtr<nsIWidget> widget = nullptr; > nsresult rv = vm->GetRootWidget(getter_AddRefs(widget)); > NS_ENSURE_SUCCESS(rv, nullptr); > return widget; > } > >+// static >+nsresult >+nsIMEStateManager::NotifyIME(Notification aNotification, >+ nsIWidget* aWidget) >+{ >+ NS_ENSURE_TRUE(aWidget, NS_ERROR_INVALID_ARG); >+ >+ TextComposition* composition = >+ CompositionEventManager::GetCompositionFor(aWidget); >+ switch (aNotification) { >+ case NOTIFY_CURSOR_POS_CHANGED: >+ return aWidget->ResetInputState(); >+ case REQUEST_COMMIT: >+ return composition ? aWidget->ResetInputState() : NS_OK; >+ case REQUEST_CANCEL: >+ return composition ? aWidget->CancelIMEComposition() : NS_OK; >+ default: >+ MOZ_NOT_REACHED("Unsupported notification"); >+ return NS_ERROR_INVALID_ARG; >+ } >+} >+ >+// static >+nsresult >+nsIMEStateManager::NotifyIME(Notification aNotification, >+ nsPresContext* aPresContext) >+{ >+ NS_ENSURE_TRUE(aPresContext, NS_ERROR_INVALID_ARG); >+ >+ nsIWidget* widget = aPresContext->GetNearestWidget(); >+ if (!widget) { >+ return NS_ERROR_NOT_AVAILABLE; >+ } >+ return NotifyIME(aNotification, widget); >+} >+ > > // nsTextStateManager notifies widget of any text and selection changes > // in the currently focused editor > // sTextStateObserver points to the currently active nsTextStateManager > // sTextStateObserver is null if there is no focused editor > > class nsTextStateManager MOZ_FINAL : public nsISelectionListener, > public nsStubMutationObserver >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 >@@ -70,16 +70,28 @@ public: > // aContent must be: > // If the editor is for <input> or <textarea>, the element. > // If the editor is for contenteditable, the active editinghost. > // If the editor is for designMode, NULL. > static void OnClickInEditor(nsPresContext* aPresContext, > nsIContent* aContent, > nsIDOMMouseEvent* aMouseEvent); > >+ // Send a notification to IME. It depends on the IME or platform spec what >+ // will occur (or not occur). >+ enum Notification >+ { >+ NOTIFY_CURSOR_POS_CHANGED, >+ REQUEST_COMMIT, >+ REQUEST_CANCEL >+ }; >+ static nsresult NotifyIME(Notification aNotification, nsIWidget* aWidget); >+ static nsresult NotifyIME(Notification aNotification, >+ nsPresContext* aPresContext); >+ > protected: > static nsresult OnChangeFocusInternal(nsPresContext* aPresContext, > nsIContent* aContent, > InputContextAction aAction); > static void SetIMEState(const IMEState &aState, > nsIContent* aContent, > nsIWidget* aWidget, > InputContextAction aAction); >diff --git a/editor/libeditor/base/nsEditor.cpp b/editor/libeditor/base/nsEditor.cpp >--- a/editor/libeditor/base/nsEditor.cpp >+++ b/editor/libeditor/base/nsEditor.cpp >@@ -2026,83 +2027,42 @@ nsEditor::GetPhonetic(nsAString& aPhonet > if (mPhonetic) > aPhonetic = *mPhonetic; > else > aPhonetic.Truncate(0); > > return NS_OK; > } > >- >-static nsresult >-GetEditorContentWindow(dom::Element *aRoot, nsIWidget **aResult) >-{ >- NS_ENSURE_TRUE(aRoot && aResult, NS_ERROR_NULL_POINTER); >- >- *aResult = 0; >- >- // Not ref counted >- nsIFrame *frame = aRoot->GetPrimaryFrame(); >- >- NS_ENSURE_TRUE(frame, NS_ERROR_FAILURE); >- >- *aResult = frame->GetNearestWidget(); >- NS_ENSURE_TRUE(*aResult, NS_ERROR_FAILURE); >- >- NS_ADDREF(*aResult); >- return NS_OK; >-} >- >-nsresult >-nsEditor::GetWidget(nsIWidget **aWidget) >-{ >- NS_ENSURE_TRUE(aWidget, NS_ERROR_NULL_POINTER); >- *aWidget = nullptr; >- >- nsCOMPtr<nsIWidget> widget; >- nsresult res = GetEditorContentWindow(GetRoot(), getter_AddRefs(widget)); >- NS_ENSURE_SUCCESS(res, res); >- NS_ENSURE_TRUE(widget, NS_ERROR_NOT_AVAILABLE); >- >- NS_ADDREF(*aWidget = widget); >- >- return NS_OK; >-} >- > NS_IMETHODIMP > nsEditor::ForceCompositionEnd() > { >- >-// We can test mInIMEMode and do some optimization for Mac and Window >-// Howerver, since UNIX support over-the-spot, we cannot rely on that >-// flag for Unix. >-// We should use LookAndFeel to resolve this >- >-#if defined(XP_MACOSX) || defined(XP_WIN) || defined(XP_OS2) >- // XXXmnakano see bug 558976, ResetInputState() has two meaning which are >- // "commit the composition" and "cursor is moved". This method name is >- // "ForceCompositionEnd", so, ResetInputState() should be used only for the >- // former here. However, ResetInputState() is also used for the latter here >- // because even if we don't have composition, we call ResetInputState() on >- // Linux. Currently, nsGtkIMModule can know the timing of the cursor move, >- // so, the latter meaning should be gone and we should remove this #if. >- if(! mInIMEMode) >- return NS_OK; >-#endif >- >- nsCOMPtr<nsIWidget> widget; >- nsresult res = GetWidget(getter_AddRefs(widget)); >- NS_ENSURE_SUCCESS(res, res); >- >- if (widget) { >- res = widget->ResetInputState(); >- NS_ENSURE_SUCCESS(res, res); >+ nsCOMPtr<nsIPresShell> ps = GetPresShell(); >+ if (!ps) { >+ return NS_ERROR_NOT_AVAILABLE; > } >- >- return NS_OK; >+ nsPresContext* pc = ps->GetPresContext(); >+ if (!pc) { >+ return NS_ERROR_NOT_AVAILABLE; >+ } >+ >+ if (!mInIMEMode) { >+ // XXXmnakano see bug 558976, ResetInputState() has two meaning which are >+ // "commit the composition" and "cursor is moved". This method name is >+ // "ForceCompositionEnd", so, ResetInputState() should be used only for the >+ // former here. However, ResetInputState() is also used for the latter here >+ // because even if we don't have composition, we call ResetInputState() on >+ // Linux. Currently, nsGtkIMModule can know the timing of the cursor move, >+ // so, the latter meaning should be gone. >+ // XXX This may commit a composition in another editor. >+ return nsIMEStateManager::NotifyIME( >+ nsIMEStateManager::NOTIFY_CURSOR_POS_CHANGED, pc); >+ } >+ >+ return nsIMEStateManager::NotifyIME(nsIMEStateManager::REQUEST_COMMIT, pc); > } > > NS_IMETHODIMP > nsEditor::GetPreferredIMEState(IMEState *aState) > { > NS_ENSURE_ARG_POINTER(aState); > aState->mEnabled = IMEState::ENABLED; > aState->mOpen = IMEState::DONT_CHANGE_OPEN_STATE; >diff --git a/editor/libeditor/base/nsEditor.h b/editor/libeditor/base/nsEditor.h >--- a/editor/libeditor/base/nsEditor.h >+++ b/editor/libeditor/base/nsEditor.h >@@ -383,20 +383,16 @@ protected: > // stub. see comment in source. > virtual bool IsBlockNode(nsINode* aNode); > > // helper for GetPriorNode and GetNextNode > nsIContent* FindNextLeafNode(nsINode *aCurrentNode, > bool aGoForward, > bool bNoBlockCrossing); > >- // Get nsIWidget interface >- nsresult GetWidget(nsIWidget **aWidget); >- >- > // install the event listeners for the editor > virtual nsresult InstallEventListeners(); > > virtual void CreateEventListeners(); > > // unregister and release our event listeners > virtual void RemoveEventListeners(); > >

Argh, sorry I wasn't going to post the whole patch as a comment. I wonder if it would make sense to merge IMEStateManager and CompositionEventManager at some point.

Comment on attachment 665314 [details] [diff] [review] part.4 Emulate the behavior of nsIWidget::ResetInputState() and nsIWidget::CancelIMEComposition() if the composition is synthesized >+ // See the comment for IsSynthesizedForTests(). >+ bool mIsSynthesized; Maybe mIsSynthesizedForTests > TextComposition* composition = > CompositionEventManager::GetCompositionFor(aWidget); >+ if (!composition || !composition->IsSynthesizedForTests()) { >+ switch (aNotification) { >+ case NOTIFY_CURSOR_POS_CHANGED: >+ return aWidget->ResetInputState(); >+ case REQUEST_COMMIT: >+ return composition ? aWidget->ResetInputState() : NS_OK; >+ case REQUEST_CANCEL: >+ return composition ? aWidget->CancelIMEComposition() : NS_OK; How can composition be null here? >+ /** >+ * Destroyed() returns true if Destroy() has been called alreay. Otherwise, >+ * false. already

(In reply to Olli Pettay [:smaug] from comment #17) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #7) > > MOZ_ASSERT is too dangerous for this case. If widget/ has bugs for this, > > they cause crash. > > But it is debug only. MOZ_ASSERT() made Nightly build crashed: https://bugzilla.mozilla.org/show_bug.cgi?id=751929 Now, doesn't it cause it?? (In reply to Olli Pettay [:smaug] from comment #18) > What do you mean with NS_EVENT_FLAG_STOP_DISPATCH not working properly? > If that flag is set, the listeners in the current event group aren't called. > (the flag is unset before calling listeners in system event group.) Ah, the system event group event may have broken the behavior because I assumed it doesn't happen. (In reply to Olli Pettay [:smaug] from comment #20) > Comment on attachment 665307 [details] [diff] [review] > part.2 Dispatch composition events on removed content for canceling the > composition if widget's IME APIs don't work > > > TextComposition::DispatchEvent(nsGUIEvent* aEvent, > > nsEventStatus* aStatus, > > nsDispatchingCallback* aCallBack) > > { > >+ if (aEvent->message == NS_COMPOSITION_UPDATE) { > >+ mLastData = static_cast<nsCompositionEvent*>(aEvent)->data; > >+ } > Do other composition events contain data? If not, could we just always assign > mLastData to static_cast<nsCompositionEvent*>(aEvent)->data; mLastData is used for checking if compositionupdate event is necessary before dispatching compositionend event. So, it needs only compositionupdate's data value. And compositionstart's data value has different meaning. That is the selected text. So, we shouldn't store it for the purpose. (In reply to Olli Pettay [:smaug] from comment #22) > I wonder if it would make sense to merge IMEStateManager and > CompositionEventManager > at some point. Yeah, CompositionEventManager can be a part of nsIMEStateManager... (In reply to Olli Pettay [:smaug] from comment #23) > > TextComposition* composition = > > CompositionEventManager::GetCompositionFor(aWidget); > >+ if (!composition || !composition->IsSynthesizedForTests()) { > >+ switch (aNotification) { > >+ case NOTIFY_CURSOR_POS_CHANGED: > >+ return aWidget->ResetInputState(); > >+ case REQUEST_COMMIT: > >+ return composition ? aWidget->ResetInputState() : NS_OK; > >+ case REQUEST_CANCEL: > >+ return composition ? aWidget->CancelIMEComposition() : NS_OK; > How can composition be null here? If it's called when there is no composition.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #24) > MOZ_ASSERT() made Nightly build crashed: > https://bugzilla.mozilla.org/show_bug.cgi?id=751929 > > Now, doesn't it cause it?? That bug was about MOZ_NOT_REACHED, not MOZ_ASSERT > (In reply to Olli Pettay [:smaug] from comment #23) > > > TextComposition* composition = > > > CompositionEventManager::GetCompositionFor(aWidget); > > >+ if (!composition || !composition->IsSynthesizedForTests()) { > > >+ switch (aNotification) { > > >+ case NOTIFY_CURSOR_POS_CHANGED: > > >+ return aWidget->ResetInputState(); > > >+ case REQUEST_COMMIT: > > >+ return composition ? aWidget->ResetInputState() : NS_OK; > > >+ case REQUEST_CANCEL: > > >+ return composition ? aWidget->CancelIMEComposition() : NS_OK; > > How can composition be null here? > > If it's called when there is no composition. Oops, silly me. I read the if() wrong :)

(In reply to Olli Pettay [:smaug] from comment #25) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #24) > > MOZ_ASSERT() made Nightly build crashed: > > https://bugzilla.mozilla.org/show_bug.cgi?id=751929 > > > > Now, doesn't it cause it?? > > That bug was about MOZ_NOT_REACHED, not MOZ_ASSERT Ah, right!

(In reply to Olli Pettay [:smaug] from comment #19) > Comment on attachment 665304 [details] [diff] [review] > part.1 Ensure a set of composition events is fired on same content > > if (NS_IsAllowedToDispatchDOMEvent(aEvent)) { > > nsPresShellEventCB eventCB(this); > > if (aEvent->eventStructType == NS_TOUCH_EVENT) { > > DispatchTouchEvent(aEvent, aStatus, &eventCB, touchIsNew); > >- } > >- else if (mCurrentEventContent) { > >- nsEventDispatcher::Dispatch(mCurrentEventContent, mPresContext, > >- aEvent, nullptr, aStatus, &eventCB); > >- } > >- else { > >- nsCOMPtr<nsIContent> targetContent; > >- if (mCurrentEventFrame) { > >- rv = mCurrentEventFrame->GetContentForEvent(aEvent, > >- getter_AddRefs(targetContent)); > >+ } else { > >+ nsCOMPtr<nsINode> eventTarget = > >+ do_QueryInterface(mCurrentEventContent); > This QI isn't needed. nsIContent inherits nINode. That causes bustage with gcc... https://tbpl.mozilla.org/php/getParsedLog.php?id=16011893&tree=Try

Oh, hmm, perhaps nsCOMPtr<nsINode> eventTarget = mCurrentEventContent.get(); would work then.

(In reply to Olli Pettay [:smaug] from comment #25) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #24) > > MOZ_ASSERT() made Nightly build crashed: > > https://bugzilla.mozilla.org/show_bug.cgi?id=751929 > > > > Now, doesn't it cause it?? > > That bug was about MOZ_NOT_REACHED, not MOZ_ASSERT Hmm, this causes new failure in test_sanityEventUtils.html. http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/test_sanityEventUtils.html?force=1#134 I don't understand what's tested by the test in this case...

Comment on attachment 665317 [details] [diff] [review] part.5 Add automated tests for CompositionEventManager >+function runForceCommitTest() >+{ >+ var events = []; >+ function eventHandler(aEvent) >+ { >+ events.push(aEvent); >+ } >+ window.addEventListener("compositionstart", eventHandler, true); >+ window.addEventListener("compositionupdate", eventHandler, true); >+ window.addEventListener("compositionend", eventHandler, true); >+ window.addEventListener("input", eventHandler, true); >+ window.addEventListener("text", eventHandler, true); >+ >+ // Make the composition in textarea commit by click in the textarea >+ textarea.focus(); >+ textarea.value = ""; >+ >+ events = []; I wonder if this is needed. You initialize the variable already before. >+ hitEventLoop(function () { >+ // XXX Currently, "input" event isn't fired on removed content. File a bug about that. Need to check what other browsers do. Run this few times in try (all platforms) before landing.

mconley: You added a reftest in bug 749663: http://mxr.mozilla.org/mozilla-central/source/layout/base/tests/bug746993-1.html?force=1 However, it doesn't make sense for me. Why did you just set the "Here's some text." to <textarea>.value and move the caret with <textarea>.selectionStart? Your text event usage is definitely wrong. That causes a MOZ_ASSERT crash with the new patches for this bug. Can I change the test like I mentioned?

> Why did you I meant "Why didn't you".

Oops, the test is for contenteditable, not <textarea>. But I still don't understand why it needs to use IME events.

Redesigned for comment 22. CompositionEventManager has gone. Now, CompositionEventManager.(h|cpp) are TextComposition.(h|cpp). They define and implement TextComposition class and TextCompositionArray class which is derived from nsAutoTArray<TextComposition, 2>. TextCompositionArray's instance is managed by nsIMEStateManager. CompositionEventManager::Dispatch() is moved to nsIMEStateManager::DispatchCompositionEvent(). And nsIMEStateManager::Shutdown() is now called from nsLayoutStatics. I realized that nsEventStateManager::Shutdown() is a part of destructor. I'll file a bug for renaming it. Shutdown() shouldn't be used for such method in our code. And now, some NS_ASSERTION() becomes MOZ_ASSERT(). By this change, bug746993-1.html and test_sanityEventUtils.html cause crash. test_sanityEventUtils.html is testing if the synthesizeText() doesn't dispatch composition events. This doesn't make sense. Now, it's testing if the method cause text event during composition. bug746993-1.html is probably using synthesizeText() for inserting ASCII characters to the contenteditable editor. That doesn't make sense. I don't understand why the test has worked since editor doesn't assume that only text event comes editor without composition events and any key events never come during composition. Now, EventUtils.sendString() is available for such purpose.

Now, the NotifyIME()'s argument is defined in nsEvent.h because it should be shared with nsIWidget API, see bug 558976.

Nothing is changed from previous patch except the file name.

Comment on attachment 670655 [details] [diff] [review] part.2 Dispatch composition events on removed content for canceling the composition if widget's IME APIs don't work Sorry for the spam, this patch doesn't need the rereview.

Comment on attachment 670657 [details] [diff] [review] part.3 XP level code shouldn't call nsIWidget::ResetInputState() and nsIWidget::CancelIMEComposition() directly ehsan, would you review the editor part? By the change, #ifdef is removed, but it doesn't matter since it's not serious performance issue and I'll work on to redesigning the widget side API.

Comment on attachment 670658 [details] [diff] [review] part.4 Emulate the behavior of nsIWidget::ResetInputState() and nsIWidget::CancelIMEComposition() if the composition is synthesized roc: Would you review the widget part? Destroyed() becomes virtual method. Therefore, if you don't like this, I'll add internal method too.

Why does Destroyed() have to be virtual? content can call non-virtual methods in widget AFAIK.

Okay, I moved mOnDestroyCalled to nsIWidget.

Comment on attachment 670652 [details] [diff] [review] part.1 Ensure a set of composition events is fired on same content I should rewrite better comment for TextCompositionArray.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #31) > mconley: > > You added a reftest in bug 749663: > http://mxr.mozilla.org/mozilla-central/source/layout/base/tests/bug746993-1. > html?force=1 > > However, it doesn't make sense for me. Why did you just set the "Here's some > text." to <textarea>.value and move the caret with > <textarea>.selectionStart? Your text event usage is definitely wrong. That > causes a MOZ_ASSERT crash with the new patches for this bug. Can I change > the test like I mentioned? See bug 746993 for the reasoning behind that test. But to answer your question about why I did X over Y, the answer is probably "ignorance". This was my first reftest, let alone my first test for the editor, so it's not really surprising that my solution was suboptimal. It's just more surprising that it passed review. Ehsan? ;)

Comment on attachment 670730 [details] [diff] [review] part.1 Ensure a set of composition events is fired on same content >+void >+nsIMEStateManager::Shutdown() >+{ >+ NS_ASSERTION(!sTextCompositions || !sTextCompositions->Length(), >+ "Still some compositions have remained"); >+ delete sTextCompositions; >+ sTextCompositions = nullptr; >+} Should this be MOZ_ASSERT() too?

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #48) > Comment on attachment 670730 [details] [diff] [review] > part.1 Ensure a set of composition events is fired on same content What is changed in this version of part 1. (I don't quite trust bugzilla's diff of different patches.)

Comment on attachment 670730 [details] [diff] [review] part.1 Ensure a set of composition events is fired on same content > nsIMEStateManager::OnDestroyPresContext(nsPresContext* aPresContext) > { > NS_ENSURE_ARG_POINTER(aPresContext); >+ >+ // First, if there is a composition in the aPresContext, clean up it. >+ if (sTextCompositions) { >+ TextCompositionArray::index_type i = >+ sTextCompositions->IndexOf(aPresContext); >+ if (i != TextCompositionArray::NoIndex) { >+ // there should be only one composition per presContext object. >+ sTextCompositions->RemoveElementAt(i); Could you add assertion MOZ_ASSERT(sTextCompositions->IndexOf(aPresContext) == TextCompositionArray::NoIndex) >+ // All active composition in the process is stored by this array. compositions .. are stored in this array. Hmm, how does this all work if you have <input> or <textarea> in a popup, say in a doorhanger. doorhanger would get its own widget, yet the code here tries to get widget from prescontext's root frame.

(In reply to Olli Pettay [:smaug] from comment #49) > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #48) > > Comment on attachment 670730 [details] [diff] [review] > > part.1 Ensure a set of composition events is fired on same content > What is changed in this version of part 1. > (I don't quite trust bugzilla's diff of different patches.) If you need the differences with the old patch which you reviewed, see comment 34. If you need the differences with the previous patch, I rewrite some comments in nsIMEStateManager.h and TextComposition.h.

I don't need the diff anymore. I ended up reviewing the patch anyway :)

(In reply to Olli Pettay [:smaug] from comment #50) > Hmm, how does this all work if you have <input> or <textarea> in a popup, > say in a doorhanger. > doorhanger would get its own widget, yet the code here tries to get widget > from prescontext's > root frame. I guess doorhanger is same as XUL <panel> like bookmark panel. I assumed that nsPresContext which is used by XUL <panel>s returns the owner's widget by GetNearesetWidget(). As far as I know, widget for panel never has native focus. So, the composition events are always handled by the owner's widget. Therefore, I don't think there is a problem about it. Do I misunderstand the GetNearestWidget() behavior? Or do you have some ideas for the bad behavior? # GetNearestWidget() should be renamed? I don't have better (and short) name for it, though.

Ah, if the popup doesn't have native focus, then this could work. Could you test manually, please.

Yeah, I can use IME on the <input> in the bookmark panel. And I can commit it by click which notifies IME the commit request by nsIEditor::forceCompositionEnd(). So, it should work fine. Anyway, I'll file a new bug for getting native IME context stricter. It should fix the issue even if it's there.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #55) > Yeah, I can use IME on the <input> in the bookmark panel. And I can commit > it by click which notifies IME the commit request by > nsIEditor::forceCompositionEnd(). So, it should work fine. > Ok, thanks. I'll re-review the part 1 then again.

fixed the points you reviewed. And I changed NS_ASSERTION() in nsIMEStateManager::Shutdown() to MOZ_ASSERT().

https://hg.mozilla.org/integration/mozilla-inbound/rev/cced04530e2f https://hg.mozilla.org/integration/mozilla-inbound/rev/bbb31887247b https://hg.mozilla.org/integration/mozilla-inbound/rev/bc1f210a91f6 https://hg.mozilla.org/integration/mozilla-inbound/rev/ce3c78a357ab https://hg.mozilla.org/integration/mozilla-inbound/rev/b486c3782846

https://hg.mozilla.org/mozilla-central/rev/cced04530e2f https://hg.mozilla.org/mozilla-central/rev/bbb31887247b https://hg.mozilla.org/mozilla-central/rev/bc1f210a91f6 https://hg.mozilla.org/mozilla-central/rev/ce3c78a357ab https://hg.mozilla.org/mozilla-central/rev/b486c3782846