1205945 - [e10s] Japanese IME of OS X 10.10 sometimes shows candidate window to bottom-left of the screen

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

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

1. Type a word with Apple's IME of 10.10 in an editor in a remote process.
2. Type space to convert the word.
3. Type new word and type space.

When you do this STR quickly, the candidate window shown in #3 may be positioned at bottom-left of the screen.

Comment 1

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

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

Comment 2

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

Attachment: part.1 Remove unnecessary member of IMEInputHandler, mLastDispatchedCompositionString

First, get rid of the unused member which I forgot to remove :-(

Comment 3

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

Attachment: part.2 Add DispatchCompositionStartEvent() and move the code of OnStartIMEComposition() into it

This is preparation of the fix.

Currently, we call On*IMEComposition() after dispatching a composition event. However, this is ugly and not guaranteed that they are always called without any bugs.

So, let's merge them to DispatchComposition*Event().

Comment 4

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

Attachment: part.3 Move the code of OnUpdateIMEComposition() into DispatchCompositionChangeEvent()

Comment 5

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

Attachment: part.4 Move the code of OnEndIMEComposition() into DispatchCompositionCommitEvent()

OnEndIMEComposition() is also called after calling CancelComposition() in OnDestroyWidget(). I just removed it and destroying the mIMECompositionString in the destructor. I think that this is safer than current behavior and enough safe since when Destroyed() returns true, most methods won't work.

Comment 6

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

Attachment: part.5 Emulate mSelectedRange at dispatching compositionchange or compositioncommit event until OnSelectionChange() is called

This is what first patch to fix this bug is.

IMEInputHandler should modify mSelectedRange immediately before dispatching composition events because OnSelectionChange() will be called asynchronously in e10s mode. IME is confused by SelectedRange() returns old range which isn't changed by the latest composition change or commit.

Comment 7

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

Attachment: part.4 Move the code of OnEndIMEComposition() into DispatchCompositionCommitEvent()

Sorry a part of change for this was included in the part.5.

Comment 8

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

Attachment: part.5 Emulate mSelectedRange at dispatching compositionchange or compositioncommit event until OnSelectionChange() is called

See the previous post of obsoleted part.5.

Comment 9

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

Attachment: part.6 IMEInputHandler::GetAttributedSubstringFromRange() should return stored composition string if the range is in the composition strin

Similarly, IME may be confused by the result of GetAttributedSubstringFromRange(). We should return the retrieved string from the cache. This makes returning font information impossible for this case. However, the font information is needed by dictionary service. So, IME doesn't need it and users must not use dictionary service during inputting composition string. So, this hack must be safe.

Comment 10

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

FYI: I want to land this tonight or tomorrow for fixing on 43. If you can review them soon, I'd like you to mark r+ some fixes are necessary. (But of course, you find bad things which you really need to check again, r- is okay. Then, I give up to land them to 43)

Comment 11

[Steven Michaud [:smichaud] (Retired)]

The most I can do so quickly is just look through the patches for anything I don't understand, or looks wrong.  I won't be able to do any testing.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8662855 [details] [diff] [review]
part.2 Add DispatchCompositionStartEvent() and move the code of OnStartIMEComposition() into it

Looks fine to me.

I do have one nit:

+   *                              Otherwise, e.g., canceled by the web app,

"web app" seems wrong here.  It's a particular kind of client, but you mean any client (or web page) at all.  How about "web page"?

Comment 13

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #12)

Actually, forget my objection to "web app".  I notice that it's used in several other places outside your patch.  Yes, the usage is confusing.  But I don't think it's confusing enough to complicate landing an urgent set of patches.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8662874 [details] [diff] [review]
part.5 Emulate mSelectedRange at dispatching compositionchange or compositioncommit  event until OnSelectionChange() is called

Seems reasonable to me.

I have another nit:

+  // FYI: The selection range might be modified by a compositionstart event

"might be" should be changed to "might have been".

Comment 15

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8662876 [details] [diff] [review]
part.6 IMEInputHandler::GetAttributedSubstringFromRange() should return stored composition string if the range is in the composition strin

> the composition string may not be handled yet

Looks reasonable, but this last part of your comment is unclear.

How about:

"its copy of the composition string may be out of date".

Comment 16

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/984d08829cc697059830c233dd85e857f1dbb57a
Bug 1205945 part.1 Remove unnecessary member of IMEInputHandler, mLastDispatchedCompositionString r=smichaud

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8c6768c36a72f2111bf9a31d59a7295ae5d60c0
Bug 1205945 part.2 Add DispatchCompositionStartEvent() and move the code of OnStartIMEComposition() into it r=smichaud

https://hg.mozilla.org/integration/mozilla-inbound/rev/49c720e166c79a50cdc824136392db944275d98b
Bug 1205945 part.3 Move the code of OnUpdateIMEComposition() into DispatchCompositionChangeEvent() r=smichaud

https://hg.mozilla.org/integration/mozilla-inbound/rev/d19d4d9812be7d347a6f985ab20fffa1f8f3ff87
Bug 1205945 part.4 Move the code of OnEndIMEComposition() into DispatchCompositionCommitEvent() r=smichaud

https://hg.mozilla.org/integration/mozilla-inbound/rev/ead91b39b94e185415db427609f76e9591ac3994
Bug 1205945 part.5 Emulate mSelectedRange at dispatching compositionchange or compositioncommit  event until OnSelectionChange() is called r=smichaud

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4d5d7cfdd8ff7b4fb66141904dbb4a62bfeb115
Bug 1205945 part.6 IMEInputHandler::GetAttributedSubstringFromRange() should return stored composition string if the range is in the composition string r=smichaud

Comment 17

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

Thank you, Steven. I hope the landing will be included by 43. For DevEdition users.

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/984d08829cc6
https://hg.mozilla.org/mozilla-central/rev/c8c6768c36a7
https://hg.mozilla.org/mozilla-central/rev/49c720e166c7
https://hg.mozilla.org/mozilla-central/rev/d19d4d9812be
https://hg.mozilla.org/mozilla-central/rev/ead91b39b94e
https://hg.mozilla.org/mozilla-central/rev/b4d5d7cfdd8f

Comment 19

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

I seem this bug is fixed. thank you!