1176955 - [e10s][TSF] Kakutei-Undo of Japanese IME doesn't work

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

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

1. Use IME which supports Kakutei-Undo like MS-IME or ATOK.
2. Commit something.
3. Ctrl + Backspace

Actual Result:

Nothing happens (sometimes, I see underline for composing string in a moment)

Expected Result:

Start composing the committed string again.

Comment 1

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

FYI: IMM doesn't have this bug.

Comment 2

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

FYI: Reconversion works both in TSF and IMM mode.

Comment 3

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

Attachment: Log at Ctrl+Backspace with ATOK 2015

> 0[7ea5b0e800]: ContentCacheInParent: 0x7ecf50c4e8 OnEventNeedingAckReceived(aWidget=0x7ec7e0d400, aMessage=NS_COMPOSITION_START), mPendingEventsNeedingAck=2
> 0[7ea5b0e800]: ContentCacheInParent: 0x7ecf50c4e8 AssignContent(aNotification=NOTIFY_IME_OF_COMPOSITION_UPDATE), Succeeded, mText.Length()=0, mSelection={ mAnchor=0, mFocus=0, mWritingMode=Horizontal, mAnchorCharRect={ x=274, y=57, width=1, height=18 }, mFocusCharRect={ x=274, y=57, width=1, height=18 }, mRect={ x=274, y=58, width=0, height=0 } }, mFirstCharRect={ x=274, y=57, width=1, height=18 }, mCaret={ mOffset=0, mRect={ x=274, y=59, width=2, height=13 } }, mTextRectArray={ mStart=0, mRects.Length()=0 }, mEditorRect={ x=248, y=52, width=199, height=28 }
> 0[7ea5b0e800]: ContentCacheInParent: 0x7ecf50c4e8 OnEventNeedingAckReceived(aWidget=0x7ec7e0d400, aMessage=NS_COMPOSITION_CHANGE), mPendingEventsNeedingAck=1
> 0[7ea5b0e800]: ISM: IMEStateManager::NotifyIME(aNotification={ mMessage=NOTIFY_IME_OF_SELECTION_CHANGE }, aWidget=0x7ec7e0d400, aOriginIsRemote=true), sFocusedIMEWidget=0x7ec7e0d400, sRemoteHasFocus=true

Looks like this selection change notification (delayed by e10s) causes committing composition by nsTextStore.

Comment 4

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

Leading SelectionSet event causes selection change notification whose mCausedByComposition is false. In non-e10s mode, it doesn't cause committing composition since there is no composition yet. In e10s mode, it's notified after following NS_COMPOSITION_START is handled by content. Therefore, it causes calling nsTextStore::CommitCompositionInternal().

We have some ways to fix this bug. My current best choice is, making mCausedByComposition of selection change notification caused by SelectionSet event true when NS_COMPOSITION_START is received before sending notification to the widget.

Comment 5

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

Attachment: part.1 TextComposition should guarantee that WidgetSelectionEvent should be handled by same content as the target of composition events when there is a composition

Although, I still think that PuppetWidget shouldn't notify IME of delayed selection change. However, this bug blocks some IME users who usually use this. So, let's fix this bug with TSF rule.

MSDN says that applications shouldn't notify selection change which is caused by TSF (or TIP) sets selection. The cause of this bug is that we notify IME of such selection change *after* starting composition (the selection change occurs before compositionstart in non-e10s mode).

Therefore, for now, we should set a flag which indicates if a selection change is caused by selection set event.

This patch prepares for that. WidgetSelectionEvent should be guaranteed that it should be dispatched to the event target, i.e., if there is composition, its target should be same as WidgetCompositionEvent. Therefore, this patch makes WidgetSelectionEvent handled via TextComposition.

Comment 6

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

Attachment: part.2 NOTIFY_IME_OF_SELECTION should have a flag which indicates if it's caused by a selection event

Then, we can add a flat to indicate if a selection change is caused by selection set event.

Comment 7

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

Attachment: part.3 nsTextStore shouldn't notify TSF of selection change which is caused by selection set event

So, we shouldn't notify TSF of selection change which is caused by a request of TSF (or TIP).

https://msdn.microsoft.com/en-us/library/windows/desktop/ms538405%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396
> Remarks
> 
> ITextStoreACPSink::OnSelectionChange is never called when the selection
> is modified by one of the ITextStoreACP interface methods, such as
> ITextStoreACP::SetSelection or ITextStoreACP::InsertTextAtSelection.

