Fix web EditableText Widget composing range error #25673

songfei · 2021-04-20T12:07:38Z

EditableText widget should receive correct composing range, I fix it.

example code:

EditableText(
                controller: controller,
                focusNode: node,
                autocorrect: false,
                style: TextStyle(
                  fontSize: 20.0,
                  color: Colors.red,
                ),
                cursorColor: Colors.red,
                backgroundCursorColor: Colors.black,
                inputFormatters: [LengthLimitingTextInputFormatter(5)],
              )

before:
https://user-images.githubusercontent.com/1343893/115393137-ef6e6580-a213-11eb-8246-6540ba6da13d.mov

fixed:
https://user-images.githubusercontent.com/1343893/115393169-fac19100-a213-11eb-866e-0cea96a626fe.mov

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

flutter/flutter#65357

Pre-launch Checklist

[ x ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
[ x ] I read the [Tree Hygiene] wiki page, which explains my responsibilities.
[ x ] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
[ x ] I listed at least one issue that this PR fixes in the description above.
[ x ] 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.
[ x ] I updated/added relevant documentation (doc comments with ///).
[ x ] I signed the [CLA].
[ x ] All existing and new tests are passing.
[ x ] The reviewer has submitted any presubmit flakes in this PR using the [engine presubmit flakes form] before re-triggering the failure.

songfei · 2021-05-06T02:56:11Z

@mdebbar @yjbanov Is there any message?

mdebbar

Thanks for the PR!

I apologize for the delay in review as I wasn't aware of this PR. Please add us as reviewers. That's the most reliable way to get our attention.

mdebbar · 2021-07-08T17:04:29Z

lib/web_ui/lib/src/engine/text_editing/text_editing.dart

+    domElement.addEventListener('compositionstart', _handleCompositionStart);
+    domElement.addEventListener('compositionupdate', _handleCompositionChange);
+    domElement.addEventListener('compositionend', _handleCompositionEnd);
+


The same needs to be repeated in a few places. Let's put it in a method:

html.EventListener? compositionStart; html.EventListener? compositionUpdate; html.EventListener? compositionEnd; void addCompositionHandlers() { compositionStart = _handleCompositionStart; compositionUpdate = _handleCompositionUpdate; compositionEnd = _handleCompositionEnd; domElement.addEventListener('compositionstart', compositionStart); domElement.addEventListener('compositionupdate', compositionChange); domElement.addEventListener('compositionend', compositionEnd); }

and call this method in all subclass of TextEditingStrategy.

mdebbar · 2021-07-08T17:05:43Z

lib/web_ui/lib/src/engine/text_editing/text_editing.dart

+    domElement.removeEventListener('compositionstart', _handleCompositionStart);
+    domElement.removeEventListener(
+        'compositionupdate', _handleCompositionChange);
+    domElement.removeEventListener('compositionend', _handleCompositionEnd);


Similar to the suggestion above, let's put this into a method:

void removeCompositionHandlers() { domElement.removeEventListener('compositionstart', compositionStart); domElement.removeEventListener( 'compositionupdate', compositionChange); domElement.removeEventListener('compositionend', compositionEnd); }

and call from this disable() implementation and the one in: https://github.com/flutter/engine/blob/master/lib/web_ui/lib/src/engine/semantics/text_field.dart#L106

mdebbar · 2021-07-08T17:06:11Z

lib/web_ui/lib/src/engine/text_editing/text_editing.dart

+      this.composingBase,
+      this.composingExtent})


Should we set a default value of -1 here?

mdebbar · 2021-07-08T18:11:48Z

lib/web_ui/lib/src/engine/text_editing/text_editing.dart

+    if (composingText != null && newEditingState.text != null) {
+      int composingBase = newEditingState.text!.lastIndexOf(composingText!);
+      if (composingBase >= 0) {
+        newEditingState = newEditingState.copyWith(
+            composingBase: composingBase,
+            composingExtent: composingBase + composingText!.length);
+      }
+    }


Overall, the PR looks good, except this part. Doing a lastIndexOf() doesn't always get you the right index. The cursor could be in the middle of the text when composing starts.

