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

ios accessibility: started ignoring route changes when presenting modal view controllers #18544

Merged
merged 3 commits into from
Jun 3, 2020
Merged

ios accessibility: started ignoring route changes when presenting modal view controllers #18544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc7d900
Started ignoring route change names when presenting modal view
gaaclarke May 20, 2020
7f36f2a
responded to some of jenns feedback
gaaclarke Jun 3, 2020
59d9c54
fixed spelling error
gaaclarke Jun 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions shell/platform/darwin/ios/framework/Source/FlutterViewController.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
@interface FlutterViewController () <FlutterBinaryMessenger, UIScrollViewDelegate>
@property(nonatomic, readwrite, getter=isDisplayingFlutterUI) BOOL displayingFlutterUI;
@property(nonatomic, assign) BOOL isHomeIndicatorHidden;
@property(nonatomic, assign) BOOL isPresentingViewControllerAnimating;
@end

// The following conditional compilation defines an API 13 concept on earlier API targets so that
Expand Down Expand Up @@ -1240,4 +1241,22 @@ - (NSObject*)valuePublishedByPlugin:(NSString*)pluginKey {
return [_engine.get() valuePublishedByPlugin:pluginKey];
}

- (void)presentViewController:(UIViewController*)viewControllerToPresent
animated:(BOOL)flag
completion:(void (^)(void))completion {
self.isPresentingViewControllerAnimating = YES;
[super presentViewController:viewControllerToPresent
animated:flag
completion:^{
self.isPresentingViewControllerAnimating = NO;
Copy link
Member

@jmagman jmagman Jun 1, 2020

Choose a reason for hiding this comment

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

This is my own UIViewController ignorance showing, but what happens if you present two in a row? I'm guessing one animation finishes before the next one starts?

Copy link
Member Author

@gaaclarke gaaclarke Jun 3, 2020

Choose a reason for hiding this comment

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

Presenting one while the other is animating? No idea. I would actually expect the first animation to be cancelled.

if (completion) {
completion();
}
}];
}

- (BOOL)isPresentingViewController {
return self.presentedViewController != nil || self.isPresentingViewControllerAnimating;
}

@end
1 change: 1 addition & 0 deletions shell/platform/darwin/ios/framework/Source/FlutterViewController_Internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ extern NSNotificationName const FlutterViewControllerShowHomeIndicator;

@interface FlutterViewController ()

@property(nonatomic, readonly) BOOL isPresentingViewController;
- (fml::WeakPtr<FlutterViewController>)getWeakPtr;
- (flutter::FlutterPlatformViewsController*)platformViewsController;

Expand Down
15 changes: 14 additions & 1 deletion shell/platform/darwin/ios/framework/Source/accessibility_bridge.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,21 @@ class PlatformViewIOS;
*/
class AccessibilityBridge final : public AccessibilityBridgeIos {
public:
/** Delegate for handling iOS operations. */
class IosDelegate {
public:
virtual ~IosDelegate() = default;
/// Returns true when the FlutterViewController associated with the `view`
/// is presenting a modal view controller.
virtual bool IsFlutterViewControllerPresentingModalViewController(UIView* view) = 0;
virtual void PostAccessibilityNotification(UIAccessibilityNotifications notification,
id argument) = 0;
};

AccessibilityBridge(UIView* view,
PlatformViewIOS* platform_view,
FlutterPlatformViewsController* platform_views_controller);
FlutterPlatformViewsController* platform_views_controller,
std::unique_ptr<IosDelegate> ios_delegate = nullptr);
~AccessibilityBridge();

