Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

iOS: A11y only disable during app resigning to background when voice over on #32820

Merged

Conversation

cyanglaz
Copy link
Contributor

This PR as another condition to when the accessibility is disabled. The new condition is when the voice over is on.

Disabling accessibility when app resigning to the background breaks XCUITests that contains a system popup. Because XCUITests uses accessibility but doesn't require the voice over feature to be on, we can keep the accessibility enabled while voice over is off so the tests would work again.

Fixes: flutter/flutter#93504

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.

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

@@ -826,7 +826,9 @@ - (void)applicationBecameActive:(NSNotification*)notification {

- (void)applicationWillResignActive:(NSNotification*)notification {
TRACE_EVENT0("flutter", "applicationWillResignActive");
self.view.accessibilityElementsHidden = YES;
if ([FlutterViewController isUIAccessibilityIsVoiceOverRunning]) {
self.view.accessibilityElementsHidden = YES;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about when someone is using a physical switch device or voice input? Does this break things in those cases?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @chunhtai

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks like it will break if voice control is turned on, unfortunately there isn't an API to detect whether voice control is on last time i checked

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we flip the sense of this and allow tests to pass some parameter/set some environment variable to override this behavior?

Copy link
Contributor Author

@cyanglaz cyanglaz Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need self.view.accessibilityElementsHidden = YES; if voice input is on? I remember self.view.accessibilityElementsHidden = YES; is to fix the exact issue where the voice over is on and the focus is jumped to flutter view.

Copy link
Contributor Author

@cyanglaz cyanglaz Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in the current code, we always hide accessibility elements. Now, with the change in this PR, we only hide the elements when voice over is turned on. This would actually be an improvement for voice input (by not hiding a11y elements when voice over is off)

I couldn't find any concrete example for voice input to test tho.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chunhtai @dnfield Friendly ping. Did my comment make sense? I'm happy to schedule a quick VC to discuss this :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the concern is whether "showing" the a11y elements when a dialog is up will confuse voice input.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's probably fine, but it would be good to test an app that has voice input turned on while it shows a permissions dialog, and make sure you can use voice input there the same as you would be able to use it in a regular native iOS app in that situation. I'm not familiar enough with voice input to know how to test this myself.

If a manual test shows that it works ok, that's probably the best we can do. If not then we need to figure out how to change this only for tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with voice control, it seems working fine. See below video:
https://drive.google.com/file/d/14DErLf2G_f_cfd6miU-DyJIhzMBWIW0L/view?usp=sharing

@zanderso
Copy link
Member

From PR triage: @dnfield @chunhtai it looks like this PR is ready for another review pass.

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cyanglaz cyanglaz 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 Apr 29, 2022
@fluttergithubbot fluttergithubbot merged commit bbc9bd8 into flutter:main Apr 29, 2022
@cyanglaz cyanglaz deleted the a11y_only_disable_when_voice_over_on branch April 29, 2022 17:42
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
platform-ios waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flutter iOS apps cannot interact with permission popups in UI Tests
5 participants