Repository navigation
[iOS] Rebind accessibility bridge after view controller changes - #187167
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
github-actions
Bot
added
platform-ios
551517b to
59d8bdb
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enables the iOS accessibility bridge to be rebound to sequential view controllers and views, ensuring that native accessibility elements like scroll views are correctly reattached. The review feedback highlights a critical issue where semantics updates could be discarded while the view controller is detached, leading to state desynchronization, and suggests an optimization for weak pointer copies and direct instance variable access in SemanticsObject.mm.
| if (!accessibility_bridge_ || !owner_controller_ || !owner_controller_.isViewLoaded) { | ||
| return; | ||
| } | ||
| accessibility_bridge_.get()->UpdateSemantics(std::move(update), actions); | ||
| [[NSNotificationCenter defaultCenter] postNotificationName:FlutterSemanticsUpdateNotification | ||
| object:owner_controller_]; |
There was a problem hiding this comment.
Returning early when owner_controller_ is nil or !owner_controller_.isViewLoaded will completely discard any semantics updates sent by the Flutter framework while the view controller is detached or before its view is loaded. This causes the AccessibilityBridge's internal objects_ cache to become out of sync with the Flutter framework's actual semantics tree. When the view controller is later reattached or its view is loaded, the bridge will populate the view with stale semantics data.
Since AccessibilityBridge::UpdateSemantics is already safe to call when view_controller_ or view_ is nil (it will update the internal objects_ cache but skip updating the non-existent view or posting notifications), we should always forward the updates to the bridge if it exists, and only guard the NSNotificationCenter post.
if (!accessibility_bridge_) {
return;
}
accessibility_bridge_->UpdateSemantics(std::move(update), actions);
if (owner_controller_ && owner_controller_.isViewLoaded) {
[[NSNotificationCenter defaultCenter] postNotificationName:FlutterSemanticsUpdateNotification
object:owner_controller_];
}
References
- Avoid duplicating state and keep only one source of truth by ensuring the accessibility bridge's internal cache is always kept in sync with the Flutter framework's semantics tree. (link)
| UIView* view = [self isAccessibilityBridgeAlive] ? self.bridge->viewIfLoaded() : nil; | ||
| if (self.scrollView.superview != view) { | ||
| [self.scrollView removeFromSuperview]; | ||
| if (view) { | ||
| [view addSubview:self.scrollView]; | ||
| } | ||
| } |
There was a problem hiding this comment.
We can optimize this method by avoiding redundant copies of the weak pointer self.bridge and bypassing the isAccessibilityBridgeAlive helper (which also copies the weak pointer). Additionally, accessing the backing instance variable _scrollView directly is more efficient and idiomatic in Objective-C than using the property getter self.scrollView within the class implementation.
auto bridge = self.bridge;
UIView* view = bridge ? bridge->viewIfLoaded() : nil;
if (_scrollView.superview != view) {
[_scrollView removeFromSuperview];
if (view) {
[view addSubview:_scrollView];
}
}
References
- Optimize for readability and performance by avoiding redundant weak pointer copies and using direct instance variable access. (link)
There was a problem hiding this comment.
Code Review
This pull request updates the iOS platform view and accessibility bridge to support rebinding the accessibility bridge to sequential FlutterViewController and FlutterView instances when the owner controller is reattached. The reviewer identified an issue where skipping semantics updates while the view controller is detached or not loaded can cause the internal semantics tree to become out of sync. They recommend always forwarding semantics updates to the AccessibilityBridge and only guarding the notification post, as well as updating the corresponding unit tests to assert that updates are preserved while detached.
Keep the existing AccessibilityBridge when PlatformViewIOS detaches from an owner FlutterViewController, and rebind it when a controller/view is attached again. This preserves the semantics tree after engines are reused across FlutterViewController instances. Skip semantics updates while no owner controller is attached because UIKit accessibility elements need a valid container. Add iOS unit coverage for detached semantics updates and reattaching a new owner controller.
59d8bdb to
43441f8
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the iOS accessibility bridge lifecycle to support rebinding to sequential FlutterViewController and FlutterView instances. Instead of resetting the accessibility bridge when a view controller is detached, the bridge is preserved and updated with the new view controller via the newly introduced SetViewController method. This change ensures that semantics updates are preserved while detached and that native UIKit views (like scroll views) are correctly reattached to the active view. Comprehensive unit tests have been added to verify these rebinding behaviors and prevent regressions. I have no additional feedback to provide as there are no review comments.
| if (!semantics_tree_enabled_) { | ||
| return; | ||
| } | ||
| FlutterPlatformViewsController* platform_views_controller = |
There was a problem hiding this comment.
tiny optional nit:
FlutterPlatformViewsController* platform_views_controller =
owner_controller_.platformViewsController ?: platform_views_controller_;
is slightly cleaner
cbracken
added
the
autosubmit
auto-submit
Bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/187167, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
|
autosubmit label was removed for flutter/flutter/187167, because This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.
|
cbracken
added
the
autosubmit
|
autosubmit label was removed for flutter/flutter/187167, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
auto-submit
Bot
removed
the
autosubmit
cbracken
added
the
autosubmit
auto-submit
Bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/187167, because - The status or check suite Tree_analyze has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
@cbracken @LongCatIsLooong It looks like |
LongCatIsLooong
added
the
autosubmit
|
autosubmit label was removed for flutter/flutter/187167, because - The status or check suite Tree_analyze has failed. Please fix the issues identified (or deflake) before re-applying this label. |
8 participants
Keep the existing
AccessibilityBridgewhenPlatformViewIOSdetaches from an ownerFlutterViewController, and rebind it when a controller/view is attached again.In add-to-app integrations that reuse a
FlutterEngine, the engine can detach from oneFlutterViewControllerand later attach to another one. When semantics are already enabled, clearing the bridge during detach drops the UIKit accessibility representation of the current semantics tree. The Flutter UI continues to render, but VoiceOver / Accessibility Inspector can no longer see Flutter fields and buttons after reattachment.This change preserves the bridge across owner-controller detach/reattach, skips semantics updates while no owner controller is attached, and rebinds the bridge to the new owner controller/view before semantics updates resume.
Fixes #186582
Pre-launch Checklist
///).