Update CompositionAwareMixin to correctly compute composingBase in Web engine #44139

Amir-P · 2023-07-29T14:10:11Z

It fixes an issue in CompositionAwareMixin causing wrong deltas being computed and reported to input clients resulting in wrong state when compositing text in rich text editors like Fleather and possibly others.

Reported deltas on Mac:

TextEditingDeltaInsertion: start: 0, end: 0, data: k
TextEditingDeltaInsertion: start: 1, end: 0, data: y
TextEditingDeltaReplacement: start: 0, end: 2, data: きょ
TextEditingDeltaInsertion: start: 2, end: 0, data: う
TextEditingDeltaInsertion: start: 3, end: 0, data: h
TextEditingDeltaReplacement: start: 0, end: 4, data: きょうは
TextEditingDeltaReplacement: start: 0, end: 4, data: 今日は

Reported deltas on Web (Before):

TextEditingDeltaInsertion: start: 0, end: 0, data: k
TextEditingDeltaInsertion: start: 1, end: 0, data: y
TextEditingDeltaReplacement: start: 0, end: 2, data: きょ
TextEditingDeltaInsertion: start: 2, end: 0, data: う
TextEditingDeltaInsertion: start: 3, end: 0, data: h
TextEditingDeltaReplacement: start: 0, end: 4, data: きょうは
TextEditingDeltaInsertion: start: 4, end: 0, data: 今日は

Reported deltas on Web (After):

TextEditingDeltaInsertion: start: 0, end: 0, data: k
TextEditingDeltaInsertion: start: 1, end: 0, data: y
TextEditingDeltaReplacement: start: 0, end: 2, data: きょ
TextEditingDeltaInsertion: start: 2, end: 0, data: う
TextEditingDeltaInsertion: start: 3, end: 0, data: h
TextEditingDeltaReplacement: start: 0, end: 4, data: きょうは
TextEditingDeltaReplacement: start: 0, end: 4, data: 今日は

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.

Renzo-Olivares

LGTM on the delta side of things, thanks for fixing this. cc @mdebbar @htoor3 for any insight/thoughts.

Renzo-Olivares · 2023-08-09T06:26:47Z

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

    }

-    final int composingBase = editingState.baseOffset! - composingText!.length;
+    final int composingBase = editingState.extentOffset! - composingText!.length;


This looks reasonable to me. Using baseOffset fails when composition changes the selection to be non-collapsed, composingBase turns out being negative so (-1,-1) is reported to the framework. This doesn't fail for non-delta input because for non-delta input we always replace the old editing state with the new one, while with delta input we require the correct ranges to correctly infer the difference.

Renzo-Olivares · 2023-08-09T06:29:28Z