From a quick try, I noticed that compositionupdate is always fired before the input event. So composingText is always up to date by the time we reach the _handleChange code here.

Instead of doing a lastIndexOf() to find the index of composingText, what if we use newEditingState.baseOffset? Something like this maybe:

final int compositionExtent = newEditingState.baseOffset final int compositionBase = compositionExtent - composingText.length;

mdebbar · 2021-07-08T18:13:33Z

lib/web_ui/test/text_editing_test.dart

-            'selectionExtent': 9
+            'selectionExtent': 9,
+            'composingBase': -1,
+            'composingExtent': -1


Let's add a test (or more) that sends a few composition events and makes sure they are handled correctly.

tkfinitefield · 2021-10-14T11:47:52Z

Is there any problem that hinders this PR from merging into main branch? I want to help if any because our project is also affected by this issue.

flutter-dashboard · 2022-01-12T18:30:28Z

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.

Hixie · 2022-02-23T00:31:55Z

@songfei Looks like there are some review comments to fix before we can land this, are you still interested in working on it?

@tkfinitefield if @songfei is no longer able to work on this I imagine they would be perfectly happy for you to take over and finish it. See the comments above for what needs to happen before it can land.

Thanks!

Hixie · 2022-05-03T23:24:46Z

@songfei Thanks for your contribution. Unfortunately as written we can't take this patch, as per the review comments above. I'm going to close this PR since there has been no progress for a while, but please don't hesitate to file a new one if you do have time to get back to it. Equally if anyone else would like to start with this PR and take it forward, you are most welcome to do so.

fix web EditableText Widget composing range

465ac51

flutter-dashboard bot added the platform-web Code specifically for the web engine label Apr 20, 2021

google-cla bot added the cla: yes label Apr 20, 2021

mdebbar reviewed Jul 8, 2021

View reviewed changes

CaseyHillers changed the base branch from master to main November 15, 2021 18:16

fluttergithubbot force-pushed the master branch from b1a8771 to 2962099 Compare November 15, 2021 18:30

godofredoc changed the base branch from master to main November 24, 2021 07:28

Hixie mentioned this pull request May 3, 2022

TextInputClient should receive correct composing range or ignore it on Flutter Web flutter/flutter#65357

Closed

Hixie closed this May 3, 2022

antholeole mentioned this pull request May 24, 2022

Fix web editable text composing range #33590

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix web EditableText Widget composing range error #25673

Fix web EditableText Widget composing range error #25673

Uh oh!

songfei commented Apr 20, 2021

Uh oh!

songfei commented May 6, 2021

Uh oh!

mdebbar left a comment •

edited

Loading

Uh oh!

mdebbar Jul 8, 2021

Uh oh!

mdebbar Jul 8, 2021

Uh oh!

mdebbar Jul 8, 2021

Uh oh!

mdebbar Jul 8, 2021

Uh oh!

mdebbar Jul 8, 2021

Uh oh!

tkfinitefield commented Oct 14, 2021

Uh oh!

flutter-dashboard bot commented Jan 12, 2022

Uh oh!

Hixie commented Feb 23, 2022

Uh oh!

Hixie commented May 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Fix web EditableText Widget composing range error #25673

Fix web EditableText Widget composing range error #25673

Uh oh!

Conversation

songfei commented Apr 20, 2021

Pre-launch Checklist

Uh oh!

songfei commented May 6, 2021

Uh oh!

mdebbar left a comment • 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!

mdebbar Jul 8, 2021

Choose a reason for hiding this comment

Uh oh!

mdebbar Jul 8, 2021

Choose a reason for hiding this comment

Uh oh!

mdebbar Jul 8, 2021

Choose a reason for hiding this comment

Uh oh!

mdebbar Jul 8, 2021

Choose a reason for hiding this comment

Uh oh!

mdebbar Jul 8, 2021

Choose a reason for hiding this comment

Uh oh!

tkfinitefield commented Oct 14, 2021

Uh oh!

flutter-dashboard bot commented Jan 12, 2022

Uh oh!

Hixie commented Feb 23, 2022

Uh oh!

Hixie commented May 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mdebbar left a comment •

edited

Loading