[web] Don't overwrite editing state with semantic updates #38271

htoor3 · 2022-12-13T23:42:00Z

We currently handle syncing text input editing state updates in 2 places: text_editing.dart (non-semantic path) and text_field.dart (semantic mode path). This redundancy is causing several text input issues in semantic mode, especially as it relates to text field value and selection state. Delegating this responsibility solely to text_editing.dart will fix this.

Fixes flutter/flutter#107634
Fixes flutter/flutter#90794

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.

yjbanov · 2022-12-14T18:00:16Z

lib/web_ui/test/engine/semantics/text_field_test.dart

@@ -159,7 +159,6 @@ void testMain() {
      expect(domDocument.activeElement, flutterViewEmbedder.glassPaneElement);
      expect(appHostNode.activeElement, strategy.domElement);
      expect(textField.editableElement, strategy.domElement);
-      expect((textField.editableElement as dynamic).value, 'hello');


This expectation used to verify that the semantics text value gets updated when the framework begins editing a text field. I think to cover that with the new approach we need a new test (or maybe just add it to this test) that sends the text editing state through the text input channel and checks that the editing state is applied to textField.editableElement.

This is a good point. I wrote it this way because we're delegating the responsibility of editing state updates to text_editing.dart, so it seems like the best place to test framework -> engine editing state updates would be in text_editing_test.dart which has existing tests that verify that behavior.

But is the idea here that we want to explicitly test whether that behavior applies specifically to semantic nodes?

I think we want to test the behavior of text_editing.dart when using a semantics editing strategy to make sure that the element's value is still being updated correctly.

…verwrite-semantic

htoor3 · 2022-12-14T22:12:52Z

lib/web_ui/test/engine/semantics/text_field_test.dart

+    });
+
+    test(
+        'Updates editing state when receiving framework messages from the text input channel',


cc @yjbanov @mdebbar

Perfect! Thanks.

mdebbar

nbayati

LGTM!

yjbanov

…lutter/engine#38271)

…117330) * 4a3a0ec67 pylint scripts under sky, remove dead scripts under sky/tools/roll (flutter/engine#38334) * 45713ea10 [web] Don't overwrite editing state with semantic updates (flutter/engine#38271)

…8271)" This reverts commit 45713ea.

…8271)" (#38562) This reverts commit 45713ea.

) * Remove editing state overwrite on semantic updates * Add tests * Fix test name * Assert that framework messages will update semantic editing state * Change test name * Whitespace

…utter#38271)" (flutter#38562) This reverts commit 45713ea.

…ates (#38271)" (#38562)" This reverts commit 5713a21.

…ates (#38271)" (#38562)" (#38854) This reverts commit 5713a21.

…lutter#117330) * 4a3a0ec67 pylint scripts under sky, remove dead scripts under sky/tools/roll (flutter/engine#38334) * 45713ea10 [web] Don't overwrite editing state with semantic updates (flutter/engine#38271)

…ates (flutter#38271)" (flutter#38562)" (flutter#38854) This reverts commit 5713a21.

htoor3 added 2 commits December 12, 2022 15:05

Remove editing state overwrite on semantic updates

717fcc0

Add tests

19d72e6

flutter-dashboard bot added the platform-web Code specifically for the web engine label Dec 13, 2022

Fix test name

acabdc0

htoor3 requested review from yjbanov, mdebbar and nbayati December 13, 2022 23:43

yjbanov reviewed Dec 14, 2022

View reviewed changes

htoor3 added 3 commits December 14, 2022 16:08

Assert that framework messages will update semantic editing state

85bb97e

Merge remote-tracking branch 'upstream/main' into remove-edit-state-o…

3bf4a84

…verwrite-semantic

Change test name

2e79071

htoor3 commented Dec 14, 2022

View reviewed changes

htoor3 added 2 commits December 15, 2022 14:00

Merge branch 'main' into remove-edit-state-overwrite-semantic

df642d4

Whitespace

0040d98

mdebbar approved these changes Dec 16, 2022

View reviewed changes

nbayati approved these changes Dec 16, 2022

View reviewed changes

htoor3 mentioned this pull request Dec 16, 2022

[web] Maintain text selection state in semantics config flutter/flutter#116754

Closed

8 tasks

yjbanov approved these changes Dec 19, 2022

View reviewed changes

htoor3 added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 19, 2022

auto-submit bot merged commit 45713ea into flutter:main Dec 19, 2022

engine-flutter-autoroll mentioned this pull request Dec 19, 2022

Roll Flutter Engine from 7bc519375b7b to 45713ea10510 (2 revisions) flutter/flutter#117330

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 19, 2022

45713ea10 [web] Don't overwrite editing state with semantic updates (f…

89043ce

…lutter/engine#38271)

XilaiZhang added a commit that referenced this pull request Dec 29, 2022

Revert "[web] Don't overwrite editing state with semantic updates (#3…

3e6d277

…8271)" This reverts commit 45713ea.

XilaiZhang mentioned this pull request Dec 29, 2022

[flutter roll] Revert "[web] Don't overwrite editing state with semantic updates" #38562

Merged

auto-submit bot pushed a commit that referenced this pull request Dec 29, 2022

Revert "[web] Don't overwrite editing state with semantic updates (#3…

5713a21

…8271)" (#38562) This reverts commit 45713ea.

loic-sharma pushed a commit to loic-sharma/flutter-engine that referenced this pull request Jan 3, 2023

Revert "[web] Don't overwrite editing state with semantic updates (fl…

e78f246

…utter#38271)" (flutter#38562) This reverts commit 45713ea.

htoor3 added a commit that referenced this pull request Jan 13, 2023

Revert "Revert "[web] Don't overwrite editing state with semantic upd…

79f41e3

…ates (#38271)" (#38562)" This reverts commit 5713a21.

auto-submit bot pushed a commit that referenced this pull request Jan 17, 2023

Revert "Revert "[web] Don't overwrite editing state with semantic upd…

5932bad

…ates (#38271)" (#38562)" (#38854) This reverts commit 5713a21.

ricardoamador pushed a commit to ricardoamador/engine that referenced this pull request Jan 25, 2023

Revert "Revert "[web] Don't overwrite editing state with semantic upd…

f55fd94

…ates (flutter#38271)" (flutter#38562)" (flutter#38854) This reverts commit 5713a21.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[web] Don't overwrite editing state with semantic updates #38271

[web] Don't overwrite editing state with semantic updates #38271

Uh oh!

htoor3 commented Dec 13, 2022

Uh oh!

yjbanov Dec 14, 2022

Uh oh!

htoor3 Dec 14, 2022

Uh oh!

mdebbar Dec 14, 2022

Uh oh!

htoor3 Dec 14, 2022

Uh oh!

yjbanov Dec 19, 2022

Uh oh!

mdebbar left a comment

Uh oh!

nbayati left a comment

Uh oh!

yjbanov left a comment

Uh oh!

Uh oh!

[web] Don't overwrite editing state with semantic updates #38271

[web] Don't overwrite editing state with semantic updates #38271

Uh oh!

Conversation

htoor3 commented Dec 13, 2022

Pre-launch Checklist

Uh oh!

yjbanov Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

htoor3 Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

mdebbar Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

htoor3 Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

yjbanov Dec 19, 2022

Choose a reason for hiding this comment

Uh oh!

mdebbar left a comment

Choose a reason for hiding this comment

Uh oh!

nbayati left a comment

Choose a reason for hiding this comment

Uh oh!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!