960871 - Get rid of WidgetTextEvent and WidgetCompositionEvent should take over the job

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

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

Currently, widget sends compositionupdate event and text event separately.

compositionupdate event is fired before text event only when composition string is updated. This check is checked by each widget.

text event is fired every composition change including only clause/caret information change. nsEditor handles only this event.


I'm planning that:

* WidgetCompositionEvent would have text ranges which is managed by refcounting. (This reduces run-time cost)
* widget would never dispatch text event.
* TextComposition would dispatch text event for nsEditor.

After this bug, I'll get rid of WidgetTextEvent.

Comment 1

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

By the changes of bug 975383, TextComposition will handle only text event. And compositionupdate event is dispatched from TextComposition when a text event changes composition string.

Therefore, what we should do here are:

1. WidgetCompositionEvent should have the text ranges.
2. NS_TEXT_TEXT should be replaced with NS_COMPOSITION_UPDATE_INTERNAL.
3. WidgetCompositionEvent should be used for the new message instead of WidgetTextEvent.
4. Remove WidgetTextEvent.

Then, all composition changes are managed by only composition events.

Comment 2

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

Attachment: part.1 Rename WidgetTextEvent::theText to WidgetTextEvent::mData

First of all, for making following patches smaller and easier to read, we should make WidgetCompositionEvent.data and WidgetTextEvent.theText same name.

In the modern coding style, they should be mData. Therefore, this patch renames WidgetTextEvent::theText to WidgetTextEvent::mData.

Comment 3

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

Attachment: part.2 Rename WidgetCompositionEvent::data to WidgetCompositionEvent::mData

This patch renames WidgetCompositionEvent::data to WidgetCompositionEvent::mData.

Comment 4

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

Attachment: part.3 Remove WidgetTextEvent::isChar since it's always false on all platforms

WidgetTextEvent has isChar for UIEvent.isChar attribute. However, it's never set true and it's not implemented by WidgetCompositionEvent. So, we can just remove it.

NOTE: UIEvent.isChar isn't defined by any web standards.

Comment 5

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

Attachment: part.4 Rename NS_TEXT_TEXT to NS_COMPOSITION_CHANGE and fix comments which mention text events

NS_TEXT_TEXT event should be renamed to NS_COMPOSITION_CHANGE because it notifies editor of any composition string change including clause information.

Note that the difference between NS_COMPOSITION_UPDATE and NS_COMPOSITION_CHANGE is that NS_COMPOSITION_UPDATE is fired *only* when composition *string* is changed. On the other hand, NS_COMPOSITION_CHANGE (NS_TEXT_TEXT) is fired even when composition string is *not* changed. It may be fired for notifying editor of clause boundary change and/or caret position change. Additionally, DOM compositionupdate which is caused by NS_COMPOSITION_UPDATE should be dispatched *before* actually composition string is changed. Therefore, we need to keep dispatching two events for DOM compositionupdate event and an internal event for notifying nsEditor of a change of composition string information.

Comment 6

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

And we should call NS_COMPOSITION_CHANGE event as "compositionchange" event internally. If we need to document its DOM event, then, we should call it as "DOM text event". The "text" event might be handled by some addons, therefore, we shouldn't change the name for the compatibility but I don't like NS_COMPOSITION_TEXT for its name. Because "text" event isn't clear what the event notifies somebody of.

Comment 7

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

Attachment: part.5 Copy mRanges and related methods from WidgetTextEvent to WidgetCompositionEvent

Before changing from WidgetTextEvent to WidgetCompositionEvent for NS_COMPOSITION_CHANGE event class, we need to duplicate all members of WidgetTextEvent to WidgetCompositionEvent.

Comment 8

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

Attachment: part.6 Use WidgetCompositionEvent for NS_COMPOSITION_CHANGE instead of WidgetTextEvent

This patch changes all WidgetTextEvent instances to WidgetCompositionEvent's. The variable names of them will be renamed later.

This patch makes some code which handle both WidgetTextEvent and WidgetCompositionEvent simpler!

Comment 9

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

I wonder, document.createEvent("textevent") should keep creating DOM UIEvent rather than CompositionEvent?