void UpdateSemantics(flutter::SemanticsNodeUpdates nodes,
Expand Down Expand Up @@ -75,6 +87,7 @@ class AccessibilityBridge final : public AccessibilityBridgeIos {
int32_t previous_route_id_;
std::unordered_map<int32_t, flutter::CustomAccessibilityAction> actions_;
std::vector<int32_t> previous_routes_;
std::unique_ptr<IosDelegate> ios_delegate_;

FML_DISALLOW_COPY_AND_ASSIGN(AccessibilityBridge);
};
Expand Down
55 changes: 48 additions & 7 deletions shell/platform/darwin/ios/framework/Source/accessibility_bridge.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#import "flutter/shell/platform/darwin/ios/framework/Source/accessibility_bridge.h"
#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterEngine_Internal.h"
#import "flutter/shell/platform/darwin/ios/framework/Source/FlutterViewController_Internal.h"
#import "flutter/shell/platform/darwin/ios/framework/Source/accessibility_text_entry.h"

#import "flutter/shell/platform/darwin/ios/platform_view_ios.h"
Expand All @@ -13,17 +14,54 @@
FLUTTER_ASSERT_NOT_ARC

namespace flutter {
namespace {

FlutterViewController* _Nullable GetFlutterViewControllerForView(UIView* view) {
// There is no way to get a view's view controller in UIKit directly, this is
Copy link
Member

@xster xster Jun 3, 2020

Choose a reason for hiding this comment

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

Looks like platformviewios right above it has a FlutterViewController anyway. We can just refactor this private constructor to take a FVC if needed.

Copy link
Member Author

@gaaclarke gaaclarke Jun 3, 2020

Choose a reason for hiding this comment

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

Yea, i can look into it. i'll split it into a different PR to keep the bug fix and the refactor separate.

Copy link
Member

@xster xster Jun 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

SG

Copy link
Member Author

@gaaclarke gaaclarke Jun 3, 2020

Choose a reason for hiding this comment

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

#18800

// somewhat of a hacky solution to get that. This could be eliminated if the
// bridge actually kept a reference to a FlutterViewController instead of a
// UIView.
id nextResponder = [view nextResponder];
if ([nextResponder isKindOfClass:[FlutterViewController class]]) {
return nextResponder;
} else if ([nextResponder isKindOfClass:[UIView class]]) {
return GetFlutterViewControllerForView(nextResponder);
} else {
return nil;
}
}

class DefaultIosDelegate : public AccessibilityBridge::IosDelegate {
public:
bool IsFlutterViewControllerPresentingModalViewController(UIView* view) override {
FlutterViewController* viewController = GetFlutterViewControllerForView(view);
if (viewController) {
return viewController.isPresentingViewController;
} else {
return false;
}
}

void PostAccessibilityNotification(UIAccessibilityNotifications notification,
id argument) override {
UIAccessibilityPostNotification(notification, argument);
}
};
} // namespace

AccessibilityBridge::AccessibilityBridge(UIView* view,
PlatformViewIOS* platform_view,
FlutterPlatformViewsController* platform_views_controller)
FlutterPlatformViewsController* platform_views_controller,
std::unique_ptr<IosDelegate> ios_delegate)
: view_(view),
platform_view_(platform_view),
platform_views_controller_(platform_views_controller),
objects_([[NSMutableDictionary alloc] init]),
weak_factory_(this),
previous_route_id_(0),
previous_routes_({}) {
previous_routes_({}),
ios_delegate_(ios_delegate ? std::move(ios_delegate)
: std::make_unique<DefaultIosDelegate>()) {
accessibility_channel_.reset([[FlutterBasicMessageChannel alloc]
initWithName:@"flutter/accessibility"
binaryMessenger:platform_view->GetOwnerViewController().get().engine.binaryMessenger
Expand Down Expand Up @@ -137,15 +175,18 @@

layoutChanged = layoutChanged || [doomed_uids count] > 0;
if (routeChanged) {
NSString* routeName = [lastAdded routeName];
UIAccessibilityPostNotification(UIAccessibilityScreenChangedNotification, routeName);
if (!ios_delegate_->IsFlutterViewControllerPresentingModalViewController(view_)) {
NSString* routeName = [lastAdded routeName];
ios_delegate_->PostAccessibilityNotification(UIAccessibilityScreenChangedNotification,
routeName);
}
} else if (layoutChanged) {
// TODO(goderbauer): figure out which node to focus next.
UIAccessibilityPostNotification(UIAccessibilityLayoutChangedNotification, nil);
ios_delegate_->PostAccessibilityNotification(UIAccessibilityLayoutChangedNotification, nil);
Copy link
Member

@goderbauer goderbauer Jun 1, 2020

Choose a reason for hiding this comment

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

Why run the notification through the delegate as well?

Copy link
Member Author

@gaaclarke gaaclarke Jun 1, 2020

Choose a reason for hiding this comment

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

For testing. We can't test that someone has called UIAccessibilityPostNotification so the delegate allows us to do dependency injection.

Copy link
Member

@goderbauer goderbauer Jun 1, 2020

Choose a reason for hiding this comment

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

That makes sense. Thanks for the explanation :)

}
if (scrollOccured) {
// TODO(tvolkert): provide meaningful string (e.g. "page 2 of 5")
UIAccessibilityPostNotification(UIAccessibilityPageScrolledNotification, @"");
ios_delegate_->PostAccessibilityNotification(UIAccessibilityPageScrolledNotification, @"");
}
}

Expand Down Expand Up @@ -233,7 +274,7 @@ static bool DidFlagChange(const flutter::SemanticsNode& oldNode,
NSString* type = annotatedEvent[@"type"];
if ([type isEqualToString:@"announce"]) {
NSString* message = annotatedEvent[@"data"][@"message"];
UIAccessibilityPostNotification(UIAccessibilityAnnouncementNotification, message);
ios_delegate_->PostAccessibilityNotification(UIAccessibilityAnnouncementNotification, message);
}
}

Expand Down
124 changes: 124 additions & 0 deletions shell/platform/darwin/ios/framework/Source/accessibility_bridge_test.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,22 @@ void OnPlatformViewRegisterTexture(std::shared_ptr<Texture> texture) override {}
void OnPlatformViewUnregisterTexture(int64_t texture_id) override {}
void OnPlatformViewMarkTextureFrameAvailable(int64_t texture_id) override {}
};

class MockIosDelegate : public AccessibilityBridge::IosDelegate {
public:
bool IsFlutterViewControllerPresentingModalViewController(UIView* view) override {
return result_IsFlutterViewControllerPresentingModalViewController_;
};

void PostAccessibilityNotification(UIAccessibilityNotifications notification,
id argument) override {
if (on_PostAccessibilityNotification_) {
on_PostAccessibilityNotification_(notification, argument);
}
}
std::function<void(UIAccessibilityNotifications, id)> on_PostAccessibilityNotification_;
bool result_IsFlutterViewControllerPresentingModalViewController_ = false;
};
} // namespace
} // namespace flutter

