-
Notifications
You must be signed in to change notification settings - Fork 28.7k
[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
[Web][Engine] Fix composingBaseOffset and composingExtentOffset value when input japanese text #161593
Conversation
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. |
There was a problem hiding this 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:
- Create a text field using the
showKeyboard
helper. - Set
composingText
to a non-null value. - Send a
TextInput.setEditingState
. - 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:
showKeyboard(inputType: 'text'); editingStrategy.composingText = '뮤'; flutter/engine/src/flutter/lib/web_ui/test/engine/text_editing_test.dart
Lines 689 to 696 in e0676b4
const MethodCall setEditingState = MethodCall('TextInput.setEditingState', <String, dynamic>{ 'text': 'abcd', 'selectionBase': 2, 'selectionExtent': 3, }); sendFrameworkMessage(codec.encodeMethodCall(setEditingState)); checkInputEditingState(textEditing!.strategy.domElement, 'abcd', 2, 3);
I hope this helps. Let me know if you have any more questions.
engine/src/flutter/lib/web_ui/lib/src/engine/text_editing/text_editing.dart
Outdated
Show resolved
Hide resolved
engine/src/flutter/lib/web_ui/lib/src/engine/text_editing/text_editing.dart
Outdated
Show resolved
Hide resolved
engine/src/flutter/lib/web_ui/lib/src/engine/text_editing/text_editing.dart
Outdated
Show resolved
Hide resolved
engine/src/flutter/lib/web_ui/lib/src/engine/text_editing/text_editing.dart
Outdated
Show resolved
Hide resolved
engine/src/flutter/lib/web_ui/lib/src/engine/text_editing/text_editing.dart
Outdated
Show resolved
Hide resolved
engine/src/flutter/lib/web_ui/lib/src/engine/text_editing/text_editing.dart
Outdated
Show resolved
Hide resolved
engine/src/flutter/lib/web_ui/lib/src/engine/text_editing/text_editing.dart
Outdated
Show resolved
Hide resolved
5e8a061
to
04f2777
Compare
engine/src/flutter/lib/web_ui/test/engine/text_editing_test.dart
Outdated
Show resolved
Hide resolved
}), | ||
), | ||
); | ||
checkInputEditingState(textEditing!.strategy.domElement, 'へんかん', 4, 4); |
There was a problem hiding this comment.
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.
'composingBase': 0, | ||
'composingExtent': 4, |
There was a problem hiding this comment.
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
.
4aa715d
to
f5a2456
Compare
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. |
There was a problem hiding this 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!
engine/src/flutter/lib/web_ui/lib/src/engine/text_editing/text_editing.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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,
- Sometimes the composing range cannot be determined in cases where the selection extent becomes smaller than the composing text extent.
- 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
.
engine/src/flutter/lib/web_ui/test/engine/text_editing_test.dart
Outdated
Show resolved
Hide resolved
engine/src/flutter/lib/web_ui/test/engine/text_editing_test.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
engine/src/flutter/lib/web_ui/test/engine/text_editing_test.dart
Outdated
Show resolved
Hide resolved
engine/src/flutter/lib/web_ui/test/engine/text_editing_test.dart
Outdated
Show resolved
Hide resolved
@koji-1009, Doing it in this PR sounds good to me. |
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 |
OK, I'll investigate. |
9e5212e
to
532a258
Compare
…fset value when input japanese text (flutter/flutter#161593)
…fset value when input japanese text (flutter/flutter#161593)
…fset value when input japanese text (flutter/flutter#161593)
…fset value when input japanese text (flutter/flutter#161593)
…fset value when input japanese text (flutter/flutter#161593)
…fset value when input japanese text (flutter/flutter#161593)
…fset value when input japanese text (flutter/flutter#161593)
…fset value when input japanese text (flutter/flutter#161593)
…fset value when input japanese text (flutter/flutter#161593)
…fset value when input japanese text (flutter/flutter#161593)
…fset value when input japanese text (flutter/flutter#161593)
…fset value when input japanese text (flutter/flutter#161593)
…fset value when input japanese text (flutter/flutter#161593)
…fset value when input japanese text (flutter/flutter#161593)
…fset value when input japanese text (flutter/flutter#161593)
…fset value when input japanese text (flutter/flutter#161593)
…fset value when input japanese text (flutter/flutter#161593)
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.