Mantaroh Yoshinaga[:mantaroh] Reporter
	Description • 9 years ago

Can't convert to kanji when edit the MDN articles.

Environment:
 Mac OS 10.10.3 (Yosemite)
 IME : ことえり
 Firefox Nightly(41.0a1) : e10s enable
 
Reproduce step:
1) Open the MDN Edit page.(I opened the yield article[1])
2) Input [テスト] and convert to [Test]

Actual Result:
 can't input [Test] character.
 and conversion candidates is invisible when convert to kanji(see attachment).

Expected Result:
 can input the [Test] character.

Additional information:
 - reproduced text field of other site.(BMO, Google, etc..)

[1] https://developer.mozilla.org/ja/docs/JavaScript/Reference/Operators/yield

	Makoto Kato [:m_kato] Assignee
	Comment 1 • 9 years ago

Most has been fixed by some fixes.   But when Yoshinaga-san discuss with me, composition string sometimes disappears when converting composition string.

	Makoto Kato [:m_kato] Assignee
	Comment 2 • 9 years ago

This log is on e10s.
1980113664[1003aa1a0]: 11d7bb060 IMEInputHandler::DispatchCompositionChangeEvent, aText="今日はいい天気です", aAttrString="今(U+4ECA)日(U+65E5)は(U+306F)い(U+3044)い(U+3044)天(U+5929)気(U+6C17)で(U+3067)す(U+3059)", aSelectedRange={ location=0, length=3 }, Destroyed()=FALSE
1980113664[1003aa1a0]: 11d7bb060 IMEInputHandler::GetRangeCount, aAttrString="今
(U+4ECA)日(U+65E5)は(U+306F)い(U+3044)い(U+3044)天(U+5929)気(U+6C17)で(U+3067)す
(U+3059)", count=2
1980113664[1003aa1a0]: 11d7bb060 IMEInputHandler::ConvertToTextRangeType, aUnderlineStyle=2, aSelectedRange.length=3,
1980113664[1003aa1a0]: 11d7bb060 IMEInputHandler::CreateTextRangeArray, range={ mStartOffset=0, mEndOffset=3, mRangeType=NS_TEXTRANGE_SELECTEDCONVERTEDTEXT }
1980113664[1003aa1a0]: 11d7bb060 IMEInputHandler::ConvertToTextRangeType, aUnderlineStyle=1, aSelectedRange.length=3,
1980113664[1003aa1a0]: 11d7bb060 IMEInputHandler::CreateTextRangeArray, range={ mStartOffset=3, mEndOffset=9, mRangeType=NS_TEXTRANGE_CONVERTEDTEXT }
1980113664[1003aa1a0]: 11d7bb060 IMEInputHandler::CreateTextRangeArray, range={ mStartOffset=3, mEndOffset=3, mRangeType=NS_TEXTRANGE_CARETPOSITION }
1980113664[1003aa1a0]: 11d7bb060 IMEInputHandler::SelectedRange, Destroyed()=FALSE, mSelectedRange={ location=11, length=0 }

Non-e10s, when calling SelectedRange, mSelectedRange is not found state...

	Makoto Kato [:m_kato] Assignee
	Comment 3 • 9 years ago

Now, TextInputHandler resets mSelectedRange by OnSelectionChange.  But When using Apple IME, SelectedRange is called before updating selection cache by OnSelectionChange.

So, before dispatching composition change in SetMarkText, we have to set temporary range to mSelectedRange.

	Makoto Kato [:m_kato] Assignee
	Comment 4 • 9 years ago

Apple input method queries selection range using selectedRange. But this timeing is too early for e10s.  So Gecko will returns caret position before converting composition.  So we set temporary range until OnSelectionChange

	Makoto Kato [:m_kato] Assignee
	Comment 5 • 9 years ago

Current implmentation is that OnSelectionChange marks dirty for selection range.  But OnSelectionChange notification already has current selection.  So we should use it.

	Makoto Kato [:m_kato] Assignee
	Comment 7 • 9 years ago

Comment on attachment 8631957 [details] [diff] [review]
Part 1. Set temporary range until OnSelectionChange is called