Comment 8

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

I'll try to review these tomorrow (well, it is already tomorrow in Finland).

Comment 9

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

Comment on attachment 8633556 [details] [diff] [review]
part.1 TextComposition should guarantee that WidgetSelectionEvent should be handled by same content as the target of composition events when there is a composition

I don't understand mPropagationStopped handling here.
We deal with all the stuff in PreHandleEvent and 
aSelectionEvent->mFlags.mPropagationStopped = true; is set in 
TextComposition::HandleSelectionEvent when dispatched to child process.
So I don't see any case where
if ( ... || aSelectionEvent->mFlags.mPropagationStopped) is true

Comment 10

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

Comment on attachment 8633557 [details] [diff] [review]
part.2 NOTIFY_IME_OF_SELECTION should have a flag which indicates if it's caused by a selection event

>@@ -383,18 +385,26 @@ TextComposition::DispatchSelectionEvent(
>   // If the content is a container of TabParent, composition should be in the
>   // remote process.
>   if (aTabParent) {
>     unused << aTabParent->SendSelectionEvent(*aSelectionEvent);
>     aSelectionEvent->mFlags.mPropagationStopped = true;
>     return;
>   }
> 
>+
extra newline

>   ContentEventHandler handler(aPresContext);
>+  AutoRestore<bool> saveHandlingSelectionEvent(sHandlingSelectionEvent);
...
>+  sHandlingSelectionEvent = false;
You don't need this since you have saveHandlingSelectionEvent, which will anyway set the value to whatever
it was.

Comment 11

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

Attachment: part.1 TextComposition should guarantee that WidgetSelectionEvent should be handled by same content as the target of composition events when there is a composition

Sure. I was copied it from IMEStateManager::DispatchCompositionEvent(). Therefore, it's checked.

(just removed the mPropagationStopped check and logging it)

Comment 12

[Makoto Kato [:m_kato]]

Comment on attachment 8633559 [details] [diff] [review]
part.3 nsTextStore shouldn't notify TSF of selection change which is caused by selection set event

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

::: widget/windows/nsTextStore.cpp
@@ +662,5 @@
> +public:
> +  GetWritingModeName(const WritingMode& aWritingMode)
> +  {
> +    if (!aWritingMode.IsVertical()) {
> +      Assign("Horizontal");

Use AssignLiteral instead

@@ +666,5 @@
> +      Assign("Horizontal");
> +      return;
> +    }
> +    if (aWritingMode.IsVerticalLR()) {
> +      Assign("Vertical (LR)");

Use AssignLiteral instead

@@ +669,5 @@
> +    if (aWritingMode.IsVerticalLR()) {
> +      Assign("Vertical (LR)");
> +      return;
> +    }
> +    Assign("Vertical (RL)");

Use AssignLiteral instead

Comment 13

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

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/d59f353ae9098335b5e7da4d61c64dc9c52fe11c
changeset:  d59f353ae9098335b5e7da4d61c64dc9c52fe11c
user:       Masayuki Nakano <[e-mail]>
date:       Fri Jul 17 11:25:00 2015 +0900
description:
Bug 1176955 part.1 TextComposition should guarantee that WidgetSelectionEvent should be handled by same content as the target of composition events when there is a composition r=smaug

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/5eab052ea8e4d650b835ee36cc3afcace853e338
changeset:  5eab052ea8e4d650b835ee36cc3afcace853e338
user:       Masayuki Nakano <[e-mail]>
date:       Fri Jul 17 11:25:00 2015 +0900
description:
Bug 1176955 part.2 NOTIFY_IME_OF_SELECTION should have a flag which indicates if it's caused by a selection event r=smaug

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/c1ca83e38b78dcad470919a478e23b0e6cec4005
changeset:  c1ca83e38b78dcad470919a478e23b0e6cec4005
user:       Masayuki Nakano <[e-mail]>
date:       Fri Jul 17 11:25:00 2015 +0900
description:
Bug 1176955 part.3 nsTextStore shouldn't notify TSF of selection change which is caused by selection set event r=m_kato

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d59f353ae909
https://hg.mozilla.org/mozilla-central/rev/5eab052ea8e4
https://hg.mozilla.org/mozilla-central/rev/c1ca83e38b78