unhide uitextinput when focused #23776

chunhtai · 2021-01-19T22:24:49Z

To make the UITextInput work properly, its accessibilityElementsHidden must return NO.

This pr makes sure to toggle it on or off when it is focused. However, I am not sure if there will be any side effect when doing this. Will write test once we confirm this is what we want to do.

Fixes flutter/flutter#71885

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.
The reviewer has submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2021-01-19T22:24:52Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

gaaclarke

Nice detective work. I can't think of why this wouldn't be an acceptable fix. My biggest concern would be that we could swipe to have voiceover announce a widget that should be hidden after showTextInput is called but before hideTextInput. I'd make sure that couldn't happen with manual testing.

gaaclarke · 2021-01-20T01:36:35Z

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

  // mimic the semantics tree from Flutter. We want the text field to be represented as a
  // `TextInputSemanticsObject` in that `SemanticsObject` tree rather than in this
  // `FlutterTextInputView` bridge which doesn't appear above a text field from the Flutter side.


We should update this comment.

gaaclarke · 2021-01-20T01:37:51Z

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

 @property(nonatomic, readonly) CATransform3D editableTransform;
 @property(nonatomic, assign) CGRect markedRect;
 @property(nonatomic) BOOL isVisibleToAutofill;
+@property(nonatomic) BOOL hidden;


This should be called something like accessibilityHidden.

chunhtai · 2021-01-20T23:23:25Z

@gaaclarke thanks for reminding, this PR doesn't work, i am able to swipe to focus the FlutterTextInputView.

I couldn't find a way to be able to make voiceover respond to UITextInput event while keeping the FlutterTextInputView hidden from accessibility focus.

At this moment, I am thinking whether there is a way to point the TextInputSemanticsObject to the FlutterTextInputView

chinmaygarde · 2021-01-21T22:37:00Z

@chunhtai: Based on your last comment, it looks like this PR is not quite ready to land. Is that accurate? Can we close it or mark it WIP?

chunhtai · 2021-01-27T19:50:54Z

I have revised this pr. Test will be added once we confirm we want to proceed with this fix

There are three fail-safes I added to the fluttertextinputview

Add back the fluttertextinputview hider, This prevent fluttertextinputview from receiving swipping-to-focus
Adds delay enable logic to fluttertextinputview. It is a known issue that the fluttertextinputview may receive focus when it is activated during screen changes. This delay makes sure the voiceover cannot find the fluttertextinputview while it try to find the new focus due to screen changes.
fluttertextinputview will direct the focus back to the current active textfield if it ever receives focus. I added this fail-safe as the last resort. I have not able to focus the fluttertextinputview after adding the above two fail-safes, but i assumed it is still possible. If such unfortunate event happens, this will make sure the focus is not stuck at the fluttertextinputview.

chunhtai · 2021-01-27T19:52:54Z

@gaaclarke @xster Can you take another look whether this is what we want to do? I will add more tests once we confirm that.

gaaclarke

Jeez, this is hard to review. I understand it's a hack. Why did you choose 0.5s? If the problem we are trying to fix is the missing of the announcement of typed characters it seems like 0.5s could be ample time to miss a character.

gaaclarke · 2021-01-27T21:13:40Z

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