Apple Input method seems to be SelectedRange after SetMarkedText immediately.
So since content process doesn't send OnSelectionChange to parent process, mSelectedRange wll be caret position / data.

So we should set temporary range until content process and cache into parent process updates selection.

	Makoto Kato [:m_kato] Assignee
	Comment 8 • 9 years ago

Comment on attachment 8630325 [details] [diff] [review]
Part 2. Improve OnSelectionChange implementation

Since NOTIFY_IME_OF_SELECTION_CHANGE already has selection information, we should set it to reduce query.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 9 • 9 years ago

Comment on attachment 8631957 [details] [diff] [review]
Part 1. Set temporary range until OnSelectionChange is called

>   if (!str.IsEmpty()) {
>     OnUpdateIMEComposition([aAttrString string]);
> 
>+    // Set temprary range for Apple IM with e10s because SelectedRange may

nit: Could you call it "Apple Japanese IME"? "Japanese" should be clearer for the other developers to check the behavior when they change around here.

>+    // return invalid range until OnSelectionChange is called from content
>+    // process.
>+    // This value will be updated by OnSelectionChange soon.
>+    mSelectedRange.location = aSelectedRange.location + mMarkedRange.location;
>+    mSelectedRange.length = aSelectedRange.length;

I'm not sure if this is correct, but not odd.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 10 • 9 years ago

Comment on attachment 8630325 [details] [diff] [review]
Part 2. Improve OnSelectionChange implementation

>+void
>+IMEInputHandler::OnSelectionChange(const IMENotification& aIMENotification)
>+{
>+  MOZ_LOG(gLog, LogLevel::Info,
>+    ("%p IMEInputHandler::OnSelectionChange", this));
>+
>+  if (aIMENotification.mSelectionChangeData.mOffset == UINT32_MAX) {
>+    mSelectedRange.location = NSNotFound;
>+    mRangeForWritingMode.location = NSNotFound;

Please set those length values 0.

>+    return;
>+  }
>+
>+  mWritingMode = aIMENotification.mSelectionChangeData.GetWritingMode();
>+  mRangeForWritingMode =
>+    NSMakeRange(aIMENotification.mSelectionChangeData.mOffset,
>+                aIMENotification.mSelectionChangeData.mLength);
>+  mSelectedRange = mRangeForWritingMode;


In SelectedRange(), mSelectedRange should be set only when mIMEHasFocus is true. Please do same thing.
# Although, I feel this odd, we can avoid to store duplicated selection ranges because mIMEHasFocus can be always available... It's not scope of this bug.

> 3107   mWritingMode = selection.GetWritingMode();
> 3108   mRangeForWritingMode = NSMakeRange(selection.mReply.mOffset,
> 3109                                      selection.mReply.mString.Length());
> 3110 
> 3111   if (mIMEHasFocus) {
> 3112     mSelectedRange = mRangeForWritingMode;
> 3113   }

With the changes, r=me.

	Makoto Kato [:m_kato] Assignee
	Comment 11 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=15f9405251db

	Makoto Kato [:m_kato] Assignee
	Comment 12 • 9 years ago

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/027ab2903c0d9e337212eebfe50457e77da50bc3
changeset:  027ab2903c0d9e337212eebfe50457e77da50bc3
user:       Makoto Kato <m_kato@ga2.so-net.ne.jp>
date:       Tue Jul 21 21:47:32 2015 +0900
description:
Bug 1167105 - Part 1. Set temporary range until OnSelectionChange is called. r=masayuki

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/1430d6bbbc6f00d49dfb270eb3478323fac1e159
changeset:  1430d6bbbc6f00d49dfb270eb3478323fac1e159
user:       Makoto Kato <m_kato@ga2.so-net.ne.jp>
date:       Tue Jul 21 21:47:54 2015 +0900
description:
Bug 1167105 - Part 2. Improve OnSelectionChange implementation. r=masayuki

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 13 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/027ab2903c0d
https://hg.mozilla.org/mozilla-central/rev/1430d6bbbc6f