Revert text cursor position calculation windows #51164

deniz-lee · 2024-03-04T08:05:05Z

Reverts the logic for calculating the TextField input text cursor position when inputting Korean characters (3.13.9)

Issue occurred since 3.16.x
When entering Korean, the text cursor is behind single space. flutter#140739

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

When entering Korean text, the input cursor must be located on the right side of the input text.
However, when entering Korean text, the input cursor is located to the left of the entered character.

Recorded Video about the issue:

Before (origin/main):
https://github.com/flutter/engine/assets/121159478/25d86b34-677c-4bc4-bc5e-b0179f36484c
Modified (origin/revert_text_cursor_position_calculation_windows)
https://github.com/flutter/engine/assets/121159478/7a53799f-7c1d-404b-be33-d4568b9a001c

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.

…ition when inputting Korean characters (as 3.13.9) - Issue occurred since 3.16.x - flutter/flutter#140739

flutter-dashboard · 2024-03-04T08:05:07Z

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.

loic-sharma · 2024-03-04T16:35:05Z

@LongCatIsLooong this is related to your changes in #49314, would you be able to review this?

LongCatIsLooong

After reading https://learn.microsoft.com/en-us/windows/win32/api/imm/nf-imm-immgetcompositionstringa it's still not super obvious to me what could have caused the caret to be misplaced.

/cc @cbracken

long TextInputManager::GetComposingCursorPosition() const {
  if (window_handle_ == nullptr) {
    return false;
  }

  ImmContext imm_context(window_handle_);
  if (imm_context.IsValid()) {
    // Read the cursor position within the composing string.
    return ImmGetCompositionString(imm_context.get(), GCS_CURSORPOS, nullptr,
                                   0);
  }
  return -1;
}

This looks a bit suspicious as the documentation says ImmGetCompositionString returns the byte length of the string. Does this always work for UTF16?

LongCatIsLooong · 2024-03-04T18:53:13Z

shell/platform/windows/text_input_plugin.cc

  std::string text_before_change = active_model_->GetText();
  TextRange composing_before_change = active_model_->composing_range();
  active_model_->AddText(text);
+  cursor_pos += active_model_->composing_range().extent();


IIRC cursor_pos is relative to the start of the new composing region. So this does not seem to be right.

However I suspect the AddText is no longer needed (and neither is text_after_change but that's unrelated to this PR.

It is related below:
Use start instead of extent for Windows IME cursor position #45667
Fix macOS text composing #49314

It was applied to solve problems when inputting Japanese and Chinese, but new issues are occurring in Korean.

Do you by chance have a repro with stack frames (especially the input arguments of this method when the cursor is placed at the wrong location)?

In the case of Korean, cursor_pos is based on the end of the new composing area.
When entering Korean, the text cursor is behind single space. #140739
https://bugs.chromium.org/p/chromium/issues/detail?id=653160

This PR did not pass the unittests below.

UpdateSelectionWhileComposing (text_input_model_unittests.cc),

CompositionCursorPos (text_input_plugin_unittest.cc)

So, this PR commits are incomplete and needs improvement.
However, I don't fully understand process of that unittest cases, so I'm checking it further.

@loic-sharma , @LongCatIsLooong When I looked it up, there was no branching with regular expressions in the engine code before.
So I'm not sure if adding regular expressions to check Korean is the right way, but I tried modifying the code to fix the issue.

When I ran the unit tests on the local engine, they all passed, but I am not sure if the difference in clang library version is the cause, but the unit tests failed on Mac and Linux.

Even if the above pull request is not approved, please reexamine to confirm the fix for the Korean issue.

LongCatIsLooong · 2024-03-05T01:24:24Z

shell/platform/common/text_input_model.cc

      composing_range_.collapsed() ? selection_ : composing_range_;
  text_.replace(rangeToDelete.start(), rangeToDelete.length(), text);
  composing_range_.set_end(composing_range_.start() + text.length());
+#if FML_OS_WIN


My understanding is that this class is meant to be an OS-agnostic abstraction, so having OS dependant logic here seems weird.

This issue (#140739) only occurs on windows.
It seems to have occurred after the modifications below.
#45667

Correct - this class is common code used by multiple OSes.

We should avoid #ifdef as much as possible since Flutter is used on many platforms, including some which are not part of our repo -- developers for those embedders can't patch in platform-specific #ifdefs so neither should we. (We'd also need platform specific additional testing etc.)

Instead, we should make the patches in the platform-specific embedder that requires updated behaviour. For the case where all platforms need an update we can put it in the common code.

I agree that it is not a good idea to include platform branching statements in code in the common area.

LongCatIsLooong · 2024-03-05T01:27:14Z

shell/platform/windows/text_input_plugin.cc

  std::string text_before_change = active_model_->GetText();
  TextRange composing_before_change = active_model_->composing_range();
  active_model_->AddText(text);
+  cursor_pos += active_model_->composing_range().extent();


Do you by chance have a repro with stack frames (especially the input arguments of this method when the cursor is placed at the wrong location)?

…an included in composing text.

gspencergoog · 2024-03-28T18:09:02Z

Hi @deniz-lee are you planning to follow up on the code review suggestions?

If not, could you please close this PR to get it off of our review queue?

(Desktop triage)

flutter-dashboard · 2024-04-17T18:14:09Z

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.

deniz-lee added 2 commits March 4, 2024 16:41

Reverts the logic for calculating the TextField input text cursor pos…

23d5261

…ition when inputting Korean characters (as 3.13.9) - Issue occurred since 3.16.x - flutter/flutter#140739

modify header includes

5926fa9

github-actions bot added affects: desktop platform-windows labels Mar 4, 2024

deniz-lee mentioned this pull request Mar 4, 2024

Reverts the logic for calculating the TextField input text cursor position when inputting Korean characters (3.13.9) #51161

Closed

8 tasks

LongCatIsLooong reviewed Mar 4, 2024

View reviewed changes

LongCatIsLooong reviewed Mar 5, 2024

View reviewed changes

deniz-lee added 5 commits March 11, 2024 10:57

Remove platform branching codes in the common area, add to check kore…

bc80cd9

…an included in composing text.

reformatting codes

10aefb3

reformatting codes

9a032ed

Modify checking to korean regular expression to wregex function

5387704

Remove unused variable.

d3640b1

deniz-lee requested review from LongCatIsLooong and cbracken March 11, 2024 09:14

deniz-lee closed this May 3, 2024

Revert text cursor position calculation windows #51164

Revert text cursor position calculation windows #51164

Uh oh!

Conversation

deniz-lee commented Mar 4, 2024

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Mar 4, 2024

Uh oh!

loic-sharma commented Mar 4, 2024

Uh oh!

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Mar 4, 2024

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Mar 4, 2024

Choose a reason for hiding this comment

Uh oh!

deniz-lee Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Mar 5, 2024

Choose a reason for hiding this comment

Uh oh!

deniz-lee Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deniz-lee Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Mar 5, 2024

Choose a reason for hiding this comment

Uh oh!

deniz-lee Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deniz-lee Mar 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Mar 5, 2024

Choose a reason for hiding this comment

Uh oh!

gspencergoog commented Mar 28, 2024

Uh oh!

flutter-dashboard bot commented Apr 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

deniz-lee Mar 5, 2024 •

edited

Loading

deniz-lee Mar 5, 2024 •

edited

Loading

deniz-lee Mar 11, 2024 •

edited

Loading

deniz-lee Mar 5, 2024 •

edited

Loading

cbracken Mar 5, 2024 •

edited

Loading

deniz-lee Mar 6, 2024 •

edited

Loading