Closed Bug 911951 Opened 11 years ago Closed 11 years ago

nsIDOMWindowUtils::SendTextEvent() should be able to treat 4 or more clauses

Categories

(Core :: DOM: Events, defect)

Product:
Core
Component:
DOM: Events
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla26

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: dev-doc-needed, inputmethod, jp-critical)

Attachments

(4 files, 6 obsolete files)

part.3 Reimplement CompositionManager.setComposition() and CompositionManager.endComposition()
5.09 KB, patch
xyuan
: review+
Details | Diff | Splinter Review
part.2 Reimplement synthesizeText() in EventUtils.js and remove nsIDOMWindowUtils.COMPOSITION_ATTR_* from all tests
50.23 KB, patch
smaug
: review+
Details | Diff | Splinter Review
part.1 Redesign nsIDOMWindowUtils::SendTextEvent() with nsICompositionStringSynthesizer (r=smaug)
17.37 KB, patch
roc
: superreview+
Details | Diff | Splinter Review
part.4 Clear the wrong clause information and the caret information at failing to dispatch event
1.74 KB, patch
smaug
: review+
Details | Diff | Splinter Review 
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.
blocking-b2g: --- → koi?
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.
Attachment #799973 - Flags: review?(bugs)
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().
Attachment #799974 - Flags: review?(bugs)
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.
Attachment #799973 - Flags: review?(bugs) → review-
Attachment #799974 - Flags: review?(bugs)
> 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.
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.
Attachment #800652 - Flags: review?(bugs)
Attachment #800654 - Flags: review?(bugs)
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.
Attachment #800652 - Flags: review?(bugs) → review-
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.
Attachment #803000 - Flags: review?(bugs)
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
Attachment #803000 - Flags: review?(bugs) → review+
Attachment #803002 - Flags: review?(bugs) → review+
Thank you, smaug! I changed nsGlobalWindow to nsPIDOMWindow for attempting to minimize losing the type information as far as possible.
Attachment #803000 - Attachment is obsolete: true
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
Attachment #803050 - Flags: superreview?(roc)
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.
Attachment #803105 - Flags: review?(bugs)
Comment on attachment 803105 [details] [diff] [review]
part.4 Clear the wrong clause information and the caret information at failing to dispatch event

Indeed.
Attachment #803105 - Flags: review?(bugs) → review+
Attachment #803050 - Flags: superreview?(roc) → superreview+
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
Attachment #800655 - Flags: review?(xyuan)
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);
Attachment #800655 - Flags: review?(xyuan) → review+
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`
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...
Keywords: dev-doc-needed
blocking-b2g: koi? → ---
Removing koi? since this is in gecko 26 which will be the basis for Firefox OS 1.2.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: