Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@cbracken
Copy link
Member

@cbracken cbracken commented Mar 5, 2022

When the embedder receives a TextInput.clearClient message from the
framework (typically when a text field loses focus), if the user is
currently inputting composing text using an IME, commit the composing
text, end composing, and clear the IME's composing state.

This also exposes a public editingState getter on
FlutterTextInputPlugin as part of the TestMethods informal protocol.
This allows us to get at the text editing state as a dictionary in
tests.

Issue: flutter/flutter#92060

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 Hixie said 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.

When the embedder receives a TextInput.clearClient message from the
framework (typically when a text field loses focus), if the user is
currently inputting composing text using an IME, commit the composing
text, end composing, and clear the IME's composing state.

This also exposes a public `editingState` getter on
FlutterTextInputPlugin as part of the TestMethods informal protocol.
This allows us to get at the text editing state as a dictionary in
tests.

Issue: flutter/flutter#92060
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question about the text in the field after unfocusing, otherwise LGTM 👍

I'm not the strictest person for engine code quality, but the way you tested this seems ok to me. I also have struggled to find ways to test stuff like this without exposing something, and it's better than not testing it properly I think.

Comment on lines +280 to +281
_activeModel->CommitComposing();
_activeModel->EndComposing();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the end result that the composing text (e.g. "nihao") is in the field or the first choice characters ("你好")? If I try this on non-Flutter web or Mac, it seems to keep the composing text.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep the end result is that we commit the current composing text and it remains in the field. Then we clear it from the IME's buffer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool that matches what I see on native then 👍

Comment on lines +370 to +371
if (!composing_range.collapsed() && !_activeModel->composing()) {
_activeModel->BeginComposing();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this let you resume composing after unfocusing the field and coming back? Or maybe it's just cleanup.

Copy link
Member Author

@cbracken cbracken Mar 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is technically unrelated cleanup. This just ensures that if the framework tells us we have composing text, but the engine wasn't already composing, we set it to composing mode. Adding the test uncovered this bug.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants