Resolved candidate area positioning issues for input methods on Windows. #52926

uppet · 2024-05-20T03:41:12Z

Fixed the positioning of the candidate selection area for third-party input methods on Windows, and addressed the occasional flickering issue with the candidate selection window of the built-in input method. On the Windows platform, by processing only the caret_rect, we eliminate the delay issues with third-party input methods failing to notify input promptly, and the built-in system input method no longer experiences candidate selection area flickering. In MoveImeWindow, the check for ime_active_ has been omitted, allowing the input method's candidate selection area to update its position more timely.

The issue can be reproduced by using Sogou or Baidu input method as well as Microsoft Pinyin. The candidate selection area's flickering can be observed when switching input among multiple text inputs while typing in Chinese.

Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.

List which issues are fixed by this PR. You must list at least one issue.

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C , Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2024-05-20T03:41:14Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

uppet · 2024-05-20T06:38:12Z

Okay. I think I should fix those test on old code path

Fixed the positioning of the candidate selection area for third-party input methods on Windows, and addressed the occasional flickering issue with the candidate selection window of the built-in input method. On the Windows platform, by processing only the caret_rect, we eliminate the delay issues with third-party input methods failing to notify input promptly, and the built-in system input method no longer experiences candidate selection area flickering. In MoveImeWindow, the check for ime_active_ has been omitted, allowing the input method's candidate selection area to update its position more timely. The issue can be reproduced by using Sogou or Baidu input method as well as Microsoft Pinyin. The candidate selection area's flickering can be observed when switching input among multiple text inputs while typing in Chinese.

loic-sharma · 2024-07-22T17:09:09Z

cc @LongCatIsLooong Would you be a good candidate to review this? If not, would you know who could be?

LongCatIsLooong

Would you be a good candidate to review this? If not, would you know who could be

I'm not familiar with the Windows IME protocol. Based on the git history it would be @cbracken

The framework stops sending caret rects if the selection is non-empty (i.e. instead of showing a caret we would show the selection highlight). I'm not sure if it's possible on Windows for the IME candidate view to remain visible when the selection becomes non-empty.

LongCatIsLooong · 2024-07-22T20:38:27Z

shell/platform/windows/text_input_manager.cc

