Dispatch compositionupdate from TextComposition rather than widget

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Blocks 1 bug, {inputmethod})

Trunk
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments)

8.89 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.30 KB, patch
jchen
: review+
Details | Diff | Splinter Review
1.69 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
2.81 KB, patch
karlt
: review+
Details | Diff | Splinter Review
1.62 KB, patch
emk
: review+
Details | Diff | Splinter Review
4.06 KB, patch
emk
: review+
Details | Diff | Splinter Review
2.64 KB, patch
xyuan
: review+
Details | Diff | Splinter Review
2.19 KB, patch
smaug
: review+
Details | Diff | Splinter Review
70.41 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Spinning out from bug 960871.

Dispatching compositionupdate event needs to check the composition string is actually updating or not.

It's possible to check in TextComposition. Therefore, it's very redundant doing it in widget level.
A compositionupdate should be dispatched when text event changes composition string. In other words, text event doesn't change composition string, compositionupdate event shouldn't be dispatched.

This spec is annoying for every widget because they need to store last dispatched composition string and compare WidgetTextEvent::theText with it before dispatching the text event.

This is redundant and error prone. Actually, looks like that Android widget dispatches compositionupdate event every time.

For avoiding this issue, widget should only dispatch text events during composition. And TextComposition should dispatch compositionupdate if it's necessary. Then, we can guarantee that compositionupdate events are dispatched correctly on all platforms.

First, this patch ignores compositionupdate events dispatched by widget. And call MaybyDispatchCompositionUpdate() before dispatching text event.
Attachment #8495005 - Flags: review?(bugs)
By part.2 - part.7, all compositionupdate dispatchers have gone, therefore, we can remove NS_COMPOSITION_UPDATE handlers from ESM since it maybe redirects compositionupdate events coming from widget to another process.

Similarly, IMEStateManager::DispatchCompositionEvent() doesn't need to handle NS_COMPOSITION_UPDATE because it's called by PresShell::HandleEventInternal() for dispatching composition events which come from widget into the tree.
Attachment #8495012 - Flags: review?(bugs)
Attachment #8495012 - Flags: review?(bugs) → review+
Comment on attachment 8495013 [details] [diff] [review]
part.9 Remove compositionupdate event dispatchers from all tests

>     editor.addEventListener("compositionstart", handler, true);
>     editor.addEventListener("compositionend", handler, true);
>-    editor.addEventListener("compositionupdate", handler, true);
Why do we want to remove this?

>     editor.removeEventListener("compositionstart", handler, true);
>     editor.removeEventListener("compositionend", handler, true);
>-    editor.removeEventListener("compositionupdate", handler, true);
And this?


We do want to still somehow test that compositionupdate is dispatched.
Please explain, and possibly fix in a follow up patch (in this bug).
Attachment #8495013 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #10)
> Comment on attachment 8495013 [details] [diff] [review]
> part.9 Remove compositionupdate event dispatchers from all tests
> 
> >     editor.addEventListener("compositionstart", handler, true);
> >     editor.addEventListener("compositionend", handler, true);
> >-    editor.addEventListener("compositionupdate", handler, true);
> Why do we want to remove this?
> 
> >     editor.removeEventListener("compositionstart", handler, true);
> >     editor.removeEventListener("compositionend", handler, true);
> >-    editor.removeEventListener("compositionupdate", handler, true);
> And this?
> 
> 
> We do want to still somehow test that compositionupdate is dispatched.
> Please explain, and possibly fix in a follow up patch (in this bug).

Oh, you're right. It's just my mistake! I'll remove these changes from the patch. This patch wants to remove just the dispatcher of compositionupdate event in all tests.
Comment on attachment 8495005 [details] [diff] [review]
part.1 TextComposition should dispatch compositionupdate event automatically if text event changes composition string


