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.

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

[Accessibility] Ship ASExperimentalDoNotCacheAccessibilityElements #1888

Merged
merged 1 commit into from
Jul 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 0 additions & 1 deletion Source/ASExperimentalFeatures.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
};

Expand Down
3 changes: 1 addition & 2 deletions Source/ASExperimentalFeatures.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
27 changes: 9 additions & 18 deletions Source/Details/_ASDisplayViewAccessiblity.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
29 changes: 0 additions & 29 deletions Source/Private/ASDisplayNode+UIViewBridge.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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;
Expand Down
2 changes: 0 additions & 2 deletions Tests/ASConfigurationTests.mm
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
ASExperimentalDispatchApply,
ASExperimentalDrawingGlobal,
ASExperimentalOptimizeDataControllerPipeline,
ASExperimentalDoNotCacheAccessibilityElements,
};

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

Expand Down