Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[Web][Engine] Fix composingBaseOffset and composingExtentOffset value when input japanese text #161593

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 33 commits into from
Jun 3, 2025

Conversation

koji-1009
Copy link
Contributor

@koji-1009 koji-1009 commented Jan 14, 2025

fix #159671

When entering Japanese text and operating shift + ← || → || ↑ || ↓ while composing a character, setSelectionRange set (0,0) and the composing text is disappeared. For this reason, disable shit + arrow text shortcuts on web platform.

Movie

fixed

fixed.mov

master branch

master.mov

Pre-launch Checklist

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

@github-actions github-actions bot added a: text input Entering text in a text field or keyboard related problems engine flutter/engine repository. See also e: labels. platform-web Web applications specifically labels Jan 14, 2025
@koji-1009 koji-1009 marked this pull request as ready for review January 14, 2025 23:10
@koji-1009 koji-1009 changed the title fix: Skip updating selection range during composition [Web][Engine] Skip updating selection range during composition Jan 14, 2025
@flutter-dashboard
Copy link

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

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

@koji-1009 koji-1009 changed the title [Web][Engine] Skip updating selection range during composition [Engine][Web] Skip updating selection range during composition Jan 16, 2025
@justinmc justinmc requested a review from mdebbar February 13, 2025 21:50
Copy link
Contributor

@mdebbar mdebbar left a comment

Choose a reason for hiding this comment

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

Thank you for contributing a fix to this issue!

The test should go into this file: engine/src/flutter/lib/web_ui/test/engine/text_editing_test.dart. This file has many tests that do various things that you can copy from.

Here's how the test could look like:

  1. Create a text field using the showKeyboard helper.
  2. Set composingText to a non-null value.
  3. Send a TextInput.setEditingState.
  4. Expect the selection change hasn't been applied using checkInputEditingState.

These are some tests that may be relevant / similar to what you'll be writing:

I hope this helps. Let me know if you have any more questions.

@koji-1009 koji-1009 force-pushed the fix/web_composing_range branch from 5e8a061 to 04f2777 Compare March 21, 2025 12:32
}),
),
);
checkInputEditingState(textEditing!.strategy.domElement, 'へんかん', 4, 4);
Copy link
Contributor

Choose a reason for hiding this comment

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

In all these checks, the numbers match what was sent in selectionBase and selectionExtent. So it's not achieving the goal of the test which is to verify that selection wasn't updated on the DOM element.

To test this, you want to send some random numbers in selectionBase and selectionExtent, then check that the DOM element's selection hasn't changed to those random numbers.

Comment on lines 1960 to 1961
'composingBase': 0,
'composingExtent': 4,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does Flutter always send composingBase and composingExtent when the user is composing text? If yes, then we don't really need the new isComposing boolean, right? We can simply check for the presence of composingBase and/or composingExtent inside EditingState.applyToDomElement.

@koji-1009 koji-1009 force-pushed the fix/web_composing_range branch from 4aa715d to f5a2456 Compare March 22, 2025 00:01
@koji-1009
Copy link
Contributor Author

Sorry. I misunderstood that I needed to simulate the movement of Japanese IMEs. A test has been added to confirm this added process. Thank you for your support.

@koji-1009 koji-1009 requested a review from mdebbar March 22, 2025 05:20
Copy link
Contributor

@mdebbar mdebbar left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for contributing the fix!

Copy link
Contributor

@Renzo-Olivares Renzo-Olivares left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution and patience @koji-1009. I'm not sure if the selection being updated while composing is the root cause of the issue here.

When logging myself I see a few things,

  1. Sometimes the composing range cannot be determined in cases where the selection extent becomes smaller than the composing text extent.
  2. The framework is sending an update to the engine when we press shift + arrow key right/left.

I think 2 is important here because we want all text editing shortcuts to be handled natively on the web, so the framework shouldn't be sending this update when shift + arrow key right/left is pressed. I think 1 is less important since it doesn't interfere with the native webs composition, since composing range does not exist in native web and is not applied to the dom element, right now it is only used by the framework to draw the composing underline (should be a separate issue).

I think the fix for this should be in the framework, we can add the shift + arrow left/right to the web disabling shortcuts here

static final Map<ShortcutActivator, Intent> _webDisablingTextShortcuts =
.

  const SingleActivator(LogicalKeyboardKey.arrowLeft, shift: true):
      const DoNothingAndStopPropagationTextIntent(),
  const SingleActivator(LogicalKeyboardKey.arrowRight, shift: true):
      const DoNothingAndStopPropagationTextIntent(),

I tried this out and it seems to match the behavior I am observing in html's native TextArea.

Copy link
Contributor Author

@koji-1009 koji-1009 left a comment

Choose a reason for hiding this comment

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

@Renzo-Olivares
Thanks also for clarifying the root cause!

Because I hadn't noticed _webDisablingTextShortcuts, I thought it was correct not to call setSelectionRange while converting with the (Japanese) IME. It certainly seems to be a more suitable modification for flutter web to deal with on the framework side.

Adding 4 patterns, left and right, up and down, seems to solve the problem. Would the fix be to add a commit to this PR, or would it be better to open another PR?


Thank you so much for your support. I really appreciate it.

@Renzo-Olivares
Copy link
Contributor

@koji-1009, Doing it in this PR sounds good to me.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. and removed engine flutter/engine repository. See also e: labels. platform-web Web applications specifically labels Apr 15, 2025
@koji-1009 koji-1009 changed the title [Engine][Web] Skip updating selection range during composition [Web] Disable shift + arrow text shortcuts Apr 15, 2025
@Renzo-Olivares
Copy link
Contributor

Renzo-Olivares commented Apr 16, 2025

Looks like the failing test were because we previously moved these shortcuts to be handled by the framework to fix some issues related to scrolling, see #117217 and #114264. In that case i'm not sure we want to add them back to the disabling shortcuts since it would break those features again. We might want to do this another way since I noticed shortcuts like page down/page up/home/end #135454 are also behaving differently then native while composing.

I think maybe we should handle this at the EditableText level. For the affected actions, when on the web and composing we defer to the embedder.

@koji-1009
Copy link
Contributor Author

OK, I'll investigate.

@koji-1009 koji-1009 marked this pull request as draft April 16, 2025 01:27
@koji-1009 koji-1009 force-pushed the fix/web_composing_range branch from 9e5212e to 532a258 Compare April 17, 2025 00:58
@github-actions github-actions bot added engine flutter/engine repository. See also e: labels. platform-web Web applications specifically labels Apr 17, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: text input Entering text in a text field or keyboard related problems engine flutter/engine repository. See also e: labels. framework flutter/packages/flutter repository. See also f: labels. platform-web Web applications specifically will affect goldens Changes to golden files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Web] Composing Japanese letters disappear when shifting converting area
4 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.