nsIDOMWindowUtils::SendTextEvent() was designed for automated test. Therefore, it was enough the method to treat 3 or less clauses. However, b2g will use it for internal VKB/IME API. So, now, we need to get rid of the limitation for Japanese IME.

This patch replaces a lot of arguments of nsIDOMWindowUtils::SendTextEvent() with only nsICompositionString. nsICompositionString can store all information of composition string such as the string, length values of clauses, attributes of clauses and caret offset and length. The instance of nsICompositionString is created by: > Cc["@mozilla.org/dom/compositionstring;1"].createInstance(_EU_Ci.nsICompositionString) This fixes a bug in AppendClause() > -static void > -AppendClause(int32_t aClauseLength, uint32_t aClauseAttr, > - nsTArray<nsTextRange>* aRanges) > -{ > - NS_PRECONDITION(aRanges, "aRange is null"); > - if (aClauseLength == 0) { > - return; > - } > - nsTextRange range; > - range.mStartOffset = aRanges->Length() == 0 ? 0 : > - aRanges->ElementAt(aRanges->Length() - 1).mEndOffset + 1; The +1 is wrong. The start offset must be same as end offset of previous range. FYI: I have a plan to add an interface to nsICompositionString for specifying text range style, but this is out of scope of this bug.

Updates for part.1. Note that: > @@ -988,27 +987,27 @@ function runCompositionTest() > synthesizeComposition({ type: "compositionupdate", > data: "\u30E9\u30FC\u30E1\u30F3\u3055\u884C\u3053\u3046" }); > synthesizeText( > { "composition": > { "string": "\u30E9\u30FC\u30E1\u30F3\u3055\u884C\u3053\u3046", > "clauses": > [ > { "length": 5, > - "attr": nsIDOMWindowUtils.COMPOSITION_ATTR_SELECTEDCONVERTEDTEXT }, > + "attr": COMPOSITION_ATTR_SELECTEDCONVERTEDTEXT }, > { "length": 3, > - "attr": nsIDOMWindowUtils.COMPOSITION_ATTR_CONVERTEDTEXT } > + "attr": COMPOSITION_ATTR_CONVERTEDTEXT } > ] > }, > "caret": { "start": 5, "length": 0 } > }); > > if (!checkContent("\u30E9\u30FC\u30E1\u30F3\u3055\u884C\u3053\u3046", > "runCompositionTest", "#1-12") || > - !checkSelection(8, "", "runCompositionTest", "#1-12")) { > + !checkSelection(5, "", "runCompositionTest", "#1-12")) { > @@ -3257,39 +3256,39 @@ function runMaxLengthTest() > synthesizeComposition({ type: "compositionupdate", > data: "\u30E9\u30FC\u30E1\u30F3\u6700\u9AD8" }); > synthesizeText( > { "composition": > { "string": "\u30E9\u30FC\u30E1\u30F3\u6700\u9AD8", > "clauses": > [ > { "length": 4, > - "attr": nsIDOMWindowUtils.COMPOSITION_ATTR_SELECTEDCONVERTEDTEXT }, > + "attr": COMPOSITION_ATTR_SELECTEDCONVERTEDTEXT }, > { "length": 2, > - "attr": nsIDOMWindowUtils.COMPOSITION_ATTR_CONVERTEDTEXT } > + "attr": COMPOSITION_ATTR_CONVERTEDTEXT } > ] > }, > "caret": { "start": 4, "length": 0 } > }); > > if (!checkContent("\u30E9\u30FC\u30E1\u30F3\u6700\u9AD8", kDesc, "#1-10") || > - !checkSelection(6, "", kDesc, "#1-10")) { > + !checkSelection(4, "", kDesc, "#1-10")) { These are bug of this test. As I mentioned above, nsIDOMWindowUtils::SendTextEvent() have made wrong text ranges (previous range's end offset +1 == start offset). That made this wrong test pass. The selection should return normal selection offset. I.e., should return caret position. > return; > } > > // commit the composition string > synthesizeText( > { "composition": > { "string": "\u30E9\u30FC\u30E1\u30F3\u6700\u9AD8", > "clauses": > [ > { "length": 0, "attr": 0 } > ] > }, > - "caret": { "start": 8, "length": 0 } > + "caret": { "start": 6, "length": 0 } This is another bug of the test. The composition string length is 6, so, 8 is too large value. This causes an error in dom::CompositionString::DispatchEvent().

I'll request review for this after part.1 gets r+. Test builds: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=8461160bfedb

Comment on attachment 799973 [details] [diff] [review] part.1 Redesign nsIDOMWindowUtils::SendTextEvent() with nsICompositionString Please don't add DOMBaseFactory.cpp You can use nsLayoutModule.cpp. Could you make Dispatch scriptable. It could take nsIDOMWindow as param, and from that the binary could access the widget the same was as what DOMWindowUtils does.

> Could you make Dispatch scriptable. It could take nsIDOMWindow as param, and > from that the binary could access the widget the same was as what > DOMWindowUtils does. Hmm, it's possible but: * Isn't it messy that dispatching composition event API is in nsIDOMWindowUtils but dispatching text event API is in nsICompositionString? * Is nsICompositionString a good name? It becomes a text event dispatcher if you see it from JS. However, I don't like to use "TextEvent" in its name because I want to get rid of nsTextEvent in the future. I'm thinking that nsTextEvent should be merged with nsCompositionEvent.

How about this idea? 1. Rename nsICompositionString to nsICompositionStringSynthesizer 1. Create nsIDOMWindowUtils::createCompositionStringSynthesizer() which returns an instance for nsICompositionStringSynthesizer associated with the DOM window. 2. Create nsICompositionStringSynthesizer::synthesize() Then, all composition string related API is in nsIDOMWindowUtils. And nsICompositionString instance don't need to duplicate the code for checking security and getting target widget from DOM window. Note that getCompositionStringSynthesize() isn't a good name. nsICompositionString shouldn't be a singleton because dispatching text event can be nested. For example, text event handler may cause committing the composition.

Testing new patches: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=4dbd83af6b4b My approach needs to store nsIWidget in CompositionStringSynthesizer. So, probably, it should be cycle collectable.

Fix some nits: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=895bd079df85

Comment on attachment 800652 [details] [diff] [review] part.1 Redesign nsIDOMWindowUtils::SendTextEvent() with nsICompositionStringSynthesizer Creating nsICompositionStringSynthesizer via nsIDOMWindowUtils makes all synthesizing composition related event APIs in nsIDOMWindowUtils. Nobody cannot find a way to dispatch text event by this. Now, CompositionStringSynthesizer stores nsIWidget with strong reference. Therefore it's now cycle collectable. Additionally, it cannot be recycle for dispatching two or more events because there is no reason to support such function. Therefore, clear() is dropped.

nsIWidget implementations aren't cycle collectable, so cycle collecting CompositionStringSynthesizer doesn't really give us anything. Looking the rest of the patches soon.

(In reply to Olli Pettay [:smaug] from comment #12) > nsIWidget implementations aren't cycle collectable, so cycle collecting > CompositionStringSynthesizer doesn't really give us anything. Oh, really? I don't understand cycle collector system well. In short term, we don't need the cycle collector. So, if it's not necessary, I'll just remove the cycle collector code from it. > Looking the rest of the patches soon. Thanks in advance.

Comment on attachment 800652 [details] [diff] [review] part.1 Redesign nsIDOMWindowUtils::SendTextEvent() with nsICompositionStringSynthesizer >+NS_IMPL_CYCLE_COLLECTION_1(CompositionStringSynthesizer, mWidget) >+ >+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(CompositionStringSynthesizer) >+ NS_INTERFACE_MAP_ENTRY(nsISupports) >+ NS_INTERFACE_MAP_ENTRY(nsICompositionStringSynthesizer) >+NS_INTERFACE_MAP_END >+ >+NS_IMPL_CYCLE_COLLECTING_ADDREF(CompositionStringSynthesizer) >+NS_IMPL_CYCLE_COLLECTING_RELEASE(CompositionStringSynthesizer) So, don't use cycle collecting macros. >+ // XXX How should we set false for this on b2g? >+ textEvent.mFlags.mIsSynthesizedForTests = true; Could you actually file a followup about this. >+ >+ nsEventStatus status; Please initialize status. >+ nsresult rv = mWidget->DispatchEvent(&textEvent, status); >+ *aDefaultPrevented = (status == nsEventStatus_eConsumeNoDefault); >+ >+ // Release the widget since it's no longer necessary. >+ mWidget = nullptr; Hmm, odd setup. I would expect that one can use the same CompositionStringSynthesizer several times. So, clear other than mWidget here (which I'm proposing to change to nsWeakPtr mWindow) >+class CompositionStringSynthesizer MOZ_FINAL : >+ public nsICompositionStringSynthesizer >+{ >+public: >+ CompositionStringSynthesizer(nsIWidget* aWidget); >+ ~CompositionStringSynthesizer(); >+ >+ NS_DECL_CYCLE_COLLECTING_ISUPPORTS >+ NS_DECL_CYCLE_COLLECTION_CLASS(CompositionStringSynthesizer) >+ >+ NS_DECL_NSICOMPOSITIONSTRINGSYNTHESIZER >+ >+private: >+ nsCOMPtr<nsIWidget> mWidget; I would prefer if this was nsWeakPtr to window, similar to what DOMWindowUtils has, and then access widget when needed. That would prevent possible cycles. I could take a quick look on the new patch.

Comment on attachment 800654 [details] [diff] [review] part.2 Reimplement synthesizeText() in EventUtils.js and remove nsIDOMWindowUtils.COMPOSITION_ATTR_* from all tests Update nsICompositionString.ATTR_* to use the newer interface name

(In reply to Olli Pettay [:smaug] from comment #14) > So, clear other than mWidget here (which I'm proposing to change to > nsWeakPtr mWindow) Ah, sounds reasonable. > >+class CompositionStringSynthesizer MOZ_FINAL : > >+ public nsICompositionStringSynthesizer > >+{ > >+public: > >+ CompositionStringSynthesizer(nsIWidget* aWidget); > >+ ~CompositionStringSynthesizer(); > >+ > >+ NS_DECL_CYCLE_COLLECTING_ISUPPORTS > >+ NS_DECL_CYCLE_COLLECTION_CLASS(CompositionStringSynthesizer) > >+ > >+ NS_DECL_NSICOMPOSITIONSTRINGSYNTHESIZER > >+ > >+private: > >+ nsCOMPtr<nsIWidget> mWidget; > I would prefer if this was nsWeakPtr to window, similar to what > DOMWindowUtils has, and then access > widget when needed. That would prevent possible cycles. > > I could take a quick look on the new patch. I'm preparing the new patch, I'll take a couple of hours...

Testing... https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=407217ccee86 FYI: widget/tests/test_composition_text_querycontent.xul passes locally.

Comment on attachment 803000 [details] [diff] [review] part.1 Redesign nsIDOMWindowUtils::SendTextEvent() with nsICompositionStringSynthesizer >+NS_IMPL_ISUPPORTS1(CompositionStringSynthesizer, >+ nsICompositionStringSynthesizer) >+ >+CompositionStringSynthesizer::CompositionStringSynthesizer( >+ nsGlobalWindow* aWindow) >+{ >+ nsCOMPtr<nsISupports> supports = do_QueryObject(aWindow); >+ mWindow = do_GetWeakReference(supports); Couldn't the ctor take just nsIDOMWindow* and then here mWindow = do_GetWeakReference(aWindow); >- >+ nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow); >+ NS_ENSURE_TRUE(window, NS_ERROR_NOT_AVAILABLE); >+ >+ nsGlobalWindow* globalWindow = static_cast<nsGlobalWindow*>(window.get()); >+ NS_ADDREF(*aResult = new CompositionStringSynthesizer(globalWindow)); > return NS_OK; Then you could just pass window as param here. >+ /** >+ * Synthesize composition string with given information by dispatching >+ * proper event. 'a proper event' >+ * >+ * If clauses have never been set, this dispatches a commit event. >+ * If clauses are not fill all over the composition string, this throw an filled ....throws, I think >+ * error. >+ * >+ * After dispatching event, this clears all information about composition all the information about the composition

Thank you, smaug! I changed nsGlobalWindow to nsPIDOMWindow for attempting to minimize losing the type information as far as possible.

Comment on attachment 803050 [details] [diff] [review] part.1 Redesign nsIDOMWindowUtils::SendTextEvent() with nsICompositionStringSynthesizer (r=smaug) Roc: Could you sr this? You can see the actual use case of the new APIs in the part.2 patch. https://bugzilla.mozilla.org/attachment.cgi?id=803002&action=diff

Sorry, there are 2 bugs for reusing CompositionStringSynthesizer after DispatchEvent(). DispatchEvent() throws an error if appended clauses or set caret information doesn't match with the composition string. Then, we need to clear the "wrong" composition string information for making the instance reusable.

Comment on attachment 803105 [details] [diff] [review] part.4 Clear the wrong clause information and the caret information at failing to dispatch event Indeed.

Comment on attachment 800655 [details] [diff] [review] part.3 Reimplement CompositionManager.setComposition() and CompositionManager.endComposition() Thank you, roc! Yuan: Could you check this patch? I have no environment to test this patch. So, this patch hasn't been tested yet. I'd like you to check this at review. The binary is here: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=76733cfb2306

Comment on attachment 800655 [details] [diff] [review] part.3 Reimplement CompositionManager.setComposition() and CompositionManager.endComposition() Review of attachment 800655 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/chrome/content/forms.js @@ +1111,3 @@ > } > + } else { > + clauseLens.push(0); The cursor should be put at the end if |clauses| is not passed.

Comment on attachment 800655 [details] [diff] [review] part.3 Reimplement CompositionManager.setComposition() and CompositionManager.endComposition() Review of attachment 800655 [details] [diff] [review]: ----------------------------------------------------------------- r=me. Tested on b2g and only a small issue needs to be fixed. ::: b2g/chrome/content/forms.js @@ +1111,3 @@ > } > + } else { > + clauseLens.push(0); I'm sorry this code doesn't set the cursor position but the first clause length. If |clauses| is not passed, we need to set the length of the first clause to be equal to that of the composition text. Otherwise we'll get the following error: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsICompositionStringSynthesizer.dispatchEvent]" nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: chrome://browser/content/forms.js :: cm_setComposition :: line 1143" data: no] I fixed this error by changing the code to clauseLens.push(len);

Comment on attachment 800655 [details] [diff] [review] part.3 Reimplement CompositionManager.setComposition() and CompositionManager.endComposition() Review of attachment 800655 [details] [diff] [review]: ----------------------------------------------------------------- There is one thing I missed. ::: b2g/chrome/content/forms.js @@ +1127,5 @@ > domWindowUtils.sendCompositionEvent('compositionupdate', text, ''); > } > + let compositionString = domWindowUtils.createCompositionStringSynthesizer(); > + compositionString.setString(text); > + for (var i = 0; i = clauseLens.length; i++) { Also `i = clauseLens.length` should be `i < clauseLens.length`

https://hg.mozilla.org/integration/mozilla-inbound/rev/50a2ec396387 https://hg.mozilla.org/integration/mozilla-inbound/rev/bf88b54e5db4 https://hg.mozilla.org/integration/mozilla-inbound/rev/cbaab4e2562d https://hg.mozilla.org/integration/mozilla-inbound/rev/11cc413aad1c Thank you very much, smaug, roc and Yuan!!!

https://hg.mozilla.org/mozilla-central/rev/50a2ec396387 https://hg.mozilla.org/mozilla-central/rev/bf88b54e5db4 https://hg.mozilla.org/mozilla-central/rev/cbaab4e2562d https://hg.mozilla.org/mozilla-central/rev/11cc413aad1c

This change requires some dev docs. As I see nsIDOMWindowUtils::SendTextEvent() was now removed in favor of the new function createCompositionStringSynthesizer(). Sebastian

Indeed, but I don't have enough time to write the document for now...

Removing koi? since this is in gecko 26 which will be the basis for Firefox OS 1.2.