Closed Bug 1065835 Opened 8 years ago Closed 7 years ago

TextComposition should have API to commit or cancel composition and if widget doesn't perform it synchronously, it should synthesize composition end event

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod)

Attachments

(6 files, 10 obsolete files)

23.73 KB, patch
smaug
: review+
Details | Diff | Splinter Review
12.45 KB, patch
Details | Diff | Splinter Review
9.19 KB, patch
Details | Diff | Splinter Review
10.96 KB, patch
karlt
: review+
Details | Diff | Splinter Review
5.54 KB, patch
smaug
: review+
Details | Diff | Splinter Review
17.76 KB, patch
karlt
: review+
Details | Diff | Splinter Review
This might be useful for e10s (in PuppetWidget) and is really useful on GTK.

Requesting to commit or cancel composition to IME could be asynchronously. E.g., GTK, this depends on active IME. Now, GTK widget has workaround for this. But e10s might want this too.

Let's implement this hack in TextComposition.
Callers of NotifyIME(REQUEST_TO_CANCEL_COMPOSITION) and NotifyIME(REQUEST_TO_COMMIT_COMPOSITION) assumes that committing or canceling composition synchronously. This patch makes TextComposition handles the request and synthesizes committing or canceling composition by dispatching events if native IME does not do it synchronously.

Such situation may occur only on Linux as far as I know. But on the other platforms, it could occur. So, implementing this helps all platforms.

Additionally, if we land this patch, PuppetWidget *might* not require to request them with synchronous communication.
Attachment #8491289 - Flags: review?(bugs)
TextComposition should handle request for compositions synthesized for tests (via nsIDOMWindowUtils).

This is better for composition test because it's more similar to native IME handler.
Attachment #8491291 - Flags: review?(bugs)
Part.1 makes TextComposition destroyed at receiving compositionend event fired by native IME handler even if it synthesized committing or canceling composition events.

However, if NotifyIME(REQUEST_TO_*) is called during script blocker is enabled, i.e., during not safe to dispatch DOM events, composition and text events fired by native IME may be discarded by PresShell if native IME commits the composition synchronously. Then, the instance is not released and it causes some troubles at starting new composition on the widget.

For fixing the issue, this patch makes PresShell tell TextComposition that it discards a composition or text event.

If native compositionend event is already discarded, IMEStateManager will destroy the TextComposition instance after handling synthesized compositionend event (if native events are discarded, TextComposition synthesizes commit/cancel events with runnable events. Therefore, synthesized events are handled later).
Attachment #8491297 - Flags: review?(bugs)
Currently, synthesizing commit or cancel composition on Linux is implemented in GTK with complicated flag. But we can remove that by the changes!
By the changes, GTK now commits composition string twice when moving focus to a new editor during composition by click. The reason is that at clicking on another editor causes NOTIFY_IME_OF_CURSOR_POS_CHANGED which is called nsEditor::ForceCompositionEnd() *before* blur event handler of the old editor requests to commit composition. Therefore, NOTIFY_IME_OF_CURSOR_POS_CHANGED causes committing composition in the old editor and then, TextComposition synthesizes composition again.

Now, it is the time to remove the legacy request from our code. Instead, nsGTKIMModule should use NOTIFY_IME_OF_SELECTION_CHANGE.
Attachment #8491301 - Flags: review?(bugs)
Oops, part.3 includes a bug fix of part.2. Sorry for the spam.
Attachment #8491291 - Attachment is obsolete: true
Attachment #8491297 - Attachment is obsolete: true
Attachment #8491291 - Flags: review?(bugs)
Attachment #8491297 - Flags: review?(bugs)
Attachment #8491308 - Flags: review?(bugs)
FYI: part.4 and part.5 should be separated to another bug. However, even if we do so, we need to land them once because part.1 - part.3 breaks current GTK's IME handling.
Fix bad comment in nsPlaintextEditor.
Attachment #8491301 - Attachment is obsolete: true
Attachment #8491301 - Flags: review?(bugs)
Attachment #8491319 - Flags: review?(bugs)
The patches help to fix bug 975383 which is now important for improving performance in e10s mode.
Blocks: 975383
Comment on attachment 8491289 [details] [diff] [review]
part.1 Synthesize composition events for committing composition string if requesting to commit of IME isn't performed synchronously

