Last Comment Bug 543789 - Implement DOM3 composition events
: Implement DOM3 composition events
Status: RESOLVED FIXED
: dev-doc-complete, inputmethod
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- major (vote)
: mozilla9
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
Mentors:
data:text/html,<input id="i"><label><...
Depends on: 750439 622247 697842 705057 713502 724471
Blocks: 622245 700897
  Show dependency treegraph
 
Reported: 2010-02-02 11:06 PST by Elliot Block
Modified: 2013-01-07 16:52 PST (History)
17 users (show)
masayuki: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Demonstration of events: compositionstart (works), compositionend (works), compositionupdate (fails) (637 bytes, text/html)
2010-02-02 11:08 PST, Elliot Block
no flags Details
Patch part.1 Add DOM3 composition events (28.84 KB, patch)
2011-08-18 05:58 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
roc: superreview+
Details | Diff | Splinter Review
Patch part.2 Initialize data property of compositionstart event in nsEventStateManager::PreHandleEvent() (3.94 KB, patch)
2011-08-18 06:10 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
Details | Diff | Splinter Review
Patch part.3 Implement DOM3 composition event on Windows (11.55 KB, patch)
2011-08-18 06:15 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
jmathies: review+
Details | Diff | Splinter Review
Patch part.4 Implement DOM3 composition event on Linux (6.43 KB, patch)
2011-08-18 06:16 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
karlt: feedback+
Details | Diff | Splinter Review
Patch part.5 Implement DOM3 composition event on Mac (4.36 KB, patch)
2011-08-18 06:17 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
smichaud: review+
Details | Diff | Splinter Review
Patch part.6 Implement DOM3 composition event on Android (2.83 KB, patch)
2011-08-18 06:18 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Patch part.7 Dispatch compositionupdate event and set data value of compositionend event in all IME handling tests (49.69 KB, patch)
2011-08-18 06:20 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
roc: superreview+
Details | Diff | Splinter Review
Patch part.8 Add composition event tests (24.12 KB, patch)
2011-08-18 06:22 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Patch part.4 Implement DOM3 composition event on Linux (15.84 KB, patch)
2011-09-21 04:58 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Patch part.5 Implement DOM3 composition event on Mac (4.55 KB, patch)
2011-09-21 05:00 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
Details | Diff | Splinter Review
Patch part.6 Implement DOM3 composition event on Android (3.22 KB, patch)
2011-09-21 09:24 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
doug.turner: review+
bugs: review+
Details | Diff | Splinter Review
Patch part.4 Implement DOM3 composition event on Linux (13.92 KB, patch)
2011-09-21 22:52 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
karlt: review+
Details | Diff | Splinter Review
Patch part.8 Add composition event tests (25.95 KB, patch)
2011-09-21 23:45 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
bugs: review+
Details | Diff | Splinter Review

Description Elliot Block 2010-02-02 11:06:38 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/532.5 (KHTML, like Gecko) Chrome/4.0.249.78 Safari/532.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20100201 Minefield/3.7a1pre

Using IME should send compositionupdate events whenever the IME composition text changes, not just compositionstart/compositionend at start and finish. 

compositionupdate is critical for keeping a web app in sync with user input, e.g. otherwise you have no idea when/that the user changed/entered input.

http://www.w3.org/TR/DOM-Level-3-Events/#event-type-compositionupdate

Reproducible: Always

Steps to Reproduce:
1. Make a text <input> element

2. Listen for compositionupdate event on that element

inputElement.addEventListener('compositionupdate', function() {alert('updated');}, true);

3. Install and activate any IME.  Anything works, e.g. Windows Chinese Simplified IME.

4. Type into your input box
Actual Results:  
No compositionupdate event sent.

E.g. in the example, no alert fires, since no event handled, since no event sent.

Expected Results:  
Event sent and handled

If you retry repro steps you'll see that compositionstart and compositionend events work properly, just not compositionupdate.

If you check the Windows events you can verify that Windows System Chinese Simplified IME does indeed send the windows WM_ composition (update) events.  They just don't get propagated to Javascript.
Comment 1 Elliot Block 2010-02-02 11:08:45 PST
Created attachment 424819 [details]
Demonstration of events: compositionstart (works), compositionend (works), compositionupdate (fails)
Comment 2 Elliot Block 2010-02-02 11:09:27 PST
Also note same issue for IME-integrated tools such as IME pad.
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-02-02 17:51:56 PST
Is that stable?
Comment 4 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-02-02 21:17:48 PST
DOM 3 Events is still just a draft, not exactly something to implement.
So for now use 'text' event, not compositionupdate.

