REFACTOR: split up accessibility bridge and semantics object #17507
Conversation
gaaclarke
force-pushed
the
split-out-semantics-object
branch
from
027f0f1 to
f6698f8
Compare
gaaclarke
changed the title
|
@jmagman to help with ObjC review if possible. |
FYI all objc changes were just moving code around and switching references from AccessibilityBridge to AccessibilityBridgeIos. The net result is that AccessibilityBridge no longer has the ability to set a SemanticObject's parent. |
|
landing, remaining ci tasks are for unaffected platforms and are taking forever. |
| /** | ||
| * A node in the iOS semantics tree. | ||
| */ | ||
| @interface SemanticsObject : UIAccessibilityElement |
There was a problem hiding this comment.
Classes should be prefixed.
Can any of these classes be put into their own files? The implementation file is giant. I know some of them are using private categories to set private properties.
| @@ -0,0 +1,156 @@ | |||
| // Copyright 2013 The Flutter Authors. All rights reserved. | |||
There was a problem hiding this comment.
I know you're refactoring and moving code that you didn't necessarily write or want to edit, so I'm just going to do a full review and you can take the suggestions that make sense for you.
There was a problem hiding this comment.
Nice, this already merged, want to make a PR with the choice cuts so we don't lose it?
There was a problem hiding this comment.
Also, tread lightly, the accessibility stuff doesn't have good automated testing.
| constexpr int32_t kRootNodeId = 0; | ||
|
|
||
| @class FlutterPlatformViewSemanticsContainer; | ||
|
|
There was a problem hiding this comment.
Add NS_ASSUME_NONNULL_BEGIN and _END to all edited headers and fix add nullability decorators.
| * NO for isAccessibilityElement). | ||
| */ | ||
| @interface SemanticsObjectContainer : UIAccessibilityElement | ||
| - (instancetype)init __attribute__((unavailable("Use initWithSemanticsObject instead"))); |
There was a problem hiding this comment.
NS_UNAVAILABLE
Also mark +new as NS_UNAVAILABLE
|
|
||
| #pragma mark - Designated initializers | ||
|
|
||
| - (instancetype)init __attribute__((unavailable("Use initWithBridge instead"))); |
There was a problem hiding this comment.
NS_UNAVAILABLE
Also mark +new unavailable.
Is -[UIAccessibilityElement initWithAccessibilityContainer:] supposed to be available?
| if (controller) { | ||
| _platformView = [controller->GetPlatformViewByID(object.node.platformViewId) view]; | ||
| } | ||
| self.accessibilityElements = @[ _semanticsObject, _platformView ]; |
There was a problem hiding this comment.
Don't use self in init to access properties. _ accessibilityElements = ...
| #pragma mark - UIAccessibilityContainer overrides | ||
|
|
||
| - (NSInteger)accessibilityElementCount { | ||
| NSInteger count = [[_semanticsObject children] count] + 1; |
| if (element == _semanticsObject) | ||
| return 0; |
| if ([child hasChildren]) | ||
| return [child accessibilityContainer]; |
| for (size_t i = 0; i < [children count]; i++) { | ||
| SemanticsObject* child = children[i]; | ||
| if ((![child hasChildren] && child == element) || | ||
| ([child hasChildren] && [child accessibilityContainer] == element)) | ||
| return i + 1; | ||
| } |
There was a problem hiding this comment.
-enumerateObjectsUsingBlock to get fast enumeration and index.
|
@gaaclarke Do you have a follow-up PR to handle @jmagman's comments, perchance? |
5 participants
Follow up refactor for pr: #17499
This should be a noop change, just splitting up a cyclical dependency and giving greater control over visibility of SemanticObject to AccessibilityBridge. More could be done to split out files in SemanticsObject in the future.
There is technically a slight performance cost with the introduction of virtual methods. This will make testing SemanticsObjects easier though.