[ASDisplayViewAccessibility] A few accessibility improvements (#1812)

* [ASDisplayViewAccessibility] A few accessibility improvements This diff includes a few improvements for accessibility in Texture. * When determining a node’s accessibility elements, ignore any elements that are hidden, transparent, or out of the window. This matches UIKit’s behavior. * When sorting the accessible elements and their origins are equal, give precedence to elements with shorter accessibility frames, followed by elements with narrower widths. * Allow the ability to customize the comparator block that sorts the accessibility elements * Create an experiment to stop caching accessibilityElements in `_ASDisplayView`. If we cache elements, we will require users to clear the cache (by calling `setAccessibilityElements` to get the side effect of clearing the cache) when nodes change their hidden state or opacity. This seems like a lot to request of the user. We will put this in an experiment so we can see the perf implication is both when using voice over and when not using voice over. A few other notes: * I got rid of the `ASAccessibilityElementPositioning` protocol in favor of passing `NSObjects` to the sort comparator. `NSObject` implements the informal `UIAccessibilityProtocol` and therefore has an `accessibilityFrame` property. * I removed `static` from the `SortAccessibilityElements()` method definition. This allows me to declare it as `extern` in test it via unit tests.
TextureGroup · May 6, 2020 · d085cd6 · d085cd6
1 parent 508bd2b
commit d085cd6
Show file tree

Hide file tree

Showing 9 changed files with 267 additions and 51 deletions.
diff --git a/Podfile.lock b/Podfile.lock
@@ -2,35 +2,16 @@ PODS:
   - FBSnapshotTestCase/Core (2.1.4)
   - JGMethodSwizzler (2.0.1)
   - OCMock (3.4.1)
-  - PINCache (3.0.1-beta.7):
-    - PINCache/Arc-exception-safe (= 3.0.1-beta.7)
-    - PINCache/Core (= 3.0.1-beta.7)
-  - PINCache/Arc-exception-safe (3.0.1-beta.7):
-    - PINCache/Core
-  - PINCache/Core (3.0.1-beta.7):
-    - PINOperation (~> 1.1.1)
-  - PINOperation (1.1.1)
-  - PINRemoteImage (3.0.0-beta.14):
-    - PINRemoteImage/PINCache (= 3.0.0-beta.14)
-  - PINRemoteImage/Core (3.0.0-beta.14):
-    - PINOperation
-  - PINRemoteImage/PINCache (3.0.0-beta.14):
-    - PINCache (= 3.0.1-beta.7)
-    - PINRemoteImage/Core
 
 DEPENDENCIES:
   - FBSnapshotTestCase/Core (~> 2.1)
   - JGMethodSwizzler (from `https://github.com/JonasGessner/JGMethodSwizzler`, branch `master`)
   - OCMock (= 3.4.1)
-  - PINRemoteImage (= 3.0.0-beta.14)
 
 SPEC REPOS:
-  https://github.com/cocoapods/specs.git:
+  https://github.com/CocoaPods/Specs.git:
     - FBSnapshotTestCase
     - OCMock
-    - PINCache
-    - PINOperation
-    - PINRemoteImage
 
 EXTERNAL SOURCES:
   JGMethodSwizzler:
@@ -46,10 +27,7 @@ SPEC CHECKSUMS:
   FBSnapshotTestCase: 094f9f314decbabe373b87cc339bea235a63e07a
   JGMethodSwizzler: 7328146117fffa8a4038c42eb7cd3d4c75006f97
   OCMock: 2cd0716969bab32a2283ff3a46fd26a8c8b4c5e3
-  PINCache: 7cb9ae068c8f655717f7c644ef1dff9fd573e979
-  PINOperation: a6219e6fc9db9c269eb7a7b871ac193bcf400aac
-  PINRemoteImage: 81bbff853acc71c6de9e106e9e489a791b8bbb08
 
-PODFILE CHECKSUM: 445046ac151568c694ff286684322273f0b597d6
+PODFILE CHECKSUM: 345a6700f5fdec438ef5553e1eebf62653862733
 
-COCOAPODS: 1.6.0
+COCOAPODS: 1.9.1
diff --git a/Schemas/configuration.json b/Schemas/configuration.json
@@ -24,6 +24,7 @@
                     "exp_did_enter_preload_skip_asm_layout",
                     "exp_dispatch_apply",
                     "exp_oom_bg_dealloc_disable",
+                    "exp_do_not_cache_accessibility_elements",
                 ]
     		}
 		}

diff --git a/Source/ASExperimentalFeatures.h b/Source/ASExperimentalFeatures.h
@@ -31,6 +31,7 @@ typedef NS_OPTIONS(NSUInteger, ASExperimentalFeatures) {
   ASExperimentalDrawingGlobal = 1 << 10,                                    // exp_drawing_global
   ASExperimentalOptimizeDataControllerPipeline = 1 << 11,                   // exp_optimize_data_controller_pipeline
   ASExperimentalTraitCollectionDidChangeWithPreviousCollection = 1 << 12,   // exp_trait_collection_did_change_with_previous_collection
+  ASExperimentalDoNotCacheAccessibilityElements = 1 << 13,                  // exp_do_not_cache_accessibility_elements
   ASExperimentalFeatureAll = 0xFFFFFFFF
 };
 

