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

iOS accessibility ignore layout change if app is not active #32511

Closed
wants to merge 1 commit into from

Conversation

cyanglaz
Copy link
Contributor

@cyanglaz cyanglaz commented Apr 7, 2022

When the app is not active, make the accessibility bridge ignore layout changes, which leads to refocus a11y to an item on the inactive app.

Fixes: flutter/flutter#93030
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.

@jmagman
Copy link
Member

jmagman commented Apr 7, 2022

@gaaclarke may be familiar with this

@cyanglaz cyanglaz requested a review from gaaclarke April 7, 2022 20:20

if (!ios_delegate_->IsFlutterViewControllerPresentingModalViewController(view_controller_)) {
if (!ios_delegate_->IsFlutterViewControllerPresentingModalViewController(view_controller_) &&
[UIApplication sharedApplication].applicationState == UIApplicationStateActive) {
Copy link
Contributor Author

@cyanglaz cyanglaz Apr 7, 2022

Choose a reason for hiding this comment

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

I had a typo here that made it "worked". Looks like with a popup on the screen, the state is still active, so [UIApplication sharedApplication].applicationState should not be used as an indicator here. Will need to find another way to do it. Setting this PR to draft until I fix it

@cyanglaz cyanglaz marked this pull request as draft April 7, 2022 20:26
@@ -817,7 +817,6 @@ - (void)dealloc {

- (void)applicationBecameActive:(NSNotification*)notification {
TRACE_EVENT0("flutter", "applicationBecameActive");
self.view.accessibilityElementsHidden = NO;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am confused at the change, it seems like the a11y was hidden when the view is not active prior to this change. and this change makes it so it does not hide the a11y when the view is not active?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code was a workaround to fix the same issue that this PR is trying to fix. Since we found the root cause and potentially a fix with this PR; we can remove the old workaround.
The workaround was introduced in 0be0303

@flutter-dashboard
Copy link

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

LGTM, this is a draft let me know if you want me to approve it. It's got tests and makes sense to me.

@@ -932,7 +932,7 @@ - (void)testHideOverlay {
engine.viewController = nil;
}

- (void)testHideA11yElements {
- (void)testDoNotHideA11yElements {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should just rip out this test now that we are undoing the work that caused it.

@cyanglaz
Copy link
Contributor Author

cyanglaz commented Apr 7, 2022

@gaaclarke surprisedly the application state is active while there's a popup, so the fix actually didn't work. (Doesn't make sense to me) I'm trying to see if there is a way to detect the application state more accurately.

@cyanglaz
Copy link
Contributor Author

cyanglaz commented Apr 7, 2022

So the application state is actually updated after UpdateSemantics. This means in UpdateSemantics, we would never know about the application state that we needed to determine if we need to do a layout change.

@cyanglaz
Copy link
Contributor Author

cyanglaz commented Apr 20, 2022

This solution won't work as the accessibility update is fired before we know the app is going to be resigned active. I have provided a less satisfying fix so that we can turn the tests back on: #32820

@cyanglaz cyanglaz closed this Apr 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iOS a11y focus gets drawn back to Flutter from system dialog
4 participants