Does some other browser support compositionupdate already? Note, that
all may change a bit.
Comment 5 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-02-02 21:22:29 PST
And since Gecko has supported compositionstart, text and compositionend events
for ages, I'd suggest supporting those for now, since that way
people using old browsers can use the web site too.


But still, this is NEW. We need to add support for compositionupdate at some point, maybe pretty soon. Depends on how fast DOM 3 Events stabilizes.
Note, the latest draft is here http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html
Comment 6 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-02-02 21:31:35 PST
A testcase http://mozilla.pettay.fi/moztests/key_event_test.html
Comment 7 Makoto Kato [:m_kato] 2010-02-02 21:34:32 PST
(In reply to comment #4)

> Does some other browser support compositionupdate already? Note, that
> all may change a bit.

Chrome 4 seems to support compositionupdate event.  Safari 4 and Opera 10.50 don't support all IME event.
Comment 8 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-02-02 21:41:10 PST
(In reply to comment #7)
> (In reply to comment #4)
> Chrome 4 seems to support compositionupdate event.
That is their bad. The event may very well change a bit still.
And there sure are open issues with that event.

But yeah, we could start implementing it. First by firing compositionupdate
right after text event.
(Note, DOM 3 Events composition events are based on gecko's composition events)
Comment 9 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-02-02 22:18:22 PST
I have a question. Should the compositionupdate event be fired only when the data value is changed? For example, our text event is dispatched when the caret position is changed in composition. But it seems that the event is redundant for web developers. If so, we need to check before we fire the event.

And looks like the draft still has some issues, e.g., compositionstart event is cancelable, but on GTK2 with uim or iBus, we cannot discard/commit the composition forcedly. It has made some bugs on gecko, unfortunately.
# I think the cancelable should be an option. The spec shouldn't guarantee that it's cancelable.

And also I'm not sure whether the compositionend is cancelable with TSF. TSF requests to lock the document during composition. At that time, it shouldn't be cancelable.
Comment 10 Elliot Block 2010-10-08 16:41:54 PDT
Just wanted to comment that IE9, latest Safari and latest Chrome all appear to support compositionupdate events (as well as compositionstart and compositionend) and they make developing an IME-enabled application significantly better across browsers!  THanks!
Comment 11 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-12-17 01:20:26 PST
I'll work on this next alpha cycle.
Comment 12 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-12-17 02:28:30 PST
Note for myself:

1. Can the other browsers cancel the composition by compositionstart event? Especially on Windows.

2. Should text event be dispatched only on editable element?

3. NS_TEXT_TEXT shouldn't cause textInput event being dispatched. textInput event should be dispatched by nsEditor.

4. If textInput event calls input.value, then, the result contains the inputted character(s)? Or the method returns previous text contents of the editor?

5. What's DOM_INPUT_METHOD_OPTION? What's the difference between it and DOM_INPUT_METHOD_SCRIPT?

6. Should untrust textInput event cause text inputting on our editor?

7. Untrust composition event must not affect to trusted composition transaction.

Anything else?

# I'll post the issues to W3C ML if it's needed. But I should research more before that.
Comment 13 Masayuki Nakano [:masayuki] (Mozilla Japan) 2010-12-31 01:14:39 PST
I filed bug 622245 because text event needs some change which are not related to composition events. In this bug, I'll implement only DOM3 composition events.
Comment 14 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-08-18 05:58:32 PDT
Created attachment 554045 [details] [diff] [review]
Patch part.1 Add DOM3 composition events

This patch adds NS_COMPOSITION_UPDATE, adds data member to nsCompositionEvent and makes nsDOMCompositionEvent with nsIDOMCompositionEvent.

nsCompositionEvent doesn't have locale for now. We need to get the IME's locale on each platform when we implement it. That needs more time, so, it should be done in another bug in the future. However, web developer can set the locale value by initCompositionEvent() for compatibility.

And also, the compositionstart event isn't cancelable contrary to DOM3 Events current draft. I posted the issue to www-dom.
http://lists.w3.org/Archives/Public/www-dom/2011JulSep/0109.html
http://lists.w3.org/Archives/Public/www-dom/2011JulSep/0112.html

nsCompositionEvent inherits nsGUIEvent directly because it doesn't need the modifier keys.

# I'm not sure who is a good reviewer for nsDOMClassInfo.cpp and nsEventNameList.h, bz?
Comment 15 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-08-18 06:10:57 PDT
Created attachment 554056 [details] [diff] [review]
Patch part.2 Initialize data property of compositionstart event in nsEventStateManager::PreHandleEvent()

This patch initializes data property of NS_COMPOSITION_START. The value is always current selected text. I think that when the event is generated, the selected value should be set. If we get the selected text when web app accesses the data property, it's odd in some case. For example:

var compositionstart;

function compositionStartHandler(e)
{
  if (!compositionstart) {
    compositionstart = e;
  }
}

function foo()
{
   var currentSelectedText = compositionstart.data;
}

This is probably unintentional behavior. However, if a compositionstart event handler changes the selection, later compositionstart event handlers get first selected text, not the actual selected text, though.

The data value of compositionstart doesn't depend on platform spec, so, this can be implemented on ESM for all platforms.
Comment 16 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-08-18 06:15:12 PDT
Created attachment 554057 [details] [diff] [review]
Patch part.3 Implement DOM3 composition event on Windows

This patch makes widget for Windows dispatch compositionupdate only when the composition string is changed. And this sets the latest composition string to the data value of compositionend.

# I'll request widget's reviewers after smaug's review.
Comment 17 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-08-18 06:16:43 PDT
Created attachment 554058 [details] [diff] [review]
Patch part.4 Implement DOM3 composition event on Linux

For Linux (gtk and qt).
Comment 18 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-08-18 06:17:41 PDT
Created attachment 554059 [details] [diff] [review]
Patch part.5 Implement DOM3 composition event on Mac

For Mac.
Comment 19 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-08-18 06:18:27 PDT
Created attachment 554062 [details] [diff] [review]
Patch part.6 Implement DOM3 composition event on Android

For Android.
Comment 20 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-08-18 06:20:35 PDT
Created attachment 554063 [details] [diff] [review]
Patch part.7 Dispatch compositionupdate event and set data value of compositionend event in all IME handling tests

Changes nsIDOMWindowUtils::SendCompositionEvent() and fixes all current tests which are using it.
Comment 21 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-08-18 06:22:07 PDT
Created attachment 554064 [details] [diff] [review]
Patch part.8 Add composition event tests

I'll request review for this after part.7 gets r+.
Comment 22 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-09-16 17:56:17 PDT
Comment on attachment 554045 [details] [diff] [review]
Patch part.1 Add DOM3 composition events


>+    // XXX compositionstart is cancelable in draft of DOM3 Events.
>+    //     However, it doesn't make sence for us, we cannot cancel composition
>+    //     when we sends compositionstart event.
send


> TabParent::SendCompositionEvent(nsCompositionEvent& event)
> {
>-  mIMEComposing = event.message == NS_COMPOSITION_START;
>+  mIMEComposing = event.message != NS_COMPOSITION_END;
>   mIMECompositionStart = NS_MIN(mIMESelectionAnchor, mIMESelectionFocus);
>   if (mIMECompositionEnding)
>     return true;
>   event.seqno = ++mIMESeqno;
>   return PBrowserParent::SendCompositionEvent(event);
> }
Could you explain this change.


> public:
>   nsCompositionEvent(PRBool isTrusted, PRUint32 msg, nsIWidget *w)
>-    : nsInputEvent(isTrusted, msg, w, NS_COMPOSITION_EVENT)
>+    : nsGUIEvent(isTrusted, msg, w, NS_COMPOSITION_EVENT)
>   {
>+    // XXX compositionstart is cancelable in draft of DOM3 Events.
>+    //     However, it doesn't make sence for us, we cannot cancel composition
>+    //     when we sends compositionstart event.
>+    flags |= NS_EVENT_FLAG_CANT_CANCEL;
Why is this needed here and in DOM event?


Those explained/fixed, r=me
Comment 23 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-09-16 23:51:20 PDT
Comment on attachment 554064 [details] [diff] [review]
Patch part.8 Add composition event tests

Thank you for your review, please check the tests too.
Comment 24 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-09-17 00:01:12 PDT
(In reply to Olli Pettay [:smaug] from comment #22)
> > TabParent::SendCompositionEvent(nsCompositionEvent& event)
> > {
> >-  mIMEComposing = event.message == NS_COMPOSITION_START;
> >+  mIMEComposing = event.message != NS_COMPOSITION_END;
> >   mIMECompositionStart = NS_MIN(mIMESelectionAnchor, mIMESelectionFocus);
> >   if (mIMECompositionEnding)
> >     return true;
> >   event.seqno = ++mIMESeqno;
> >   return PBrowserParent::SendCompositionEvent(event);
> > }
> Could you explain this change.

mIMEComposing should be TRUE between NS_COMPOSITION_START and NS_COMPOSITION_END. The composition events must be fired NS_COMPOSITION_START -> NS_COMPOSITION_UPDATE * n -> NS_COMPOSITION_END. So, it receives NS_COMPOSITION_START or NS_COMPOSITION_UPDATE is received, it means it's composing.

> > public:
> >   nsCompositionEvent(PRBool isTrusted, PRUint32 msg, nsIWidget *w)
> >-    : nsInputEvent(isTrusted, msg, w, NS_COMPOSITION_EVENT)
> >+    : nsGUIEvent(isTrusted, msg, w, NS_COMPOSITION_EVENT)
> >   {
> >+    // XXX compositionstart is cancelable in draft of DOM3 Events.
> >+    //     However, it doesn't make sence for us, we cannot cancel composition
> >+    //     when we sends compositionstart event.
> >+    flags |= NS_EVENT_FLAG_CANT_CANCEL;
> Why is this needed here and in DOM event?

When IME composition is starting, we dispatch a compositionstart event. Therefore, the composition has been already started by IME. We cannot cancel it even if web application calls preventDefault(). It doesn't make sense that editor *always* handles the cancelable compositionstart event even if it was canceled. Therefore, I think that the compositionstart event shouldn't be cancelable in Gecko.
Comment 25 Karl Tomlinson (:karlt) 2011-09-19 02:13:08 PDT
Comment on attachment 554058 [details] [diff] [review]
Patch part.4 Implement DOM3 composition event on Linux

It looks like mLastDispatchedCompositionString can be merged into the existing mCompositionString by doing the following:

  Change OnChangeCompositionNative() to GetCompositionString() into a
  temporary variable (instead of into mCompositionString).

  Change OnChangeCompositionNative() and CommitCompositionBy() to pass their
  string to DispatchTextEvent() instead of setting mCompositionString.

  Let DispatchTextEvent() update mCompositionString, so it can compare the
  string passed as a parameter with the old value to decide whether to
  dispatch an NS_COMPOSITION_UPDATE event.

  Perhaps it is still necessary to add a Truncate() of the string in
  DispatchCompositionStart() because DispatchCompositionEnd() has some early
  returns, or can the variables be reset in DispatchCompositionEnd before the
  early returns?
Comment 26 Steven Michaud [:smichaud] (Retired) 2011-09-19 09:35:53 PDT
Comment on attachment 554059 [details] [diff] [review]
Patch part.5 Implement DOM3 composition event on Mac

This basically looks fine to me.

But shouldn't you return early from IMEInputHandler::DispatchTextEvent() if DispatchEvent(compositionUpdate) causes Destroyed() to return PR_TRUE?
Comment 27 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-09-21 04:58:37 PDT
Created attachment 561442 [details] [diff] [review]
Patch part.4 Implement DOM3 composition event on Linux
Comment 28 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-09-21 05:00:41 PDT
Created attachment 561443 [details] [diff] [review]
Patch part.5 Implement DOM3 composition event on Mac

> But shouldn't you return early from IMEInputHandler::DispatchTextEvent() if 
> DispatchEvent(compositionUpdate) causes Destroyed() to return PR_TRUE?

Sure.
Comment 29 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-09-21 09:24:30 PDT
Created attachment 561497 [details] [diff] [review]
Patch part.6 Implement DOM3 composition event on Android

fix a nit. we need to truncate the stored composition string at starting composition. I'll test it tomorrow but please review this patch, we don't have much time for mozilla 9.
Comment 30 Doug Turner (:dougt) 2011-09-21 09:46:32 PDT
Comment on attachment 561497 [details] [diff] [review]
Patch part.6 Implement DOM3 composition event on Android

Review of attachment 561497 [details] [diff] [review]:
-----------------------------------------------------------------

// XXX We must check whether this widget is destroyed or not
		// before dispatching next event. However, Android's
		// nsWindow has never checked it...

I do not understand this part?  Do we need a follow up bug?
Comment 31 Jim Mathies [:jimm] 2011-09-21 09:54:54 PDT
Comment on attachment 554057 [details] [diff] [review]
Patch part.3 Implement DOM3 composition event on Windows

Review of attachment 554057 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/src/windows/nsTextStore.cpp
@@ +1284,1 @@
>        }

You've invoked mWindow on line 1260, so I don't think there's a need to check mWindow in all of these if statements. We'll never get this far if it's invalid.

@@ +1429,5 @@
> +    mWindow->InitEvent(compositionUpdate);
> +    compositionUpdate.data = mCompositionString;
> +    mLastDispatchedCompositionString = mCompositionString;
> +    mWindow->DispatchWindowEvent(&compositionUpdate);
> +    if (!mWindow || mWindow->Destroyed()) {

ditto here.

@@ +1453,5 @@
>    textEvent.theText.ReplaceSubstring(NS_LITERAL_STRING("\r\n"),
>                                       NS_LITERAL_STRING("\n"));
>    mWindow->DispatchWindowEvent(&textEvent);
>  
> +  if (!mWindow || mWindow->Destroyed()) {

same here.
Comment 32 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-09-21 09:57:07 PDT
(In reply to Doug Turner (:dougt) from comment #30)
> Comment on attachment 561497 [details] [diff] [review]
> Patch part.6 Implement DOM3 composition event on Android
> 
> Review of attachment 561497 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> // XXX We must check whether this widget is destroyed or not
> 		// before dispatching next event. However, Android's
> 		// nsWindow has never checked it...
> 
> I do not understand this part?  Do we need a follow up bug?

Yes. Android's Firefox probably has crash bugs for some event handling. DispatchEvent() event may cause a DOM event. The DOM event's handler might destroy the widget. So, nsWindow should be held by kungFuDeathGrip before it calls DispatchEvent(). And it should check whether the widget is going to be destroyed or not. If it's destroying, it should not continue any jobs.
Comment 33 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-09-21 10:02:30 PDT
(In reply to Jim Mathies [:jimm] from comment #31)
> You've invoked mWindow on line 1260, so I don't think there's a need to
> check mWindow in all of these if statements. We'll never get this far if
> it's invalid.

I don't think so, mWindow becomes NULL when the nsTextStore instance looses focus.
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsTextStore.cpp#1445
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsTextStore.cpp#140

The event handler might change focus. So, the mWindow might be NULL after nsTextStore calls mWindow->DispatchEvent().
Comment 34 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-09-21 10:04:13 PDT
But probably nsTextStore doesn't check mWindow after dispatching event in other places, though... Fortunately, nsTextStore isn't used in default settings...
Comment 35 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-09-21 10:05:05 PDT
dougt:

see comment 32.
Comment 36 Jim Mathies [:jimm] 2011-09-21 11:30:04 PDT
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #33)
> (In reply to Jim Mathies [:jimm] from comment #31)
> > You've invoked mWindow on line 1260, so I don't think there's a need to
> > check mWindow in all of these if statements. We'll never get this far if
> > it's invalid.
> 
> I don't think so, mWindow becomes NULL when the nsTextStore instance looses
> focus.
> http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsTextStore.
> cpp#1445
> http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsTextStore.
> cpp#140
> 
> The event handler might change focus. So, the mWindow might be NULL after
> nsTextStore calls mWindow->DispatchEvent().

Ah, ok I see. So anytime we call dispatch we need to recheck. Sounds good.
Comment 37 Karl Tomlinson (:karlt) 2011-09-21 20:08:21 PDT
Comment on attachment 561442 [details] [diff] [review]
Patch part.4 Implement DOM3 composition event on Linux

>-    void SetTextRangeList(nsTArray<nsTextRange> &aTextRangeList);
>+    void SetTextRangeList(const nsAString& aCompositionString,
>+                          nsTArray<nsTextRange> &aTextRangeList);

mDispatchedCompositionString is updated before SetTextRangeList is called,
which means this change is unnecessary?

>-    PRBool DispatchCompositionEnd();
>+    PRBool DispatchCompositionEnd(const nsAString& aCommitString);

>   Perhaps it is still necessary to add a Truncate() of the string in
>   DispatchCompositionStart() because DispatchCompositionEnd() has some early
>   returns, or can the variables be reset in DispatchCompositionEnd before the
>   early returns?

It seems that resetting the string before the early return has caused quite
some trouble because of the need to dispatch the string in the
NS_COMPOSITION_END event.  Would it be simpler to Truncate() in the
!mLastFocusedWindow and normal return paths from DispatchCompositionEnd(), so
that DispatchCompositionEnd can use mDispatchedCompositionString instead of
the parameter?
(I assume mDispatchedCompositionString is already empty if !mIsComposing.)

>-    mCompositionString = aString;
>-    if (!DispatchTextEvent(PR_FALSE)) {
>+    nsAutoString commitString(aString);
>+    if (!DispatchTextEvent(commitString, PR_FALSE)) {
>         return PR_FALSE;
>     }
>     // We should dispatch the compositionend event here because some IMEs
>     // might not fire "preedit_end" native event.
>-    return DispatchCompositionEnd(); // Be aware, widget can be gone
>+    return DispatchCompositionEnd(commitString); // Be aware, widget can be gone

That would make the commitString copy here unnecessary, if I follow correctly.

>-    DispatchCompositionEnd(); // Be aware, widget can be gone
>+    // Be aware, widget can be gone
>+    DispatchCompositionEnd(mDispatchedCompositionString);
>+    mDispatchedCompositionString.Truncate();

I'm not clear what is happening here but perhaps changing
DispatchCompositionEnd() will make this change unnecessary?
Comment 38 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-09-21 22:52:28 PDT
Created attachment 561662 [details] [diff] [review]
Patch part.4 Implement DOM3 composition event on Linux

Thank you, karlt.
Comment 39 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-09-21 23:45:48 PDT
Created attachment 561673 [details] [diff] [review]
Patch part.8 Add composition event tests

updated for latest trunk.
Comment 40 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-09-21 23:55:25 PDT
Oops, test_sanityEventUtils.html part should be moved to part.7 before landing.
Comment 41 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-09-21 23:55:55 PDT
Comment on attachment 561497 [details] [diff] [review]
Patch part.6 Implement DOM3 composition event on Android


>+            if (mIMEComposing &&
>+                event.theText != mIMELastDispatchedComposingText) {
>+                nsCompositionEvent compositionUpdate(PR_TRUE,
>+                                                     NS_COMPOSITION_UPDATE,
>+                                                     this);
>+                InitEvent(compositionUpdate, nsnull);
>+                compositionUpdate.data = event.theText;
>+                mIMELastDispatchedComposingText = event.theText;
>+                DispatchEvent(&compositionUpdate);
>+                // XXX We must check whether this widget is destroyed or not
>+                //     before dispatching next event.  However, Android's
>+                //     nsWindow has never checked it...
Please file a followup to fix this.
Comment 42 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-09-22 00:18:05 PDT
Comment on attachment 561673 [details] [diff] [review]
Patch part.8 Add composition event tests

rs=me
Comment 43 Karl Tomlinson (:karlt) 2011-09-22 01:06:00 PDT
Comment on attachment 561662 [details] [diff] [review]
Patch part.4 Implement DOM3 composition event on Linux

Thanks for tidying this up.  Looks good.
Comment 45 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-09-22 02:23:38 PDT
tryserver's URL:
https://tbpl.mozilla.org/?tree=Try&rev=53b636c2510c
Comment 48 Eric Shepherd [:sheppy] 2011-11-29 12:41:18 PST
I've done a copy-edit and clean-up pass on those docs :masayuki wrote; they could use a technical review to double-check them for accuracy but otherwise they're done, and mentioned on Firefox 9 for developers.
Comment 49 Brad Lassey [:blassey] (use needinfo?) 2012-01-27 23:38:30 PST
Alex, do we need this on aurora for Fennec 11?
Comment 50 Alex Pakhotin (:alexp) 2012-01-27 23:51:33 PST
(In reply to Brad Lassey [:blassey] from comment #49)
> Alex, do we need this on aurora for Fennec 11?

I believe this change is already there. It was pushed to m-c in September.

Note You need to log in before you can comment on or make changes to this bug.