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] Add .isAccessibilityContainer property, allowing automatic aggregation of children's a11y labels. #468

Merged
merged 3 commits into from
Aug 20, 2017

Conversation

appleguy
Copy link
Member

After consulting Apple documentation and working with some a11y experts,
we've found that aggregating objects that have a11y labels but are not
themselves interactable is significantly preferred for these users.

It makes it much quicker to navigate scrolling content if VoiceOver only
stops to select entire cells, and then allows drilling down into the cell
to select individual components. This implementation achieves that behavior.

We should consider enabling isAccessibilityContainer by default on ASCellNode.
This would be an improvement for 95% of a11y use cases. Aggregation can be
enabled or disabled on any node.

…matic aggregation of children's a11y labels.

After consulting Apple documentation and working with some a11y experts,
we've found that aggregating objects that have a11y labels but are not
themselves interactable is significantly preferred for these users.

It makes it much quicker to navigate scrolling content if VoiceOver only
stops to select entire cells, and then allows drilling down into the cell
to select individual components. This implementation achieves that behavior.

We should consider enabling isAccessibilityContainer by default on ASCellNode.
This would be an improvement for 95% of a11y use cases. Aggregation can be
enabled or disabled on any node.
@appleguy appleguy self-assigned this Jul 23, 2017
Copy link
Member

@Adlai-Holler Adlai-Holler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

* an aggregation of all child nodes' accessibility labels. Nodes in this node's subtree that are also accessibility containers will
* not be included in this aggregation, and will be exposed as separate accessibility elements to UIKit.
*/
@property (nonatomic, assign) BOOL isAccessibilityContainer;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Just acknowledging that while this is not Objective-C convention (having is in the property name), this follows the UIAccessibility convention so good call.

  2. Should we use objective-C atomics here? Not a big deal either way.

NSMutableArray<ASAccessibilityElement *> *labeledNodes = [NSMutableArray array];
NSMutableArray<ASAccessibilityCustomAction *> *actions = [NSMutableArray array];
std::queue<ASDisplayNode *> queue;
queue.push(container);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somebody should build an NSFastEnumeration-conforming class that takes a BFS/DFS option, a collection, and a children block. Thinking out loud.

@appleguy
Copy link
Member Author

Great idea on the enumerator utility!

We probably should set a standard for BOOLs. I worry it might miss some transactional atomicity, although in this case it is probably fine.

Copy link
Contributor

@maicki maicki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

I think we are in a kind of interesting spot with having properties atomic and some gated via the instance lock. IMHO I agree with @appleguy we should set a standard going forward and add that to our coding conventions.

@ay8s
Copy link
Collaborator

ay8s commented Aug 18, 2017

👍 Thanks for this one @appleguy. We're focussing a bunch on accessibility and have tweaks to our cell nodes but containers would make much more sense than the current route we've taken.

@appleguy appleguy merged commit 5cf05f3 into master Aug 20, 2017
@appleguy appleguy deleted the AccessibilityContainer branch August 20, 2017 20:17
bernieperez pushed a commit to AtomTickets/Texture that referenced this pull request Apr 25, 2018
…matic aggregation of children's a11y labels. (TextureGroup#468)

After consulting Apple documentation and working with some a11y experts,
we've found that aggregating objects that have a11y labels but are not
themselves interactable is significantly preferred for these users.

It makes it much quicker to navigate scrolling content if VoiceOver only
stops to select entire cells, and then allows drilling down into the cell
to select individual components. This implementation achieves that behavior.

We should consider enabling isAccessibilityContainer by default on ASCellNode.
This would be an improvement for 95% of a11y use cases. Aggregation can be
enabled or disabled on any node.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants