[ASDisplayViewAccessibility] A few accessibility improvements#1812
Merged
rcancro merged 9 commits intoTextureGroup:masterfrom May 6, 2020
Merged
[ASDisplayViewAccessibility] A few accessibility improvements#1812rcancro merged 9 commits intoTextureGroup:masterfrom
rcancro merged 9 commits intoTextureGroup:masterfrom
Conversation
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.
jparise
reviewed
May 5, 2020
rcancro
commented
May 5, 2020
jparise
approved these changes
May 5, 2020
Changed name of `defaultComparator` to `defaultAccessibilityComparator` For some reason a accessibility test keeps failing on github, but runs fine locally. Changed the tests to make sure the array contains the proper items but not enforce order.
This was the last error: `fatal: unable to access 'https://github.com/pinterest/PINCache.git/': Could not resolve host: github.com`
nguyenhuy
reviewed
May 6, 2020
Member
There was a problem hiding this comment.
Please also update ASConfigurationTests.mm and configuration.json with your experiment
nguyenhuy
approved these changes
May 6, 2020
Member
nguyenhuy
left a comment
nguyenhuy
left a comment
There was a problem hiding this comment.
Nits aside, LGTM. Feel free to land after addressing them, and thank you for continue improving a11y support!
* early exit if our view has no window when creating accessibility elements (This change also requiring making sure nodes in accessibility tests were in a window) * fixed test around experiments.
piotrdebosz
pushed a commit
to getstoryteller/Texture
that referenced
this pull request
Mar 1, 2021
…eGroup#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.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This diff includes a few improvements for accessibility in Texture.
_ASDisplayView. If we cache elements, we will require users to clear the cache (by callingsetAccessibilityElementsto 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:
ASAccessibilityElementPositioningprotocol in favor of passingNSObjectsto the sort comparator.NSObjectimplements the informalUIAccessibilityprotocol and therefore has anaccessibilityFrameproperty.staticfrom theSortAccessibilityElements()method definition. This allows me to declare it asexternin test it via unit tests.