848672 - [TSF] Redesign focus and IME state handling of nsTextStore

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

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

Attachment: Patch

nsTextStore's focus handling is odd.

1. When editable element loses focus, nsTextStore is destroyed but it doesn't tell which ITfDocumentMgr gets focus.
2. When IME enabled state is changed without focus change, nsTextStore::SetInputContextInternal() doesn't work well. This is the cause of bug 789881.
3. When IME enabled state becomes enabled without focus change, nsTextStore doesn't tell which ITfDocumentMgr gets focus. This causes composition string isn't rendered in our window.

The attached patch changes:

1. Creates another ITfDocumentMgr instance (sTsfDisabledDocumentMgr) and its context whose content is empty (sTsfDisabledContext).
2. When IME state becomes disabled, nsTextStore isn't created. Instead of that, it sets focus to the new ITfDocumentMgr instance of #1.
3. In SetInputContext(), if the focus isn't changed but the enabled state is changing, this emulates focus change by calling OnFocusChange().

By this change, widget::IMEHandler::SetInputContext() doesn't need to use IMM path if it's in TSF mode.

Comment 1

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

Comment on attachment 722049 [details] [diff] [review]
Patch

Oops, I realized that this patch probably breaks password field's behavior because it makes the focused document not provide the input scope since the focused document's context is empty.

I'll redesign the patch.

Comment 2

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

Attachment: Patch

Comment 3

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

Attachment: Patch

This patch makes:

1. TSF mode work without IMM's context.
2. a document whose context is empty and disables keyboard used when the content is not editable.
3. focused document automatically managed by ITfThreadMgr::AssociateFocs().

Especially #3 makes TSF mode stable while our process or other resource is busy.

Comment 4

[Jim Mathies [:jimm]]

Comment on attachment 729428 [details] [diff] [review]
Patch

This code looks fine, although I'm unable to test the actual functionality here.

Do we have a way to create tests for some of this tsf work?

Comment 5

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

(In reply to Jim Mathies [:jimm] from comment #4)
> Comment on attachment 729428 [details] [diff] [review]
> Patch
> 
> This code looks fine, although I'm unable to test the actual functionality
> here.
> 
> Do we have a way to create tests for some of this tsf work?

I think that it's not impossible since there was a test for nsTextStore behavior (FYI: it was disabled due to a lot of bugs). However, I don't think that it's time to do that now. The reasons are:

1. The test was based on the behavior of nsTextStore, so, if our understanding about TSF spec is wrong, the test makes fixing other bugs slower.
2. Additionally, I still don't understand the TSF spec enough. So, current nsTextStore may be created with my misunderstanding yet.
3. I'd like to improve TSF mode as far as possible. It's nice if we can make TSF mode enabled in default settings.

So, I believe that current TSF implementation is still immature for creating the test.

I'm testing at least one day with tryserver build for every TSF bug fix. By this, I become a heavy user of twitter.com :-)

Anyway, I'll keep to work hard on TSF stuff at least all of this year. I hope that it helps Metrofox's quality.

Thank you for your review!

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/75a76772b690

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/75a76772b690