Skip to content

Commit

Permalink
Fix multiple issues around accessibility handlinig (facebookarchive#1537
Browse files Browse the repository at this point in the history 
)

* Improve Accessibility Implementation

- Fixes calculations for accessibilityFrame of ASAccessibilityCustomAction and ASAccessibilityElement
- Fix crash for accessibility elements within layer backed nodes
- Fix accessibility action label updating after other actions change the accessibility label of the corresponding node

* Address comments
  • Loading branch information
@maicki
maicki authored Jun 5, 2019
1 parent ab8ea06 commit 19fb124
Show file tree
Hide file tree
Showing 4 changed files with 121 additions and 51 deletions.
81 changes: 44 additions & 37 deletions Source/Details/_ASDisplayViewAccessiblity.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,13 @@
#import <AsyncDisplayKit/ASAvailability.h>
#import <AsyncDisplayKit/ASCollectionNode.h>
#import <AsyncDisplayKit/ASDisplayNodeExtras.h>
#import <AsyncDisplayKit/ASDisplayNode+FrameworkPrivate.h>
#import <AsyncDisplayKit/ASDisplayNode+Ancestry.h>
#import <AsyncDisplayKit/ASDisplayNodeInternal.h>
#import <AsyncDisplayKit/ASTableNode.h>

#import <queue>

NS_INLINE UIAccessibilityTraits InteractiveAccessibilityTraitsMask() {
return UIAccessibilityTraitLink | UIAccessibilityTraitKeyboardKey | UIAccessibilityTraitButton;
}

#pragma mark - UIAccessibilityElement

@protocol ASAccessibilityElementPositioning
Expand All @@ -36,7 +34,7 @@ @protocol ASAccessibilityElementPositioning
static void SortAccessibilityElements(NSMutableArray *elements)
{
ASDisplayNodeCAssertNotNil(elements, @"Should pass in a NSMutableArray");

static SortAccessibilityElementsComparator comparator = nil;
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
Expand All @@ -55,22 +53,25 @@ static void SortAccessibilityElements(NSMutableArray *elements)
[elements sortUsingComparator:comparator];
}

static CGRect ASAccessibilityFrameForNode(ASDisplayNode *node) {
CALayer *layer = node.layer;
return [layer convertRect:node.bounds toLayer:ASFindWindowOfLayer(layer).layer];
}

@interface ASAccessibilityElement : UIAccessibilityElement<ASAccessibilityElementPositioning>

@property (nonatomic) ASDisplayNode *node;
@property (nonatomic) ASDisplayNode *containerNode;

+ (ASAccessibilityElement *)accessibilityElementWithContainer:(UIView *)container node:(ASDisplayNode *)node containerNode:(ASDisplayNode *)containerNode;
+ (ASAccessibilityElement *)accessibilityElementWithContainer:(UIView *)container node:(ASDisplayNode *)node;

@end

@implementation ASAccessibilityElement

