Closed Bug 1200980 Opened 8 years ago Closed 8 years ago

[e10s][TSF] Candidate window is sometimes not positioned properly because IMEContentObserver sometimes fails to notify IME of selection change

Categories

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

Product:
Core
Component:
DOM: UI Events & Focus Handling
Platform:
x86
Windows
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla43
Tracking Flags:
Tracking Status
e10s + ---
firefox43 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod)

Attachments

(5 files)

part.1 Log the behavior of IMEContentObserver for debugging
26.63 KB, patch
smaug
: review+
Details | Diff | Splinter Review
part.2 QueryContentEvent should be handled via IMEContentObserver if there is an instance of it
14.25 KB, patch
smaug
: review+
Details | Diff | Splinter Review
part.3 IMEContentObserver should use its selection cache at handling NS_QUERY_SELECTED_TEXT
5.82 KB, patch
smaug
: review+
Details | Diff | Splinter Review
part.4 nsPlaintextEditor should notify editor observers of the end of edit action when NS_COMPOSITION_CHANGE isn't followed by NS_COMPOSITION_END
5.28 KB, patch
smaug
: review+
Details | Diff | Splinter Review
part.5 Fix window_composition_text_querycontent.xul for the new input event behavior
11.09 KB, patch
arai
: review+
smaug
: review+
Details | Diff | Splinter Review 
When I type text in reply field of twitter.com, candidate window (or other TIP windows) is sometimes not positioned properly. As far as I debugged, selection is changed much odd before expected selection is set. Then, IMEContentObserver fails to notify IME of selection change due to new selection range isn't changed from cached selection range but the decision is wrong.
I understand the half of what occurs.

widget can use NS_QUERY_SELECTED_TEXT whenever. In following case, selection cached by widget becomes different from actual selection:

1. IMEContentObserver notifies IME of selection change when selection change is A.
2. selection is changed to B, but IMEContentObserver cannot send a selection change notification at this time.
3. widget uses NS_QUERY_SELECTED_TEXT, then, B is cached.
4. selection is changed again to A. After that, IMEContentObserver becomes possible to send a selection change notification. However, in this case, the selection isn't changed since #1. Therefore, IMEContentObserver doesn't send selection change notification.

So, I think that when NS_QUERY_SELECTED_TEXT is handled, IMEContentObserver should be notified the fact and the result.

# The big mystery is, why selection B is caused with twitter. The selection range sometimes may be computed with whole contents of the document...
Let's log the behavior of IMEContentObserver for debugging.

Hmm, smaug, if you don't have much time, do you have some idea who can review IMEContentObserver?
Flags: needinfo?(bugs)
For preparing to make IMEContentObserver handle WidgetQueryContentEvent, EventStateManager should send events to IMEContentObserver if there is an instance of it.
If IMEContentObserver caches selection range and the query should be handled with native line breakers, IMEContentObserver should handle NS_QUERY_SELECTED_TEXT instead of ContentEventHandler. Then, we can guarantee that the result of NS_QUERY_SELECTED_TEXT and the latest NOTIFY_IME_OF_SELECTTION_CHANGE's data are always same.
Some IME may set composition string empty but not end the composition. In such case, IMEContentObserver doesn't receive EditAction() of nsIEditActionListener. That causes orange in widget/test_composition_text_querycontent.xul. So, nsPlaintextEditor should notify nsIEditActionListener instances of EditAction() if NS_COMPOSITION_CHANGE isn't followed by NS_COMPOSITION_END. (I.e., NS_COMPOSITION_CHANGE whose mData is empty string should cause notifying nsIEditActionListener of EditAction() unless it's dispatched by TextComposition when it receives NS_COMPOSITION_COMMIT(_AS_IS).)
By the change of part.4, input event should be fired when composition string is changed actually unless if it's not followed by compositionend.

So, your test causes new orange. And I should add some tests for checking that no redundant input events fired immediately before compositionend.
Attachment #8656597 - Flags: review?(arai.unmht)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #10)
> Created attachment 8656595 [details] [diff] [review]
> part.4 nsPlaintextEditor should notify editor observers of the end of edit
> action when NS_COMPOSITION_CHANGE isn't followed by NS_COMPOSITION_END
> 
> Some IME may set composition string empty but not end the composition. In
> such case, IMEContentObserver doesn't receive EditAction() of
> nsIEditActionListener. That causes orange in
> widget/test_composition_text_querycontent.xul. So, nsPlaintextEditor should
> notify nsIEditActionListener instances of EditAction() if
> NS_COMPOSITION_CHANGE isn't followed by NS_COMPOSITION_END. (I.e.,
> NS_COMPOSITION_CHANGE whose mData is empty string should cause notifying
> nsIEditActionListener of EditAction() unless it's dispatched by
> TextComposition when it receives NS_COMPOSITION_COMMIT(_AS_IS).)