diff --git a/Source/ASExperimentalFeatures.mm b/Source/ASExperimentalFeatures.mm
@@ -24,7 +24,8 @@
                                       @"exp_oom_bg_dealloc_disable",
                                       @"exp_drawing_global",
                                       @"exp_optimize_data_controller_pipeline",
-                                      @"exp_trait_collection_did_change_with_previous_collection"]));
+                                      @"exp_trait_collection_did_change_with_previous_collection",
+                                      @"exp_do_not_cache_accessibility_elements"]));
   if (flags == ASExperimentalFeatureAll) {
     return allNames;
   }

diff --git a/Source/Details/_ASDisplayViewAccessiblity.h b/Source/Details/_ASDisplayViewAccessiblity.h
@@ -14,3 +14,17 @@
 // should still work as long as accessibility is enabled, this framework provides no guarantees on
 // their correctness. For details, see
 // https://developer.apple.com/documentation/objectivec/nsobject/1615147-accessibilityelements
+
+// After recusively collecting all of the accessibility elements of a node, they get sorted. This sort determines
+// the order that a screen reader will traverse the elements. By default, we sort these elements based on their
+// origin: lower y origin comes first, then lower x origin. If 2 nodes have an equal origin, the node with the smaller
+// height is placed before the node with the smaller width. If two nodes have the exact same rect, we throw up our hands
+// and return NSOrderedSame.
+//
+// In general this seems to work fairly well. However, if you want to provide a custom sort you can do so via
+// setUserDefinedAccessibilitySortComparator(). The two elements you are comparing are NSObjects, which conforms to the
+// informal UIAccessibility protocol, so you can safely compare properties like accessibilityFrame.
+typedef NSComparisonResult (^ASSortAccessibilityElementsComparator)(NSObject *, NSObject *);
+
+// Use this method to supply your own custom sort comparator used to determine the order of the accessibility elements
+void setUserDefinedAccessibilitySortComparator(ASSortAccessibilityElementsComparator userDefinedComparator);
diff --git a/Source/Details/_ASDisplayViewAccessiblity.mm b/Source/Details/_ASDisplayViewAccessiblity.mm
@@ -10,6 +10,7 @@
 #ifndef ASDK_ACCESSIBILITY_DISABLE
 
 #import <AsyncDisplayKit/_ASDisplayView.h>
+#import <AsyncDisplayKit/_ASDisplayViewAccessiblity.h>
 #import <AsyncDisplayKit/ASAvailability.h>
 #import <AsyncDisplayKit/ASCollectionNode.h>
 #import <AsyncDisplayKit/ASDisplayNodeExtras.h>
@@ -21,43 +22,55 @@
 
 #pragma mark - UIAccessibilityElement
 
-@protocol ASAccessibilityElementPositioning
+static ASSortAccessibilityElementsComparator currentAccessibilityComparator = nil;
+static ASSortAccessibilityElementsComparator defaultAccessibilityComparator = nil;
 
-@property (nonatomic, readonly) CGRect accessibilityFrame;
-
-@end
-
-typedef NSComparisonResult (^SortAccessibilityElementsComparator)(id<ASAccessibilityElementPositioning>, id<ASAccessibilityElementPositioning>);
+void setUserDefinedAccessibilitySortComparator(ASSortAccessibilityElementsComparator userDefinedComparator) {
+  currentAccessibilityComparator = userDefinedComparator ?: defaultAccessibilityComparator;
+}
 
 /// Sort accessiblity elements first by y and than by x origin.