+ (ASAccessibilityElement *)accessibilityElementWithContainer:(UIView *)container node:(ASDisplayNode *)node containerNode:(ASDisplayNode *)containerNode
+ (ASAccessibilityElement *)accessibilityElementWithContainer:(UIView *)container node:(ASDisplayNode *)node
{
ASAccessibilityElement *accessibilityElement = [[ASAccessibilityElement alloc] initWithAccessibilityContainer:container];
accessibilityElement.node = node;
accessibilityElement.containerNode = containerNode;
accessibilityElement.accessibilityIdentifier = node.accessibilityIdentifier;
accessibilityElement.accessibilityLabel = node.accessibilityLabel;
accessibilityElement.accessibilityHint = node.accessibilityHint;
Expand All @@ -86,8 +87,7 @@ + (ASAccessibilityElement *)accessibilityElementWithContainer:(UIView *)containe

- (CGRect)accessibilityFrame
{
CGRect accessibilityFrame = [self.containerNode convertRect:self.node.bounds fromNode:self.node];
return UIAccessibilityConvertFrameToScreenCoordinates(accessibilityFrame, self.accessibilityContainer);
return ASAccessibilityFrameForNode(self.node);
}

@end
Expand All @@ -96,18 +96,15 @@ - (CGRect)accessibilityFrame

@interface ASAccessibilityCustomAction : UIAccessibilityCustomAction<ASAccessibilityElementPositioning>

@property (nonatomic) UIView *container;
@property (nonatomic) ASDisplayNode *node;
@property (nonatomic) ASDisplayNode *containerNode;

@end

@implementation ASAccessibilityCustomAction

- (CGRect)accessibilityFrame
{
CGRect accessibilityFrame = [self.containerNode convertRect:self.node.bounds fromNode:self.node];
return UIAccessibilityConvertFrameToScreenCoordinates(accessibilityFrame, self.container);
return ASAccessibilityFrameForNode(self.node);
}

@end
Expand All @@ -116,19 +113,26 @@ - (CGRect)accessibilityFrame
static void CollectUIAccessibilityElementsForNode(ASDisplayNode *node, ASDisplayNode *containerNode, id container, NSMutableArray *elements)
{
ASDisplayNodeCAssertNotNil(elements, @"Should pass in a NSMutableArray");

ASDisplayNodePerformBlockOnEveryNodeBFS(node, ^(ASDisplayNode * _Nonnull currentNode) {
// For every subnode that is layer backed or it's supernode has subtree rasterization enabled
// we have to create a UIAccessibilityElement as no view for this node exists
if (currentNode != containerNode && currentNode.isAccessibilityElement) {
UIAccessibilityElement *accessibilityElement = [ASAccessibilityElement accessibilityElementWithContainer:container node:currentNode containerNode:containerNode];
UIAccessibilityElement *accessibilityElement = [ASAccessibilityElement accessibilityElementWithContainer:container node:currentNode];
[elements addObject:accessibilityElement];
}
});
}

static void CollectAccessibilityElementsForContainer(ASDisplayNode *container, UIView *view, NSMutableArray *elements) {
UIAccessibilityElement *accessiblityElement = [ASAccessibilityElement accessibilityElementWithContainer:view node:container containerNode:container];
static void CollectAccessibilityElementsForContainer(ASDisplayNode *container, UIView *view,
NSMutableArray *elements) {
ASDisplayNodeCAssertNotNil(view, @"Passed in view should not be nil");
if (view == nil) {
return;
}
UIAccessibilityElement *accessiblityElement =
[ASAccessibilityElement accessibilityElementWithContainer:view
node:container];

NSMutableArray<ASAccessibilityElement *> *labeledNodes = [[NSMutableArray alloc] init];
NSMutableArray<ASAccessibilityCustomAction *> *actions = [[NSMutableArray alloc] init];
Expand All @@ -140,28 +144,28 @@ static void CollectAccessibilityElementsForContainer(ASDisplayNode *container, U
// value and do not perform the aggregation.
BOOL shouldAggregateSubnodeLabels =
(container.accessibilityLabel.length == 0) ||
(container.accessibilityTraits & InteractiveAccessibilityTraitsMask());
(container.accessibilityTraits & ASInteractiveAccessibilityTraitsMask());

ASDisplayNode *node = nil;
while (!queue.empty()) {
node = queue.front();
queue.pop();

if (node != container && node.isAccessibilityContainer) {
CollectAccessibilityElementsForContainer(node, view, elements);
UIView *containerView = node.isLayerBacked ? view : node.view;
CollectAccessibilityElementsForContainer(node, containerView, elements);
continue;
}

if (node.accessibilityLabel.length > 0) {
if (node.accessibilityTraits & InteractiveAccessibilityTraitsMask()) {
if (node.accessibilityTraits & ASInteractiveAccessibilityTraitsMask()) {
ASAccessibilityCustomAction *action = [[ASAccessibilityCustomAction alloc] initWithName:node.accessibilityLabel target:node selector:@selector(performAccessibilityCustomAction:)];
action.node = node;
action.containerNode = node.supernode;
action.container = node.supernode.view;
[actions addObject:action];

node.accessibilityCustomAction = action;
} else if (node == container || shouldAggregateSubnodeLabels) {
// Even though not surfaced to UIKit, create a non-interactive element for purposes of building sorted aggregated label.
ASAccessibilityElement *nonInteractiveElement = [ASAccessibilityElement accessibilityElementWithContainer:view node:node containerNode:container];
ASAccessibilityElement *nonInteractiveElement = [ASAccessibilityElement accessibilityElementWithContainer:view node:node];
[labeledNodes addObject:nonInteractiveElement];
}
}
Expand Down Expand Up @@ -195,36 +199,39 @@ static void CollectAccessibilityElementsForContainer(ASDisplayNode *container, U
}

/// Collect all accessibliity elements for a given view and view node
static void CollectAccessibilityElementsForView(UIView *view, NSMutableArray *elements)
static void CollectAccessibilityElements(ASDisplayNode *node, NSMutableArray *elements)
{
ASDisplayNodeCAssertNotNil(elements, @"Should pass in a NSMutableArray");

ASDisplayNode *node = view.asyncdisplaykit_node;
ASDisplayNodeCAssertFalse(node.isLayerBacked);
if (node.isLayerBacked) {
return;
}

BOOL anySubNodeIsCollection = (nil != ASDisplayNodeFindFirstNode(node,
^BOOL(ASDisplayNode *nodeToCheck) {
return ASDynamicCast(nodeToCheck, ASCollectionNode) != nil ||
ASDynamicCast(nodeToCheck, ASTableNode) != nil;
}));

UIView *view = node.view;

if (node.isAccessibilityContainer && !anySubNodeIsCollection) {
CollectAccessibilityElementsForContainer(node, view, elements);
return;
}

// Handle rasterize case
if (node.rasterizesSubtree) {
CollectUIAccessibilityElementsForNode(node, node, view, elements);
return;
}

for (ASDisplayNode *subnode in node.subnodes) {
if (subnode.isAccessibilityElement) {

// An accessiblityElement can either be a UIView or a UIAccessibilityElement
if (subnode.isLayerBacked) {
// No view for layer backed nodes exist. It's necessary to create a UIAccessibilityElement that represents this node
UIAccessibilityElement *accessiblityElement = [ASAccessibilityElement accessibilityElementWithContainer:view node:subnode containerNode:node];
UIAccessibilityElement *accessiblityElement = [ASAccessibilityElement accessibilityElementWithContainer:view node:subnode];
[elements addObject:accessiblityElement];
} else {
// Accessiblity element is not layer backed just add the view as accessibility element
Expand All @@ -233,7 +240,7 @@ static void CollectAccessibilityElementsForView(UIView *view, NSMutableArray *el
} else if (subnode.isLayerBacked) {
// Go down the hierarchy of the layer backed subnode and collect all of the UIAccessibilityElement
CollectUIAccessibilityElementsForNode(subnode, node, view, elements);
} else if ([subnode accessibilityElementCount] > 0) {
} else if (subnode.accessibilityElementCount > 0) {
// UIView is itself a UIAccessibilityContainer just add it
[elements addObject:subnode.view];
}
Expand All @@ -253,13 +260,13 @@ @implementation _ASDisplayView (UIAccessibilityContainer)
- (void)setAccessibilityElements:(NSArray *)accessibilityElements
{
ASDisplayNodeAssertMainThread();
_accessibilityElements = accessibilityElements;
_accessibilityElements = nil;
}

- (NSArray *)accessibilityElements
{
ASDisplayNodeAssertMainThread();

ASDisplayNode *viewNode = self.asyncdisplaykit_node;
if (viewNode == nil) {
return @[];
Expand All @@ -282,7 +289,7 @@ - (NSArray *)accessibilityElements
return @[];
}
NSMutableArray *accessibilityElements = [[NSMutableArray alloc] init];
CollectAccessibilityElementsForView(self.view, accessibilityElements);
CollectAccessibilityElements(self, accessibilityElements);
SortAccessibilityElements(accessibilityElements);
return accessibilityElements;
}
Expand Down
13 changes: 13 additions & 0 deletions Source/Private/ASDisplayNode+FrameworkPrivate.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,11 @@ __unused static NSString * _Nonnull NSStringFromASHierarchyStateChange(ASHierarc
*/
@property (nonatomic) ASHierarchyState hierarchyState;

/**
* Represent the current custom action in representation for the node
*/
@property (nonatomic, weak) UIAccessibilityCustomAction *accessibilityCustomAction;

/**
* @abstract Return if the node is range managed or not
*
Expand Down Expand Up @@ -307,6 +312,14 @@ __unused static NSString * _Nonnull NSStringFromASHierarchyStateChange(ASHierarc

@end

/**
* Defines interactive accessibility traits which will be exposed as UIAccessibilityCustomActions
* for nodes within nodes that have isAccessibilityContainer is YES
*/
NS_INLINE UIAccessibilityTraits ASInteractiveAccessibilityTraitsMask() {
return UIAccessibilityTraitLink | UIAccessibilityTraitKeyboardKey | UIAccessibilityTraitButton;
}

@interface ASDisplayNode (AccessibilityInternal)
- (NSArray *)accessibilityElements;
@end;
Expand Down
13 changes: 13 additions & 0 deletions Source/Private/ASDisplayNode+UIViewBridge.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
// Licensed under Apache 2.0: http://www.apache.org/licenses/LICENSE-2.0
//

#import <AsyncDisplayKit/ASDisplayNode+FrameworkPrivate.h>
#import <AsyncDisplayKit/_ASCoreAnimationExtras.h>
#import <AsyncDisplayKit/_ASPendingState.h>
#import <AsyncDisplayKit/ASInternalHelpers.h>
Expand Down Expand Up @@ -1100,13 +1101,25 @@ - (NSString *)accessibilityLabel
- (void)setAccessibilityLabel:(NSString *)accessibilityLabel
{
_bridge_prologue_write;
NSString *oldAccessibilityLabel = _getFromViewOnly(accessibilityLabel);
_setAccessibilityToViewAndProperty(_accessibilityLabel, accessibilityLabel, accessibilityLabel, accessibilityLabel);
if (AS_AVAILABLE_IOS_TVOS(11, 11)) {
NSAttributedString *accessibilityAttributedLabel = accessibilityLabel ? [[NSAttributedString alloc] initWithString:accessibilityLabel] : nil;
_setAccessibilityToViewAndProperty(_accessibilityAttributedLabel, accessibilityAttributedLabel, accessibilityAttributedLabel, accessibilityAttributedLabel);
}

// We need to update action name when it's changed to reflect the latest state.
// Note: Update the custom action itself won't work when a11y is inside a list of custom actions
// in which one action results in a name change in the next action. In that case the UIAccessibility
// will hold the old action strongly until a11y jumps out of the list of custom actions.
// Thus we can only update name in place to have the change take effect.
BOOL needsUpdateActionName = self.isNodeLoaded && ![oldAccessibilityLabel isEqualToString:accessibilityLabel] && (0 != (_accessibilityTraits & ASInteractiveAccessibilityTraitsMask()));
if (needsUpdateActionName) {
self.accessibilityCustomAction.name = accessibilityLabel;
}
}


- (NSAttributedString *)accessibilityAttributedLabel
{
_bridge_prologue_read;
Expand Down
65 changes: 51 additions & 14 deletions Tests/ASDisplayViewAccessibilityTests.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#import <AsyncDisplayKit/ASConfiguration.h>
#import <AsyncDisplayKit/ASConfigurationInternal.h>
#import <OCMock/OCMock.h>
#import "ASDisplayNodeTestsHelper.h"

@interface ASDisplayViewAccessibilityTests : XCTestCase
@end
Expand Down Expand Up @@ -88,38 +89,74 @@ - (void)testThatContainerAccessibilityLabelOverrideStopsAggregation
[node.view.accessibilityElements.firstObject accessibilityLabel]);
}

- (void)testAccessibilityLayerBackedContainerWithinAccessibilityContainer
{
ASDisplayNode *container = [[ASDisplayNode alloc] init];
container.frame = CGRectMake(50, 50, 200, 600);
container.isAccessibilityContainer = YES;

ASDisplayNode *subContainer = [[ASDisplayNode alloc] init];
subContainer.frame = CGRectMake(50, 50, 200, 600);

subContainer.layerBacked = YES;
subContainer.isAccessibilityContainer = YES;
[container addSubnode:subContainer];

ASTextNode *text1 = [[ASTextNode alloc] init];
text1.attributedText = [[NSAttributedString alloc] initWithString:@"hello"];
text1.frame = CGRectMake(50, 100, 200, 200);
text1.layerBacked = YES;
[subContainer addSubnode:text1];

ASTextNode *text2 = [[ASTextNode alloc] init];
text2.attributedText = [[NSAttributedString alloc] initWithString:@"world"];
text2.frame = CGRectMake(50, 300, 200, 200);
text2.layerBacked = YES;
[subContainer addSubnode:text2];

NSArray<UIAccessibilityElement *> *accessibilityElements = container.view.accessibilityElements;
XCTAssertEqual(accessibilityElements.count, 2);
XCTAssertEqualObjects(accessibilityElements[1].accessibilityLabel, @"hello, world");
}

- (void)testAccessibilityNonLayerbackedNodesOperationInNonContainer
{
ASDisplayNode *contianer = [[ASDisplayNode alloc] init];
contianer.frame = CGRectMake(50, 50, 200, 600);
contianer.backgroundColor = [UIColor grayColor];
ASDisplayNode *container = [[ASDisplayNode alloc] init];
UIWindow *window = [[UIWindow alloc] initWithFrame:CGRectMake(0, 0, 320, 560)];
[window addSubnode:container];
[window makeKeyAndVisible];

container.frame = CGRectMake(50, 50, 200, 600);
container.backgroundColor = [UIColor grayColor];
// Do any additional setup after loading the view, typically from a nib.
ASTextNode *text1 = [[ASTextNode alloc] init];
text1.attributedText = [[NSAttributedString alloc] initWithString:@"hello"];
text1.frame = CGRectMake(50, 100, 200, 200);
[contianer addSubnode:text1];
[contianer layoutIfNeeded];
[contianer.layer displayIfNeeded];
NSArray<UIAccessibilityElement *> *elements = contianer.view.accessibilityElements;
[container addSubnode:text1];
[container layoutIfNeeded];
[container.layer displayIfNeeded];
NSArray<UIAccessibilityElement *> *elements = container.view.accessibilityElements;
XCTAssertTrue(elements.count == 1);
XCTAssertTrue([[elements.firstObject accessibilityLabel] isEqualToString:@"hello"]);
ASTextNode *text2 = [[ASTextNode alloc] init];
text2.attributedText = [[NSAttributedString alloc] initWithString:@"world"];
text2.frame = CGRectMake(50, 300, 200, 200);
[contianer addSubnode:text2];
[contianer layoutIfNeeded];
[contianer.layer displayIfNeeded];
NSArray<UIAccessibilityElement *> *updatedElements = contianer.view.accessibilityElements;
[container addSubnode:text2];
[container layoutIfNeeded];
[container.layer displayIfNeeded];
ASCATransactionQueueWait(nil);
NSArray<UIAccessibilityElement *> *updatedElements = container.view.accessibilityElements;
XCTAssertTrue(updatedElements.count == 2);
XCTAssertTrue([[updatedElements.firstObject accessibilityLabel] isEqualToString:@"hello"]);
XCTAssertTrue([[updatedElements.lastObject accessibilityLabel] isEqualToString:@"world"]);
ASTextNode *text3 = [[ASTextNode alloc] init];
text3.attributedText = [[NSAttributedString alloc] initWithString:@"!!!!"];
text3.frame = CGRectMake(50, 400, 200, 100);
[text2 addSubnode:text3];
[contianer layoutIfNeeded];
[contianer.layer displayIfNeeded];
NSArray<UIAccessibilityElement *> *updatedElements2 = contianer.view.accessibilityElements;
[container layoutIfNeeded];
[container.layer displayIfNeeded];
ASCATransactionQueueWait(nil);
NSArray<UIAccessibilityElement *> *updatedElements2 = container.view.accessibilityElements;
//text3 won't be read out cause it's overshadowed by text2
XCTAssertTrue(updatedElements2.count == 2);
XCTAssertTrue([[updatedElements2.firstObject accessibilityLabel] isEqualToString:@"hello"]);
Expand Down

0 comments on commit 19fb124

Please sign in to comment.