> void
>+TextComposition::MayDispatchCompositionUpdate(const WidgetTextEvent* aEvent)
>+{
>+  if (Destroyed()) {
>+    return;
>+  }
>+
>+  if (mLastData == aEvent->theText) {
>+    return;
>+  }
>+
>+  WidgetCompositionEvent compositionUpdate(aEvent->mFlags.mIsTrusted,
>+                                           NS_COMPOSITION_UPDATE,
>+                                           aEvent->widget);
>+
>+  mLastData = compositionUpdate.data = aEvent->theText;
>+
>+  nsEventStatus status = nsEventStatus_eConsumeNoDefault;
>+  EventDispatcher::Dispatch(mNode, mPresContext,
>+                            &compositionUpdate, nullptr, &status, nullptr);
>+}

Oops, this method should copy WidgetEvent::time and WidgetEvent::timeStamp.

I'll modify it later. Ignore this at reviewing.
Hmm hmm, this all is hard to review.
Looking.
(In reply to Olli Pettay [:smaug] from comment #12)
> Hmm hmm, this all is hard to review.
> Looking.

Sorry. I have no idea to separate the patches more, especially, part.1.

The main purpose is part.1. The others are follow ups for fixing regressions of part.1.

I'd like to make TextComposition commit or cancel composition synchronously at requesting it to IME even if IME doesn't behave so.
Comment on attachment 8491289 [details] [diff] [review]
part.1 Synthesize composition events for committing composition string if requesting to commit of IME isn't performed synchronously

> TextComposition::CompositionEventDispatcher::CompositionEventDispatcher(
>-                                               nsPresContext* aPresContext,
>+                                               TextComposition* aComposition,
>                                                nsINode* aEventTarget,
>                                                uint32_t aEventMessage,
>-                                               const nsAString& aData) :
>-  mPresContext(aPresContext), mEventTarget(aEventTarget),
>-  mEventMessage(aEventMessage), mData(aData)
>+                                               const nsAString& aData,
>+                                               bool aIsSynthesizedEvent)
>+  : mTextComposition(aComposition)
>+  , mEventTarget(aEventTarget)
>+  , mWidget(mTextComposition->GetWidget())
Why we need mWidget? Can you not get it from mTextComposition when needed?


>@@ -190,16 +193,29 @@ private:
> 
>   // See the comment for IsComposing().
>   bool mIsComposing;
> 
>   // mIsEditorHandlingEvent is true while editor is modifying the composition
>   // string.
>   bool mIsEditorHandlingEvent;
> 
>+  // mIsRequestingCommit or mIsRequestingCancel is true while the instance
>+  // requesting commit or cancel the composition.
while we're requesting commit or canceling the composition.

>  However, some native IME
>+  // may not use different string (e.g., empty string) for commit or
>+  // non-empty string for cancel.  
I don't understand this comment.

>+ This dependency is bad thing for web apps.
>+  // Against this, we should override the coming data with last composition
>+  // string or empty string before dispatching DOM events into the tree.
nor this.

>+  bool mIsRequestingCommit;
>+  bool mIsRequestingCancel;
>+
>+  // mRequestedToCommitOrCancel is true if this composition was requested
>+  // commit or cancel itself.
>+  bool mRequestedToCommitOrCancel;

I don't understand this.
How is mRequestedToCommitOrCancel different to (mIsRequestingCommit || mIsRequestingCancel)?
Perhaps mDidRequestCommitOrCancel would emphasize that the request was just done. But please explain in the comment
how  mRequestedToCommitOrCancel is different to (mIsRequestingCommit || mIsRequestingCancel).


So, some clarifications needed.




