Improve UIAccessiblityContainer / -accessibilityElements override support #1525

maicki · 2019-05-28T19:57:53Z

Add provide a way for ASDisplayNodes to overwrite UIAccessibilityContainer subclass methods
For ASDisplayNodes that acting like a container we provide a default implementation and handling for subnodes
We add a new experimental flag: ASExperimentalExposeTextLinksForA11Y which is by default NO
Provides concrete implementation of the UIAccessibitliyContainer methods within ASTextNode2 which based on the ASExperimentalExposeTextLinksForA11Y property will return the text as well as all links as accessibility elements

Source/ASTableView.mm

Source/ASTextNode+Beta.h

Source/Details/_ASDisplayView.mm

Source/Private/ASDisplayNode+UIViewBridge.mm

Source/Details/_ASDisplayViewAccessiblity.mm

ghost · 2019-05-29T18:03:38Z

🚫 CI failed with log

nguyenhuy · 2019-05-29T18:53:34Z

I'll do a thorough review later, but here are some thoughts on the ability to override accessibilityElements which is also what #1488 tries to achieve:

I think it's a great idea to let -[ASDisplayNode accessibilityElements] be a point of customization. That is, subclasses can override the method to provide custom elements. Another thing we need to do is make it publicly available. Right now it's declared in ASDisplayNode+FrameworkPrivate.h.
We need to either expose _accessibilityElements ivar also (it's in ASDisplayNodeInternal.h now), or make it clear in the documentation of -[ASDisplayNode accessibilityElements] that clients are responsible for caching the elements themselves, if need to.
I think there is still a valid use case for the framework to allow customization via an accessibilityElementsBlock. For example an app may want to override custom elements of the root node of an ASViewController without having to subclass the node. I think we have existing cases in the framework in which we allow both mechanisms for customization. layoutSpecBlock and -layoutSpecThatFits: is a good example. If we all agree on this, we can coordinate on adding the block after this diff landed. Thoughts? Also, cc @raycsh017.

maicki · 2019-05-29T20:15:47Z

@nguyenhuy Thanks for your thoughts:

I don't think we should expose _accessibilityElements instead of making it clear in the documentation that clients have to do their own caching. This aligns with the behavior and documentation UIKit is stating for overwriting the UIAccessibilityContainer methods.
I don't think we should provide something like accessibilityElementsBlock though. One reason I would like to avoid is that it would clutter up the API (layoutSpecBlock in theory is great if we would go with composition only, but has it's downsides like subclass support and Texture is still based a lot on subclassing). Imho for now, we should add the most frictionless API based on subclassing, based on the UIKit implementation and documentation.

raycsh017 · 2019-05-29T20:58:58Z

Hmm I don't think exposing accessibilityElements is too out of line with sticking with the UIKit documentation, since I believe UIKit supports assigning a value to accessibilityElements for customization (would be nil if not set).

For accessibilityElementsBlock, we don't have to add that API, I think the point is really to create a way for people to override accessibilityElements without having to subclass. One main use case I see is overriding the accessibilityElements of the main node or view on a view controller. While not impossible, it would be pretty annoying to create a subclass to reroute the accessibilityElements to focus on other elements (e.g. child view controller's elements).

Ignore this bit if you already considered this, but I've noticed that we invalidate accessibilityElements whenever we add a subnode or a subview. We should consider what might be the expected behavior here? I encountered this when I was finding a way to override accessibilityElements myself, thought it might be useful to bringing it up here too :)

One last thing, I am pretty new to Texture, so let me know if there's anything I might've overlooked!

maicki · 2019-05-29T21:14:22Z

@raycsh017

I meant it's not about exposing accessibilityElements it's about exposing _accessibilityElements (the instance variable). accessibilityElements will be exposed.

It's the same with UIKit therefore I would like to not add a new API at least within this PR. To be able to overwrite you have to subclass within UIKit. We could pick that accessibilityElementsBlock in a follow up PR. This PR has the goal to add a new foundational API.