Additionally, I found that when NS_COMPOSITION_CHANGE is handled by nsPlaintextEditor, BeforeEdit() is called twice. One of the calls is redundant. So, it should be ignored with warning for now.

And the reason why nsPlaintextEditor needs not to call EditAction() if it's NS_COMPOSITION_END is that input event shouldn't be fired immediately before compositionend event since input event follows compositionend event. So, we need to kill the redundant input event which caused some trouble (and doesn't make sense firing two times both before and after compositionend).
Which all patches need reviews, and how soon? I'll reopen my review queue by the end of the week
(I just have a bad habit to review patches whenever there is something in the queue, which means I end up writing my own patches rather slowly ;) ).
Flags: needinfo?(bugs)
> I'll reopen my review queue by the end of the week

Okay, that's fine to me. I wait next week.
Comment on attachment 8656597 [details] [diff] [review]
part.5 Fix window_composition_text_querycontent.xul for the new input event behavior

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

Thank you for fixing the test :)
Those changes sound reasonable.  I'd ask additional review from smaug (maybe next week?).
Attachment #8656597 - Flags: review?(arai.unmht) → review+
Thank you for the review. Okay, I'll request it when he gets much time.
Comment on attachment 8656581 [details] [diff] [review]
part.1 Log the behavior of IMEContentObserver for debugging

Let's log the behavior of IMEContentObserver for debugging.

And I found some bugs with the log. I'll file the bug after this bug.
Attachment #8656581 - Flags: review?(bugs)
Comment on attachment 8656588 [details] [diff] [review]
part.2 QueryContentEvent should be handled via IMEContentObserver if there is an instance of it

For preparing to make IMEContentObserver handle WidgetQueryContentEvent, EventStateManager should send events to IMEContentObserver if there is an instance of it.
Attachment #8656588 - Flags: review?(bugs)
Comment on attachment 8656592 [details] [diff] [review]
part.3 IMEContentObserver should use its selection cache at handling NS_QUERY_SELECTED_TEXT

If IMEContentObserver caches selection range and the query should be handled with native line breakers, IMEContentObserver should handle NS_QUERY_SELECTED_TEXT instead of ContentEventHandler. Then, we can guarantee that the result of NS_QUERY_SELECTED_TEXT and the latest NOTIFY_IME_OF_SELECTTION_CHANGE's data are always same.
Attachment #8656592 - Flags: review?(bugs)
Comment on attachment 8656595 [details] [diff] [review]
part.4 nsPlaintextEditor should notify editor observers of the end of edit action when NS_COMPOSITION_CHANGE isn't followed by NS_COMPOSITION_END