Expand Down Expand Up @@ -238,4 +254,112 @@ - (void)testSemanticsDeallocated {
XCTAssertNil(gMockPlatformView);
}

- (void)testAnnouncesRouteChanges {
flutter::MockDelegate mock_delegate;
auto thread_task_runner = CreateNewThread("AccessibilityBridgeTest");
flutter::TaskRunners runners(/*label=*/self.name.UTF8String,
/*platform=*/thread_task_runner,
/*raster=*/thread_task_runner,
/*ui=*/thread_task_runner,
/*io=*/thread_task_runner);
auto platform_view = std::make_unique<flutter::PlatformViewIOS>(
/*delegate=*/mock_delegate,
/*rendering_api=*/flutter::IOSRenderingAPI::kSoftware,
/*task_runners=*/runners);
id mockFlutterView = OCMClassMock([FlutterView class]);
std::string label = "some label";

NSMutableArray<NSDictionary<NSString*, id>*>* accessibility_notifications =
[[[NSMutableArray alloc] init] autorelease];
Copy link
Member

@jmagman jmagman Jun 1, 2020

Choose a reason for hiding this comment

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

Should retain and release this instead of letting the autorelease pool handle it (usually -autorelease if you're returning a value from a method, say, not if you actually are in control of the scope). But yeah it's a test so whatever.

Copy link
Member Author

@gaaclarke gaaclarke Jun 3, 2020

Choose a reason for hiding this comment

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

In ObjC++ the rules are a bit different because of RAII. If I didn't put an autorelease here I should have a RAII wrapper around the pointer... I tried to get this file compiling with ARC but there is a hangup supporting that. Hopefully we can migrate it.

auto ios_delegate = std::make_unique<flutter::MockIosDelegate>();
ios_delegate->on_PostAccessibilityNotification_ =
[accessibility_notifications](UIAccessibilityNotifications notification, id argument) {
[accessibility_notifications addObject:@{
@"notification" : @(notification),
@"argument" : argument ? argument : [NSNull null],
}];
};
__block auto bridge =
std::make_unique<flutter::AccessibilityBridge>(/*view=*/mockFlutterView,
/*platform_view=*/platform_view.get(),
/*platform_views_controller=*/nil,
/*ios_delegate=*/std::move(ios_delegate));

flutter::CustomAccessibilityActionUpdates actions;
flutter::SemanticsNodeUpdates nodes;

flutter::SemanticsNode route_node;
route_node.id = 1;
route_node.label = label;
route_node.flags = static_cast<int32_t>(flutter::SemanticsFlags::kScopesRoute) |
static_cast<int32_t>(flutter::SemanticsFlags::kNamesRoute);
route_node.label = "route";
nodes[route_node.id] = route_node;
flutter::SemanticsNode root_node;
root_node.id = kRootNodeId;
root_node.label = label;
root_node.childrenInTraversalOrder = {1};
root_node.childrenInHitTestOrder = {1};
nodes[root_node.id] = root_node;
bridge->UpdateSemantics(/*nodes=*/nodes, /*actions=*/actions);

XCTAssertEqual([accessibility_notifications count], 1ul);
Copy link
Member

@jmagman jmagman Jun 1, 2020

Choose a reason for hiding this comment

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

I don't think the UL is necessary any more (there was a time where you'd get a compilation warning, but they fixed that a few years ago).

Copy link
Member Author

@gaaclarke gaaclarke Jun 3, 2020

Choose a reason for hiding this comment

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

I just removed it to see what happens and it still creates an error.

XCTAssertEqualObjects(accessibility_notifications[0][@"argument"], @"route");
XCTAssertEqual([accessibility_notifications[0][@"notification"] unsignedIntValue],
UIAccessibilityScreenChangedNotification);
}