Comment 10

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

Attachment: part.7 Get rid of WidgetTextEvent

Just removes WidgetTextEvent completely.

Comment 11

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

Attachment: part.8 Rename variable names of NS_COMPOSITION_CHANGE event

Renaming variables of WidgetTextEvent to compositionChangeEvent, compChangeEvent or something. I used similar name around the code.

Comment 12

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

Attachment: part.9 Rename methods and classes which handled WidgetTextEvent

Renaming method names which handled WidgetTextEvent.

Comment 13

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

Attachment: part.10 Some methods which took both WidgetCompositionEvent and WidgetTextEvent should take only WidgetCompositionEvent

Some methods have taken both WidgetCompositionEvent and WidgetTextEvent by its argument. This patch changes such method's argument from WidgetGUIEvent to WidgetCompositionEvent. This is clearer and reduces the number of virtual calls of AsCompositionEvent().

Comment 14

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

Attachment: part.11 Rename EventUtils.synthesizeText() to EventUtils.synthesizeCompositionChange()

Renaming EventUtils.synthesizeText() to EventUtils.synthesizeCompositionChange().

Comment 15

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

Attachment: part.12 Get rid of TextEvent() of PBrowser

Now, we can get rid of PBrowser.TextEvent(). Instead, ESM should use SendCompositionEvent() for NS_COMPOSITION_CHANGE.

Comment 16

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

Basically, these patches almost don't change actual behavior. The only one difference is that DOM event which is caused by NS_COMPOSITION_CHANGE is changed from UIEvent to CompositionEvent. I believe that this must not cause any problems with actual addons. And also I believe that it does makes sense that WidgetCompositionEvent always causes DOM CompositionEvent even if it's a non-standard event. (With this change, text event handler can access its data attribute.)

Comment 17

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

Comment on attachment 8499334 [details] [diff] [review]
part.3 Remove WidgetTextEvent::isChar since it's always false on all platforms

lovely :/
I wonder if isChar was ever really used. It is ancient code.

Comment 18

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

Comment on attachment 8499336 [details] [diff] [review]
part.4 Rename NS_TEXT_TEXT to NS_COMPOSITION_CHANGE and fix comments which mention text events