I think we should keep that behavior to invalidate the accessibilityElements within addSubview: / removeSubview: for now. Did you run into any problems we may should consider in this case? Also very open for any improvement suggestions :)

ghost · 2019-05-30T04:06:48Z

🚫 CI failed with log

ghost · 2019-05-30T19:54:55Z

🚫 CI failed with log

Adlai-Holler

Good deal.

Source/ASTextNode2.mm

Adlai-Holler · 2019-05-30T21:07:52Z

Tests/ASTextNode2Tests.mm

+                [_textNode.accessibilityElements[0] accessibilityLabel], _textNode.accessibilityLabel);
+  XCTAssertTrue([[_textNode.accessibilityElements[0] accessibilityLabel] isEqualToString:_attributedText.string],
+                @"First accessibility element incorrectly returns \n%@\n when it should be \n%@\n",
+                [_textNode.accessibilityElements[0] accessibilityLabel], _textNode.accessibilityLabel);


Use XCTAssertEqual and XCTAssertEqualObjects so we get usable errors, throughout this PR.

Adlai-Holler · 2019-06-03T16:03:06Z

Source/ASTextNode2.mm

+- (NSInteger)accessibilityElementCount
+{
+  if (
+      !ASActivateExperimentalFeature(ASExperimentalTextNode2A11YContainer)) {


Source/ASTextNode2.mm

ghost · 2019-06-03T18:45:46Z

🚫 CI failed with log

raycsh017 · 2019-06-04T00:36:16Z

Ah, we are not making accessibilityElements assignable, are we? I think I mistook exposing accessibilityElements as making it assignable.

What I was trying to get at was, I was prototyping making accessibilityElements assignable, and found out that the variable gets reset whenever you add a node/view, which wasn't very intuitive and took some time to figure that out.

If that's not the case, never mind. Putting accessibilityElementsBlock in a separate PR sounds good 👍

ghost · 2019-07-17T16:43:54Z

	1 Warning
⚠️	This is a big PR, please consider splitting it up to ease code review.

Generated by 🚫 Danger

ghost · 2019-07-17T16:45:34Z

🚫 CI failed with log

nguyenhuy · 2019-07-17T17:08:22Z

@maicki Can you rebase with master to fix the CI?

maicki · 2019-07-17T17:10:35Z

The current state is quit old. Closing for now.

maicki requested review from Adlai-Holler, nguyenhuy and appleguy May 28, 2019 19:57

appleguy changed the title ~~Improve UIAccessiblityContainer overwrites support~~ Improve UIAccessiblityContainer / -accessibilityElements override support May 28, 2019

appleguy reviewed May 28, 2019

View reviewed changes

Source/ASTableView.mm Outdated Show resolved Hide resolved

appleguy reviewed May 28, 2019

View reviewed changes

Source/ASTextNode+Beta.h Outdated Show resolved Hide resolved

appleguy reviewed May 28, 2019

View reviewed changes

Source/Details/_ASDisplayView.mm Outdated Show resolved Hide resolved

appleguy reviewed May 28, 2019

View reviewed changes

Source/Private/ASDisplayNode+UIViewBridge.mm Outdated Show resolved Hide resolved

Adlai-Holler reviewed May 28, 2019

View reviewed changes

Source/Details/_ASDisplayViewAccessiblity.mm Outdated Show resolved Hide resolved

maicki force-pushed the MSImproveUIAccessibilityContainerOverwrites branch 3 times, most recently from 5faa3f5 to 6579d5b Compare May 29, 2019 17:31

TextureGroup deleted a comment May 29, 2019

maicki force-pushed the MSImproveUIAccessibilityContainerOverwrites branch from 75d911a to 18928c9 Compare May 29, 2019 20:18

maicki changed the title ~~Improve UIAccessiblityContainer / -accessibilityElements override support~~ [WIP] Improve UIAccessiblityContainer / -accessibilityElements override support May 29, 2019

maicki force-pushed the MSImproveUIAccessibilityContainerOverwrites branch 2 times, most recently from 0f5be75 to d40e069 Compare May 30, 2019 16:52

maicki changed the title ~~[WIP] Improve UIAccessiblityContainer / -accessibilityElements override support~~ Improve UIAccessiblityContainer / -accessibilityElements override support May 30, 2019

maicki force-pushed the MSImproveUIAccessibilityContainerOverwrites branch from 6b338e7 to d5753bf Compare May 30, 2019 20:27

Adlai-Holler reviewed May 30, 2019

View reviewed changes

maicki force-pushed the MSImproveUIAccessibilityContainerOverwrites branch from 5d68d97 to a9aa0e1 Compare May 30, 2019 21:11

maicki force-pushed the MSImproveUIAccessibilityContainerOverwrites branch from a9aa0e1 to f644525 Compare May 30, 2019 21:12

Revamp Accessibility API for more flexibility

0a57392

maicki force-pushed the MSImproveUIAccessibilityContainerOverwrites branch from 5c93838 to 0a57392 Compare May 31, 2019 11:54

maicki added 2 commits May 31, 2019 10:27

Fix ASTextNode2 accessibilityElementCount

9f6394f

Small improvements

6e36749

maicki force-pushed the MSImproveUIAccessibilityContainerOverwrites branch from 61bb804 to 6e36749 Compare May 31, 2019 17:49

maicki and others added 4 commits May 31, 2019 14:14

Copy ore a11y attributes over

481cb47

More tests

a3e508a

Try something different

adb8423

Merge branch 'master' into MSImproveUIAccessibilityContainerOverwrites

d610dc2

Adlai-Holler approved these changes Jun 3, 2019

View reviewed changes

nguyenhuy mentioned this pull request Jul 17, 2019

Support setting accessibilityElements #1488

Closed

maicki closed this Jul 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve UIAccessiblityContainer / -accessibilityElements override support #1525

Improve UIAccessiblityContainer / -accessibilityElements override support #1525

maicki commented May 28, 2019 •

edited

Loading

ghost commented May 29, 2019

nguyenhuy commented May 29, 2019 •

edited

Loading

maicki commented May 29, 2019 •

edited

Loading

raycsh017 commented May 29, 2019 •

edited

Loading

maicki commented May 29, 2019 •

edited

Loading

ghost commented May 30, 2019

ghost commented May 30, 2019

Adlai-Holler left a comment

Adlai-Holler May 30, 2019

Adlai-Holler Jun 3, 2019

ghost commented Jun 3, 2019

raycsh017 commented Jun 4, 2019

ghost commented Jul 17, 2019

ghost commented Jul 17, 2019

nguyenhuy commented Jul 17, 2019

maicki commented Jul 17, 2019

Improve UIAccessiblityContainer / -accessibilityElements override support #1525

Improve UIAccessiblityContainer / -accessibilityElements override support #1525

Conversation

maicki commented May 28, 2019 • edited Loading

ghost commented May 29, 2019

nguyenhuy commented May 29, 2019 • edited Loading

maicki commented May 29, 2019 • edited Loading

raycsh017 commented May 29, 2019 • edited Loading

maicki commented May 29, 2019 • edited Loading

ghost commented May 30, 2019

ghost commented May 30, 2019

Adlai-Holler left a comment

Choose a reason for hiding this comment

Adlai-Holler May 30, 2019

Choose a reason for hiding this comment

Adlai-Holler Jun 3, 2019

Choose a reason for hiding this comment

ghost commented Jun 3, 2019

raycsh017 commented Jun 4, 2019

ghost commented Jul 17, 2019

ghost commented Jul 17, 2019

nguyenhuy commented Jul 17, 2019

maicki commented Jul 17, 2019

maicki commented May 28, 2019 •

edited

Loading

nguyenhuy commented May 29, 2019 •

edited

Loading

maicki commented May 29, 2019 •

edited

Loading

raycsh017 commented May 29, 2019 •

edited

Loading

maicki commented May 29, 2019 •

edited

Loading