Some IME may set composition string empty but not end the composition. In such case, IMEContentObserver doesn't receive EditAction() of nsIEditActionListener. That causes orange in widget/test_composition_text_querycontent.xul. So, nsPlaintextEditor should notify nsIEditActionListener instances of EditAction() if NS_COMPOSITION_CHANGE isn't followed by NS_COMPOSITION_END. (I.e., NS_COMPOSITION_CHANGE whose mData is empty string should cause notifying nsIEditActionListener of EditAction() unless it's dispatched by TextComposition when it receives NS_COMPOSITION_COMMIT(_AS_IS).)
Attachment #8656595 - Flags: review?(bugs)
Comment on attachment 8656597 [details] [diff] [review]
part.5 Fix window_composition_text_querycontent.xul for the new input event behavior

By the change of part.4, input event should be fired when composition string is changed actually unless if it's not followed by compositionend.

So, Fujisawa-san's test causes new orange. And I should add some tests for checking that no redundant input events fired immediately before compositionend.
Attachment #8656597 - Flags: review?(bugs)
Comment on attachment 8656581 [details] [diff] [review]
part.1 Log the behavior of IMEContentObserver for debugging

>+    MOZ_LOG(sIMECOLog, LogLevel::Debug,
>+      ("IMECO: 0x%p IMEContentObserver::FlushMergeableNotifications(), "
>+       "crating FocusSetEvent...", this));
creating ?

>   if (mTextChangeData.IsValid()) {
>+    MOZ_LOG(sIMECOLog, LogLevel::Debug,
>+      ("IMECO: 0x%p IMEContentObserver::FlushMergeableNotifications(), "
>+       "crating TextChangeEvent...", this));
ditto


>+    MOZ_LOG(sIMECOLog, LogLevel::Debug,
>+      ("IMECO: 0x%p IMEContentObserver::FlushMergeableNotifications(), "
>+       "crating SelectionChangeEvent...", this));
and here

>+    MOZ_LOG(sIMECOLog, LogLevel::Debug,
>+      ("IMECO: 0x%p IMEContentObserver::FlushMergeableNotifications(), "
>+       "crating PositionChangeEvent...", this));
.
Attachment #8656581 - Flags: review?(bugs) → review+
Attachment #8656592 - Flags: review?(bugs) → review+
Attachment #8656595 - Flags: review?(bugs) → review+
Comment on attachment 8656597 [details] [diff] [review]
part.5 Fix window_composition_text_querycontent.xul for the new input event behavior

>+  is(result.input, false, "runRedundantChangeTest: input shouldn't be fired after synthesizing composition commit-as-is but before compositionend");
Well, the comment isn't quite right, since if result.inputaftercompositionend is true, there sure was 'input' event
Attachment #8656597 - Flags: review?(bugs) → review+
Comment on attachment 8656588 [details] [diff] [review]
part.2 QueryContentEvent should be handled via IMEContentObserver if there is an instance of it

>+ContentEventHandler::HandleQueryContentEvent(WidgetQueryContentEvent* aEvent)
So this has switch-case for all the WidgetQueryContentEvent types

>+EventStateManager::HandleQueryContentEvent(WidgetQueryContentEvent* aEvent)
And so does this.
Is there no way to merge these two?

Hmm, maybe not.
Attachment #8656588 - Flags: review?(bugs) → review+
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/363401fd972c08d2321022704d16f9f05442bc5f
changeset:  363401fd972c08d2321022704d16f9f05442bc5f
user:       Masayuki Nakano <masayuki@d-toybox.com>
date:       Tue Sep 08 12:54:14 2015 +0900
description:
Bug 1200980 part.1 Log the behavior of IMEContentObserver for debugging r=smaug

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/92127319836ac2c4704c92a40e9d4967fcac93ae
changeset:  92127319836ac2c4704c92a40e9d4967fcac93ae
user:       Masayuki Nakano <masayuki@d-toybox.com>
date:       Tue Sep 08 12:54:14 2015 +0900
description:
Bug 1200980 part.2 QueryContentEvent should be handled via IMEContentObserver if there is an instance of it r=smaug

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/62a79bc9cf9eb720a0f9cadf797a0186a54c253b
changeset:  62a79bc9cf9eb720a0f9cadf797a0186a54c253b
user:       Masayuki Nakano <masayuki@d-toybox.com>
date:       Tue Sep 08 12:54:14 2015 +0900
description:
Bug 1200980 part.3 IMEContentObserver should use its selection cache at handling NS_QUERY_SELECTED_TEXT r=smaug

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/48fe95399f05dae43cad3559f25dd2bfbdaa1dbb
changeset:  48fe95399f05dae43cad3559f25dd2bfbdaa1dbb
user:       Masayuki Nakano <masayuki@d-toybox.com>
date:       Tue Sep 08 12:54:14 2015 +0900
description:
Bug 1200980 part.4 nsPlaintextEditor should notify editor observers of the end of edit action when NS_COMPOSITION_CHANGE isn't followed by NS_COMPOSITION_END r=smaug

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/cc22943628f46797ebb085567ed293f1cdf7018d
changeset:  cc22943628f46797ebb085567ed293f1cdf7018d
user:       Masayuki Nakano <masayuki@d-toybox.com>
date:       Tue Sep 08 12:54:14 2015 +0900
description:
Bug 1200980 part.5 Fix window_composition_text_querycontent.xul for the new input event behavior r=smaug
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.