-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactored accessibleElements
to accessibilityElements
#1069
Changes from 3 commits
a3eecd2
586cc46
0a5b3a2
2018422
6703c38
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,5 +19,4 @@ | |
#import <AsyncDisplayKit/_ASDisplayView.h> | ||
|
||
@interface _ASDisplayView (UIAccessibilityContainer) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's remove this whole category definition (line 21 and 23) |
||
@property (copy, nonatomic) NSArray *accessibleElements; | ||
@end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -243,7 +243,7 @@ static void CollectAccessibilityElementsForView(_ASDisplayView *view, NSMutableA | |
} | ||
|
||
@interface _ASDisplayView () { | ||
NSArray *_accessibleElements; | ||
NSArray *_accessibilityElements; | ||
} | ||
|
||
@end | ||
|
@@ -252,43 +252,26 @@ @implementation _ASDisplayView (UIAccessibilityContainer) | |
|
||
#pragma mark - UIAccessibility | ||
|
||
- (void)setAccessibleElements:(NSArray *)accessibleElements | ||
- (void)setAccessibilityElements:(NSArray *)accessibilityElements | ||
{ | ||
_accessibleElements = nil; | ||
_accessibilityElements = nil; | ||
} | ||
|
||
- (NSArray *)accessibleElements | ||
- (NSArray *)accessibilityElements | ||
{ | ||
ASDisplayNode *viewNode = self.asyncdisplaykit_node; | ||
if (viewNode == nil) { | ||
return @[]; | ||
} | ||
|
||
if (_accessibleElements != nil) { | ||
return _accessibleElements; | ||
if (_accessibilityElements == nil) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we are using _accessibilityElements without grabbing the lock, we need to ensure it is only used on one thread. It probably is - the main thread - though it is possible to imagine some of our code (E.g. in ASDisplayNode+Yoga.mm or somewhere else in the layout system) wanting to invalidate the current accessibilityElements and accidentally doing it on a background thread, which would be bad! As such, let's assert this pre-existing assumption of the code by adding ASDisplayNodeAssertThreadIsMain() (check ASAssert.h if this name is incorrect). Add it both here and in the set: method. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
NSMutableArray *accessibilityElements = [[NSMutableArray alloc] init]; | ||
CollectAccessibilityElementsForView(self, accessibilityElements); | ||
SortAccessibilityElements(accessibilityElements); | ||
_accessibilityElements = accessibilityElements; | ||
} | ||
|
||
NSMutableArray *accessibleElements = [[NSMutableArray alloc] init]; | ||
CollectAccessibilityElementsForView(self, accessibleElements); | ||
SortAccessibilityElements(accessibleElements); | ||
_accessibleElements = accessibleElements; | ||
|
||
return _accessibleElements; | ||
} | ||
|
||
- (NSInteger)accessibilityElementCount | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey @jiawernlim ! Are you sure we can remove this accessors? I created a sample project and tried to verify that, but all of these accessors do not call through to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @maicki , thanks for checking! I wrote a small unit test to test the accessors, and they appear to work fine. I've added those tests here. Could you cross-reference with your sample project and see if there's anything I'm missing in the tests? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jiawernlim I just ran your test and it seems to be failing: Here is also the example project that shows the behavior I said before: Also the part of the Apple documentation that describes the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @maicki Lim and I have found some pretty crazy behavior here. If accessibility is enabled, say by turning on the accessibility inspector, the system links the private
So basically, you get 0 if no accessibility and otherwise you get the array. And, these implementations are faster than the naïve implementations of Also, the documentation on So if we remove them, we get better performance but people calling them directly when accessibility is disabled will get 0. One crazy option is to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Crazy town! Based on that, let's move forward with the diff. Less code in this area is better in the first place. We should put some safety in though. We could use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hello @maicki, just checking in to see if you have any suggestions on better ways to determine if accessibility is on. We could definitely just proceed with using What are your thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is only an issue for the unit tests passing, and only when the inspector is on, I think that is an OK compromise. Could you add a comment to the top of the test in question calling out this behavior? Something like
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @appleguy, the problem is that those unit tests will fail if the accessibility inspector is not on, and as such it'd fail on most systems by default. There does not seem to be an elegant way to test if the inspector is on, nor is there any way I have been able to find to programmatically turn on the inspector. That unit test could be removed, though then we would have no guarantees that they'd continue to work in the future. If we take this route, then perhaps we could add warnings in the header file "_ASDisplayViewAccessibility.h" to recommend use of That being said, the UIKit docs quite specifically state that "Containers can implement this ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jiawernlim Sorry didn't see your messages a couple of days ago, but I would say let's get this PR landed :) Yeah unfortunately it seems there is really no way to detect if the accessibility inspector is running. I would suggest let's comment out the unit tests and add a comment why they are commented out and add some kind of warning in the header file (as you mentioned the docs for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
{ | ||
return self.accessibleElements.count; | ||
} | ||
|
||
- (id)accessibilityElementAtIndex:(NSInteger)index | ||
{ | ||
return self.accessibleElements[index]; | ||
} | ||
|
||
- (NSInteger)indexOfAccessibilityElement:(id)element | ||
{ | ||
return [self.accessibleElements indexOfObjectIdenticalTo:element]; | ||
return _accessibilityElements; | ||
} | ||
|
||
@end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
// | ||
// ASDisplayViewAccessibilityTests.mm | ||
// Texture | ||
// | ||
// Copyright (c) 2018-present, Pinterest, Inc. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
|
||
#import <XCTest/XCTest.h> | ||
|
||
#import <AsyncDisplayKit/ASDisplayNode.h> | ||
|
||
@interface ASDisplayViewAccessibilityTests : XCTestCase | ||
@end | ||
|
||
@implementation ASDisplayViewAccessibilityTests | ||
|
||
- (void)testAccessibilityElementsAccessors | ||
{ | ||
// Setup nodes with accessibility info | ||
ASDisplayNode *node = nil; | ||
ASDisplayNode *subnode = nil; | ||
node = [[ASDisplayNode alloc] init]; | ||
subnode = [[ASDisplayNode alloc] init]; | ||
NSString *label = @"foo"; | ||
subnode.isAccessibilityElement = YES; | ||
subnode.accessibilityLabel = label; | ||
[node addSubnode:subnode]; | ||
|
||
XCTAssertEqualObjects([node.view.accessibilityElements.firstObject accessibilityLabel], label); | ||
XCTAssertEqualObjects([[node.view accessibilityElementAtIndex:0] accessibilityLabel], label); | ||
XCTAssertEqual(node.view.accessibilityElementCount, 1); | ||
XCTAssertEqual([node.view indexOfAccessibilityElement:node.view.accessibilityElements.firstObject], 0); | ||
} | ||
|
||
@end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jiawernlim had this file been missing? It's fine to add it here if it's the only copy; maybe Xcode fixed a state problem without you adding this explicitly as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you're referring to line 1261? I checked and it looks like "ASCollectionsTests.m" was not in the
children()
list before, so yup it's probably Xcode that fixed some kind of state issue.