Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refactored accessibleElements to accessibilityElements #1069

Merged
merged 5 commits into from
Aug 24, 2018
Merged

Refactored accessibleElements to accessibilityElements #1069

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a3eecd2
Refactored `accessibleElements` to `accessibilityElements`, and remov…
jiawernlim Aug 8, 2018
586cc46
Added tests for the accessors to ASDisplayView.accessibilityElements.
jiawernlim Aug 9, 2018
0a5b3a2
Merge branch 'master' into master
appleguy Aug 24, 2018
2018422
Commented out tests for older a11y accessors & added relevant warnings.
jiawernlim Aug 24, 2018
6703c38
Merge branch 'master' of github.com:jiawernlim/Texture
jiawernlim Aug 24, 2018
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
- Add an experimental feature that reuses CTFramesetter objects in ASTextNode2 to improve performance. [Adlai Holler](https://github.com/Adlai-Holler)
- Add NS_DESIGNATED_INITIALIZER to ASViewController initWithNode: [Michael Schneider](https://github.com/maicki) [#1054](https://github.com/TextureGroup/Texture/pull/1054)
- Optimize text stack by removing unneeded copying. [Adlai Holler](https://github.com/Adlai-Holler)
- Renamed `accessibleElements` to `accessibilityElements` and removed the re-definition of the property in ASDisplayView. [Jia Wern Lim](https://github.com/jiawernlim)

## 2.7
- Fix pager node for interface coalescing. [Max Wang](https://github.com/wsdwsd0829) [#877](https://github.com/TextureGroup/Texture/pull/877)
Expand Down
2 changes: 1 addition & 1 deletion Source/ASDisplayNode+Yoga.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ - (void)calculateLayoutFromYogaRoot:(ASSizeRange)rootConstrainedSize

// Reset accessible elements, since layout may have changed.
ASPerformBlockOnMainThread(^{
[(_ASDisplayView *)self.view setAccessibleElements:nil];
[(_ASDisplayView *)self.view setAccessibilityElements:nil];
});

ASDisplayNodePerformBlockOnEveryYogaChild(self, ^(ASDisplayNode * _Nonnull node) {
Expand Down
8 changes: 4 additions & 4 deletions Source/Details/_ASDisplayView.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,8 @@ @implementation _ASDisplayView
BOOL _inHitTest;
BOOL _inPointInside;

NSArray *_accessibleElements;
CGRect _lastAccessibleElementsFrame;
NSArray *_accessibilityElements;
CGRect _lastAccessibilityElementsFrame;

_ASDisplayViewMethodOverrides _methodOverrides;
}
Expand Down Expand Up @@ -303,7 +303,7 @@ - (void)addSubview:(UIView *)view
[super addSubview:view];

#ifndef ASDK_ACCESSIBILITY_DISABLE
self.accessibleElements = nil;
self.accessibilityElements = nil;
#endif
}

Expand All @@ -312,7 +312,7 @@ - (void)willRemoveSubview:(UIView *)subview
[super willRemoveSubview:subview];

#ifndef ASDK_ACCESSIBILITY_DISABLE
self.accessibleElements = nil;
self.accessibilityElements = nil;
#endif
}

Expand Down
1 change: 0 additions & 1 deletion Source/Details/_ASDisplayViewAccessiblity.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,4 @@
#import <AsyncDisplayKit/_ASDisplayView.h>

@interface _ASDisplayView (UIAccessibilityContainer)
Copy link
Member

Choose a reason for hiding this comment

The 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
37 changes: 10 additions & 27 deletions Source/Details/_ASDisplayViewAccessiblity.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ static void CollectAccessibilityElementsForView(_ASDisplayView *view, NSMutableA
}

@interface _ASDisplayView () {
NSArray *_accessibleElements;
NSArray *_accessibilityElements;
}

@end
Expand All @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Contributor

@maicki maicki Aug 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

The 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 accessibilityElements getter. I also looked into Hopper and the default implementation just returns 0 for e.g. accessibilityElementCount.

screen shot 2018-08-09 at 7 28 56 am

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jiawernlim I just ran your test and it seems to be failing:
screen shot 2018-08-09 at 7 08 24 pm

Here is also the example project that shows the behavior I said before:
iOSPlayground.zip

Also the part of the Apple documentation that describes the UIAccessibilityContainer API:
https://developer.apple.com/library/archive/documentation/UserExperience/Conceptual/iPhoneAccessibility/Making_Application_Accessible/Making_Application_Accessible.html#//apple_ref/doc/uid/TP40008785-CH102-SW10

Copy link
Member

@Adlai-Holler Adlai-Holler Aug 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

The 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 UIAccessibility framework, which overrides these methods to be based on accessibilityElements:

screen shot 2018-08-10 at 10 27 22 am

  • Note: accessibilityContainerElements is a private function that caches and observes the public accessibilityElements.

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 accessibilityElementAtIndex: and friends, which we can't get at if we override them. So there's that.

Also, the documentation on -accessibilityElements says explicitly This can be used as an alternative to implementing the dynamic methods. implying that it's OK to do either.

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 #if DEBUG and if someone calls them when accessibility is disabled (could detect by using the public functions or by searching the runtime for the private NSObjectAccessibility accessibility class) then we make an assertion. That gets us the production performance and some safety. Thoughts?

Copy link
Contributor

@maicki maicki Aug 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

The 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 UIAccessibilityIsVoiceOverRunning for detecting (not a fan of the private class) and throw an assertion in this place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 UIAccessibilityIsVoiceOverRunning, but as mentioned earlier it will still fail when developers have the accessibility inspector on but voiceover off. We could also go with the option that Adlai had suggested, which was to search for the private class, but I agree that's not an elegant solution either.

What are your thoughts?

Copy link
Member

Choose a reason for hiding this comment

The 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

// NOTE: If the Accessibility Inspector is running, this test may fail.
// For details, see https://github.com/TextureGroup/Texture/pull/1069/files#r208951338

Copy link
Contributor Author

@jiawernlim jiawernlim Aug 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The 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 accessibilityElements exclusively, and also mention that no guarantees are offered on the older accessors e.g. accessibilityElementCount.

That being said, the UIKit docs quite specifically state that "Containers can implement this (accessibilityElements) property instead of the dynamic methods to support the retrieval of the contained elements." So realistically, the correctness of the older accessors should not be an issue as long as accessibility is enabled. Hence, I personally don't think this should be an issue even without having the unit tests, especially if we added some kind of warning in the header file.

Copy link
Contributor

@maicki maicki Aug 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The 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 accessibilityElements we should be relatively save there) and we should be good to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. @maicki @appleguy I added warnings in the header and commented out the test lines for the older dynamic a11y accessors.

Let me know if you think of anything else. Otherwise I think we should be good to land this as long as tests pass.

{
return self.accessibleElements.count;
}

- (id)accessibilityElementAtIndex:(NSInteger)index
{
return self.accessibleElements[index];
}

- (NSInteger)indexOfAccessibilityElement:(id)element
{
return [self.accessibleElements indexOfObjectIdenticalTo:element];
return _accessibilityElements;
}

@end
Expand Down