Closed Bug 1176955 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Content Processes, defect)

x86
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
e10s + ---
firefox41 --- affected
firefox42 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 2 open bugs)

Details

(Keywords: inputmethod)

Attachments

(4 files, 1 obsolete file)

44.11 KB, text/plain
Details
21.57 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.57 KB, patch
m_kato
: review+
Details | Diff | Splinter Review
9.64 KB, patch
smaug
: review+
Details | Diff | Splinter Review
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.
> 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.
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.
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.
Attachment #8633556 - Flags: review?(bugs)
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.
Attachment #8633559 - Flags: review?(m_kato)
I'll try to review these tomorrow (well, it is already tomorrow in Finland).
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
Attachment #8633556 - Flags: review?(bugs) → review-
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.
Attachment #8633557 - Flags: review?(bugs) → review+
Sure. I was copied it from IMEStateManager::DispatchCompositionEvent(). Therefore, it's checked.

(just removed the mPropagationStopped check and logging it)
Attachment #8633556 - Attachment is obsolete: true
Attachment #8634498 - Flags: review?(bugs)
Attachment #8634498 - Flags: review?(bugs) → review+
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
Attachment #8633559 - Flags: review?(m_kato) → review+
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/d59f353ae9098335b5e7da4d61c64dc9c52fe11c
changeset:  d59f353ae9098335b5e7da4d61c64dc9c52fe11c
user:       Masayuki Nakano <masayuki@d-toybox.com>
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 <masayuki@d-toybox.com>
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 <masayuki@d-toybox.com>
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
You need to log in before you can comment on or make changes to this bug.