lib/web_ui/test/engine/composition_test.dart


    group('determine composition state', () {
-      test('should return new composition state if valid new composition', () {
+      test('should return new composition state - compositing middle of text', () {


This is probably outside of the scope of this PR, but I noticed this test has the selection of the EditingState set to (100, 100), while the text it contains is only testing. The selection should always be in the bounds of the text.

#44139 (comment)

htoor3

Thank you for submitting this change! It looks mostly good, I left a few minor comments

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

htoor3 · 2023-08-09T15:31:36Z

lib/web_ui/test/engine/composition_test.dart

                composingExtentOffset: expectedComposingBase + composingText.length));
      });
+
+      test('should return new composition state - compositing from beginning of text', () {


This looks great! There are some untested code paths in determineCompositionState that we should probably add tests for - mainly the null checks and if composingBase < 0 (will this still occur with this change?)

I can't think of any scenario that this can happen but it's still possible. Do you think I can address adding tests along with #44139 (comment) in another PR? @htoor3

That works for me @Amir-P

…Base in Web engine (flutter/engine#44139)

…132522) flutter/engine@56a0e27...785b249 2023-08-14 amirpanahandeh@yahoo.com Update CompositionAwareMixin to correctly compute composingBase in Web engine (flutter/engine#44139) 2023-08-14 stuartmorgan@google.com Add application:openURLs: forwarding on macOS (flutter/engine#44689) 2023-08-14 skia-flutter-autoroll@skia.org Roll Skia from a4377099b25a to 69ea58157190 (1 revision) (flutter/engine#44692) 2023-08-14 chillers@google.com Update embedder_semantics_update.h imports to include flutter namespace (flutter/engine#44670) 2023-08-14 skia-flutter-autoroll@skia.org Roll Dart SDK from 3295a353980a to d445f5a18876 (1 revision) (flutter/engine#44691) 2023-08-14 dnfield@google.com Revert "Make run_tests.py assert that the ios test dylib is at least as new as libFlutter.dylib" (flutter/engine#44690) 2023-08-14 skia-flutter-autoroll@skia.org Roll Skia from 1cf6f71c8120 to a4377099b25a (1 revision) (flutter/engine#44688) 2023-08-14 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from JKTVoxVrHjZjBaxG4... to uIGMbDkXoIcpqWjgR... (flutter/engine#44687) 2023-08-14 jonahwilliams@google.com [Impeller] Conditionally set command debug info. (flutter/engine#44274) 2023-08-14 kjlubick@users.noreply.github.com Migrate more GL calls of GrBackend* (flutter/engine#44682) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from JKTVoxVrHjZj to uIGMbDkXoIcp If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC chinmaygarde@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Adds missing tests for `CompositionAwareMixin`. Check #44139 (comment) and #44139 (comment) [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

…b engine (flutter#44139) It fixes an issue in `CompositionAwareMixin` causing wrong deltas being computed and reported to input clients resulting in wrong state when compositing text in rich text editors like [Fleather](https://github.com/fleather-editor/fleather) and possibly others. Reported deltas on Mac: ```console TextEditingDeltaInsertion: start: 0, end: 0, data: k TextEditingDeltaInsertion: start: 1, end: 0, data: y TextEditingDeltaReplacement: start: 0, end: 2, data: ã��ã�� TextEditingDeltaInsertion: start: 2, end: 0, data: ã�� TextEditingDeltaInsertion: start: 3, end: 0, data: h TextEditingDeltaReplacement: start: 0, end: 4, data: ã��ã��ã��ã�¯ TextEditingDeltaReplacement: start: 0, end: 4, data: ä»�æ�¥ã�¯ ``` Reported deltas on Web (Before): ```console TextEditingDeltaInsertion: start: 0, end: 0, data: k TextEditingDeltaInsertion: start: 1, end: 0, data: y TextEditingDeltaReplacement: start: 0, end: 2, data: ã��ã�� TextEditingDeltaInsertion: start: 2, end: 0, data: ã�� TextEditingDeltaInsertion: start: 3, end: 0, data: h TextEditingDeltaReplacement: start: 0, end: 4, data: ã��ã��ã��ã�¯ TextEditingDeltaInsertion: start: 4, end: 0, data: ä»�æ�¥ã�¯ ``` Reported deltas on Web (After): ```console TextEditingDeltaInsertion: start: 0, end: 0, data: k TextEditingDeltaInsertion: start: 1, end: 0, data: y TextEditingDeltaReplacement: start: 0, end: 2, data: ã��ã�� TextEditingDeltaInsertion: start: 2, end: 0, data: ã�� TextEditingDeltaInsertion: start: 3, end: 0, data: h TextEditingDeltaReplacement: start: 0, end: 4, data: ã��ã��ã��ã�¯ TextEditingDeltaReplacement: start: 0, end: 4, data: ä»�æ�¥ã�¯ ``` * Fixes flutter/flutter#131335 * Fixes fleather-editor/fleather#150 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

Adds missing tests for `CompositionAwareMixin`. Check flutter#44139 (comment) and flutter#44139 (comment) [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

github-actions bot added the platform-web Code specifically for the web engine label Jul 29, 2023

Amir-P force-pushed the fix/wrong_delta_when_compositing_on_web branch 3 times, most recently from 1963f94 to edc289f Compare August 4, 2023 15:13

Renzo-Olivares self-requested a review August 8, 2023 20:43

Renzo-Olivares approved these changes Aug 9, 2023

View reviewed changes

htoor3 reviewed Aug 9, 2023

View reviewed changes

Update CompositionAwareMixin to correctly compute composingBase

169cb2f

Amir-P force-pushed the fix/wrong_delta_when_compositing_on_web branch from edc289f to 169cb2f Compare August 10, 2023 10:03

Update null-check

93be04a

Amir-P requested a review from htoor3 August 10, 2023 10:07

htoor3 approved these changes Aug 11, 2023

View reviewed changes

Renzo-Olivares added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 14, 2023

auto-submit bot merged commit 785b249 into flutter:main Aug 14, 2023

engine-flutter-autoroll mentioned this pull request Aug 14, 2023

Roll Flutter Engine from 56a0e2715dca to 785b24900a36 (10 revisions) flutter/flutter#132522

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 14, 2023

785b24900 Update CompositionAwareMixin to correctly compute composing…

c6df45a

…Base in Web engine (flutter/engine#44139)

Amir-P deleted the fix/wrong_delta_when_compositing_on_web branch August 15, 2023 04:13

This was referenced Aug 15, 2023

Typing Japanese alphabet does not yield correct result fleather-editor/fleather#150

Closed

Add more tests for CompositionAwareMixin #44717

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Update CompositionAwareMixin to correctly compute composingBase in Web engine #44139

Update CompositionAwareMixin to correctly compute composingBase in Web engine #44139

Uh oh!

Amir-P commented Jul 29, 2023 •

edited

Loading

Uh oh!

Renzo-Olivares left a comment

Uh oh!

Renzo-Olivares Aug 9, 2023

Uh oh!

Renzo-Olivares Aug 9, 2023

Uh oh!

Amir-P Aug 10, 2023

Uh oh!

htoor3 left a comment

Uh oh!

Uh oh!

htoor3 Aug 9, 2023

Uh oh!

Amir-P Aug 10, 2023

Uh oh!

htoor3 Aug 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Update CompositionAwareMixin to correctly compute composingBase in Web engine #44139

Update CompositionAwareMixin to correctly compute composingBase in Web engine #44139

Uh oh!

Conversation

Amir-P commented Jul 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Renzo-Olivares left a comment

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares Aug 9, 2023

Choose a reason for hiding this comment

Uh oh!

Renzo-Olivares Aug 9, 2023

Choose a reason for hiding this comment

Uh oh!

Amir-P Aug 10, 2023

Choose a reason for hiding this comment

Uh oh!

htoor3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

htoor3 Aug 9, 2023

Choose a reason for hiding this comment

Uh oh!

Amir-P Aug 10, 2023

Choose a reason for hiding this comment

Uh oh!

htoor3 Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Amir-P commented Jul 29, 2023 •

edited

Loading