974318 - WidgetTextEvent should store text ranges with a class inherited from nsTArray

Status: RESOLVED FIXED

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

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Currently, WidgetTextEvent stores text ranges with raw C array of TextRange and counter for storing length of the array.

At creating nsDOMTextEvent, we create nsPrivateTextRangeList which is refcountable with copying the raw C array.

So, if WidgetTextEvent stores text ranges with a class inherited from nsTArray which is refcountable, we can get rid of nsPrivateTextRangeList and its related code.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Add WidgetTextEvent::mRanges which is an array class of TextRange

First, create refcountable class which is derived from nsTArray of TextRange.

By this change, native composition event handlers can never use stack for allocating the array. However, current code allocates the copy of it in the heap (nsPrivateTextRange). Therefore, it's not matter.

Additionally, this approach avoids making its copy even if somebody wants to store it.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 TextComposition should store TextRangeArray at editor handling text event and nsEditor should refer it

This patch makes TextComposition stores the TextRange array.

Then, nsEditor can refer it directly. This patch makes text event handling simpler with following patches.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.3 Remove nsIPrivateTextRange and nsIPrivateTextEvent

By the part.2 patch, we can get rid of nsPrivateTextRange!

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.4 Remove nsDOMTextEvent

By removing the nsPrivateTextRange, now, nsDOMTextEvent doesn't have specific members. So, we can remove nsDOMTextEvent and use nsDOMUIEvent (for .isChar).

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.5 Use WidgetTextEvent::mRanges on Windows

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.6 Use WidgetTextEvent::mRanges on Mac

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.7 Use WidgetTextEvent::mRanges on GTK

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.8 Use WidgetTextEvent::mRanges on Android

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.9 Use WidgetTextEvent::mRanges on CompositionStringSynthesizer

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.10 Remove WidgetTextEvent::rangeCount and WidgetTextEvent::rangeArray

Finally, remove WidgetTextEvent::rangeCount and WidgetTextEvent::rangeArray.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: All changes by the patches

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8382161 [details] [diff] [review]
part.2 TextComposition should store TextRangeArray at editor handling text event and nsEditor should refer it

Sorry, Ehsan, for the spam. I'll request this review after smaug's review.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

FYI: Qt widget hasn't support IME composition yet. Therefore, there is no change in widget/qt.

Comment 14

[(no longer active)]