-static void SortAccessibilityElements(NSMutableArray *elements)
+void SortAccessibilityElements(NSMutableArray *elements)
 {
   ASDisplayNodeCAssertNotNil(elements, @"Should pass in a NSMutableArray");
-
-  static SortAccessibilityElementsComparator comparator = nil;
+
   static dispatch_once_t onceToken;
   dispatch_once(&onceToken, ^{
-      comparator = ^NSComparisonResult(id<ASAccessibilityElementPositioning> a, id<ASAccessibilityElementPositioning> b) {
-        CGPoint originA = a.accessibilityFrame.origin;
-        CGPoint originB = b.accessibilityFrame.origin;
-        if (originA.y == originB.y) {
-          if (originA.x == originB.x) {
-            return NSOrderedSame;
+    defaultAccessibilityComparator = ^NSComparisonResult(NSObject *a, NSObject *b) {
+      CGPoint originA = a.accessibilityFrame.origin;
+      CGPoint originB = b.accessibilityFrame.origin;
+      if (originA.y == originB.y) {
+        if (originA.x == originB.x) {
+          // if we have the same origin, favor shorter items. If heights are the same, favor thinner items. If size is the same ¯\_(ツ)_/¯
+          CGSize sizeA = a.accessibilityFrame.size;
+          CGSize sizeB = b.accessibilityFrame.size;
+          if (sizeA.height == sizeB.height) {
+            if (sizeA.width == sizeB.width) {
+              return NSOrderedSame;
+            }
+            return (sizeA.width < sizeB.width) ? NSOrderedAscending : NSOrderedDescending;
           }
-          return (originA.x < originB.x) ? NSOrderedAscending : NSOrderedDescending;
+          return (sizeA.height < sizeB.height) ? NSOrderedAscending : NSOrderedDescending;
         }
-        return (originA.y < originB.y) ? NSOrderedAscending : NSOrderedDescending;
-      };
+        return (originA.x < originB.x) ? NSOrderedAscending : NSOrderedDescending;
+      }
+      return (originA.y < originB.y) ? NSOrderedAscending : NSOrderedDescending;
+    };
+
+    if (!currentAccessibilityComparator) {
+      currentAccessibilityComparator = defaultAccessibilityComparator;
+    }
   });
-  [elements sortUsingComparator:comparator];
+
+  [elements sortUsingComparator:currentAccessibilityComparator];
 }
 
 static CGRect ASAccessibilityFrameForNode(ASDisplayNode *node) {
   CALayer *layer = node.layer;
   return [layer convertRect:node.bounds toLayer:ASFindWindowOfLayer(layer).layer];
 }
 
-@interface ASAccessibilityElement : UIAccessibilityElement<ASAccessibilityElementPositioning>
+@interface ASAccessibilityElement : UIAccessibilityElement
 
 @property (nonatomic) ASDisplayNode *node;
 
@@ -93,7 +106,7 @@ - (CGRect)accessibilityFrame
 
 #pragma mark - _ASDisplayView / UIAccessibilityContainer
 
-@interface ASAccessibilityCustomAction : UIAccessibilityCustomAction<ASAccessibilityElementPositioning>
+@interface ASAccessibilityCustomAction : UIAccessibilityCustomAction
 
 @property (nonatomic) ASDisplayNode *node;
 
@@ -214,6 +227,11 @@ static void CollectAccessibilityElements(ASDisplayNode *node, NSMutableArray *el
 
   UIView *view = node.view;
 
+  // If we don't have a window, let's just bail out
+  if (!view.window) {
+    return;
+  }
+
   if (node.isAccessibilityContainer && !anySubNodeIsCollection) {
     CollectAccessibilityElementsForContainer(node, view, elements);
     return;
@@ -224,8 +242,18 @@ static void CollectAccessibilityElements(ASDisplayNode *node, NSMutableArray *el
     CollectUIAccessibilityElementsForNode(node, node, view, elements);
     return;
   }
-
+  
   for (ASDisplayNode *subnode in node.subnodes) {
+    // If a node is hidden or has an alpha of 0.0 we should not include it
+    if (subnode.hidden || subnode.alpha == 0.0) {
+        continue;
+    }
+    // If a subnode is outside of the view's window, exclude it
+    CGRect nodeInWindowCoords = [node convertRect:subnode.frame toNode:nil];
+    if (!CGRectIntersectsRect(view.window.frame, nodeInWindowCoords)) {
+      continue;
+    }
+
     if (subnode.isAccessibilityElement) {
       // An accessiblityElement can either be a UIView or a UIAccessibilityElement
       if (subnode.isLayerBacked) {
@@ -275,7 +303,9 @@ - (NSArray *)accessibilityElements
     return @[];
   }
 
-  if (_accessibilityElements == nil) {
+  // when items become hidden/visible we have to manually clear the _accessibilityElements in order to get an updated version
+  // Instead, let's try computing the elements every time and see how badly it affects performance.
+  if (_accessibilityElements == nil || ASActivateExperimentalFeature(ASExperimentalDoNotCacheAccessibilityElements)) {
     _accessibilityElements = [viewNode accessibilityElements];
   }
   return _accessibilityElements;

diff --git a/Tests/ASConfigurationTests.mm b/Tests/ASConfigurationTests.mm
@@ -29,7 +29,9 @@
   ASExperimentalDispatchApply,
   ASExperimentalOOMBackgroundDeallocDisable,
   ASExperimentalDrawingGlobal,
-  ASExperimentalOptimizeDataControllerPipeline
+  ASExperimentalOptimizeDataControllerPipeline,
+  ASExperimentalTraitCollectionDidChangeWithPreviousCollection,
+  ASExperimentalDoNotCacheAccessibilityElements,
 };
 
 @interface ASConfigurationTests : ASTestCase <ASConfigurationDelegate>
@@ -53,7 +55,9 @@ + (NSArray *)names {
     @"exp_dispatch_apply",
     @"exp_oom_bg_dealloc_disable",
     @"exp_drawing_global",
-    @"exp_optimize_data_controller_pipeline"
+    @"exp_optimize_data_controller_pipeline",
+    @"exp_trait_collection_did_change_with_previous_collection",
+    @"exp_do_not_cache_accessibility_elements",
   ];
 }