>   if (aType.EqualsLiteral("compositionstart")) {
>     msg = NS_COMPOSITION_START;
>   } else if (aType.EqualsLiteral("compositionend")) {
>     msg = NS_COMPOSITION_END;
>   } else if (aType.EqualsLiteral("compositionupdate")) {
>-    msg = NS_COMPOSITION_UPDATE;
>+    // 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.
Could we at least warn here using NS_WARNING
Attachment #8495005 - Flags: review?(bugs) → review+
Comment on attachment 8495006 [details] [diff] [review]
part.2 Remove compositionupdate dispatchers in nsWindow of Android

compoisitonupdate is dispatched by XP part (mozilla::TextComposition) if it's necessary and ignored even if widget dispatched.

So, we should remove compositionupdate dispatcher from widget.
Attachment #8495006 - Flags: review?(nchen)
Comment on attachment 8495007 [details] [diff] [review]
part.3 Remove compositionupdate dispatchers in TextInputHandler of Cocoa

compoisitonupdate is dispatched by XP part (mozilla::TextComposition) if it's necessary and ignored even if widget dispatched.

So, we should remove compositionupdate dispatcher from widget.
Attachment #8495007 - Flags: review?(smichaud)
Comment on attachment 8495008 [details] [diff] [review]
part.4 Remove compositionupdate dispatchers in nsGtkIMModule of GTK

compoisitonupdate is dispatched by XP part (mozilla::TextComposition) if it's necessary and ignored even if widget dispatched.

So, we should remove compositionupdate dispatcher from widget.
Attachment #8495008 - Flags: review?(karlt)
Comment on attachment 8495009 [details] [diff] [review]
part.5 Remove compositionupdate dispatchers in nsIMM32Handler of Windows

compoisitonupdate is dispatched by XP part (mozilla::TextComposition) if it's necessary and ignored even if widget dispatched.

So, we should remove compositionupdate dispatcher from widget.
Attachment #8495009 - Flags: review?(VYV03354)
Comment on attachment 8495010 [details] [diff] [review]
part.6 Remove compositionupdate dispatchers in nsTextStore of Windows

ditto
Attachment #8495010 - Flags: review?(VYV03354)
Comment on attachment 8495011 [details] [diff] [review]
part.7 Remove compositionupdate dispatchers in forms.js of B2G

compoisitonupdate is dispatched by XP part (mozilla::TextComposition) if it's necessary and ignored even if widget dispatched.

So, we should remove compositionupdate dispatcher from widget.

FYI: I got flame, but I've not succeeded to build and test this patch, so, this patch isn't tested. If you can, I'd like you check the behavior (not causes JS error and can input text via IME).
Attachment #8495011 - Flags: review?(xyuan)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 9/20 - 9/23, JST) from comment #18)
> Comment on attachment 8495011 [details] [diff] [review]
> part.7 Remove compositionupdate dispatchers in forms.js of B2G
> FYI: I got flame, but I've not succeeded to build and test this patch, so,
> this patch isn't tested. If you can, I'd like you check the behavior (not
> causes JS error and can input text via IME).

Okay, I probably succeeded to build and flash my flame with the patch. And looks like it works!
Attachment #8495007 - Flags: review?(smichaud) → review+
Attachment #8495006 - Flags: review?(nchen) → review+
Comment on attachment 8495009 [details] [diff] [review]
part.5 Remove compositionupdate dispatchers in nsIMM32Handler of Windows

Is mLastDispatchedCompositionString still needed?
Attachment #8495009 - Flags: review?(VYV03354) → review+
Comment on attachment 8495010 [details] [diff] [review]
part.6 Remove compositionupdate dispatchers in nsTextStore of Windows

Same as above. (i.e. Is mLastData still useful?)
Attachment #8495010 - Flags: review?(VYV03354) → review+
(In reply to Masatoshi Kimura [:emk] from comment #20)
> Comment on attachment 8495009 [details] [diff] [review]
> part.5 Remove compositionupdate dispatchers in nsIMM32Handler of Windows
> 
> Is mLastDispatchedCompositionString still needed?

Yes, this is used for setting data value of compositionend. However, I guess that it will be removed if compositionend event is also dispatched from TextComposition automatically.

(In reply to Masatoshi Kimura [:emk] from comment #21)
> Comment on attachment 8495010 [details] [diff] [review]
> part.6 Remove compositionupdate dispatchers in nsTextStore of Windows
> 
> Same as above. (i.e. Is mLastData still useful?)

I checked this again. I guess I can get rid of this in this bug. Good catch.
Attachment #8495008 - Flags: review?(karlt) → review+
Comment on attachment 8495011 [details] [diff] [review]
part.7 Remove compositionupdate dispatchers in forms.js of B2G

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

How can we change the composition text after starting composition?
Currently we may call setComposition() more than one times before calling endComposition(). The patch prevents a successive setComposition() calling to change the composition text.
Please make sure the patch doesn't break the following user case:

1. calling setComposition('a') to start composition and set the composition text to 'a'
2. calling setComposition('ab') to change the composition text to 'ab'
3. calling endComposition('ab') to end composition and commit 'ab'
Attachment #8495011 - Flags: review?(xyuan) → review-
Comment on attachment 8495011 [details] [diff] [review]
part.7 Remove compositionupdate dispatchers in forms.js of B2G

(In reply to Yuan Xulei [:yxl] (PTO until Oct 8) from comment #23)
> How can we change the composition text after starting composition?

From forms.js, it's possible to use nsICompositionStringSynthesizer. See here:
http://mxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.js#1244

Note that nsEditor doesn't handle compositionupdate event. It's just a notification event for web apps immediately before modifying composition string. Instead, DOM text event (not defined by any web stnadards) is handled by ns(Plaintext)Editor and it causes the composition string. nsICompositionStringSynthesizer is the dispatcher of a DOM text event.

What this bug makes is that compositionupdate events which don't affect our behavior dispatched by TextComposition class when it receives DOM text event and composition string is actually changed.

So, removing compositionupdate dispatcher from form.js doesn't change any behavior. The removed job is performed by TextComposition automatically. See part.1 patch if you want to confirm what I actually does in this bug.

> Currently we may call setComposition() more than one times before calling
> endComposition(). The patch prevents a successive setComposition() calling
> to change the composition text.
> Please make sure the patch doesn't break the following user case:
> 
> 1. calling setComposition('a') to start composition and set the composition
> text to 'a'
> 2. calling setComposition('ab') to change the composition text to 'ab'
> 3. calling endComposition('ab') to end composition and commit 'ab'

I confirmed on my flame:

1. Activate built-in Chinese IME.
2. Type 'm'
3. Type 'a'
4. Tap the editor

Then, "ma" is committed. This must be right result.
Attachment #8495011 - Flags: review- → review?(xyuan)
Comment on attachment 8495011 [details] [diff] [review]
part.7 Remove compositionupdate dispatchers in forms.js of B2G

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

Thanks for clarification.
Attachment #8495011 - Flags: review?(xyuan) → review+
Depends on: 1081993
You need to log in before you can comment on or make changes to this bug.