Fix the Japanese IME problem on macOS reported in the following issue. #166291
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. 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.If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
github-actions
bot
added
engine
|
cc: @cbenhagen , @justinmc |
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
@cbracken can you take a look? |
cbracken
left a comment
cbracken
left a comment
There was a problem hiding this comment.
Thank you for your patch and apologies for missing this while I was out!
I think this is fine but I added a suggestion that might be a little cleaner. I'd like to know if you tried that approach, and if so, was it problematic?
| // !range.collapsed()), selection == composing_range will failed. Therefore, the selection | ||
| // cursor should only be placed at the beginning of composing_range. | ||
| flutter::TextRange composing_range = _activeModel->composing_range(); | ||
| _activeModel->SetSelection(flutter::TextRange(composing_range.start())); |
There was a problem hiding this comment.
Thanks for spotting this. This feels like we're working around a bug in TextInputRange::AddText/TextInputRange::DeleteSelected.
I think when I wrote that code, I made the incorrect assumption that during composing, we'd only ever have a selection that matches the composing range.
I wonder if we should fix the logic there or at least add some assertions. If you look at what happens in the case where the selection is a subrange of the composing range, it deletes the selection and resets the composing range.
Instead it looks like what we should be doing is setting the selection to a collapsed selection at the start of the composing range. Here's the current implementation:
void TextInputModel::AddText(const std::u16string& text) {
DeleteSelected();
if (composing_) {
// Delete the current composing text, set the cursor to composing start.
text_.erase(composing_range_.start(), composing_range_.length());
selection_ = TextRange(composing_range_.start());
composing_range_.set_end(composing_range_.start() + text.length());
}
size_t position = selection_.position();
text_.insert(position, text);
selection_ = TextRange(position + text.length());
}I wonder if we should change that to:
void TextInputModel::AddText(const std::u16string& text) {
if (composing_) {
// Delete the current composing text, set the cursor to composing start.
text_.erase(composing_range_.start(), composing_range_.length());
selection_ = TextRange(composing_range_.start());
composing_range_.set_end(composing_range_.start() + text.length());
} else {
DeleteSelected();
}
size_t position = selection_.position();
text_.insert(position, text);
selection_ = TextRange(position + text.length());
}I think the above is equivalent to your patch, but if we wanted to be a little more paranoid:
void TextInputModel::AddText(const std::u16string& text) {
if (composing_ && composing_range_ != selection_) {
selection_ = TextRange{composing_range_.start()};
}
DeleteSelected();
if (composing_) {
// Delete the current composing text, set the cursor to composing start.
text_.erase(composing_range_.start(), composing_range_.length());
selection_ = TextRange(composing_range_.start());
composing_range_.set_end(composing_range_.start() + text.length());
}
size_t position = selection_.position();
text_.insert(position, text);
selection_ = TextRange(position + text.length());
}There was a problem hiding this comment.
My RP was not comprehensive enough to grasp the overall structure, and I focused on ensuring the consistency of input/output for bugs that would prevent the app from being released when using Flutter.
If that can be achieved, I will leave the final revision method up to you.
There was a problem hiding this comment.
Isn't TextInputModel::AddText, which was presented, common code for all platforms?
I believe that this issue is limited to macOS IME behavior.
|
@cbracken can you take another look at this? |
cbracken
left a comment
cbracken
left a comment
There was a problem hiding this comment.
Apologies for the delay -- this lgtm.
cbracken
added
the
autosubmit
auto-submit
bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/166291, because This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.
|
|
@loic-sharma would you mind doing a secondary review? |
|
While we are waiting for @loic-sharma s review, can we also resolve the merge conflicts? |
vashworth
added
the
autosubmit
|
autosubmit label was removed for flutter/flutter/166291, because Pull request flutter/flutter/166291 is not in a mergeable state. |
auto-submit
bot
removed
the
autosubmit
|
The presubmit failures do look real. |
Japanese IME input is not working properly on macOS flutter#160935
|
I've fixed up the formatting issue locally and re-pushed. |
cbracken
added
the
autosubmit
flutter-dashboard
bot
removed
the
autosubmit
|
@hidea Thanks for the fix! PRのマージが遅くなってすみませんでした。 |
|
@cbracken |
flutter/flutter@c7362b4...0ab008a 2025-06-22 [email protected] Roll Skia from 203469ef4672 to fcd1c55da9cc (1 revision) (flutter/flutter#170986) 2025-06-22 [email protected] Roll Dart SDK from e5db5e0f81ba to 98db1db5ff65 (2 revisions) (flutter/flutter#170979) 2025-06-21 [email protected] Normalize input decoration theme (flutter/flutter#168981) 2025-06-21 [email protected] Roll Skia from dceb857b1c47 to 203469ef4672 (1 revision) (flutter/flutter#170959) 2025-06-21 [email protected] Roll Dart SDK from 6325aeacaef4 to e5db5e0f81ba (1 revision) (flutter/flutter#170958) 2025-06-21 [email protected] Roll Dart SDK from a6a3d65e33d7 to 6325aeacaef4 (2 revisions) (flutter/flutter#170955) 2025-06-21 [email protected] Roll Skia from 773188560221 to dceb857b1c47 (1 revision) (flutter/flutter#170954) 2025-06-21 [email protected] Roll Dart SDK from 750eaa028445 to a6a3d65e33d7 (1 revision) (flutter/flutter#170953) 2025-06-21 [email protected] Fix the Japanese IME problem on macOS reported in the following issue. (flutter/flutter#166291) 2025-06-21 [email protected] Roll Fuchsia Test Scripts from IfPORaYjQ2zP4bcq-... to 2wEtX_lDNhHhDNsP6... (flutter/flutter#170946) 2025-06-20 [email protected] Roll Skia from 104460420c48 to 773188560221 (2 revisions) (flutter/flutter#170942) 2025-06-20 [email protected] Normalize AppBarTheme (flutter/flutter#169130) 2025-06-20 [email protected] Close CupertinoContextMenu overlay if the widget is disposed or a new route is pushed (flutter/flutter#170186) 2025-06-20 [email protected] Roll Dart SDK from a554bdd0a2cc to 750eaa028445 (1 revision) (flutter/flutter#170933) 2025-06-20 [email protected] Roll Skia from b638003de37e to 104460420c48 (1 revision) (flutter/flutter#170932) 2025-06-20 [email protected] Roll Packages from 0ec4053 to 7f41e75 (1 revision) (flutter/flutter#170925) 2025-06-20 [email protected] Roll Skia from f1e68950ea7b to b638003de37e (5 revisions) (flutter/flutter#170923) 2025-06-20 [email protected] [licenses_cpp] jun17 (flutter/flutter#170845) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…r#9472) flutter/flutter@c7362b4...0ab008a 2025-06-22 [email protected] Roll Skia from 203469ef4672 to fcd1c55da9cc (1 revision) (flutter/flutter#170986) 2025-06-22 [email protected] Roll Dart SDK from e5db5e0f81ba to 98db1db5ff65 (2 revisions) (flutter/flutter#170979) 2025-06-21 [email protected] Normalize input decoration theme (flutter/flutter#168981) 2025-06-21 [email protected] Roll Skia from dceb857b1c47 to 203469ef4672 (1 revision) (flutter/flutter#170959) 2025-06-21 [email protected] Roll Dart SDK from 6325aeacaef4 to e5db5e0f81ba (1 revision) (flutter/flutter#170958) 2025-06-21 [email protected] Roll Dart SDK from a6a3d65e33d7 to 6325aeacaef4 (2 revisions) (flutter/flutter#170955) 2025-06-21 [email protected] Roll Skia from 773188560221 to dceb857b1c47 (1 revision) (flutter/flutter#170954) 2025-06-21 [email protected] Roll Dart SDK from 750eaa028445 to a6a3d65e33d7 (1 revision) (flutter/flutter#170953) 2025-06-21 [email protected] Fix the Japanese IME problem on macOS reported in the following issue. (flutter/flutter#166291) 2025-06-21 [email protected] Roll Fuchsia Test Scripts from IfPORaYjQ2zP4bcq-... to 2wEtX_lDNhHhDNsP6... (flutter/flutter#170946) 2025-06-20 [email protected] Roll Skia from 104460420c48 to 773188560221 (2 revisions) (flutter/flutter#170942) 2025-06-20 [email protected] Normalize AppBarTheme (flutter/flutter#169130) 2025-06-20 [email protected] Close CupertinoContextMenu overlay if the widget is disposed or a new route is pushed (flutter/flutter#170186) 2025-06-20 [email protected] Roll Dart SDK from a554bdd0a2cc to 750eaa028445 (1 revision) (flutter/flutter#170933) 2025-06-20 [email protected] Roll Skia from b638003de37e to 104460420c48 (1 revision) (flutter/flutter#170932) 2025-06-20 [email protected] Roll Packages from 0ec4053 to 7f41e75 (1 revision) (flutter/flutter#170925) 2025-06-20 [email protected] Roll Skia from f1e68950ea7b to b638003de37e (5 revisions) (flutter/flutter#170923) 2025-06-20 [email protected] [licenses_cpp] jun17 (flutter/flutter#170845) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…r#9472) flutter/flutter@c7362b4...0ab008a 2025-06-22 [email protected] Roll Skia from 203469ef4672 to fcd1c55da9cc (1 revision) (flutter/flutter#170986) 2025-06-22 [email protected] Roll Dart SDK from e5db5e0f81ba to 98db1db5ff65 (2 revisions) (flutter/flutter#170979) 2025-06-21 [email protected] Normalize input decoration theme (flutter/flutter#168981) 2025-06-21 [email protected] Roll Skia from dceb857b1c47 to 203469ef4672 (1 revision) (flutter/flutter#170959) 2025-06-21 [email protected] Roll Dart SDK from 6325aeacaef4 to e5db5e0f81ba (1 revision) (flutter/flutter#170958) 2025-06-21 [email protected] Roll Dart SDK from a6a3d65e33d7 to 6325aeacaef4 (2 revisions) (flutter/flutter#170955) 2025-06-21 [email protected] Roll Skia from 773188560221 to dceb857b1c47 (1 revision) (flutter/flutter#170954) 2025-06-21 [email protected] Roll Dart SDK from 750eaa028445 to a6a3d65e33d7 (1 revision) (flutter/flutter#170953) 2025-06-21 [email protected] Fix the Japanese IME problem on macOS reported in the following issue. (flutter/flutter#166291) 2025-06-21 [email protected] Roll Fuchsia Test Scripts from IfPORaYjQ2zP4bcq-... to 2wEtX_lDNhHhDNsP6... (flutter/flutter#170946) 2025-06-20 [email protected] Roll Skia from 104460420c48 to 773188560221 (2 revisions) (flutter/flutter#170942) 2025-06-20 [email protected] Normalize AppBarTheme (flutter/flutter#169130) 2025-06-20 [email protected] Close CupertinoContextMenu overlay if the widget is disposed or a new route is pushed (flutter/flutter#170186) 2025-06-20 [email protected] Roll Dart SDK from a554bdd0a2cc to 750eaa028445 (1 revision) (flutter/flutter#170933) 2025-06-20 [email protected] Roll Skia from b638003de37e to 104460420c48 (1 revision) (flutter/flutter#170932) 2025-06-20 [email protected] Roll Packages from 0ec4053 to 7f41e75 (1 revision) (flutter/flutter#170925) 2025-06-20 [email protected] Roll Skia from f1e68950ea7b to b638003de37e (5 revisions) (flutter/flutter#170923) 2025-06-20 [email protected] [licenses_cpp] jun17 (flutter/flutter#170845) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
6 participants
flutter/engine#57286
Fixes #160935