1376417 - [e10s][Cocoa] Google Japanese IME's suggest window is positioned at wrong position if current composition was started with committing previous composition

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

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

Similar to bug 1376407, Google Japanese IME's suggest window is positioned at previous composition if current composition is started with committing the previous composition.

STR:
0. Set key assign of Google Japanese Input is ATOK.
1. Type something in an editor in e10s mode and convert the word (but do no commit it).
2. Type new string.

ER:
Suggest window of Google Japanese Input should be positioned proper position.

AR:
It's positioned based on the previous composition offset.

> [Main Thread]: I/TextInputHandlerWidgets 0x10e698a20 IMEInputHandler::DispatchCompositionCommitEvent, aCommitString=0x0x7fff5e062a60 ("あい"), Destroyed()=FALSE, mView=0x10e6f0130, mWidget=0x120909c00, inputContext=0x1090bc7a0, mIsIMEComposing=TRUE
> [Main Thread]: I/TextInputHandlerWidgets 0x10e698a20 IMEInputHandler::SelectedRange, Destroyed()=FALSE, mSelectedRange={ location=2, length=0 }
> [Main Thread]: I/TextInputHandlerWidgets 0x10e698a20 IMEInputHandler::HasMarkedText, mMarkedRange={ location=9223372036854775807, length=0 }
> ...
> [Main Thread]: I/TextInputHandlerWidgets 0x10e698a20 IMEInputHandler::SetMarkedText, aAttrString="う", aSelectedRange={ location=1, length=0 }, aReplacementRange=0x7fff5e061af0 { location=9223372036854775807, length=0 }, Destroyed()=FALSE, IgnoreIMEComposition()=FALSE, IsIMEComposing()=FALSE, mMarkedRange={ location=9223372036854775807, length=0 }, keyevent=0x1c2572840, keydownHandled=FALSE, keypressDispatched=FALSE, causedOtherKeyEvents=FALSE, compositionDispatched=TRUE
> [Main Thread]: I/TextInputHandlerWidgets 0x10e698a20 IMEInputHandler::SelectedRange, Destroyed()=FALSE, mSelectedRange={ location=2, length=0 }
> [Main Thread]: I/TextInputHandlerWidgets 0x10e698a20 IMEInputHandler::DispatchCompositionStartEvent, mSelectedRange={ location=2, length=0 }, Destroyed()=FALSE, mView=0x10e6f0130, mWidget=0x120909c00, inputContext=0x1090bc7a0, mIsIMEComposing=FALSE
> [Main Thread]: I/TextInputHandlerWidgets 0x10e698a20 IMEInputHandler::SelectedRange, Destroyed()=FALSE, mSelectedRange={ location=2, length=0 }
> [Main Thread]: I/TextInputHandlerWidgets 0x10e698a20 IMEInputHandler::DispatchCompositionChangeEvent, aText="う", aAttrString="う", aSelectedRange={ location=1, length=0 }, Destroyed()=FALSE, mView=0x10e6f0130, mWidget=0x120909c00, inputContext=0x1090bc7a0, mIsIMEComposing=TRUE
> [Main Thread]: I/TextInputHandlerWidgets 0x10e698a20 IMEInputHandler::GetRangeCount, aAttrString="う", count=1
> [Main Thread]: I/TextInputHandlerWidgets 0x10e698a20 IMEInputHandler::ConvertToTextRangeType, aUnderlineStyle=1, aSelectedRange.length=0,
> [Main Thread]: I/TextInputHandlerWidgets 0x10e698a20 IMEInputHandler::CreateTextRangeArray, range={ mStartOffset=0, mEndOffset=1, mRangeType=TextRangeType::eRawClause }
> [Main Thread]: I/TextInputHandlerWidgets 0x10e698a20 IMEInputHandler::CreateTextRangeArray, range={ mStartOffset=1, mEndOffset=1, mRangeType=TextRangeType::eCaret }
> [Main Thread]: I/TextInputHandlerWidgets 0x10e698a20 TextInputHandler::HandleKeyDownEvent, called interpretKeyEvents
> [Main Thread]: I/TextInputHandlerWidgets 0x10e698a20 TextInputHandler::HandleKeyDownEvent, wasComposing=TRUE, IsIMEComposing()=TRUE
> [Main Thread]: I/TextInputHandlerWidgets 0x10e698a20 TextInputHandler::HandleKeyDownEvent, keydown handled=FALSE, keypress handled=FALSE, causedOtherKeyEvents=FALSE, compositionDispatched=TRUE
> [Main Thread]: I/TextInputHandlerWidgets 
> [Main Thread]: I/TextInputHandlerWidgets 0x10e698a20 IMEInputHandler::HasMarkedText, mMarkedRange={ location=2, length=1 }
> [Main Thread]: I/TextInputHandlerWidgets 0x10e698a20 IMEInputHandler::MarkedRange, mMarkedRange={ location=2, length=1 }
> [Main Thread]: I/TextInputHandlerWidgets 0x10e698a20 IMEInputHandler::HasMarkedText, mMarkedRange={ location=2, length=1 }
> [Main Thread]: I/TextInputHandlerWidgets 0x10e698a20 IMEInputHandler::FirstRectForCharacterRange, Destroyed()=FALSE, aRange={ location=2, length=1 }, aActualRange=0x7fff5e0657c8 }
> [Main Thread]: I/TextInputHandlerWidgets 0x10e698a20 IMEInputHandler::FirstRectForCharacterRange, useCaretRect=FALSE rect={ x=694.500000, y=405.500000, width=13.000000, height=16.000000 }, actualRange={ location=0, length=1 }
> [Main Thread]: I/TextInputHandlerWidgets 0x10e698a20 IMEInputHandler::FirstRectForCharacterRange, Destroyed()=FALSE, aRange={ location=2, length=1 }, aActualRange=0x7fff5e0654e0 }
> [Main Thread]: I/TextInputHandlerWidgets 0x10e698a20 IMEInputHandler::FirstRectForCharacterRange, useCaretRect=FALSE rect={ x=694.500000, y=405.500000, width=13.000000, height=16.000000 }, actualRange={ location=0, length=1 }
> [Main Thread]: I/TextInputHandlerWidgets 0x10e698a20 IMEInputHandler::GetAttributedSubstringFromRange, aRange={ location=2, length=1 }, aActualRange=0x7fff5e0653f8, Destroyed()=FALSE
> [Main Thread]: I/TextInputHandlerWidgets 0x10e698a20 IMEInputHandler::GetAttributedSubstringFromRange, computed with mIMECompositionString (result string="な")
> [Main Thread]: I/TextInputHandlerWidgets 0x10e698a20 IMEInputHandler::OnSelectionChange

