Refactored `accessibleElements` to `accessibilityElements` #1069

jiawernlim · 2018-08-08T23:33:38Z

Refactored accessibleElements to accessibilityElements and removed the re-definition of the property in ASDisplayView.

With this refactor, the field can now be used as a single access point into the accessibility elements of a view. Also, removing the re-definition of the property in _ASDisplayViewAccessibility.h enables clients to make use of the field and its associated helper methods directly from the UIAccessibilityContainer API rather than having Texture roll its own implementation.

…ed the re-definition of the property. With this refactor, the field can now be used as a single access point into the accessibility elements of a view. Also, removing the re-definition of the property in _ASDisplayViewAccessibility.h enables us to make use of the field and its associated helper methods directly from the `UIAccessibilityContainer` API rather than rolling our own implementation.

maicki · 2018-08-09T14:29:50Z

Source/Details/_ASDisplayViewAccessiblity.mm

-  return _accessibleElements;
-}
-
- (NSInteger)accessibilityElementCount


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.

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?

@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:
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

@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:

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?

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.

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?

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

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.

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

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.

Adlai-Holler · 2018-08-10T17:03:58Z

@maicki Would you mind unblocking the CI build for Lim?

ghost · 2018-08-10T17:46:25Z

🚫 CI failed with log

maicki · 2018-08-10T17:53:21Z

@Adlai-Holler Based on the investigation within
#1069 (comment) the test will fail on CI though as there is not accessibility enabled.

appleguy · 2018-08-24T05:13:11Z

AsyncDisplayKit.xcodeproj/project.pbxproj

@@ -1255,6 +1257,8 @@
 		058D09C5195D04C000B7D73C /* Tests */ = {
 			isa = PBXGroup;
 			children = (
+				F3F698D1211CAD4600800CB1 /* ASDisplayViewAccessibilityTests.mm */,
+				CC35CEC520DD87280006448D /* ASCollectionsTests.m */,


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

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.

appleguy · 2018-08-24T05:14:18Z

Source/Details/_ASDisplayViewAccessiblity.h

@@ -19,5 +19,4 @@
 #import <AsyncDisplayKit/_ASDisplayView.h>

 @interface _ASDisplayView (UIAccessibilityContainer)


Let's remove this whole category definition (line 21 and 23)

appleguy · 2018-08-24T05:17:11Z

Source/Details/_ASDisplayViewAccessiblity.mm

 {
  ASDisplayNode *viewNode = self.asyncdisplaykit_node;
  if (viewNode == nil) {
    return @[];
  }

-  if (_accessibleElements != nil) {
-    return _accessibleElements;
+  if (_accessibilityElements == nil) {


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.

appleguy · 2018-08-24T05:20:11Z

Source/Details/_ASDisplayViewAccessiblity.mm

-  return _accessibleElements;
-}
-
- (NSInteger)accessibilityElementCount


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

ghost · 2018-08-24T06:34:02Z

🚫 CI failed with log

Also added assertions that the getter and setter for the accessibilityElements property are used only on the main thread.

maicki · 2018-08-24T18:41:19Z

@jiawernlim LGTM let's get it in. Thanks for working on this!

…oup#1069) * Refactored `accessibleElements` to `accessibilityElements`, and removed the re-definition of the property. With this refactor, the field can now be used as a single access point into the accessibility elements of a view. Also, removing the re-definition of the property in _ASDisplayViewAccessibility.h enables us to make use of the field and its associated helper methods directly from the `UIAccessibilityContainer` API rather than rolling our own implementation. * Added tests for the accessors to ASDisplayView.accessibilityElements. * Commented out tests for older a11y accessors & added relevant warnings. Also added assertions that the getter and setter for the accessibilityElements property are used only on the main thread.

maicki reviewed Aug 9, 2018

View reviewed changes

Added tests for the accessors to ASDisplayView.accessibilityElements.

586cc46

Merge branch 'master' into master

0a5b3a2

appleguy approved these changes Aug 24, 2018

View reviewed changes

jiawernlim added 2 commits August 24, 2018 10:11

Commented out tests for older a11y accessors & added relevant warnings.

2018422

Also added assertions that the getter and setter for the accessibilityElements property are used only on the main thread.

Merge branch 'master' of github.com:jiawernlim/Texture

6703c38

maicki merged commit 1d31fc2 into TextureGroup:master Aug 24, 2018

maicki mentioned this pull request Oct 27, 2018

A11y for scrollnode #1188

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored `accessibleElements` to `accessibilityElements` #1069

Refactored `accessibleElements` to `accessibilityElements` #1069

jiawernlim commented Aug 8, 2018 •

edited

Loading

maicki Aug 9, 2018 •

edited

Loading

jiawernlim Aug 9, 2018

maicki Aug 10, 2018

Adlai-Holler Aug 10, 2018 •

edited

Loading

maicki Aug 10, 2018 •

edited

Loading

jiawernlim Aug 20, 2018

appleguy Aug 24, 2018

jiawernlim Aug 24, 2018 •

edited

Loading

maicki Aug 24, 2018 •

edited

Loading

jiawernlim Aug 24, 2018

Adlai-Holler commented Aug 10, 2018

ghost commented Aug 10, 2018

maicki commented Aug 10, 2018

appleguy Aug 24, 2018

jiawernlim Aug 24, 2018

appleguy Aug 24, 2018

appleguy Aug 24, 2018

jiawernlim Aug 24, 2018

appleguy Aug 24, 2018

ghost commented Aug 24, 2018

maicki commented Aug 24, 2018 •

edited

Loading

		@@ -19,5 +19,4 @@
		#import <AsyncDisplayKit/_ASDisplayView.h>

		@interface _ASDisplayView (UIAccessibilityContainer)

Refactored accessibleElements to accessibilityElements #1069

Refactored accessibleElements to accessibilityElements #1069

Conversation

jiawernlim commented Aug 8, 2018 • edited Loading

maicki Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adlai-Holler Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

maicki Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiawernlim Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

maicki Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adlai-Holler commented Aug 10, 2018

ghost commented Aug 10, 2018

maicki commented Aug 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Aug 24, 2018

maicki commented Aug 24, 2018 • edited Loading

Refactored `accessibleElements` to `accessibilityElements` #1069

Refactored `accessibleElements` to `accessibilityElements` #1069

jiawernlim commented Aug 8, 2018 •

edited

Loading

maicki Aug 9, 2018 •

edited

Loading

Adlai-Holler Aug 10, 2018 •

edited

Loading

maicki Aug 10, 2018 •

edited

Loading

jiawernlim Aug 24, 2018 •

edited

Loading

maicki Aug 24, 2018 •

edited

Loading

maicki commented Aug 24, 2018 •

edited

Loading