Fix accessibility focus loss when first focusing on text field #17803

xster · 2020-04-18T07:16:06Z

I hope #17493 had good tests since I had to merge :)

The nature of the bug is that despite vague documentation, accessibilityElementsHidden on UIView doesn't seem to also hide its subviews from accessibility. When we first focus on a text field from the Flutter side, we create and add a FlutterTextInputView onto the view hierarchy above everything else. We intended to hide it but it never worked and since the accessibility bridge also sends various UIAccessibilityPostNotification, the new, higher, invisible FlutterTextInputView takes the focus.

I really wanted to have an integration test for this but unfortunately concluded that we can't have an exact test for this because:

VoiceOver (or any assistive technologies with accessibility focus) isn't implemented on the simulator.
Xcode has an accessibility debugger but unfortunately there are no APIs. The undocumented UIAccessibilityPostNotification(UIAccessibilityResumeAssistiveTechnologyNotification, UIAccessibilityNotificationVoiceOverIdentifier) crashes on simulator and UIAccessibilityNotificationSwitchControlIdentifier is input only (doesn't have focus highlighting).
The accessibility debugger also doesn't work the same way as the VoiceOver. The original issue doesn't actually reproduce using accessibility focus via the simulator accessibility debugger.

I added some integration tests for the general accessibility bridge semantics node translation and focusing functionalities, but it doesn't test this issue specifically.

xster · 2020-04-18T07:16:29Z

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

    _nonAutofillInputView = [[FlutterTextInputView alloc] init];
    _nonAutofillInputView.secureTextEntry = NO;
-    _nonAutofillInputView = [[FlutterTextInputView alloc] init];
+    _nonAutofillSecureInputView = [[FlutterTextInputView alloc] init];


This must be a bug

gaaclarke

Code looks fine. Did you see it's breaking a unit test? Man, these scenario tests are a mess. There is so many moving pieces it is going to be a nightmare to maintain this if it breaks since the linkage in the code isn't apparent. I wonder if there is some better way we can start organizing them so the dart code and the platform code are in the same folder so it's a bit more clear.

gaaclarke · 2020-04-20T17:06:18Z

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


-    if (![_activeView isDescendantOfView:_inputHider]) {
-      [_inputHider addSubview:_activeView];
+    if (!_activeView.window) {


if (_activeView.window != keyWindow) would be a bit safer.

ah right, thanks

gaaclarke · 2020-04-20T17:09:32Z

testing/scenario_app/ios/Scenarios/Scenarios/AppDelegate.m

-    flutterViewController = [[FlutterViewController alloc] initWithEngine:engine
-                                                                  nibName:nil
-                                                                   bundle:nil];
+    return [[FlutterViewController alloc] initWithEngine:engine nibName:nil bundle:nil];


Can you throw a FLUTTER_ASSERT_ARC or FLUTTER_ASSERT_NOT_ARC at the top of this file?

none of the files in testing/ has it. Do you think it's something we'll run into?

It's hard to review files if you don't know if they are ARC or not. You need to import FlutterMacros.h. The line I've highlighted is correct for ARC, a leak for not ARC.

I just meant whether we've had cases where we built the engine locally or on LUCI without ARC in the past? I would imagine everything failing spectacularly everywhere if we did rather than having to gate on a file-per-file basis. But I don't really have much context, I'm just guessing.

ARC can be turned on and off on a file-by-file basis. The Engine is built without ARC, this file is part of a whole different binary though and looks as if it is being compiled with ARC, which is fine. In a world where some objective-c code is ARC and some is not; it's hard to do code reviews. The macro is helpful until that magical day that will never come where all of our code is ARC.

got it. Thanks for the context. Added

gaaclarke · 2020-04-20T17:23:53Z

testing/scenario_app/ios/Scenarios/ScenariosUITests/TextSemanticsFocusTest.m

+  XCUIElement* textInputSemanticsObject =
+      [[[self.application textFields] matchingIdentifier:@"flutter textfield"] element];
+  XCTAssertTrue([textInputSemanticsObject waitForExistenceWithTimeout:5]);
+  XCTAssertEqual([textInputSemanticsObject valueForKey:@"hasKeyboardFocus"], @(NO));


XCTAssertFalse, you are doing pointer comparisons on objects.

FYI alternatively you could use, xctassertequalobjects.

I think @NO/__objc_no is a singleton and is a true-y (!= nil) value 🤣
But XCTAssertEqualObjects does show a nicer error when it doesn't match. Changed.

gaaclarke · 2020-04-20T17:24:45Z

testing/scenario_app/ios/Scenarios/ScenariosUITests/TextSemanticsFocusTest.m

+  textInputSemanticsObject =
+      [[[self.application textFields] matchingIdentifier:@"focused flutter textfield"] element];
+  XCTAssertTrue([textInputSemanticsObject waitForExistenceWithTimeout:5]);
+  XCTAssertEqual([textInputSemanticsObject valueForKey:@"hasKeyboardFocus"], @(YES));


XCTAssertTrue

gaaclarke · 2020-04-20T17:25:09Z

testing/scenario_app/ios/Scenarios/ScenariosUITests/TextSemanticsFocusTest.m

+  // real text entry events to the framework.
+  delegateTextInput = [[self.application textViews] element];
+  XCTAssertTrue([delegateTextInput waitForExistenceWithTimeout:5]);
+  XCTAssertEqual([delegateTextInput valueForKey:@"hasKeyboardFocus"], @(YES));


XCTAssertTrue

gaaclarke · 2020-04-20T17:26:19Z

testing/scenario_app/lib/src/channel_util.dart

+    ).buffer.asByteData(),
+    callback,
+  );
+}


