Refactoring text editing. Strategy pattern is used to handle different browser/operating system and a11y behavior. #14131

nturgut · 2019-12-04T18:08:34Z

Refactoring text editing. Strategy pattern is used to handle different browser/operating system and a11y behavior.

There are 5 different text editing strategies:

DefaultTextEditingStrategy
AndroidTextEditingStrategy
IOSTextEditingStrategy
FirefoxTextEditingStrategy
AccesibiltyModeTextEditingStrategy

Tested manually for:

Chrome on Linux desktop
Chrome on Linux desktop a11y on with chromevox
Firefox on Linux desktop
Chrome on Android
Chrome on Android a11y on with talkback
Safari on IOS
Safari on IOS a11y on with voiceover

Pre-work for: flutter/flutter#43034

mdebbar

This looks really good! Cleaned up a lot of the if-else mess that we used to have.

I have a general comment about the new structure. We now have the following components:

(HybridTextEditing) <-> (TextEditingElement) <-> (TextEditingStrategy)

And it looks like TextEditingElement is an extra layer that isn't doing much other than delegating to TextEditingStrategy. What if we remove that layer, and provide a factory constructor on TextEditingStrategy that knows which class to instantiate based on browserEngine. And then, HybridTextEditing would talk directly with the TextEditingStrategy.

I didn't think deeply about it so maybe it won't work well, but I wanted to share the thought with you anyway. Especially in the case of accessibility, I'm not sure yet how we can do that.

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

nturgut · 2019-12-11T02:17:21Z

This looks really good! Cleaned up a lot of the if-else mess that we used to have.

I have a general comment about the new structure. We now have the following components:
(HybridTextEditing) <-> (TextEditingElement) <-> (TextEditingStrategy)
And it looks like TextEditingElement is an extra layer that isn't doing much other than delegating to TextEditingStrategy. What if we remove that layer, and provide a factory constructor on TextEditingStrategy that knows which class to instantiate based on browserEngine. And then, HybridTextEditing would talk directly with the TextEditingStrategy.

I didn't think deeply about it so maybe it won't work well, but I wanted to share the thought with you anyway. Especially in the case of accessibility, I'm not sure yet how we can do that.

I totally agree. Plus you are right accessibility has a special case. Shall I do it in the next PR since this will change semantics as well?

mdebbar · 2019-12-11T05:10:42Z

Shall I do it in the next PR since this will change semantics as well?

That sounds good!

nturgut · 2019-12-12T16:05:55Z

This looks really good! Cleaned up a lot of the if-else mess that we used to have.
I have a general comment about the new structure. We now have the following components:
(HybridTextEditing) <-> (TextEditingElement) <-> (TextEditingStrategy)
And it looks like TextEditingElement is an extra layer that isn't doing much other than delegating to TextEditingStrategy. What if we remove that layer, and provide a factory constructor on TextEditingStrategy that knows which class to instantiate based on browserEngine. And then, HybridTextEditing would talk directly with the TextEditingStrategy.
I didn't think deeply about it so maybe it won't work well, but I wanted to share the thought with you anyway. Especially in the case of accessibility, I'm not sure yet how we can do that.
I totally agree. Plus you are right accessibility has a special case. Shall I do it in the next PR since this will change semantics as well?

I did the change cause I realize, I had touch semantics code for the unit tests anyway.

Please have another look.

…different browser/operating system and a11y behaviour. Unit tests are missing. Documentation needs updating.

…textediting. fixing the unit tests. fixing comments

mdebbar

Looks good. Thanks for doing this!

I only have a few minor comments about docs and renaming.

mdebbar · 2019-12-12T18:31:39Z

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

-  /// There is no callback to know if the keyboard is up and how much the screen
-  /// has shifted. Therefore instead of listening to the shift and passing this
-  /// information to Flutter Framework, we are trying to stop the shift.
+  /// These event listener will be removed in [disable].


I think it's important to mention that event listeners need to be added to the _subscriptions list in order to be cleaned up properly.

mdebbar · 2019-12-12T18:35:47Z

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