>@@ -223,61 +239,76 @@ private:
>   void EditorDidHandleTextEvent();
> 
>   /**
>    * DispatchEvent() dispatches the aEvent to the mContent synchronously.
>    * The caller must ensure that it's safe to dispatch the event.
>    */
>   void DispatchEvent(WidgetGUIEvent* aEvent,
>                      nsEventStatus* aStatus,
>-                     EventDispatchingCallback* aCallBack);
>+                     EventDispatchingCallback* aCallBack,
>+                     bool aIsSynthesized);
>+
>+  /**
>+   * MayDispatchCompositionUpdate() may dispatches compositionupdate event
may dispatch a compositionupdate
But perhaps call this method MaybeDispatchCompositionUpdate.


>+   * WARNING: The caller must check if a call of this method caused calling
>+   *          Destroy() of the instance before using other members.
Couldn't the method return false if Destroy was called?
Attachment #8491289 - Flags: review?(bugs) → review-
Comment on attachment 8491308 [details] [diff] [review]
part.2 TextComposition::RequetToCommit() should support to handle even if the composition is synthesized for test

I guess. code moves are always a bit hard to review.
Attachment #8491308 - Flags: review?(bugs) → review+
Comment on attachment 8491309 [details] [diff] [review]
part.3 Destroy TextComposition instance after handling synthesized compositionend event when synthesized events for a request to commit or cancel is caused by PresShell discarding native compositionen

Hmm, the setup is getting quite complicated.
Attachment #8491309 - Flags: review?(bugs) → review+
Attachment #8491319 - Flags: review?(bugs) → review+
Okay, I separated the part of overriding commit string at requesting commit or canceling.

(In reply to Olli Pettay [:smaug] from comment #14)
> Comment on attachment 8491289 [details] [diff] [review]
> part.1 Synthesize composition events for committing composition string if
> requesting to commit of IME isn't performed synchronously
> 
> > TextComposition::CompositionEventDispatcher::CompositionEventDispatcher(
> >-                                               nsPresContext* aPresContext,
> >+                                               TextComposition* aComposition,
> >                                                nsINode* aEventTarget,
> >                                                uint32_t aEventMessage,
> >-                                               const nsAString& aData) :
> >-  mPresContext(aPresContext), mEventTarget(aEventTarget),
> >-  mEventMessage(aEventMessage), mData(aData)
> >+                                               const nsAString& aData,
> >+                                               bool aIsSynthesizedEvent)
> >+  : mTextComposition(aComposition)
> >+  , mEventTarget(aEventTarget)
> >+  , mWidget(mTextComposition->GetWidget())
> Why we need mWidget? Can you not get it from mTextComposition when needed?

Good point. Removed.

> >  However, some native IME
> >+  // may not use different string (e.g., empty string) for commit or
> >+  // non-empty string for cancel.  
> I don't understand this comment.
> 
> >+ This dependency is bad thing for web apps.
> >+  // Against this, we should override the coming data with last composition
> >+  // string or empty string before dispatching DOM events into the tree.
> nor this.

Well, please see next patch's comment.

> >+  bool mIsRequestingCommit;
> >+  bool mIsRequestingCancel;
> >+
> >+  // mRequestedToCommitOrCancel is true if this composition was requested
> >+  // commit or cancel itself.
> >+  bool mRequestedToCommitOrCancel;
> 
> I don't understand this.
> How is mRequestedToCommitOrCancel different to (mIsRequestingCommit ||
> mIsRequestingCancel)?
> Perhaps mDidRequestCommitOrCancel would emphasize that the request was just
> done. But please explain in the comment
> how  mRequestedToCommitOrCancel is different to (mIsRequestingCommit ||
> mIsRequestingCancel).

Please check the comment in the patch.

mRequestedToCommitOrCancel is true after mIsRequestingCommit and mIsRequestingCancel are set false. I.e., mIsRequesting* are true *only* during requesting it (calling NotifyIME()).
Attachment #8491289 - Attachment is obsolete: true
Attachment #8492119 - Flags: review?(bugs)
This patch overrides the data values which are caused by our request of commit or canceling composition. We won't depend on native IME behavior with this patch.
Attachment #8492122 - Flags: review?(bugs)
Now, we can remove the hack in nsGtkIMModule which makes the code complicated and could cause some bugs.
Attachment #8491298 - Attachment is obsolete: true
Attachment #8492125 - Flags: review?(karlt)
Attachment #8492125 - Attachment description: part.4 nsGtkIMModule doesn't need to synthesize composition events for committing composition synchronously since TextComposition now supports it → part.5 nsGtkIMModule doesn't need to synthesize composition events for committing composition synchronously since TextComposition now supports it
Let's remove NOTIFY_IME_OF_CURSOR_POS_CHANGE which is used only for calling gtk_im_context_reset() when there is not composition.

Instead, nsGtkIMModule should handle NOTIFY_IME_OF_SELECTION_CHANGE which is notified when selection in the focused editor is changed. This is better to detect the caret position change.

And also there is a bug of notifying selection change. TextEventHandlingMarker tells when editor starts to handle a text event from its constructor and when editor ends handling a text event from its destructor. With this notification, TextComposition may ignore selection change and text change which are caused by IME. However, in current code, TextEventHandlingMarker is destroyed before a call of NotifiyEditorObservers(eNotifyEditorObserversOfEnd):
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/nsPlaintextEditor.cpp#884

Therefore, TextComposition is notified a selection change caused by a text event after TextEventHandlingMarker tells that editor ends handling it. For fixing this, nsPlaintextEditor::UpdateIMEComposition() should put TextEventHandlingMarker outermost. By this change, it's always destroyed at the end of UpdateIMEComposition().
Attachment #8491319 - Attachment is obsolete: true
Attachment #8492127 - Flags: review?(karlt)
Attachment #8492127 - Flags: review?(ehsan.akhgari)
Comment on attachment 8492122 [details] [diff] [review]
part.2 TextComposition should override commit string with the last data or empty string during requesting commit or canceling composition

>+  // IME may commit composition with empty string for a request of commit or
for a commit request 

>+  // with non-empty string for a request of cancel.  We should prevent such
for a cancel request


>+  // unexpected result.  E.g., web apps may be confused if they implement
>+  // autocomplete by the unexpected commit string.
I don't quite understand "by the unexpected commit string"

>   // mIsRequestingCommit or mIsRequestingCancel is true *only* while we're
>   // requesting commit or canceling the composition.  In other words, while
>   // one of these values is true, we're handling the request.
>+  // However, some native IME may use non-empty string for canceling request.
>+  // Or they may use a string unlike the last data (e.g., might cancel the
>+  // composition for commit request).  It's bad to depend on native IME.
>+  // Against this, we should override the coming data with last composition
>+  // string or empty string before dispatching DOM events into the tree.
>   bool mIsRequestingCommit;
>   bool mIsRequestingCancel;
I don't understand the comment you're adding here.
The flags are about doing commit or cancel. But your comment starts talking about
strings and such.



Would like to see those comments clarified.
Attachment #8492122 - Flags: review?(bugs) → review-
Comment on attachment 8492119 [details] [diff] [review]
part.1 Synthesize composition events for committing composition string if requesting to commit of IME isn't performed synchronously

>+  // is not synthesizing commit event, that means that the IME commits or
>+  // cancels the composition asynchronously.  Typically, iBus behaves so.
>+  // Then, synthesized events which were dispatched immediately after
>+  // the request have already commits our editor's composition string and
have already committed?

>+  // tell it to web apps.
told

>+
>+  // mRequestedToCommitOrCancel is true *after* we requested to commit or
>+  // cancel the composition to IME.  In other words, we already requested of
>+  // IME that it commits or cancels current composition.
requested IME to commit or cancel the current composition
Attachment #8492119 - Flags: review?(bugs) → review+
Comment on attachment 8492127 [details] [diff] [review]
part.6 Remove NOTIFY_IME_OF_CURSOR_POS_CHANGED and nsGtkIMModule should handle NOTIFY_IME_OF_SELECTION_CHANGE instead (r=smaug)

>     if (MOZ_UNLIKELY(!mIMModule)) {
>         switch (aIMENotification.mMessage) {
>-            case NOTIFY_IME_OF_CURSOR_POS_CHANGED:
>             case REQUEST_TO_COMMIT_COMPOSITION:
>             case REQUEST_TO_CANCEL_COMPOSITION:
>             case NOTIFY_IME_OF_FOCUS:
>             case NOTIFY_IME_OF_BLUR:
>               return NS_ERROR_NOT_AVAILABLE;
>             default:
>               break;
>         }
>     }
>     switch (aIMENotification.mMessage) {
>-        // TODO: We should replace NOTIFY_IME_OF_CURSOR_POS_CHANGED with
>-        //       NOTIFY_IME_OF_SELECTION_CHANGE.  The required behavior is
>-        //       really different from committing composition.
>-        case NOTIFY_IME_OF_CURSOR_POS_CHANGED:
>         case REQUEST_TO_COMMIT_COMPOSITION:
>             return mIMModule->CommitIMEComposition(this);
>         case REQUEST_TO_CANCEL_COMPOSITION:
>             return mIMModule->CancelIMEComposition(this);
>         case NOTIFY_IME_OF_FOCUS:
>             mIMModule->OnFocusChangeInGecko(true);
>             return NS_OK;
>         case NOTIFY_IME_OF_BLUR:
>             mIMModule->OnFocusChangeInGecko(false);
>             return NS_OK;
>         case NOTIFY_IME_OF_COMPOSITION_UPDATE:
>             mIMModule->OnUpdateComposition();
>             return NS_OK;
>+        case NOTIFY_IME_OF_SELECTION_CHANGE:
>+            mIMModule->OnSelectionChange(this);
>+            return NS_OK;

Sending NOTIFY_IME_OF_SELECTION_CHANGE to an nsWindow with !mIMModule would
crash when calling OnSelectionChange().

There is an existing similar problem with NOTIFY_IME_OF_COMPOSITION_UPDATE.
mIMModule can be null if this is a popup window.  I don't know whether or
not this method is called on popup windows, but it seems it could, giving the
lack of any documentation indicating otherwise.

Should the "break" in the first switch be replaced with return NS_OK?

However, what is the difference in behavior now between CancelIMEComposition,
CommitIMEComposition and OnSelectionChange?  Can these all be merged into a
single function, probably ResetIME?

>+nsWindow::GetIMEUpdatePreference()
>+{
>+    nsIMEUpdatePreference updatePreference(
>+        nsIMEUpdatePreference::NOTIFY_SELECTION_CHANGE);
>+    updatePreference.DontNotifyChangesCausedByComposition();
>+    return updatePreference;

Can you explain the change in NOTIFY_CHANGES_CAUSED_BY_COMPOSITION please? 

What is the consequence of the PuppetWidget change?
Attachment #8492127 - Flags: review?(karlt) → review-
Attachment #8492127 - Flags: review?(ehsan.akhgari) → review+
(In reply to Olli Pettay [:smaug] from comment #23)
> >+  // unexpected result.  E.g., web apps may be confused if they implement
> >+  // autocomplete by the unexpected commit string.
> I don't quite understand "by the unexpected commit string"

Oops, sorry. I rewrite the comment, please check it.

> >   // mIsRequestingCommit or mIsRequestingCancel is true *only* while we're
> >   // requesting commit or canceling the composition.  In other words, while
> >   // one of these values is true, we're handling the request.
> >+  // However, some native IME may use non-empty string for canceling request.
> >+  // Or they may use a string unlike the last data (e.g., might cancel the
> >+  // composition for commit request).  It's bad to depend on native IME.
> >+  // Against this, we should override the coming data with last composition
> >+  // string or empty string before dispatching DOM events into the tree.
> >   bool mIsRequestingCommit;
> >   bool mIsRequestingCancel;
> I don't understand the comment you're adding here.
> The flags are about doing commit or cancel. But your comment starts talking
> about
> strings and such.

Ah, I just cancel to add this comment because this is explained in the DispatchEvent() and that is not related to the flags directly. So, the explanation of the reason why they are necessary isn't needed at the flag definition.
Attachment #8492122 - Attachment is obsolete: true
Comment on attachment 8494340 [details] [diff] [review]
part.2 TextComposition should override commit string with the last data or empty string during requesting commit or canceling composition

Oops, please check above comment for the detail.
Attachment #8494340 - Flags: review?(bugs)
Good catch.

I think that if there is no mIMModule, IME is not available. So, any requests and notifications are not notified to IME (and nsGtkIMModule). Therefore, returning "not available" error is better, I think.

I merged CommitIMEComposition() and CancelIMEComposition() to EndIMEComposition() because they try just to *end* the active composition. If GTK would create new API for committing or canceling composition, we should redesign the new method again.

However, I don't merge OnSelectionChange() because it's really different from EndIMEComposition(). OnSelectionChange() is a notification of selection change and it notifies IME of the change with gtk_im_context_reset().

Currently, EndIMEComposition() does really same as OnSelectionChange(), though. However, some IME may keep composition even if we call gtk_im_context_reset(). Such IME might commit or cancel composition at blur. Therefore, EndIMEComposition() should do it too in another bug.

Therefore, OnSelectionChange() should be independent.
Attachment #8492127 - Attachment is obsolete: true
Attachment #8494349 - Flags: review?(karlt)
karlt:

PuppetWidget mediates NOTIFY_IME_OF_SELECTION_CHANGE.
http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/PuppetWidget.cpp#420

Therefore, we need to just remove the NOTIFY_IME_OF_CURSOR_POS_CHANGED from here due to obsoleted.
Comment on attachment 8494349 [details] [diff] [review]
part.6 Remove NOTIFY_IME_OF_CURSOR_POS_CHANGED and nsGtkIMModule should handle NOTIFY_IME_OF_SELECTION_CHANGE instead (r=smaug+ehsan)

In nsGtkIMModule::OnSelectionChange(nsWindow* aCaller):

>+    GtkIMContext *im = GetContext();
>+    if (MOZ_UNLIKELY(!im)) {
>+        PR_LOG(gGtkIMLog, PR_LOG_ALWAYS,
>+            ("    FAILED, there are no context"));
>+        return;
>+    }
>+
>+    gtk_im_context_reset(im);

Why not use "ResetIME();" to replace this code?
Attachment #8494349 - Flags: review?(karlt) → review+
Attachment #8494340 - Flags: review?(bugs) → review+
Depends on: 1081992
Depends on: 1140832
Depends on: 1138159
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.