(In reply to comment #12)
> Comment on attachment 8382161 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=8382161
> part.2 TextComposition should store TextRangeArray at editor handling text
> event and nsEditor should refer it
> 
> Sorry, Ehsan, for the spam. I'll request this review after smaug's review.

No worries! Sounds good.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8382174 [details] [diff] [review]
part.9 Use WidgetTextEvent::mRanges on CompositionStringSynthesizer

>   var compositionString = utils.createCompositionStringSynthesizer();
>   compositionString.setString(aEvent.composition.string);
>   if (aEvent.composition.clauses[0].length) {
>     for (var i = 0; i < aEvent.composition.clauses.length; i++) {
>-      compositionString.appendClause(aEvent.composition.clauses[i].length,
>-                                     aEvent.composition.clauses[i].attr);
>+      switch (aEvent.composition.clauses[i].attr) {
>+        case compositionString.ATTR_RAWINPUT:
>+        case compositionString.ATTR_SELECTEDRAWTEXT:
>+        case compositionString.ATTR_CONVERTEDTEXT:
>+        case compositionString.ATTR_SELECTEDCONVERTEDTEXT:
>+          compositionString.appendClause(aEvent.composition.clauses[i].length,
>+                                         aEvent.composition.clauses[i].attr);
>+          break;
>+        case 0:
>+          // Ignore dummy clasue for the argument.
clause

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8382161 [details] [diff] [review]
part.2 TextComposition should store TextRangeArray at editor handling text event and nsEditor should refer it

>+  // Retruns the clauses and/or caret range of the composition string.
Returns

> /* ============ protected methods ================== */
>-static SelectionType TextRangeToSelection(int aTextRangeType)
>+static SelectionType
>+ToSelectionType(uint32_t aTextRangeType)

will continue reviewing here...)

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #16)
> Comment on attachment 8382161 [details] [diff] [review]
> part.2 TextComposition should store TextRangeArray at editor handling text
> event and nsEditor should refer it
> 
> >+  // Retruns the clauses and/or caret range of the composition string.
> Returns
> 
> > /* ============ protected methods ================== */
> >-static SelectionType TextRangeToSelection(int aTextRangeType)
> >+static SelectionType
> >+ToSelectionType(uint32_t aTextRangeType)
> 
> will continue reviewing here...)

I'll request review of editor part to Ehsan. So, you can review the part roughly ;-)

Thanks for a lot of review!

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8382168 [details] [diff] [review]
part.5 Use WidgetTextEvent::mRanges on Windows

WidgetTextEvent::mRanges is nsRefPtr<TextRangeArray>, TextRangeArray is derived from nsAutoTArray<TextRange, 10>.

When there is composition string, this stores the clause ranges and/or caret position.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8382170 [details] [diff] [review]
part.6 Use WidgetTextEvent::mRanges on Mac

WidgetTextEvent::mRanges is nsRefPtr<TextRangeArray>, TextRangeArray is derived from nsAutoTArray<TextRange, 10>.

When there is composition string, this stores the clause ranges and/or caret position.

FYI: TextRangeArray is conflict with Mac's native type name. Therefore, this patch specifies mozilla:: explicitly.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8382171 [details] [diff] [review]
part.7 Use WidgetTextEvent::mRanges on GTK

WidgetTextEvent::mRanges is nsRefPtr<TextRangeArray>, TextRangeArray is derived from nsAutoTArray<TextRange, 10>.

When there is composition string, this stores the clause ranges and/or caret position.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.8 Use WidgetTextEvent::mRanges on Android

WidgetTextEvent::mRanges is nsRefPtr<TextRangeArray>, TextRangeArray is derived from nsAutoTArray<TextRange, 10>.

When there is composition string, this stores the clause ranges and/or caret position.

Comment 22

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8384528 [details] [diff] [review]
part.8 Use WidgetTextEvent::mRanges on Android

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

Thanks! Looks great!

::: widget/android/nsWindow.cpp
@@ +178,5 @@
>      mIMEMaskEventsCount(1), // Mask IME events since there's no focus yet
>      mIMEUpdatingContext(false),
>      mIMESelectionChanged(false)
>  {
> +    mIMERanges = new TextRangeArray();

Add it to the initializer list above? i.e. "mIMERanges(new TextRangeArray()),"

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8382161 [details] [diff] [review]
part.2 TextComposition should store TextRangeArray at editor handling text event and nsEditor should refer it


>+    // Caret needs special handling since its length may be 0 and if it's not
>+    // specified explicitly, we need to handle ourselves later.
handle it

>+    if (textRange.mRangeType == NS_TEXTRANGE_CARETPOSITION) {
>+      NS_ASSERTION(!setCaret, "The ranges already has caret position");
>+      NS_ASSERTION(!textRange.Length(), "nsEditor doesn't support wide caret");
Should you use MOZ_ASSERT here

>+    // If the clause length is 0, it's should be a bug.
>+    if (!textRange.Length()) {
>+      NS_WARNING("Any clauses must not be empty");
>+      continue;
>+    }
So should we just warn or could we assert?



A bit tricky to review since the patch changes coding style and makes us use TextRangeArray.

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #23)
> >+    if (textRange.mRangeType == NS_TEXTRANGE_CARETPOSITION) {
> >+      NS_ASSERTION(!setCaret, "The ranges already has caret position");
> >+      NS_ASSERTION(!textRange.Length(), "nsEditor doesn't support wide caret");
> Should you use MOZ_ASSERT here

No, we hit the assertions in the patch :-(

We need to fix the bugs in other bugs... And the second assertion is really layout's fault. We need to support wide caret, see bug 226784 and bug 38415 comment 41.

So, I won't change the assertion in this bug, is it OK?

> >+    // If the clause length is 0, it's should be a bug.
> >+    if (!textRange.Length()) {
> >+      NS_WARNING("Any clauses must not be empty");
> >+      continue;
> >+    }
> So should we just warn or could we assert?

So, it's risky too... I think that we should try it on next cycle because even if it causes regression, I don't have much time for fixing them.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8382161 [details] [diff] [review]
part.2 TextComposition should store TextRangeArray at editor handling text event and nsEditor should refer it

Ehsan, could you review the editor part?

We don't need to use nsIPrivateTextRange(List) anymore because WidgetTextEvent has refcountable array of TextRange. This patch makes TextComposition store it at handling the event in focused editor. Then, nsEditor doesn't need to manage the list itself.

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I filed bug 979112 for wide caret support.

Comment 27

[(no longer active)]

Can you please upload a diff -w version of the patch?  There seems to be some code movement going on...

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 (-w)

Sure, but this might not help you...

Comment 29

[(no longer active)]

Comment on attachment 8382161 [details] [diff] [review]
part.2 TextComposition should store TextRangeArray at editor handling text event and nsEditor should refer it

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

r=me with the below fixed.

::: editor/libeditor/base/IMETextTxn.cpp
@@ +233,5 @@
> +      NS_ASSERTION(!textRange.Length(), "nsEditor doesn't support wide caret");
> +      // NOTE: If the caret position is larger than max length of the editor
> +      //       content, this may fail.
> +      rv = selection->Collapse(mElement, mOffset + textRange.mStartOffset);
> +      setCaret = NS_SUCCEEDED(rv);

Here you're changing the meaning of the code if setCaret is set to true in the first iteration but to false in the second one.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

(Ok for not adding assertions then)

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ea20343a2ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/5abcc356d54a
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d78ee924b64
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a605f80e8da
https://hg.mozilla.org/integration/mozilla-inbound/rev/a56a87f85fbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/865a904ad55b
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ef0f64644b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/31fd85d6ad5a
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ecb31d619af
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d5c6f6a8f81

Thank you!

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3ea20343a2ce
https://hg.mozilla.org/mozilla-central/rev/5abcc356d54a
https://hg.mozilla.org/mozilla-central/rev/3d78ee924b64
https://hg.mozilla.org/mozilla-central/rev/5a605f80e8da
https://hg.mozilla.org/mozilla-central/rev/a56a87f85fbe
https://hg.mozilla.org/mozilla-central/rev/865a904ad55b
https://hg.mozilla.org/mozilla-central/rev/0ef0f64644b2
https://hg.mozilla.org/mozilla-central/rev/31fd85d6ad5a
https://hg.mozilla.org/mozilla-central/rev/8ecb31d619af
https://hg.mozilla.org/mozilla-central/rev/5d5c6f6a8f81

Comment 33

[Jacek Caban]

I landed a trivial GCC (mingw) fixup:

https://hg.mozilla.org/integration/mozilla-inbound/rev/cdb9a5b0d4b9

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cdb9a5b0d4b9