- (void)testAnnouncesIgnoresRouteChangesWhenModal {
flutter::MockDelegate mock_delegate;
auto thread_task_runner = CreateNewThread("AccessibilityBridgeTest");
flutter::TaskRunners runners(/*label=*/self.name.UTF8String,
/*platform=*/thread_task_runner,
/*raster=*/thread_task_runner,
/*ui=*/thread_task_runner,
/*io=*/thread_task_runner);
auto platform_view = std::make_unique<flutter::PlatformViewIOS>(
/*delegate=*/mock_delegate,
/*rendering_api=*/flutter::IOSRenderingAPI::kSoftware,
/*task_runners=*/runners);
id mockFlutterView = OCMClassMock([FlutterView class]);
std::string label = "some label";

NSMutableArray<NSDictionary<NSString*, id>*>* accessibility_notifications =
[[[NSMutableArray alloc] init] autorelease];
auto ios_delegate = std::make_unique<flutter::MockIosDelegate>();
ios_delegate->on_PostAccessibilityNotification_ =
[accessibility_notifications](UIAccessibilityNotifications notification, id argument) {
[accessibility_notifications addObject:@{
@"notification" : @(notification),
@"argument" : argument ? argument : [NSNull null],
}];
};
ios_delegate->result_IsFlutterViewControllerPresentingModalViewController_ = true;
__block auto bridge =
std::make_unique<flutter::AccessibilityBridge>(/*view=*/mockFlutterView,
/*platform_view=*/platform_view.get(),
/*platform_views_controller=*/nil,
/*ios_delegate=*/std::move(ios_delegate));

flutter::CustomAccessibilityActionUpdates actions;
flutter::SemanticsNodeUpdates nodes;

flutter::SemanticsNode route_node;
route_node.id = 1;
route_node.label = label;
route_node.flags = static_cast<int32_t>(flutter::SemanticsFlags::kScopesRoute) |
static_cast<int32_t>(flutter::SemanticsFlags::kNamesRoute);
route_node.label = "route";
nodes[route_node.id] = route_node;
flutter::SemanticsNode root_node;
root_node.id = kRootNodeId;
root_node.label = label;
root_node.childrenInTraversalOrder = {1};
root_node.childrenInHitTestOrder = {1};
nodes[root_node.id] = root_node;
bridge->UpdateSemantics(/*nodes=*/nodes, /*actions=*/actions);

XCTAssertEqual([accessibility_notifications count], 0ul);
Copy link
Member

@jmagman jmagman Jun 1, 2020

Choose a reason for hiding this comment

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

Same.

Copy link
Member Author

@gaaclarke gaaclarke Jun 3, 2020

Choose a reason for hiding this comment

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

ack

}

@end