+  // with a semantics update sent to the engine. The voiceover will focus
+  // the newly attached active view while performing accessibility update.
+  // This results in accessibility focus stuck at the FlutterTextInputView.
+  [NSTimer scheduledTimerWithTimeInterval:_kUITextInputAccessibilityEnablingDelaySeconds


This causes the plugin to live beyond the point it would be dealloc'd previously. If you do this you should maintain a reference to the timer and invalidate it in dealloc.

gaaclarke · 2021-01-27T21:15:38Z

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

+ (UIAccessibilityElement*)backingTextInputAccessibilityObject {
+  return _backingTextInputAccessibilityObject;
+}
+
+ (void)setBackingTextInputAccessibilityObject:(UIAccessibilityElement*)element {
+  _backingTextInputAccessibilityObject = element;
+}


There is no need to make these methods, this should just be a property.

gaaclarke · 2021-01-27T21:17:14Z

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

+ * There are other cases the `FlutterTextInputView` may receive
+ * focus. One example is during screen changes, the accessibility
+ * tree will undergo a dramatic structural update. The Voiceover may
+ * decide to focus the `FlutterTextInputView` that is not involved
+ * in the structural update instead. If that happens, the
+ * `FlutterTextInputView` will make a best effort to direct the
+ * focus back to the `SemanticsObject`.


Couldn't this result in duplicate Voiceover announcements? One for the FlutterTextInputView and one SemanticsObject?

that is true, that is why i put the first two fail safe and hope it can catch all of the corner cases.

If this text view does receive the focus, duplicate announcement is still much better than focus just stuck at this textview.

I have tried a number of different hack such as changing the trait and label of this text view to prevent voiceover from pronouncing any word when the text view is focused, but none of them work. If you have a textview without any trait or label, it will pronounce some random phrase such as "blue sky" when it is focused.

gaaclarke · 2021-01-27T21:23:11Z

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

+  // with a semantics update sent to the engine. The voiceover will focus
+  // the newly attached active view while performing accessibility update.
+  // This results in accessibility focus stuck at the FlutterTextInputView.
+  [NSTimer scheduledTimerWithTimeInterval:_kUITextInputAccessibilityEnablingDelaySeconds


The proper constant name in objective-c doesn't have a _ at the beginning.

chunhtai · 2021-01-27T22:06:30Z

Jeez, this is hard to review. I understand it's a hack. Why did you choose 0.5s? If the problem we are trying to fix is the missing of the announcement of typed characters it seems like 0.5s could be ample time to miss a character.

I apologize for putting out such confusing workaround. I couldn't find a more systematic wait for delay this enable. The 0.5s is an eye ball value. we can probably decrease it even more, but overall do you think this is something we want to do?

Another idea is that we can wait until we detect any flutter semantics object has received focus before we enable the text view, but it will be much bigger than 0.5s.

chunhtai · 2021-02-01T21:49:39Z

@gaaclarke Hi 0.5 seems to be the lowest possible timeout; otherwise, the may still focus the textfield. I am not sure if this is device specific, but if the other low end iOS device can still focus textfield with 0.5 delay, they will be caught in fail safe 3.

chunhtai · 2021-02-01T21:51:00Z

I will do some more clean up, but i think this is the best we can do for now.

chinmaygarde · 2021-02-04T22:33:25Z

@gaaclarke Does your previous approval need to be revised? If so, can you review again please?

gaaclarke · 2021-02-04T22:49:08Z

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

+NSTimer* _enableFlutterTextInputViewAccessibilityTimer;
+


This is a global variable as written, you need to put this in braces.

oops nice catch. will update

gaaclarke · 2021-02-05T00:13:20Z

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

+- (void)removeEnableFlutterTextInputViewAccessibilityTimer {
+  if (_enableFlutterTextInputViewAccessibilityTimer) {
+    [_enableFlutterTextInputViewAccessibilityTimer invalidate];
+    _enableFlutterTextInputViewAccessibilityTimer = nil;


This is a leak, sorry i didn't see it earlier. You need to retain this below, and release this here.

gaaclarke · 2021-02-05T18:48:50Z

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

 - (void)dealloc {
  [self hideTextInput];
  [_reusableInputView release];
+  [_inputHider release];


there needs to be [_enableFlutterTextInputViewAccessibilityTimer release] as well =)

that will be trigger by hideTextInput, my thinking is that the timer is created in showtextinput, so it should be removed in hidetextinput

Yea, I think in practice you are right since the timer will retain the FlutterTextInputPlugin until the timer fires.

gaaclarke · 2021-02-05T18:49:20Z

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

+  if (_activeView.isFirstResponder) {
+    _activeView.accessibilityEnabled = YES;
+  }
+  _enableFlutterTextInputViewAccessibilityTimer = nil;


there needs to be a release here... maybe it would just be easier if you used a property for this.

thanks for reminding.

fluttergithubbot · 2021-02-09T22:21:02Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite Mac iOS Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

yasargil · 2021-02-16T09:08:42Z

Can i request that this change get cherry picked into the 1.26 release?

flutter-dashboard bot added the platform-ios label Jan 19, 2021

chunhtai requested a review from gaaclarke January 19, 2021 22:24

googlebot added the cla: yes label Jan 19, 2021

chunhtai requested a review from xster January 19, 2021 22:25

gaaclarke approved these changes Jan 20, 2021

View reviewed changes

chunhtai added the Work in progress (WIP) Not ready (yet) for review! label Jan 21, 2021

chunhtai changed the title ~~unhide uitextinput when focused~~ [WIP]unhide uitextinput when focused Jan 21, 2021

chunhtai force-pushed the issues/71885 branch from f796fc9 to 1f26688 Compare January 27, 2021 18:56

chunhtai removed the Work in progress (WIP) Not ready (yet) for review! label Jan 27, 2021

chunhtai changed the title ~~[WIP]unhide uitextinput when focused~~ unhide uitextinput when focused Jan 27, 2021

chunhtai requested a review from gaaclarke January 27, 2021 19:51

gaaclarke reviewed Jan 27, 2021

View reviewed changes

chunhtai added the Work in progress (WIP) Not ready (yet) for review! label Feb 1, 2021

chunhtai changed the title ~~unhide uitextinput when focused~~ [WIP]unhide uitextinput when focused Feb 1, 2021

chunhtai force-pushed the issues/71885 branch from 9101a8f to 2d9e990 Compare February 3, 2021 19:23

chunhtai removed the Work in progress (WIP) Not ready (yet) for review! label Feb 3, 2021

chunhtai changed the title ~~[WIP]unhide uitextinput when focused~~ unhide uitextinput when focused Feb 3, 2021

gaaclarke suggested changes Feb 4, 2021

View reviewed changes

gaaclarke suggested changes Feb 5, 2021

View reviewed changes

gaaclarke reviewed Feb 5, 2021

View reviewed changes

chunhtai added 13 commits February 9, 2021 10:52

unhide uitextinput when focused

6694c82

add proper initialization

5239620

temp

68d1d95

update

e7dbb0b

update

3b0ec2a

update

b8a5a0f

update

fb6b480

addressing comments

a708c93

remove unnnecesary line

41a1a4f

addressing comments

6bec04c

addressing cm

e1499f2

add the autorelease

c012e5b

update

822c05c

chunhtai force-pushed the issues/71885 branch from 65537df to 822c05c Compare February 9, 2021 18:52

chunhtai added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Feb 9, 2021

fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Feb 9, 2021

chunhtai added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Feb 9, 2021

fluttergithubbot merged commit b3a18f5 into flutter:master Feb 9, 2021

engine-flutter-autoroll mentioned this pull request Feb 10, 2021

Roll Engine from 73b1d3ff3f37 to b3a18f518282 (3 revisions) flutter/flutter#75746

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 10, 2021

b3a18f5 unhide uitextinput when focused (flutter/engine#23776)

8ec6fe6

hpoul mentioned this pull request Feb 12, 2021

unable to review text in text fields with a screen reader authpass/authpass#216

Closed

hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021

unhide uitextinput when focused (flutter#23776)

10279f9

ened mentioned this pull request Apr 12, 2021

Engine crash relating to input fields disappearing flutter/flutter#80308

Closed

koji-1009 mentioned this pull request Jan 5, 2025

[iOS] Fix view removal process for AutofillContextAction.cancel #57209

Closed

8 tasks

jmagman mentioned this pull request Aug 14, 2025

[ios][engine] Fix autofill context cleanup and view lifecycle management flutter/flutter#173598

Merged

9 tasks

		NSTimer* _enableFlutterTextInputViewAccessibilityTimer;

unhide uitextinput when focused #23776

unhide uitextinput when focused #23776

Uh oh!

Conversation

chunhtai commented Jan 19, 2021

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Jan 19, 2021

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!

chunhtai commented Jan 20, 2021

Uh oh!

chinmaygarde commented Jan 21, 2021

Uh oh!

chunhtai commented Jan 27, 2021

Uh oh!

chunhtai commented Jan 27, 2021

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!

chunhtai commented Jan 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chunhtai commented Feb 1, 2021

Uh oh!

chunhtai commented Feb 1, 2021

Uh oh!

chinmaygarde commented Feb 4, 2021

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!

fluttergithubbot commented Feb 9, 2021

Uh oh!

yasargil commented Feb 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

chunhtai commented Jan 27, 2021 •

edited

Loading