Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,13 @@
composition_order_.push_back(view_id);
}

NSObject<FlutterPlatformView>* FlutterPlatformViewsController::GetPlatformViewByID(int view_id) {
if (views_.empty()) {
return nil;
}
return views_[view_id].get();
}

std::vector<SkCanvas*> FlutterPlatformViewsController::GetCurrentCanvases() {
std::vector<SkCanvas*> canvases;
for (size_t i = 0; i < composition_order_.size(); i++) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ class FlutterPlatformViewsController {

void PrerollCompositeEmbeddedView(int view_id);

NSObject<FlutterPlatformView>* GetPlatformViewByID(int view_id);
Comment thread
cbracken marked this conversation as resolved.

std::vector<SkCanvas*> GetCurrentCanvases();

SkCanvas* CompositeEmbeddedView(int view_id, const flow::EmbeddedViewParams& params);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,9 @@ class PlatformViewIOS;

class AccessibilityBridge final {
public:
AccessibilityBridge(UIView* view, PlatformViewIOS* platform_view);
AccessibilityBridge(UIView* view,
PlatformViewIOS* platform_view,
FlutterPlatformViewsController* platform_views_controller);
~AccessibilityBridge();

void UpdateSemantics(blink::SemanticsNodeUpdates nodes,
Expand All @@ -129,6 +131,10 @@ class AccessibilityBridge final {

fml::WeakPtr<AccessibilityBridge> GetWeakPtr();

FlutterPlatformViewsController* flutter_platform_views_controller() const {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

... GetPlatformViewsController() const for consistency with the rest of the engine.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

return flutter_platform_views_controller_;
};

void clearState();

private:
Expand All @@ -139,6 +145,7 @@ class AccessibilityBridge final {

UIView* view_;
PlatformViewIOS* platform_view_;
FlutterPlatformViewsController* flutter_platform_views_controller_;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

platform_views_controller for consistency with elsewhere.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

fml::scoped_nsobject<NSMutableDictionary<NSNumber*, SemanticsObject*>> objects_;
fml::scoped_nsprotocol<FlutterBasicMessageChannel*> accessibility_channel_;
fml::WeakPtrFactory<AccessibilityBridge> weak_factory_;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#import <UIKit/UIKit.h>

#include "flutter/fml/logging.h"
#include "flutter/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h"
#include "flutter/shell/platform/darwin/ios/platform_view_ios.h"

namespace {
Expand Down Expand Up @@ -165,6 +166,10 @@ - (BOOL)isAccessibilityElement {
// We enforce in the framework that no other useful semantics are merged with these nodes.
if ([self node].HasFlag(blink::SemanticsFlags::kScopesRoute))
return false;
// If the node is the placeholder for a platform view. We know it should be a container.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are you adding the asserts for this in the framework?

I would probably remove the comment and just add this case to this if above. It's just anther node that doesn't get semantics merged in.

if ([self node].platformViewId > -1) {
return false;
}
return ([self node].flags != 0 &&
[self node].flags != static_cast<int32_t>(blink::SemanticsFlags::kIsHidden)) ||
![self node].label.empty() || ![self node].value.empty() || ![self node].hint.empty() ||
Expand Down Expand Up @@ -436,6 +441,20 @@ - (nullable id)accessibilityElementAtIndex:(NSInteger)index {
return _semanticsObject;
}
SemanticsObject* child = [_semanticsObject children][index - 1];

// This 'if' block handles adding accessibility support for the embedded platform view.
// We first check if the child is a semantic node for a platform view.
// If so, we add the platform view as accessibilityElements of the child.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Avoid documenting the how, and instead focus on the what/why. Maybe something like:
// If the child is a semantic node for a platform view, inject the platform view into the iOS accessibility tree.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

shell::FlutterPlatformViewsController* flutterPlatformViewsController =
_bridge.get()->flutter_platform_views_controller();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: consider naming it controller for readability. See: Long names are long.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

if (child.node.platformViewId > -1 && flutterPlatformViewsController) {
Comment thread
goderbauer marked this conversation as resolved.
Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Similar comment here, replace -1 with a named constexpr.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

NSObject<FlutterPlatformView>* platformViewProtocolObject =
flutterPlatformViewsController->GetPlatformViewByID(child.node.platformViewId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a shorter name?

UIView* platformView = [platformViewProtocolObject view];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it safe to assume platformViewProtocolObject and [platformViewProtocolObject view] will never be nil? If not, put a guard around the next line since nil is disallowed in NSArray.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Done.

child.accessibilityElements = @[ platformView ];
return child;
}

if ([child hasChildren])
return [child accessibilityContainer];
return child;
Expand Down Expand Up @@ -485,9 +504,12 @@ - (BOOL)accessibilityScroll:(UIAccessibilityScrollDirection)direction {

namespace shell {

AccessibilityBridge::AccessibilityBridge(UIView* view, PlatformViewIOS* platform_view)
AccessibilityBridge::AccessibilityBridge(UIView* view,
PlatformViewIOS* platform_view,
FlutterPlatformViewsController* platform_views_controller)
: view_(view),
platform_view_(platform_view),
flutter_platform_views_controller_(platform_views_controller),
objects_([[NSMutableDictionary alloc] init]),
weak_factory_(this),
previous_route_id_(0),
Expand Down
6 changes: 4 additions & 2 deletions shell/platform/darwin/ios/platform_view_ios.mm
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@

if (accessibility_bridge_) {
accessibility_bridge_.reset(
new AccessibilityBridge(static_cast<FlutterView*>(owner_controller_.get().view), this));
new AccessibilityBridge(static_cast<FlutterView*>(owner_controller_.get().view), this,
[owner_controller.get() platformViewsController]));
}
// Do not call `NotifyCreated()` here - let FlutterViewController take care
// of that when its Viewport is sized. If `NotifyCreated()` is called here,
Expand Down Expand Up @@ -96,7 +97,8 @@
}
if (enabled && !accessibility_bridge_) {
accessibility_bridge_ = std::make_unique<AccessibilityBridge>(
static_cast<FlutterView*>(owner_controller_.get().view), this);
static_cast<FlutterView*>(owner_controller_.get().view), this,
[owner_controller_.get() platformViewsController]);
} else if (!enabled && accessibility_bridge_) {
accessibility_bridge_.reset();
}
Expand Down