-/// semantic tree. It also requires that the DOM element remains in the tree
-/// when the user isn't editing.
-class PersistentTextEditingElement extends TextEditingElement {
+class PersistentTextEditingElement extends DefaultTextEditingStrategy {


Should this be called PersistentTextEditingStrategy?

mdebbar · 2019-12-12T18:37:35Z

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

+  ///
+  /// It uses an HTML element to manage editing state when a custom element is
+  /// not provided via [useCustomEditableElement]
+  DefaultTextEditingStrategy _defaultEditingElement;


s/_defaultEditingElement/_defaultEditingStrategy

mdebbar · 2019-12-12T18:39:20Z

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

-  /// [_defaultEditableElement] is used instead.
-  TextEditingElement _customEditingElement;
+  /// [_defaultEditingElement] is used instead.
+  DefaultTextEditingStrategy _customEditingElement;


several variables/methods should be renamed from editing element to editing strategy.

…different browser/operating system and a11y behavior. (flutter/engine#14131)

* fb9dfe0 [fuchsia] Move async_get_default_dispatcher include to the header (flutter/engine#14351) * 3ebb7b4 Roll src/third_party/skia 75799967be60..3517aa7b14ad (3 commits) (flutter/engine#14345) * 2713225 Remove duplicate and outdated src/third_party/dart/tools/sdks entry from DEPS. (flutter/engine#14340) * 80d80ff Add ability to control dithering on Paint (flutter/engine#13868) * 8595361 Conditionally use offscreen root surface only when needed * 0a40f3d Assert that arc end caps on canvases with root surface transformations are drawn correctly. (flutter/engine#14359) * d698d96 Fix missing timeline event of flutter engine's startup time (flutter/engine#14319) * 9dc23b8 Fix missing API stream when record event in systrace (flutter/engine#14323) * 9e4c6ad Fix CGMutablePathRef memory leaks when the path is invalid. (flutter/engine#14275) * fc8cafb objcdoc fix for some ambiguity (flutter/engine#14367) * 9bafb3c [tests] Use distinct begin and end times (flutter/engine#14361) * 897ce34 Roll src/third_party/skia 3517aa7b14ad..826484f2631f (18 commits) (flutter/engine#14375) * 1ce85be [flutter_runner] Enable Skia tracing by default on Fuchsia (flutter/engine#13457) * a7b6ee5 Smart quote/dash configuration support in iOS (flutter/engine#13863) * 48ba39c Roll fuchsia/sdk/core/mac-amd64 from otkJA... to SlgE8... (flutter/engine#14407) * 0081e8c Remove unused _TypeNone enum field. (flutter/engine#14440) * d8edfea Roll src/third_party/dart d9fa37e85d5c..45db29709547 (48 commits) (flutter/engine#14453) * f650bca Refactoring text editing. Strategy pattern is used to handle different browser/operating system and a11y behavior. (flutter/engine#14131) * 4275b49 Fix type in build_fuchsia_artifacts (flutter/engine#14452) * 0c24f3d Roll src/third_party/skia 51b99659ed82..c514e7d9be6e (13 commits) (flutter/engine#14457) * ffbe2a4 [testing] Move test vsync waiters to their own TUs (flutter/engine#14456) * 181ad4e Use futures to images used for comparison with fixtures in embedder unit-tests. (flutter/engine#14465) * e0e0ac0 [testing] Make vsync waiters pluggable in shell_unittests (flutter/engine#14463)

…t browser/operating system and a11y behavior. (flutter#14131) * adding the default text editing strategy * [DRAFT] Refactoring text editing. Strategy pattern is used to handle different browser/operating system and a11y behaviour. Unit tests are missing. Documentation needs updating. * addressing PR comments * addressing PR comments. Fixing documentation * fixing persistenttextediting element which is used in a11y mode * removing texteditingelement and using texteditingstrategy from hybridtextediting. fixing the unit tests. fixing comments * fix unit tests * add todos for firefox tests * fixing chrome/android a11y issue

auto-assign bot requested a review from jason-simmons December 4, 2019 18:08

googlebot added the cla: yes label Dec 4, 2019

nturgut requested review from mdebbar and removed request for jason-simmons December 4, 2019 18:08

nturgut added platform-web Code specifically for the web engine affects: text input labels Dec 4, 2019

mdebbar reviewed Dec 4, 2019

View reviewed changes

Nurhan Turgut added 7 commits December 12, 2019 08:07

adding the default text editing strategy

1aca811

[DRAFT] Refactoring text editing. Strategy pattern is used to handle …

08ec6ec

…different browser/operating system and a11y behaviour. Unit tests are missing. Documentation needs updating.

addressing PR comments

3e4fa5f

addressing PR comments. Fixing documentation

34e79e7

fixing persistenttextediting element which is used in a11y mode

f794995

removing texteditingelement and using texteditingstrategy from hybrid…

17e5951

…textediting. fixing the unit tests. fixing comments

fix unit tests

38b37e2

nturgut force-pushed the close_connection_2 branch from 8b1ef3f to 38b37e2 Compare December 12, 2019 16:13

add todos for firefox tests

20bc4c0

nturgut changed the title ~~[DRAFT] Refactoring text editing. Strategy pattern is used to handle different browser/operating system and a11y behavior.~~ Refactoring text editing. Strategy pattern is used to handle different browser/operating system and a11y behavior. Dec 12, 2019

nturgut requested a review from mdebbar December 12, 2019 17:21

fixing chrome/android a11y issue

acceba4

mdebbar reviewed Dec 12, 2019

View reviewed changes

mdebbar approved these changes Dec 12, 2019

View reviewed changes

nturgut merged commit f650bca into flutter:master Dec 12, 2019

engine-flutter-autoroll mentioned this pull request Dec 13, 2019

Roll engine db60ebc6325c..e0e0ac0a6802 (23 commits) flutter/flutter#46930

Closed

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

f650bca Refactoring text editing. Strategy pattern is used to handle …

83c0d50

…different browser/operating system and a11y behavior. (flutter/engine#14131)

engine-flutter-autoroll mentioned this pull request Dec 13, 2019

Roll engine db60ebc6325c..e0e0ac0a6802 (23 commits) flutter/flutter#46932

Closed

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

f650bca Refactoring text editing. Strategy pattern is used to handle …

defafd3

…different browser/operating system and a11y behavior. (flutter/engine#14131)

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

f650bca Refactoring text editing. Strategy pattern is used to handle …

84fb246

…different browser/operating system and a11y behavior. (flutter/engine#14131)

engine-flutter-autoroll mentioned this pull request Dec 13, 2019

Roll engine db60ebc6325c..e0e0ac0a6802 (23 commits) flutter/flutter#46933

Merged

Refactoring text editing. Strategy pattern is used to handle different browser/operating system and a11y behavior. #14131

Refactoring text editing. Strategy pattern is used to handle different browser/operating system and a11y behavior. #14131

Uh oh!

Conversation

nturgut commented Dec 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdebbar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nturgut commented Dec 11, 2019

Uh oh!

mdebbar commented Dec 11, 2019

Uh oh!

nturgut commented Dec 12, 2019

Uh oh!

mdebbar left a comment

Choose a reason for hiding this comment

Uh oh!

mdebbar Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

mdebbar Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

mdebbar Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

mdebbar Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nturgut commented Dec 4, 2019 •

edited

Loading