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.

Also note same issue for IME-integrated tools such as IME pad.

Is that stable?

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.

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

A testcase http://mozilla.pettay.fi/moztests/key_event_test.html

(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.

(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)

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.

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!

I'll work on this next alpha cycle.

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.

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.

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?

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.

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.

For Linux (gtk and qt).

For Mac.

For Android.

Changes nsIDOMWindowUtils::SendCompositionEvent() and fixes all current tests which are using it.

I'll request review for this after part.7 gets r+.

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 on attachment 554064 [details] [diff] [review] Patch part.8 Add composition event tests Thank you for your review, please check the tests too.

(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 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 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?

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

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 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 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.

(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.

(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().

But probably nsTextStore doesn't check mWindow after dispatching event in other places, though... Fortunately, nsTextStore isn't used in default settings...

dougt: see comment 32.

(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 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?

Thank you, karlt.

updated for latest trunk.

Oops, test_sanityEventUtils.html part should be moved to part.7 before landing.

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 on attachment 561673 [details] [diff] [review] Patch part.8 Add composition event tests rs=me

Comment on attachment 561662 [details] [diff] [review] Patch part.4 Implement DOM3 composition event on Linux Thanks for tidying this up. Looks good.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebfdff9a84a5 https://hg.mozilla.org/integration/mozilla-inbound/rev/6432301d41ce https://hg.mozilla.org/integration/mozilla-inbound/rev/61f11b38c268 https://hg.mozilla.org/integration/mozilla-inbound/rev/71ffc9bbcb31 https://hg.mozilla.org/integration/mozilla-inbound/rev/8d48a0f50b4c https://hg.mozilla.org/integration/mozilla-inbound/rev/54fbf5eda914 https://hg.mozilla.org/integration/mozilla-inbound/rev/df8272331d53 https://hg.mozilla.org/integration/mozilla-inbound/rev/cc42e81d78b2 Thank you, all reviewers. The composition event can fix the problems in some websites which cannot listen keypress event of Firefox during composition.

tryserver's URL: https://tbpl.mozilla.org/?tree=Try&rev=53b636c2510c

https://hg.mozilla.org/mozilla-central/rev/ab85ce20e10e https://hg.mozilla.org/mozilla-central/rev/6432301d41ce https://hg.mozilla.org/mozilla-central/rev/61f11b38c268 https://hg.mozilla.org/mozilla-central/rev/71ffc9bbcb31 https://hg.mozilla.org/mozilla-central/rev/8d48a0f50b4c https://hg.mozilla.org/mozilla-central/rev/54fbf5eda914 https://hg.mozilla.org/mozilla-central/rev/df8272331d53 https://hg.mozilla.org/mozilla-central/rev/cc42e81d78b2

Documented: https://developer.mozilla.org/en/DOM/DOM_event_reference https://developer.mozilla.org/en/DOM/CompositionEvent https://developer.mozilla.org/en/DOM/DOM_event_reference/compositionstart https://developer.mozilla.org/en/DOM/DOM_event_reference/compositionupdate https://developer.mozilla.org/en/DOM/DOM_event_reference/compositionend

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.

Alex, do we need this on aurora for Fennec 11?

(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.