Closed Bug 1167105 Opened 7 years ago Closed 7 years ago

[e10s] Can't convert to kanji when editing the text area with Yosemite + Apple IM

Categories

(Core :: Widget: Cocoa, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

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

People

(Reporter: mantaroh, Assigned: m_kato)

References

(Blocks 1 open bug)

Details

(Keywords: inputmethod)

Attachments

(3 files, 1 obsolete file)

Attached image Screenshot-Edit_MDN.png
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
Blocks: e10s-ime
Keywords: inputmethod
Summary: Can't convert to kanji when editing the text area. → [e10s] Can't convert to kanji when editing the text area.
OS: Unspecified → Mac OS X
Hardware: Unspecified → x86_64
Most has been fixed by some fixes.   But when Yoshinaga-san discuss with me, composition string sometimes disappears when converting composition string.
Summary: [e10s] Can't convert to kanji when editing the text area. → [e10s] Can't convert to kanji when editing the text area with Yosemite + Apple IM
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...
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.
Blocks: 1153733
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
Current implmentation is that OnSelectionChange marks dirty for selection range.  But OnSelectionChange notification already has current selection.  So we should use it.
Attachment #8630323 - Attachment is obsolete: true
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.
Attachment #8631957 - Flags: review?(masayuki)
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.
Attachment #8630325 - Flags: review?(masayuki)
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.
Attachment #8631957 - Flags: review?(masayuki) → review+
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.
Attachment #8630325 - Flags: review?(masayuki) → review+
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
You need to log in before you can comment on or make changes to this bug.