@@ -164,7  164,7 @@ std::optional<std::u16string> TextInputManager::GetString(int type) const {
 }

 void TextInputManager::MoveImeWindow(HIMC imm_context) {
-  if (GetFocus() != window_handle_ || !ime_active_) {
+  if (GetFocus() != window_handle_) {


It looks like other callbacks also have the !ime_active_ check. Could you shed some light on why this check needs to be removed?

cbracken · 2024-07-23T17:18:53Z

Since we don't have proper integration tests that deal with IMEs, please ensure that this has been manually tested against Chinese, Japanese, and Korean input at minimum. Korean input, in particular, is quite different and can behave a fair bit differently from Chinese/Japanese in terms of event ordering.

cbracken · 2024-07-23T17:21:45Z

shell/platform/windows/text_input_plugin_unittest.cc

@@ -574,13  574,13 @@ TEST_F(TextInputPluginTest, TransformCursorRect) {
    arguments->AddMember("height", ime_height, allocator);

    auto message = codec.EncodeMethodCall(
-        {"TextInput.setMarkedTextRect", std::move(arguments)});
+        {"TextInput.setCaretRect", std::move(arguments)});


Without having tested this locally, isn't this going to move the candidate window along under the cursor as you type?

If so, that's pretty non-standard behaviour on Windows, at least for Japanese input -- not sure what the expected behaviour is for Sogou and others.

cbracken · 2024-07-23T17:29:11Z

@uppet thanks for your PR.

I have unchecked the mistakenly-checked "I listed at least one issue that this PR fixes in the description above."

Please update the issue description to the specific issues that you're resolving with this patch. If there is no existing issue, please file one with a reduced reproduction, expected behaviour and actual behaviour.

In particular, please confirm which IMEs on which OS versions. e.g. Sogou on Windows 7, Microsoft Pinyin on Windows 10, 11. etc.

A few Sogou-specific issues I know of:

Once you've linked to issues, please re-check that checkbox.

cbracken · 2024-07-23T17:34:45Z

shell/platform/windows/text_input_plugin.cc

@@ -22,6  22,7 @@ static constexpr char kHideMethod[] = "TextInput.hide";
 static constexpr char kSetMarkedTextRect[] = "TextInput.setMarkedTextRect";


Is this now unused? If so, it would need to be removed, but please see my comment below since at least for Japanese input, the candidate menu shouldn't move along with the cursor.

flutter-dashboard · 2024-08-12T17:35:00Z

This pull request executed golden file tests, but it has not been updated in a while (20 days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

cbracken · 2024-08-12T22:29:01Z

@uppet are you still working on this patch?

Piinks · 2024-10-15T22:40:34Z

Thank you for your contribution. I'm going to close this PR for now since there are outstanding comments, just to get this off our PR review queue. Please don't hesitate to re-open or submit a new PR if you have the time to address the review comments. Thanks!

github-actions bot added affects: desktop platform-windows labels May 20, 2024

uppet force-pushed the fixime branch from 78de9d2 to eeae95f Compare May 20, 2024 07:00

uppet added 4 commits May 21, 2024 16:44

Merge branch 'main' into fixime

ecb9cec

Merge branch 'main' into fixime

34bb1ef

Merge branch 'main' into fixime

c116513

Merge branch 'main' into fixime

c5464e3

justinmc added the affects: text input label May 28, 2024

Merge branch 'main' into fixime

75cdeb4

LongCatIsLooong reviewed Jul 22, 2024

View reviewed changes

Merge branch 'main' into fixime

1974d24

cbracken reviewed Jul 23, 2024

View reviewed changes

cbracken added the waiting for customer response label Aug 12, 2024

Piinks closed this Oct 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolved candidate area positioning issues for input methods on Windows. #52926

Resolved candidate area positioning issues for input methods on Windows. #52926

uppet commented May 20, 2024 •

edited by cbracken

Loading

flutter-dashboard bot commented May 20, 2024

uppet commented May 20, 2024 •

edited

Loading

loic-sharma commented Jul 22, 2024

LongCatIsLooong left a comment •

edited

Loading

LongCatIsLooong Jul 22, 2024

cbracken commented Jul 23, 2024

cbracken Jul 23, 2024

cbracken commented Jul 23, 2024 •

edited

Loading

cbracken Jul 23, 2024

flutter-dashboard bot commented Aug 12, 2024

cbracken commented Aug 12, 2024

Piinks commented Oct 15, 2024

		@@ -22,6 22,7 @@ static constexpr char kHideMethod[] = "TextInput.hide";
		static constexpr char kSetMarkedTextRect[] = "TextInput.setMarkedTextRect";

Resolved candidate area positioning issues for input methods on Windows. #52926

Resolved candidate area positioning issues for input methods on Windows. #52926

Conversation

uppet commented May 20, 2024 • edited by cbracken Loading

Pre-launch Checklist

flutter-dashboard bot commented May 20, 2024

uppet commented May 20, 2024 • edited Loading

loic-sharma commented Jul 22, 2024

LongCatIsLooong left a comment • edited Loading

Choose a reason for hiding this comment

LongCatIsLooong Jul 22, 2024

Choose a reason for hiding this comment

cbracken commented Jul 23, 2024

cbracken Jul 23, 2024

Choose a reason for hiding this comment

cbracken commented Jul 23, 2024 • edited Loading

cbracken Jul 23, 2024

Choose a reason for hiding this comment

flutter-dashboard bot commented Aug 12, 2024

cbracken commented Aug 12, 2024

Piinks commented Oct 15, 2024

uppet commented May 20, 2024 •

edited by cbracken

Loading

uppet commented May 20, 2024 •

edited

Loading

LongCatIsLooong left a comment •

edited

Loading

cbracken commented Jul 23, 2024 •

edited

Loading