>   } else if (aType.EqualsLiteral("compositionupdate")) {
>     // Now we don't support manually dispatching composition update with this
>-    // API.  compositionupdate is dispatched when text event modifies
>-    // composition string automatically.  For backward compatibility, this
>-    // shouldn't return error in this case.
>+    // API.  compositionupdate is dispatched when compositionchange event
The event is still "text".

> NON_IDL_EVENT(text,
>-              NS_TEXT_TEXT,
>+              NS_COMPOSITION_CHANGE,
This adds some inconsistency. "text" event, but NS_COMPOSITION_CHANGE message.
Please add comment that "text" is a legacy event.

We should add some telemetry to check if anyone in the web is actually using "text", and if not, stop
exposing it there.

>@@ -167,21 +167,26 @@
> #define NS_MUTATION_END                      (NS_MUTATION_START+6)
> 
> #define NS_USER_DEFINED_EVENT         2000
>  
> // composition events
> #define NS_COMPOSITION_EVENT_START    2200
> #define NS_COMPOSITION_START          (NS_COMPOSITION_EVENT_START)
> #define NS_COMPOSITION_END            (NS_COMPOSITION_EVENT_START + 1)
>+// NS_COMPOSITION_UPDATE is the message for DOM compositionupdate event.
>+// This event should NOT be dispatched from widget since it's dispatched
s/it's/it will be/

>+// by mozilla::TextComposition automatically if NS_COMPOSITION_CHANGE event
>+// will change composition string.
s/will change/changes/

>+// NS_COMPOSITION_CHANGE is the message for representing a change of
>+// composition string.  This should be dispatched from widget even if
>+// composition string isn't changed but the ranges are changed.  This causes
>+// cause DOM text event which is non-standard DOM event.
perhaps s/text event/"text" event/

Comment 19

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

Comment on attachment 8499339 [details] [diff] [review]
part.5 Copy mRanges and related methods from WidgetTextEvent to WidgetCompositionEvent

>+      aResult->mRanges = new mozilla::TextRangeArray();
>+      if (!aResult->mRanges) {
'new' is infallible by default, so no need for the null check.

Comment 20

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

Comment on attachment 8499344 [details] [diff] [review]
part.7 Get rid of WidgetTextEvent

So great.

Comment 21

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

Comment on attachment 8499345 [details] [diff] [review]
part.8 Rename variable names of NS_COMPOSITION_CHANGE event

>-  WidgetCompositionEvent textEvent(true, NS_COMPOSITION_CHANGE, widget);
>-  textEvent.time = PR_IntervalNow();
>-  textEvent.mData = mString;
>+  WidgetCompositionEvent compChangeEvent(true, NS_COMPOSITION_CHANGE, widget);
Perhaps this could be compositionChangeEvent or changeEvent or compositionChange

> TextComposition::EditorWillHandleTextEvent(
>-                   const WidgetCompositionEvent* aTextEvent)
>+                   const WidgetCompositionEvent* aCompChangeEvent)
aChangeEvent or aCompositionChangeEvent?
>     TextEventHandlingMarker(TextComposition* aComposition,
>-                            const WidgetCompositionEvent* aTextEvent)
>+                            const WidgetCompositionEvent* aCompChangeEvent)
ditto

>-  void EditorWillHandleTextEvent(const WidgetCompositionEvent* aTextEvent);
>+  void EditorWillHandleTextEvent(
>+         const WidgetCompositionEvent* aCompChangeEvent);
ditto

> nsPlaintextEditor::UpdateIMEComposition(nsIDOMEvent* aDOMTextEvent)
> {
>   NS_ABORT_IF_FALSE(aDOMTextEvent, "aDOMTextEvent must not be nullptr");
> 
>-  WidgetCompositionEvent* widgetTextEvent =
>+  WidgetCompositionEvent* compChangeEvent =
>     aDOMTextEvent->GetInternalNSEvent()->AsCompositionEvent();
and here

>-    WidgetCompositionEvent textEvent(true, NS_COMPOSITION_CHANGE, this);
>-    InitEvent(textEvent, nullptr);
>-    textEvent.mData = mIMEComposingText;
>-    DispatchEvent(&textEvent);
>+    WidgetCompositionEvent compChangeEvent(true, NS_COMPOSITION_CHANGE, this);
and here


and elsewhere.

Perhaps use compositionChange name consistency everywhere. That fixed, r+

Comment 22

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

Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/aad726477799
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b622a7464cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2eebbc42474
https://hg.mozilla.org/integration/mozilla-inbound/rev/279373ac52ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/97aa7fef7adc
https://hg.mozilla.org/integration/mozilla-inbound/rev/5216b7b1e4e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd15674419d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/93374aaa8f6e
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f5abfd3505c
https://hg.mozilla.org/integration/mozilla-inbound/rev/52273860a2cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a965889d97e
https://hg.mozilla.org/integration/mozilla-inbound/rev/f17271ef08d3

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aad726477799
https://hg.mozilla.org/mozilla-central/rev/3b622a7464cc
https://hg.mozilla.org/mozilla-central/rev/d2eebbc42474
https://hg.mozilla.org/mozilla-central/rev/279373ac52ff
https://hg.mozilla.org/mozilla-central/rev/97aa7fef7adc
https://hg.mozilla.org/mozilla-central/rev/5216b7b1e4e3
https://hg.mozilla.org/mozilla-central/rev/bd15674419d1
https://hg.mozilla.org/mozilla-central/rev/93374aaa8f6e
https://hg.mozilla.org/mozilla-central/rev/4f5abfd3505c
https://hg.mozilla.org/mozilla-central/rev/52273860a2cd
https://hg.mozilla.org/mozilla-central/rev/6a965889d97e
https://hg.mozilla.org/mozilla-central/rev/f17271ef08d3

Comment 24

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

Modified for this change:
https://developer.mozilla.org/en-US/docs/Mozilla/IME_handling_guide