iOS a11y text entry (~70% of it) #4575

yjbanov · 2018-01-19T22:36:50Z

This fixes many points from flutter/flutter#12786.

What works:

When "tabbed" or "dragged" into
- Text fields are announced as "Text field"
- The hint "double tap to edit"
- Content of text field is announced
- It is announced weather a text field is in edit mode
- The insert mode (e.g. character mode) is not announced.
Entered characters are echoed back
After inserting space the last word is echoed back
Swiping up or down moves the courser
Selecting text with reverse-pinch

What doesn't work:

No announcement when a text field gains focused via the "tap" gesture
The position of the cursor is not announced
Copy&paste does not work when rotor is in edit mode

/cc @cbracken who implemented our iOS text entry system

chinmaygarde

All the Objective-C objects need to be prefixed with some sort of a flutter specific identifier.

chinmaygarde · 2018-01-20T00:40:04Z

content_handler/accessibility_bridge.cc

@@ -20,7 +20,7 @@ AccessibilityBridge::AccessibilityBridge(app::ApplicationContext* context)
    : writer_(context->ConnectToEnvironmentService<maxwell::ContextWriter>()) {}

 void AccessibilityBridge::UpdateSemantics(
-    const std::vector<blink::SemanticsNode>& update) {
+    const std::unordered_map<int32_t, blink::SemanticsNode>& update) {


This cant possibly compile. The argument is being iterated over and the pair wont contain the id property.

Maybe typedef this to something and document the key.

Good catch. Which target uses this code?

I guess it's the "content_handler". Can I build and test it on a Mac? I can't find anything about it in https://github.com/flutter/engine/blob/master/CONTRIBUTING.md.

Checking that the default Fuchsia target builds should be fine. https://fuchsia.googlesource.com/docs/+/HEAD/getting_started.md

This look good now, right?

chinmaygarde · 2018-01-20T00:41:18Z

lib/ui/semantics/semantics_update.h

@@ -23,18 +23,19 @@ class SemanticsUpdate : public fxl::RefCountedThreadSafe<SemanticsUpdate>,

 public:
  ~SemanticsUpdate() override;
-  static fxl::RefPtr<SemanticsUpdate> create(std::vector<SemanticsNode> nodes);
+  static fxl::RefPtr<SemanticsUpdate> create(


Static method names start with an uppercase letter. So Create(...)

I'm not adding this method, just changing its signature. I'll happily do the clean-up, but I'd rather do that in a separate PR.

chinmaygarde · 2018-01-20T00:42:03Z

lib/ui/semantics/semantics_update.h


-  std::vector<SemanticsNode> nodes_;
+  std::unordered_map<int32_t, SemanticsNode> nodes_;


Please document what the new key means.

Created type alias SemanticsNodeUpdates and documented it.

chinmaygarde · 2018-01-20T00:44:14Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.h

@@ -8,11 +8,21 @@
 #include "flutter/shell/platform/darwin/ios/framework/Headers/FlutterChannels.h"
 #include "flutter/shell/platform/darwin/ios/framework/Source/FlutterTextInputDelegate.h"

+#import <UIKit/UIKit.h>


C headers go above the non-system C++ headers.

chinmaygarde · 2018-01-20T00:45:52Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

@@ -559,6 +559,10 @@ - (void)dealloc {
  [super dealloc];
 }

+- (FlutterTextInputView*)textInputView {


Please ensure that the signatures match. UIKit docs say that this needs to be UIView*. So lets keep that the same in both the header and implementation.

chinmaygarde · 2018-01-20T00:46:33Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.h

 #include "flutter/shell/platform/darwin/ios/framework/Source/FlutterView.h"
 #include "lib/fxl/macros.h"
 #include "third_party/skia/include/core/SkMatrix44.h"
 #include "third_party/skia/include/core/SkRect.h"

+#import <UIKit/UIKit.h>


This goes above the non-system C++ headers.

chinmaygarde · 2018-01-20T00:48:46Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.h

+ * Direct children of this semantics object. Each child's `parent` property must
+ * be equal to this object.
+ */
+@property(nonatomic, readonly) std::vector<SemanticsObject*>* children;


Please make these readonly properties const. For example, I doubt it is safe for external accessor of this object to mutate the vector of children.

So this would be something like:
@property(nonatomic, readonly) const std::vector<SemanticsObject*>& children;. If the vector itself can be nullptr, then you may use a pointer instead of a reference.

Actually SemanticsObject uses a retained mode API. It is mutated in-place as the semantics tree is updated.

chinmaygarde · 2018-01-20T00:50:27Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.h

+ */
+@property(nonatomic, readonly) std::vector<SemanticsObject*>* children;
+
+- (BOOL)willCauseLayoutChange:(const blink::SemanticsNode*)node;


This method name is not idiomatic Objective-C. -(BOOL)nodeWillCauseLayoutChange:... works better.

chinmaygarde · 2018-01-20T00:53:36Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.h

+ *
+ * This class is used by `TextInputSemanticsObject`.
+ */
+@interface InactiveTextInput : UIView<UITextInput>


There are no namespaces in Objective-C. So Flutter classes must have some sort of a prefix. So prepend FLT or Flutter to the Objective-C method name. @cbracken probably better knows the convention used.

Prefixed with Flutter, although I see we have a mix of names, some prefixed, others not. I'm guessing this namespace is not exported to developers.

Sorry - missed this patch earlier. Thanks for updating -- Flutter is the prefix we use for all the iOS side work.

chinmaygarde · 2018-01-20T00:54:14Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.h

+ *
+ * This class is used by `TextInputSemanticsObject`.
+ */
+@interface InactiveTextInput : UIView<UITextInput>


I don't see the implementation of this class and it is not in a public header. Did you forget to push some changes.

Look for changes in accessibility_bridge.mm (GitHub collapsed it)

chinmaygarde

Thanks for pointing out the collapsed file. I added more comments around the leaking ivars that are synthesized implicitly. Please also add the concrete implementations to a separate file.

chinmaygarde · 2018-01-20T01:12:55Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

  return YES;
 }

 @end

+@interface InactiveTextPosition : UITextPosition


Please put these in a separate file.

chinmaygarde · 2018-01-20T01:15:03Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

+
+@end
+
+@implementation InactiveTextPosition


Please explicitly synthesize all properties. Though the compiler will synthesize these properties for you, specifying the same makes the intent clearer and makes it obvious that the properties should be collected in dealloc in case ARC is not being used.

We do not use ARC, so all the properties implicitly synthesized and set by callers during the lifetime of the object will be leaked when this object is collected.

I'll attempt to remove these classes (see @goderbauer's suggestion to share these with the FlutterTextInputPlugin).

Done (removed these classes)

goderbauer · 2018-01-22T18:28:18Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.h

+@property(nonatomic, readonly) shell::AccessibilityBridge* bridge;
+
+/**
+ * The accessibility node used to produce this semantics object.


The semantic nodes... ?

goderbauer · 2018-01-22T18:30:45Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.h

+@end
+
+/**
+ * An implementation of `UITextInput` used for text fields that are not currently focused.


could we specify the type of focus here (e.g. input focus here as opposed to accessibility focus)?

goderbauer · 2018-01-22T18:31:38Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.h

+/**
+ * An implementation of `SemanticsObject` specialized for expressing text fields.
+ *
+ * Uses `InactiveTextInput` and `FlutterTextInputView` to express blurred and focused


Same. We should be specific about the focus type (input vs. accessibility focus) here to avoid future confusion.

goderbauer · 2018-01-22T18:32:59Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

@@ -108,8 +106,6 @@ - (instancetype)init {
  return nil;
 }

-#pragma mark - Designated initializers


Is this no longer true?

A glitch in the matrix. Reverted.

goderbauer · 2018-01-22T18:35:50Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

  return YES;
 }

 @end

+@interface InactiveTextPosition : UITextPosition


Why can we not reuse the TextPosition and TextRange from the active text input?

Is there every any danger that iOS asks us for a TextPosition or TextRange while the text field is unfocused (returning an InactiveTextPosition/Range) and then when the text field becomes focused iOS passes those InactiveTextPositions/Ranges back to the real text input view, which doesn't know what to do with them and potentially crashes?

goderbauer · 2018-01-22T18:37:52Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

+  return [self.text substringWithRange:textRange];
+}
+
+- (void)replaceRange:(UITextRange*)range withText:(NSString*)text {


Can we add comments to the empty and placeholder implementations to indicate if they have been left blank intentionally or if filling them in is a TODO?

goderbauer · 2018-01-22T18:40:51Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

+  if ([self node].HasFlag(blink::SemanticsFlags::kIsFocused)) {
+    // The text input view must have a non-trivial size for the accessibility
+    // system to send text editing events.
+    [self bridge]->textInputView().frame = CGRectMake(0.0, 0.0, 1.0, 1.0);


Just to confirm (and maybe to add to the comment): The textInputView itself is not focusable directly by the user, right? It remains hidden even though it has a size now?

Done. See the FlutterTextInputViewAccessibilityHider class in 07501fc.

goderbauer · 2018-01-22T18:41:43Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

+}
+
+- (UIAccessibilityTraits)accessibilityTraits {
+  // Adding UIAccessibilityTraitKeyboardKey to the trait list so that iOS treat it like


treat -> treats?

goderbauer · 2018-01-22T18:42:35Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

+  return [self textInputSurrogate].accessibilityTraits | UIAccessibilityTraitKeyboardKey;
+}
+
+- (CGPoint)accessibilityActivationPoint {


Why do we need to move the activation point to the center? Isn't that the default?

goderbauer · 2018-01-22T18:50:19Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

+
+#pragma mark - UIAccessibility overrides
+
+- (UIAccessibilityTraits)accessibilityTraits {


With this implementation, we currently don't support text fields that at the same time want to be "checked" or "selected" or express any other accessibility trait other than being a text field.

If that has to be that way, we should at least document that the SemanticsFlag.isTextField on iOS is mutually exclusive with many other flags.

Or, could we just or in the other accessibility traits in the TextInputSemanticsObject?

goderbauer · 2018-01-22T18:55:41Z

In the PR's description under "What works" you list

The insert mode (e.g. character mode) is not announced.

Should this be moved to "What doesn't work" or should the "not" be removed from the bullet point?

goderbauer · 2018-01-22T18:58:27Z

Also: Since the interactions here are fairly complicated, it would be sooo nice to have tests (especially to catch regressions). But setting up a test harness for this is probably outside of the scope of this PR... flutter/flutter#12287

yjbanov

TODO:

fix content_handler
move new classes into separate files
share TextRange/TextPosition impls
make sure FlutterTextInputView is not focusable

yjbanov · 2018-01-22T23:49:13Z

lib/ui/semantics/semantics_update.h

@@ -23,18 +23,19 @@ class SemanticsUpdate : public fxl::RefCountedThreadSafe<SemanticsUpdate>,

 public:
  ~SemanticsUpdate() override;
-  static fxl::RefPtr<SemanticsUpdate> create(std::vector<SemanticsNode> nodes);
+  static fxl::RefPtr<SemanticsUpdate> create(


I'm not adding this method, just changing its signature. I'll happily do the clean-up, but I'd rather do that in a separate PR.

yjbanov · 2018-01-23T00:16:55Z

lib/ui/semantics/semantics_update.h


-  std::vector<SemanticsNode> nodes_;
+  std::unordered_map<int32_t, SemanticsNode> nodes_;


Created type alias SemanticsNodeUpdates and documented it.

yjbanov · 2018-01-23T00:17:38Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.h

@@ -8,11 +8,21 @@
 #include "flutter/shell/platform/darwin/ios/framework/Headers/FlutterChannels.h"
 #include "flutter/shell/platform/darwin/ios/framework/Source/FlutterTextInputDelegate.h"

+#import <UIKit/UIKit.h>


yjbanov · 2018-01-23T00:20:16Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPlugin.mm

@@ -559,6 +559,10 @@ - (void)dealloc {
  [super dealloc];
 }

+- (FlutterTextInputView*)textInputView {


yjbanov · 2018-01-23T00:24:26Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.h

 #include "flutter/shell/platform/darwin/ios/framework/Source/FlutterView.h"
 #include "lib/fxl/macros.h"
 #include "third_party/skia/include/core/SkMatrix44.h"
 #include "third_party/skia/include/core/SkRect.h"

+#import <UIKit/UIKit.h>


yjbanov · 2018-01-23T00:47:18Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

@@ -108,8 +106,6 @@ - (instancetype)init {
  return nil;
 }

-#pragma mark - Designated initializers


A glitch in the matrix. Reverted.

yjbanov · 2018-01-23T01:36:42Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

+
+@end
+
+@implementation InactiveTextPosition


I'll attempt to remove these classes (see @goderbauer's suggestion to share these with the FlutterTextInputPlugin).

yjbanov · 2018-01-23T01:40:22Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

+  return [self.text substringWithRange:textRange];
+}
+
+- (void)replaceRange:(UITextRange*)range withText:(NSString*)text {


yjbanov · 2018-01-23T01:44:59Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

+}
+
+- (UIAccessibilityTraits)accessibilityTraits {
+  // Adding UIAccessibilityTraitKeyboardKey to the trait list so that iOS treat it like


yjbanov · 2018-01-23T01:53:39Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

+  return [self textInputSurrogate].accessibilityTraits | UIAccessibilityTraitKeyboardKey;
+}
+
+- (CGPoint)accessibilityActivationPoint {


goderbauer

LGTM for the accessibility side of things.

I defer to @cbracken and @chinmaygarde for the text input changes as well as general engine things.

goderbauer · 2018-01-23T21:05:05Z

shell/platform/darwin/ios/framework/Source/accessibility_text_entry.mm

+
+#pragma mark - UIAccessibility overrides
+
+- (void)setSemanticsNode:(const blink::SemanticsNode*)node {


nit: I believe this method and the next two are not really part of UIAccessibility protocol as indicated by the pragma above?

goderbauer · 2018-01-25T17:33:45Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

+      blink::SemanticsNode node = nodeEntry->second;
+      BOOL isTextField = node.HasFlag(blink::SemanticsFlags::kIsTextField);
+      BOOL wasTextField = object.node.HasFlag(blink::SemanticsFlags::kIsTextField);
+      if (wasTextField != isTextField) {


I believe, the way this is set up we will only act correctly on a switch from textfield to non-textfield (and vice versa) if the parent of the switched node is included in the semantics update? If only the switched node is included in the semantics update, there seems to be nothing updating that node's parent's child list to include the new node?

Maybe, instead of holding on to child objects, we need to switch that to just ids? And whenever you need the real object you need to look it up in the objects_ map, which would be the ultimate truth?

You're right. I need to restructure this.

OTOH, why don't we include parents in the update though? Tree depth is O(log(N)) of the size of the tree, so omitting parent nodes doesn't save anything performance-wise. If we had parents with the update, we could update everything in a single depth-first pass. Also, it would remove the need for the objects_ map.

WDYT?

The ideal case is that we have updates that send a single node only, so sending more nodes would be less good. :-)

True, assuming we don't lose elsewhere. For example, we currently pay for lack of parent information by having to maintain a fml::scoped_nsobject<NSMutableDictionary<NSNumber*, SemanticsObject*>> objects_ map that contains every single node in the tree (i.e. it's O(N) in size).

Right now, we assume that every node included in the update has changed (and on Android we need to tell the OS about every node that has changed). If we now include unchanged nodes, we'd have to mark those in the update to avoid reporting unchanged nodes as changed...

I was thinking of adding a SemanticsNodeUpdate class that has a SemanticsNode containing updated data. nullptr would indicate "there no update to the node itself, only its children".

However, that's coming way too close to @chinmaygarde's proposal for a unified retained-mode semantics tree, so I'm not going to do this as part of this change, and just swap out the child reference using the current system. We can come back to this later.

see 4fc35ac

update, after rebase it's now 5baa927

chinmaygarde

Minor nits but LGTM otherwise. Thanks!

chinmaygarde · 2018-01-25T19:38:41Z

content_handler/runtime_holder.h

@@ -65,7 +65,7 @@ class RuntimeHolder : public blink::RuntimeDelegate,
  std::string DefaultRouteName() override;
  void ScheduleFrame(bool regenerate_layer_tree = true) override;
  void Render(std::unique_ptr<flow::LayerTree> layer_tree) override;
-  void UpdateSemantics(std::vector<blink::SemanticsNode> update) override;
+  void UpdateSemantics(blink::SemanticsNodeUpdates update) override;


Why copy the collection in this call and not pass it by reference instead?

I realize you did not add this call. So it fine for us to address this in a future patch.

chinmaygarde · 2018-01-25T19:41:18Z

lib/ui/semantics/semantics_node.h

+ * The keys in the map are stable node IDd, and the values contain
+ * semantic information for the node corresponding to the ID.
+ */
+using SemanticsNodeUpdates = std::unordered_map<int32_t, SemanticsNode>;


We don't use javadoc/headerdoc format for docstrings in the engine. Lets follow the CPP guide for consistency go/fluttercpp#Function_Comments.

chinmaygarde · 2018-01-25T19:42:16Z

shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm

+  _bridge = nullptr;
+  _children.clear();
+  [_parent release];
+  if (_container != nil)


This nil check is unnecessary as calling the release selector on nil is a no-op already.

goderbauer

LGTM

Can you update flutter/flutter#12786 and/or file new bugs for the things that still don't work so we can track them?

yjbanov · 2018-02-05T23:14:28Z

I will update the issues.

`UIAccessibilityTraitKeyboardKey` was added in #4575 so that `TextInputSemanticsObject` supported VoiceOver gestures for text editing features, such as pinch to select text, and up/down fling to move cursor. After experimenting with it, I found that those features were still available even after removing `UIAccessibilityTraitKeyboardKey`. Fixes flutter/flutter#94465. In Touch Typing Mode: https://github.com/flutter/engine/assets/15619084/ccfe90ff-d3bc-427b-b1aa-9ec1242c0c89 In Standard Typing Mode: https://github.com/flutter/engine/assets/15619084/c78b1fb0-0816-41fb-9dd5-ed8b8a4cda2d [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

yjbanov requested a review from goderbauer January 19, 2018 22:36

googlebot added the cla: yes label Jan 19, 2018

yjbanov changed the title ~~iOS a11y text entry~~ iOS a11y text entry (~70% of it) Jan 20, 2018

chinmaygarde suggested changes Jan 20, 2018

View reviewed changes

chinmaygarde reviewed Jan 20, 2018

View reviewed changes

goderbauer reviewed Jan 22, 2018

View reviewed changes

yjbanov commented Jan 23, 2018

View reviewed changes

goderbauer approved these changes Jan 23, 2018

View reviewed changes

yjbanov force-pushed the ios-text-entry branch from d7a4ef9 to 07501fc Compare January 24, 2018 00:28

goderbauer reviewed Jan 25, 2018

View reviewed changes

chinmaygarde approved these changes Jan 25, 2018

View reviewed changes

yjbanov force-pushed the ios-text-entry branch 3 times, most recently from 4fc35ac to 5baa927 Compare January 31, 2018 20:09

goderbauer approved these changes Feb 5, 2018

View reviewed changes

yjbanov added 2 commits February 5, 2018 12:55

implement iOS text field editing in a11y mode

5ac6d64

address Chinmay's comments

95a9d0c

yjbanov force-pushed the ios-text-entry branch from 5baa927 to 5f4033e Compare February 5, 2018 21:54

replace node in child list when changing type

371e9b8

yjbanov force-pushed the ios-text-entry branch from 5f4033e to 371e9b8 Compare February 5, 2018 22:46

yjbanov merged commit 6dd49f0 into flutter:master Feb 5, 2018

yjbanov mentioned this pull request Feb 8, 2018

iOS a11y text entry is incomplete flutter/flutter#14543

Open

5 tasks

yjbanov deleted the ios-text-entry branch June 22, 2021 21:18

vashworth mentioned this pull request Apr 23, 2024

Remove UIAccessibilityTraitKeyboardKey to fix touch typing #52333

Merged

8 tasks


		std::vector<SemanticsNode> nodes_;
		std::unordered_map<int32_t, SemanticsNode> nodes_;


		#pragma mark - UIAccessibility overrides

		- (UIAccessibilityTraits)accessibilityTraits {


		#pragma mark - UIAccessibility overrides

		- (void)setSemanticsNode:(const blink::SemanticsNode*)node {

iOS a11y text entry (~70% of it) #4575

iOS a11y text entry (~70% of it) #4575

Uh oh!

Conversation

yjbanov commented Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

yjbanov commented Jan 19, 2018 •

edited

Loading