[Accessibility] Ship ASExperimentalDoNotCacheAccessibilityElements (#…

…1888)

We did not notice any effect on performance of the Pinterest app by not caching `accessibilityElements` in `_ASDisplayView`. By not caching the elements, we can be sure that the elements will be correct even when nodes change visibility state. There will be a performance impact when voice over is enabled, but providing the correct elements for the current state of a view is more important than performance in this case.

TextureGroup/Texture#1853

Source/ASExperimentalFeatures.h

@@ -28,7 +28,6 @@ typedef NS_OPTIONS(NSUInteger, ASExperimentalFeatures) {
   ASExperimentalDispatchApply = 1 << 7,                                     // exp_dispatch_apply
   ASExperimentalDrawingGlobal = 1 << 8,                                     // exp_drawing_global
   ASExperimentalOptimizeDataControllerPipeline = 1 << 9,                    // exp_optimize_data_controller_pipeline
-  ASExperimentalDoNotCacheAccessibilityElements = 1 << 10,                  // exp_do_not_cache_accessibility_elements
   ASExperimentalFeatureAll = 0xFFFFFFFF
 };
 

Source/ASExperimentalFeatures.mm

@@ -21,8 +21,7 @@
                                       @"exp_did_enter_preload_skip_asm_layout",
                                       @"exp_dispatch_apply",
                                       @"exp_drawing_global",
-                                      @"exp_optimize_data_controller_pipeline",
-                                      @"exp_do_not_cache_accessibility_elements"]));
+                                      @"exp_optimize_data_controller_pipeline"]));
   if (flags == ASExperimentalFeatureAll) {
     return allNames;
   }

Source/Details/_ASDisplayViewAccessiblity.mm

@@ -311,24 +311,15 @@ static void CollectAccessibilityElements(ASDisplayNode *node, NSMutableArray *el
   }
 }
 
-@interface _ASDisplayView () {
-  NSArray *_accessibilityElements;
-}
-
-@end
-
 @implementation _ASDisplayView (UIAccessibilityContainer)
 
 #pragma mark - UIAccessibility
 
 - (void)setAccessibilityElements:(NSArray *)accessibilityElements
 {
-  ASDisplayNodeAssertMainThread();
-  // While it looks very strange to ignore the accessibilyElements param and set _accessibilityElements to nil, it is actually on purpose.
-  // _ASDisplayView's accessibilityElements method will always defer to the node for accessibilityElements when _accessibilityElements is
-  // nil. Calling setAccessibilityElements on _ASDisplayView is basically clearing the cache and forcing _ASDisplayView to ask the node
-  // for its accessibilityElements the next time they are requested.
-  _accessibilityElements = nil;
+  // this is a no-op. You should not be setting accessibilityElements directly on _ASDisplayView.
+  // if you wish to set accessibilityElements, do so in your node. UIKit will call _ASDisplayView's
+  // accessibilityElements which will in turn ask its node for its elements.
 }
 
 - (NSArray *)accessibilityElements
@@ -340,12 +331,12 @@ - (NSArray *)accessibilityElements
     return @[];
   }
 
-  // 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;
+  // we no longer cache accessibilityElements. When caching, in order to provide correct element when items become hidden/visible
+  // we had to manually clear _accessibilityElements. This seemed like a heavy burden to place on a user, and one that is also
+  // not immediately obvious. While recomputing accessibilityElements may be expensive, this will only affect users that have
+  // voice over enabled (we checked to ensure performance did not suffer by not caching for an overall user base). For those
+  // users with voice over on, being correct is almost certainly more important than being performant.
+  return [viewNode accessibilityElements];
 }
 
 @end

Source/Private/ASDisplayNode+UIViewBridge.mm

@@ -1137,22 +1137,6 @@ - (BOOL)_locked_insetsLayoutMarginsFromSafeArea
 
 @implementation ASDisplayNode (UIViewBridgeAccessibility)
 
-// Walks up the view tree to nil out all the cached accsesibilityElements. This is required when changing
-// accessibility properties like accessibilityViewIsModal.
-- (void)invalidateAccessibilityElements
-{
-  // If we are not caching accessibilityElements we don't need to do anything here.
-  if (ASActivateExperimentalFeature(ASExperimentalDoNotCacheAccessibilityElements)) {
-    return;
-  }
-
-  // we want to check if we are on the main thread first, since _loaded checks the layer and can only be done on main
-  if (ASDisplayNodeThreadIsMain() && _loaded(self)) {
-    self.view.accessibilityElements = nil;
-    [self.supernode invalidateAccessibilityElements];
-  }
-}
-
 - (BOOL)isAccessibilityElement
 {
   _bridge_prologue_read;
@@ -1310,13 +1294,7 @@ - (BOOL)accessibilityElementsHidden
 - (void)setAccessibilityElementsHidden:(BOOL)accessibilityElementsHidden
 {
   _bridge_prologue_write;
-  BOOL oldHiddenValue = _getFromViewOnly(accessibilityElementsHidden);
   _setAccessibilityToViewAndProperty(_flags.accessibilityElementsHidden, accessibilityElementsHidden, accessibilityElementsHidden, accessibilityElementsHidden);
-
-  // if we made a change, we need to clear the view's accessibilityElements cache.
-  if (!ASActivateExperimentalFeature(ASExperimentalDoNotCacheAccessibilityElements) && self.isNodeLoaded && oldHiddenValue != accessibilityElementsHidden) {
-    [self invalidateAccessibilityElements];
-  }
 }
 
 - (BOOL)accessibilityViewIsModal
@@ -1328,16 +1306,9 @@ - (BOOL)accessibilityViewIsModal
 - (void)setAccessibilityViewIsModal:(BOOL)accessibilityViewIsModal
 {
   _bridge_prologue_write;
-  BOOL oldAccessibilityViewIsModal = _getFromViewOnly(accessibilityViewIsModal);
   _setAccessibilityToViewAndProperty(_flags.accessibilityViewIsModal, accessibilityViewIsModal, accessibilityViewIsModal, accessibilityViewIsModal);
-
-  // if we made a change, we need to clear the view's accessibilityElements cache.
-  if (!ASActivateExperimentalFeature(ASExperimentalDoNotCacheAccessibilityElements) && self.isNodeLoaded && oldAccessibilityViewIsModal != accessibilityViewIsModal) {
-    [self invalidateAccessibilityElements];
-  }
 }
 
-
 - (BOOL)shouldGroupAccessibilityChildren
 {
   _bridge_prologue_read;

Tests/ASConfigurationTests.mm

@@ -28,7 +28,6 @@
   ASExperimentalDispatchApply,
   ASExperimentalDrawingGlobal,
   ASExperimentalOptimizeDataControllerPipeline,
-  ASExperimentalDoNotCacheAccessibilityElements,
 };
 
 @interface ASConfigurationTests : ASTestCase <ASConfigurationDelegate>
@@ -51,7 +50,6 @@ + (NSArray *)names {
     @"exp_dispatch_apply",
     @"exp_drawing_global",
     @"exp_optimize_data_controller_pipeline",
-    @"exp_do_not_cache_accessibility_elements",
   ];
 }