Different from bug 1376407, IMEInputHandler stores and modifies its selection cache as I expected. However, FirstRectForCharacterRange() is called too early for the main process.

I *guess* that in this case, ContentCache can know the next character rect of previous composition string (even if there is no character). If so, we should return the rect for FirstRectForCharacterRange().

Comment 1

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

I found invalidateCharacterCoordinates of NSTextInputContext.
https://developer.apple.com/documentation/appkit/nstextinputcontext/1535165-invalidatecharactercoordinates?language=objc

We should call this at every NOTIFY_IME_OF_POSITION_CHANGE.  However, even if we do so, position of candidate window isn't modified by any IMEs. Additionally, native applications and Safari doesn't update the position too. It seems that macOS doesn't update candidate window position when invalidateCharacterCoordinates is called.

Comment 2

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

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

Comment 3

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

Attachment: Bug 1376417 - part1: IMEInputHandler should notify Cocoa of layout change with [NSTextInputContext invalidateCharacterCoordinates]

I found invalidateCharacterCoordinates in NSTextInputContext. It's available 10.6 and later.

Calling this API does NOT make visible candidate window follow window move nor something to change layout, though. But we should call it.

Review commit: https://reviewboard.mozilla.org/r/153276/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/153276/

Comment 4

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

Attachment: Bug 1376417 - part2: ContentCache should adjust composition start offset when querying content with relative offset

When IME commits composition and restarts composition immediately, query event with relative offset doesn't work as expected until the commit is handled in the remote process.

Therefore, this patch makes ContentCacheInParent store the last commit string length until it's handled in the content.  Note that ContentCache doesn't need to store all commit string lengthes which have not been handled in the remote process yet because it's important and possible to return next character of commited composition only when there are not 2 or more pending compositions.  Even if there are, i.e., the remote process is too busy, ContentCacheInParent doesn't have necessary character rect.

Review commit: https://reviewboard.mozilla.org/r/153278/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/153278/

Comment 5

[Makoto Kato [:m_kato]]

Comment on attachment 8882149 [details]
Bug 1376417 - part1: IMEInputHandler should notify Cocoa of layout change with [NSTextInputContext invalidateCharacterCoordinates]

https://reviewboard.mozilla.org/r/153276/#review158434

Humm, WebKit and Blink don't use it.  Although nyaruru-san tries it for http://crbug.com/580808 on blink, no range request after it.
But NSTextView seems to call it, it might be useful.

Comment 6

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

(In reply to Makoto Kato [:m_kato] from comment #5)
> no range request after it.

Yeah, I experimented it too. I reported a bug to Apple (id:33048731) that invalidateCharacterCoordinates should retry to get position with markedRange or selectedRange and firstRectForCharacterRect. I guess that Cocoa developers don't know about this issue because this is hard to report and check. If we'll do this, Apple can check the symptom with Nightly (or Firefox 56~) easier.

Comment 7

[Makoto Kato [:m_kato]]

Comment on attachment 8882150 [details]
Bug 1376417 - part2: ContentCache should adjust composition start offset when querying content with relative offset

https://reviewboard.mozilla.org/r/153278/#review158460

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b627ad5d95a
part1: IMEInputHandler should notify Cocoa of layout change with [NSTextInputContext invalidateCharacterCoordinates] r=m_kato
https://hg.mozilla.org/integration/autoland/rev/55ffef6341b7
part2: ContentCache should adjust composition start offset when querying content with relative offset r=m_kato

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b627ad5d95a
https://hg.mozilla.org/mozilla-central/rev/55ffef6341b7