gaaclarke · 2020-04-20T17:27:22Z

testing/scenario_app/lib/src/channel_util.dart

+
+/// Util method to replicate the behavior of a `MethodChannel` in the Flutter
+/// framework.
+void sendMethodCall({


We use the json codec typically not the standard message codec? maybe the name should reflect json or take in an optional argument of the codec?

oh ya good call. Only our system channels are being weird with json codecs. Good point. Name changed.

gaaclarke · 2020-04-20T17:29:37Z

testing/scenario_app/lib/src/send_text_focus_semantics.dart

+        currentValueLength: 0,
+        scrollChildren: 0,
+        scrollIndex: 0,
+        transform: Float64List.fromList(<double>[1.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 366.5, 0.0, 1.0]),


Isn't there a function somewhere where you can make a translation matrix? For readabilities sake you should at least have a local one.

I just copy pasted it from a real run but it doesn't actually matter for the purpose of this test. Removed.

xster · 2020-04-21T06:22:51Z

oops, I missed a lint.

ya I agree, it's a pile on right now. There were so many more things I wanted to do as I was looking at it :)

I want to clean up a bit more next

xster · 2020-04-22T07:19:07Z

shell/platform/darwin/ios/framework/Source/FlutterTextInputPluginTest.m

-  // Find the inactive autofillable input field.
+  // Find all the FlutterTextInputViews we created.
+  NSArray<FlutterTextInputView*>* inputFields = [[[[textInputPlugin textInputView] superview]
+      subviews]


can't believe this is what the autoformatter wants...

gaaclarke

lgtm modulo the presubmit failures.

flutter/engine#17803)

flutter/engine@2b94311...4f888d6 git log 2b94311..4f888d6 --first-parent --oneline 2020-04-25 [email protected] Change the repo fetch script used in integration tests (flutter/engine#17943) 2020-04-24 [email protected] Roll src/third_party/skia 1e21d14f2b8b..c12aad9485a9 (20 commits) (flutter/engine#17942) 2020-04-24 [email protected] Roll src/third_party/dart a69cb6d700f5..216e3df4526c (16 commits) (flutter/engine#17945) 2020-04-24 [email protected] Fix accessibility focus loss when first focusing on text field (flutter/engine#17803) 2020-04-24 [email protected] Roll fuchsia/sdk/core/linux-amd64 from _dAFU... to G4HpJ... (flutter/engine#17938) 2020-04-24 [email protected] [web] Fix exception when getting boxes for rich text range (flutter/engine#17933) 2020-04-24 [email protected] [web] Batch systemFontChange messages (flutter/engine#17885) 2020-04-24 [email protected] Roll fuchsia/sdk/core/linux-amd64 from kpECk... to _dAFU... (flutter/engine#17929) 2020-04-24 [email protected] Roll src/third_party/skia b965ff597315..1e21d14f2b8b (25 commits) (flutter/engine#17928) 2020-04-24 [email protected] Roll src/third_party/dart 94178e920ee8..a69cb6d700f5 (22 commits) (flutter/engine#17926)

flutter/engine#17803)

xster requested review from gaaclarke and goderbauer April 18, 2020 07:16

auto-assign bot requested a review from GaryQian April 18, 2020 07:16

googlebot added the cla: yes label Apr 18, 2020

xster removed the request for review from GaryQian April 18, 2020 07:16

xster commented Apr 18, 2020

View reviewed changes

gaaclarke reviewed Apr 20, 2020

View reviewed changes

xster force-pushed the fix-accessibility-focus-loss branch from 34688d7 to 636285d Compare April 22, 2020 07:11

xster commented Apr 22, 2020

View reviewed changes

gaaclarke approved these changes Apr 22, 2020

View reviewed changes

xster added 7 commits April 24, 2020 14:40

Fix accessibility focus loss when first focusing on text field

cca3a02

autoformat

159feab

lint

76a6c1d

review and add more functional change to match flutter#17493

279d239

autoformat

0e1d8ee

add arc check

d723fa9

we should check the xcscheme file in

28f5cca

xster force-pushed the fix-accessibility-focus-loss branch from de55c21 to 28f5cca Compare April 24, 2020 21:40

xster merged commit 2589d07 into flutter:master Apr 24, 2020

xster deleted the fix-accessibility-focus-loss branch April 24, 2020 22:44

engine-flutter-autoroll mentioned this pull request Apr 25, 2020

Roll engine 2b94311a7764..2589d07ee93d (7 commits) flutter/flutter#55612

Closed

engine-flutter-autoroll mentioned this pull request Apr 25, 2020

Roll engine 2b94311a7764..3999ef913043 (9 commits) flutter/flutter#55615

Closed

engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Apr 25, 2020

2589d07 Fix accessibility focus loss when first focusing on text field (

69bdbea

flutter/engine#17803)

engine-flutter-autoroll mentioned this pull request Apr 25, 2020

Roll engine 2b94311a7764..4f888d66250e (10 commits) flutter/flutter#55616

Closed

engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Apr 25, 2020

2589d07 Fix accessibility focus loss when first focusing on text field (

0b0ac5a

flutter/engine#17803)

engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Apr 25, 2020

2589d07 Fix accessibility focus loss when first focusing on text field (

bd66f9e

flutter/engine#17803)

engine-flutter-autoroll mentioned this pull request Apr 25, 2020

Roll engine 2b94311a7764..4f3b9295e008 (11 commits) flutter/flutter#55628

Closed

engine-flutter-autoroll mentioned this pull request Apr 27, 2020

Roll engine 2b94311a7764..66af9ea7041e (20 commits) flutter/flutter#55752

Closed

engine-flutter-autoroll mentioned this pull request Apr 27, 2020

Roll engine 4bcfae82c7c1..3b0e4153b7a4 (18 commits) flutter/flutter#55767

Closed

engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Apr 27, 2020

2589d07 Fix accessibility focus loss when first focusing on text field (

e622843

flutter/engine#17803)

engine-flutter-autoroll mentioned this pull request Apr 27, 2020

Roll engine 4bcfae82c7c1..aa00d5039637 (19 commits) flutter/flutter#55781

Closed

engine-flutter-autoroll mentioned this pull request Apr 27, 2020

Roll engine 4bcfae82c7c1..992a55c90d8f (21 commits) flutter/flutter#55785

Closed

This was referenced Apr 28, 2020

Roll engine 4bcfae82c7c1..027eff8b1955 (22 commits) flutter/flutter#55801

Closed

Roll engine 4bcfae82c7c1..50ae2b997d4b (24 commits) flutter/flutter#55807

Closed

engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Apr 28, 2020

2589d07 Fix accessibility focus loss when first focusing on text field (

e23a5d6

flutter/engine#17803)

engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Apr 28, 2020

2589d07 Fix accessibility focus loss when first focusing on text field (

8ad3f39

flutter/engine#17803)

engine-flutter-autoroll mentioned this pull request Apr 28, 2020

Roll engine 4bcfae82c7c1..22479ca5e718 (27 commits) flutter/flutter#55823

Closed

This was referenced Apr 28, 2020

Roll engine 4bcfae82c7c1..a1fdff6b1cb5 (28 commits) flutter/flutter#55828

Closed

Roll engine 4bcfae82c7c1..103c9c78fcd7 (29 commits) flutter/flutter#55830

Closed

engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Apr 28, 2020

2589d07 Fix accessibility focus loss when first focusing on text field (

6bae265

flutter/engine#17803)

engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Apr 28, 2020

2589d07 Fix accessibility focus loss when first focusing on text field (

e442b9c

flutter/engine#17803)

engine-flutter-autoroll mentioned this pull request Apr 28, 2020

Roll engine 4bcfae82c7c1..f07712bbfa87 (30 commits) flutter/flutter#55879

Closed

engine-flutter-autoroll mentioned this pull request Apr 28, 2020

Roll engine 4bcfae82c7c1..0c35a3417da7 (31 commits) flutter/flutter#55888

Closed

engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Apr 28, 2020

2589d07 Fix accessibility focus loss when first focusing on text field (

ea5a77b

flutter/engine#17803)

engine-flutter-autoroll mentioned this pull request Apr 28, 2020

Roll engine 4bcfae82c7c1..893cb3731cc6 (32 commits) flutter/flutter#55890

Closed

christopherfujino mentioned this pull request Apr 28, 2020

manual engine roll (4bcfae82c7c1 -> 0c35a3417) flutter/flutter#55891

Merged

gaaclarke mentioned this pull request Jul 29, 2021

testAccessibilityFocusOnTextSemanticsProducesCorrectIosViews flakey flutter/flutter#55778

Closed

Fix accessibility focus loss when first focusing on text field #17803

Fix accessibility focus loss when first focusing on text field #17803

Uh oh!

Conversation

xster commented Apr 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke 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!

xster commented Apr 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants