[iOS] Rebind accessibility bridge after view controller changes#187167
[iOS] Rebind accessibility bridge after view controller changes#187167smocer wants to merge 1 commit into
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 |
551517b to
59d8bdb
Compare
|
/gemini review |
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.
| UpdateAccessibilityBridgeViewController(); | ||
| 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.
Skipping semantics updates when the view controller is detached or not loaded will cause the AccessibilityBridge's internal semantics tree to become out of sync and stale. Since the Flutter framework only sends incremental semantics updates, any updates occurring during this period will be permanently lost, leading to incorrect accessibility information once a view controller is reattached or loaded.
Instead, we should always forward the semantics updates to the AccessibilityBridge so it can maintain an up-to-date internal model. The AccessibilityBridge::UpdateSemantics method is already designed to safely handle a nil view controller (it updates its internal objects and returns early before posting notifications or updating elements). We should only guard the NSNotificationCenter notification post.
UpdateAccessibilityBridgeViewController();
if (!accessibility_bridge_) {
return;
}
accessibility_bridge_.get()->UpdateSemantics(std::move(update), actions);
if (owner_controller_ && owner_controller_.isViewLoaded) {
[[NSNotificationCenter defaultCenter] postNotificationName:FlutterSemanticsUpdateNotification
object:owner_controller_];
}
| - (void)testDoesNotUpdateSemanticsWhileOwnerControllerIsDetached { | ||
| flutter::MockDelegate mock_delegate; | ||
| fml::MessageLoop::EnsureInitializedForCurrentThread(); | ||
| auto thread_task_runner = fml::MessageLoop::GetCurrent().GetTaskRunner(); | ||
| flutter::TaskRunners runners(/*label=*/self.name.UTF8String, | ||
| /*platform=*/thread_task_runner, | ||
| /*raster=*/thread_task_runner, | ||
| /*ui=*/thread_task_runner, | ||
| /*io=*/thread_task_runner); | ||
| id messenger = OCMProtocolMock(@protocol(FlutterBinaryMessenger)); | ||
| id engine = OCMClassMock([FlutterEngine class]); | ||
|
|
||
| id firstViewController = OCMClassMock([FlutterViewController class]); | ||
| id secondViewController = OCMClassMock([FlutterViewController class]); | ||
| UIView* firstView = [[UIView alloc] init]; | ||
| UIView* secondView = [[UIView alloc] init]; | ||
|
|
||
| OCMStub([firstViewController isViewLoaded]).andReturn(YES); | ||
| OCMStub([firstViewController engine]).andReturn(engine); | ||
| OCMStub([firstViewController view]).andReturn(firstView); | ||
| OCMStub([firstViewController viewIfLoaded]).andReturn(firstView); | ||
| OCMStub([secondViewController isViewLoaded]).andReturn(YES); | ||
| OCMStub([secondViewController engine]).andReturn(engine); | ||
| OCMStub([secondViewController view]).andReturn(secondView); | ||
| OCMStub([secondViewController viewIfLoaded]).andReturn(secondView); | ||
| OCMStub([engine binaryMessenger]).andReturn(messenger); | ||
|
|
||
| auto platform_view = std::make_unique<flutter::PlatformViewIOS>( | ||
| /*delegate=*/mock_delegate, | ||
| /*rendering_api=*/flutter::IOSRenderingAPI::kMetal, | ||
| /*platform_views_controller=*/nil, | ||
| /*task_runners=*/runners, | ||
| /*worker_task_runner=*/nil, | ||
| /*is_gpu_disabled_sync_switch=*/std::make_shared<fml::SyncSwitch>()); | ||
| platform_view->SetOwnerViewController(firstViewController); | ||
| platform_view->SetSemanticsTreeEnabled(true); | ||
| flutter::AccessibilityBridge* bridge = platform_view->GetAccessibilityBridge(); | ||
| XCTAssertTrue(bridge != nullptr); | ||
|
|
||
| flutter::SemanticsNode root_node; | ||
| root_node.id = kRootNodeId; | ||
| root_node.label = "root"; | ||
| flutter::SemanticsNodeUpdates initial_update; | ||
| initial_update[kRootNodeId] = root_node; | ||
| platform_view->UpdateSemantics(/*view_id=*/0, std::move(initial_update), | ||
| flutter::CustomAccessibilityActionUpdates()); | ||
| XCTAssertNotNil(firstView.accessibilityElements); | ||
|
|
||
| platform_view->SetOwnerViewController(nil); | ||
|
|
||
| flutter::SemanticsNode detached_root_node; | ||
| detached_root_node.id = kRootNodeId; | ||
| detached_root_node.label = "updated while detached"; | ||
| flutter::SemanticsNodeUpdates detached_update; | ||
| detached_update[kRootNodeId] = detached_root_node; | ||
| platform_view->UpdateSemantics(/*view_id=*/0, std::move(detached_update), | ||
| flutter::CustomAccessibilityActionUpdates()); | ||
| XCTAssertEqual(platform_view->GetAccessibilityBridge(), bridge); | ||
| XCTAssertNil(firstView.accessibilityElements); | ||
|
|
||
| platform_view->SetOwnerViewController(secondViewController); | ||
| XCTAssertEqual(platform_view->GetAccessibilityBridge(), bridge); | ||
| XCTAssertNotNil(secondView.accessibilityElements); | ||
| platform_view->SetSemanticsTreeEnabled(false); | ||
|
|
||
| [engine stopMocking]; | ||
| } |
There was a problem hiding this comment.
Since we should preserve and apply semantics updates even while the view controller is detached, this test should be renamed to testPreservesSemanticsUpdatesWhileOwnerControllerIsDetached and updated to assert that the semantics update is indeed preserved and applied to the new view controller once it is attached.
- (void)testPreservesSemanticsUpdatesWhileOwnerControllerIsDetached {
flutter::MockDelegate mock_delegate;
fml::MessageLoop::EnsureInitializedForCurrentThread();
auto thread_task_runner = fml::MessageLoop::GetCurrent().GetTaskRunner();
flutter::TaskRunners runners(/*label=*/self.name.UTF8String,
/*platform=*/thread_task_runner,
/*raster=*/thread_task_runner,
/*ui=*/thread_task_runner,
/*io=*/thread_task_runner);
id messenger = OCMProtocolMock(@protocol(FlutterBinaryMessenger));
id engine = OCMClassMock([FlutterEngine class]);
id firstViewController = OCMClassMock([FlutterViewController class]);
id secondViewController = OCMClassMock([FlutterViewController class]);
UIView* firstView = [[UIView alloc] init];
UIView* secondView = [[UIView alloc] init];
OCMStub([firstViewController isViewLoaded]).andReturn(YES);
OCMStub([firstViewController engine]).andReturn(engine);
OCMStub([firstViewController view]).andReturn(firstView);
OCMStub([firstViewController viewIfLoaded]).andReturn(firstView);
OCMStub([secondViewController isViewLoaded]).andReturn(YES);
OCMStub([secondViewController engine]).andReturn(engine);
OCMStub([secondViewController view]).andReturn(secondView);
OCMStub([secondViewController viewIfLoaded]).andReturn(secondView);
OCMStub([engine binaryMessenger]).andReturn(messenger);
auto platform_view = std::make_unique<flutter::PlatformViewIOS>(
/*delegate=*/mock_delegate,
/*rendering_api=*/flutter::IOSRenderingAPI::kMetal,
/*platform_views_controller=*/nil,
/*task_runners=*/runners,
/*worker_task_runner=*/nil,
/*is_gpu_disabled_sync_switch=*/std::make_shared<fml::SyncSwitch>());
platform_view->SetOwnerViewController(firstViewController);
platform_view->SetSemanticsTreeEnabled(true);
flutter::AccessibilityBridge* bridge = platform_view->GetAccessibilityBridge();
XCTAssertTrue(bridge != nullptr);
flutter::SemanticsNode root_node;
root_node.id = kRootNodeId;
root_node.label = "root";
flutter::SemanticsNodeUpdates initial_update;
initial_update[kRootNodeId] = root_node;
platform_view->UpdateSemantics(/*view_id=*/0, std::move(initial_update),
flutter::CustomAccessibilityActionUpdates());
XCTAssertNotNil(firstView.accessibilityElements);
platform_view->SetOwnerViewController(nil);
flutter::SemanticsNode detached_root_node;
detached_root_node.id = kRootNodeId;
detached_root_node.label = "updated while detached";
flutter::SemanticsNodeUpdates detached_update;
detached_update[kRootNodeId] = detached_root_node;
platform_view->UpdateSemantics(/*view_id=*/0, std::move(detached_update),
flutter::CustomAccessibilityActionUpdates());
XCTAssertEqual(platform_view->GetAccessibilityBridge(), bridge);
XCTAssertNil(firstView.accessibilityElements);
platform_view->SetOwnerViewController(secondViewController);
XCTAssertEqual(platform_view->GetAccessibilityBridge(), bridge);
XCTAssertNotNil(secondView.accessibilityElements);
id rootElement = secondView.accessibilityElements.firstObject;
XCTAssertEqualObjects([rootElement accessibilityLabel], @"updated while detached");
platform_view->SetSemanticsTreeEnabled(false);
[engine stopMocking];
}
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.
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
///).