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

VERIFIED FIXED in Firefox 43

Status

()

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Blocks 1 bug, {inputmethod})

Trunk
mozilla43
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(6 attachments, 2 obsolete attachments)

1.10 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
7.20 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
8.78 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
12.31 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
7.12 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
2.48 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
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.
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().
Attachment #8662855 - Flags: review?(smichaud)
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.
Attachment #8662858 - Flags: review?(smichaud)
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.
Attachment #8662864 - Flags: review?(smichaud)
Sorry a part of change for this was included in the part.5.
Attachment #8662858 - Attachment is obsolete: true
Attachment #8662864 - Attachment is obsolete: true
Attachment #8662858 - Flags: review?(smichaud)
Attachment #8662864 - Flags: review?(smichaud)
Attachment #8662869 - Flags: review?(smichaud)
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.
Attachment #8662876 - Flags: review?(smichaud)
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)
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.
Attachment #8662850 - Flags: review?(smichaud) → review+
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"?
Attachment #8662855 - Flags: review?(smichaud) → review+
(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.
Attachment #8662856 - Flags: review?(smichaud) → review+
Attachment #8662869 - Flags: review?(smichaud) → review+
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".
Attachment #8662874 - Flags: review?(smichaud) → review+
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".
Attachment #8662876 - Flags: review?(smichaud) → review+
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
Thank you, Steven. I hope the landing will be included by 43. For DevEdition users.
I seem this bug is fixed. thank you!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.