[ASDisplayNode] Implement accessibilityViewIsModal #1858
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A PR to add support for `accessibilityViewIsModal` in `CollectAccessibilityElements`. If in a list of subnodes more than 1 subnode has `accessibilityViewIsModal` marked as `YES`, then the node with the highest index in `subnodes` will be the one that is considered modal. This behavior matches UIKit. If the value of `accessibilityViewIsModal` changes, we need to clear all the cached `accessibilityElements` from that view up. I added this in ASDisplayNode’s `setAccessibilityViewIsModal` method. Note that if we ship `ASExperimentalDoNotCacheAccessibilityElements` then we can remove the invalidation step. Finally, I changed all the tests to ask the view for accessibilityElements, not the node. This is a better representation of what will really happen when UIKit asks a node’s view for its accessibility elements. It also allowed me to test that clearing the accessibilityElements was working.
jparise
approved these changes
May 28, 2020
rcancro
commented
May 29, 2020
| @@ -280,7 +294,7 @@ static void CollectAccessibilityElements(ASDisplayNode *node, NSMutableArray *el | |||
| } else if (subnode.isLayerBacked) { | |||
| // Go down the hierarchy of the layer backed subnode and collect all of the UIAccessibilityElement | |||
| CollectUIAccessibilityElementsForNode(subnode, node, view, elements); | |||
| } else if (subnode.accessibilityElementCount > 0) { | |||
| } else if (subnode.accessibilityElements.count > 0) { | |||
There was a problem hiding this comment.
Does anyone have any thoughts around this change? When I made an example app with accessibilityViewIsModal set to YES accessibilityElementCount was 1 when I turned on voice over. However, in the tests it was always 0.
I'm not really sure where/how accessibilityElementCount is being changed in my test app. It isn't a property on NSObject, but a method that should be overwritten (which we aren't doing). This comment also exists in the header of _ASDisplayViewAccessiblity.h, so I'm not sure why we are favoring accessibilityElementCount over accessibilityElements:
// WARNING: When dealing with accessibility elements, please use the `accessibilityElements`
// property instead of the older methods e.g. `accessibilityElementCount()`. While the older methods
// should still work as long as accessibility is enabled, this framework provides no guarantees on
// their correctness.
There was a problem hiding this comment.
as far as I understand we should rely on accessibilityElementCount, but
according to documentation it returns 0 by default. May be default implementation?
- (NSInteger)accessibilityElementCount {
return [[self accessibilityElements] count];
}
There was a problem hiding this comment.
I just played with this some more, and it doesn't work. I'll focus on a way to make the tests pass and revert this change.
piotrdebosz
pushed a commit
to getstoryteller/Texture
that referenced
this pull request
Mar 1, 2021
* [ASDisplayNode] Implement accessibilityViewIsModal A PR to add support for `accessibilityViewIsModal` in `CollectAccessibilityElements`. If in a list of subnodes more than 1 subnode has `accessibilityViewIsModal` marked as `YES`, then the node with the highest index in `subnodes` will be the one that is considered modal. This behavior matches UIKit. If the value of `accessibilityViewIsModal` changes, we need to clear all the cached `accessibilityElements` from that view up. I added this in ASDisplayNode’s `setAccessibilityViewIsModal` method. Note that if we ship `ASExperimentalDoNotCacheAccessibilityElements` then we can remove the invalidation step. Finally, I changed all the tests to ask the view for accessibilityElements, not the node. This is a better representation of what will really happen when UIKit asks a node’s view for its accessibility elements. It also allowed me to test that clearing the accessibilityElements was working. * add some experiment checks * fix tests and address jon’s comment * Fix tests * remove debug code
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A PR to add support for
accessibilityViewIsModalinCollectAccessibilityElements.If in a list of subnodes more than 1 subnode has
accessibilityViewIsModalmarked asYES, then the node with the highest index insubnodeswill be the one that is considered modal. This behavior matches UIKit.If the value of
accessibilityViewIsModalchanges, we need to clear all the cachedaccessibilityElementsfrom that view up. I added this in ASDisplayNode’ssetAccessibilityViewIsModalmethod. Note that if we shipASExperimentalDoNotCacheAccessibilityElementsthen we can remove the invalidation step.Finally, I changed all the tests to ask the view for accessibilityElements, not the node. This is a better representation of what will really happen when UIKit asks a node’s view for its accessibility elements. It also allowed me to test that clearing the accessibilityElements was working.