Support setting `accessibilityElements` #1488

raycsh017 · 2019-05-03T23:28:57Z

Sometimes we want to override accessibilityElements so that we can
skip over elements unnecessary for interacting with the app using VoiceOver
or limit the interaction to child viewcontrollers presented, etc.

For this purpose, this commit adds support for setting custom
accessibilityElements. If not set, accessibilityElements should still
return the elements found from the view/layer hierarchy.

CLAassistant · 2019-05-03T23:29:05Z

All committers have signed the CLA.

nguyenhuy

Thanks for submitting the diff, @raycsh017.

Do you think the API will be more extensible if we just declare -[ASDisplayNode accessibilityElements] in a public interface and thus make it open for overriding? Clients then can have their own custom array to return everytime, or modify the array returned by [super accessibilityElements].

nguyenhuy · 2019-05-07T20:03:39Z

Source/Details/_ASDisplayViewAccessiblity.mm

@@ -257,6 +258,7 @@ - (void)setAccessibilityElements:(NSArray *)accessibilityElements
 {
  ASDisplayNodeAssertMainThread();
  _accessibilityElements = nil;


It's odd that this setter doesn't consider the provided param and always sets the ivar to nil. We probably should rename it to resetAccessibilityElements or clearAccessibilityElements?

Yeah that sounds good, I wasn't sure if this was intended or we just haven't had a chance to implement this portion yet :/

I think setting the provided param to _accessibilityElements is the right way to do.

@maicki: thoughts?

Also, if the _accessibilityElements ivar is updated here, we may not even need a new _customAccessibilityElements ivar because setting a non-nil array here will cause the setter to return it (i.e it won't call [viewNode accessibilityElements].

raycsh017 · 2019-05-09T01:02:38Z

@nguyenhuy
I like that idea, but in that case, wouldn't each node have to know in what context they are being displayed? For example, if there are multiple instances of a node subclass used in different view controllers and those instances need to have different accessibilityElements based on the view controller they are in, they would need to know where and how they are being displayed.

Also, it could be somewhat problematic if you want to set accessibilityElements of the main view/node in a view controller, because that's created default by the view controller, and we would either have to swap out the view with a view subclass that overrides the accessibilityElements or place a view subclass on top of the main view.

Source/Details/_ASDisplayViewAccessiblity.mm

nguyenhuy · 2019-05-20T20:55:24Z

Source/Details/_ASDisplayViewAccessiblity.mm

@@ -257,6 +258,7 @@ - (void)setAccessibilityElements:(NSArray *)accessibilityElements
 {
  ASDisplayNodeAssertMainThread();
  _accessibilityElements = nil;


I think setting the provided param to _accessibilityElements is the right way to do.

@maicki: thoughts?

nguyenhuy · 2019-05-20T20:59:14Z

Source/Details/_ASDisplayViewAccessiblity.mm

@@ -257,6 +258,7 @@ - (void)setAccessibilityElements:(NSArray *)accessibilityElements
 {
  ASDisplayNodeAssertMainThread();
  _accessibilityElements = nil;


Also, if the _accessibilityElements ivar is updated here, we may not even need a new _customAccessibilityElements ivar because setting a non-nil array here will cause the setter to return it (i.e it won't call [viewNode accessibilityElements].

raycsh017 · 2019-05-21T00:57:32Z

Source/Details/_ASDisplayViewAccessiblity.mm

  if (_accessibilityElements == nil || ASActivateExperimentalFeature(ASExperimentalDisableAccessibilityCache)) {
-    _accessibilityElements = [viewNode accessibilityElements];
+    return [viewNode accessibilityElements];


This might be the necessary evil (in that we don't cache it and return that next time) if we want to limit the accessibilityElements storage to one for either the custom or Texture-defined a11y elements (ie. items returned with current behavior) as opposed to two. It was hard to keep track of whether the ivar contains custom a11y elements or the default especially if you consider the case where you might be dynamically adding/removing subnodes and so you need to update the a11y elements returned for a node. Fwiw, I think another benefit to this method might be the accuracy of the list of items returned from accessibilityElements.

raycsh017 · 2019-05-22T01:04:01Z

Made changes to allow modification of accessibilityElements via accessibilityElementsBlock property on display nodes. Updated the test to reflect this as well!

nguyenhuy

Overall, LGTM. I'll approve after a fix in the block setter. Would be great if there is another approval from @maicki.

nguyenhuy · 2019-05-22T17:21:10Z

Source/ASDisplayNode.mm

+- (void)setAccessibilityElementsBlock:(ASDisplayNodeAccessibilityElementsBlock)accessibilityElementsBlock
+{
+  MutexLocker l(__instanceLock__);
+  _accessibilityElementsBlock = accessibilityElementsBlock;


You probbaly want to store a copy of the provided block.

nguyenhuy · 2019-05-22T17:29:35Z

Source/Details/_ASDisplayViewAccessiblity.mm

+
+  if (_accessibilityElementsBlock) {
+    NSArray *accessibilityElementsCopy = [accessibilityElements copy];
+    accessibilityElements = _accessibilityElementsBlock(accessibilityElementsCopy);


I'm wondering if it's ok to give the block a mutable array, so that the block may modify it directly and we don't need to copy it beforehand?

I'm not sure if it's a good API design?

That would work too, I just felt this might be safer, sort of forcing people to create their own mutable array and add the object as needed and return a copy of it. Do we have an example of supporting custom blocks like this? it would be nice to follow an existing pattern too if there is one.

Sometimes we want to override `accessibilityElements` so that we can skip over elements unnecessary for interacting with the app using VoiceOver or limit the interaction to child viewcontrollers presented, etc. For this purpose, this commit introduces `accessibilityElementsBlock` that receives the default `accessibilityElements` as input and modifies the elements returned. If not set, `accessibilityElements` should still return the elements found from the node hierarchy.

nguyenhuy · 2019-07-17T16:42:55Z

My impression is that this diff is blocked/replaced by #1525 (please correct me if I'm wrong, I've lost the context regarding this area and would need to spend some time to reload). With that, I'm gonna close this PR. Feel free to reopen it if need to. Thanks all.

raycsh017 force-pushed the support-accessibilityElements-v2 branch from 9d0ba2d to 22f016e Compare May 6, 2019 16:40

raycsh017 mentioned this pull request May 6, 2019

Support for custom accessibilityElements #1493

Open

nguyenhuy reviewed May 7, 2019

View reviewed changes

nguyenhuy requested changes May 20, 2019

View reviewed changes

raycsh017 force-pushed the support-accessibilityElements-v2 branch from ee67370 to f263f4b Compare May 21, 2019 00:29

raycsh017 commented May 21, 2019

View reviewed changes

raycsh017 force-pushed the support-accessibilityElements-v2 branch from f263f4b to 187108b Compare May 22, 2019 01:00

nguyenhuy reviewed May 22, 2019

View reviewed changes

raycsh017 added 2 commits May 28, 2019 13:25

Store a copy of the accessibilityElementsBlock passed

2303d1d

raycsh017 force-pushed the support-accessibilityElements-v2 branch from b8e1548 to 2303d1d Compare May 28, 2019 20:27

nguyenhuy mentioned this pull request May 29, 2019

Improve UIAccessiblityContainer / -accessibilityElements override support #1525

Closed

nguyenhuy closed this Jul 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support setting `accessibilityElements` #1488

Support setting `accessibilityElements` #1488

raycsh017 commented May 3, 2019

CLAassistant commented May 3, 2019 •

edited

Loading

nguyenhuy left a comment •

edited

Loading

nguyenhuy May 7, 2019 •

edited

Loading

raycsh017 May 9, 2019

nguyenhuy May 20, 2019

nguyenhuy May 20, 2019

raycsh017 commented May 9, 2019 •

edited

Loading

nguyenhuy May 20, 2019

nguyenhuy May 20, 2019

raycsh017 May 21, 2019

raycsh017 commented May 22, 2019

nguyenhuy left a comment

nguyenhuy May 22, 2019

nguyenhuy May 22, 2019

raycsh017 May 23, 2019

nguyenhuy commented Jul 17, 2019

Support setting accessibilityElements #1488

Support setting accessibilityElements #1488

Conversation

raycsh017 commented May 3, 2019

CLAassistant commented May 3, 2019 • edited Loading

nguyenhuy left a comment • edited Loading

Choose a reason for hiding this comment

nguyenhuy May 7, 2019 • edited Loading

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

raycsh017 commented May 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raycsh017 commented May 22, 2019

nguyenhuy left a 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

nguyenhuy commented Jul 17, 2019

Support setting `accessibilityElements` #1488

Support setting `accessibilityElements` #1488

CLAassistant commented May 3, 2019 •

edited

Loading

nguyenhuy left a comment •

edited

Loading

nguyenhuy May 7, 2019 •

edited

Loading

raycsh017 commented May 9, 2019 •

edited

Loading