Skip to content

Conversation

@mwcampbell
Copy link
Contributor

I think this is a good thing to add before implementing the NSAccessibility text methods, which an AT shouldn't try to call on nodes that don't support text ranges. Since I've chosen to have a single Objective-C class for all types of AccessKit nodes, it's useful to let ATs know which methods are and are not supported for a particular node.

I know this method is being used, because before I added accessibilityRoleDescription to the list of selectors that are allowed by default, VoiceOver didn't speak some role descriptions, like "slider", "stepper", and "button". Maybe we'll eventually discover more selectors that need to be on the default allow list.

Copy link
Member

@DataTriny DataTriny left a comment

Choose a reason for hiding this comment

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

I'll trust you on this one.

@rain-sk
Copy link

rain-sk commented Dec 11, 2022

Have you checked if this works in practice?

In my experience, supporting isAccessibilitySelectorAllowed:@selector(accessibilityRoleDescription) did not actually work as desired. It seemed to cause problems with how VoiceOver read the roles of elements in an accessibility tree.

The only way I've found to properly support custom role descriptions is to not implement accessibilityRoleDescription and instead use setAccessibilityRoleDescription on the NSAccessibilityElement* if a node actually has a custom role description.

@rain-sk
Copy link

rain-sk commented Dec 11, 2022

In general, I've found VoiceOver's use of isAccessibilitySelectorAllowed to be inconsistent, and not reliable. It's why the direction I took was to define separate sub-interfaces of NSAccessibilityElement<NSAccessibilityElement>.

@mwcampbell
Copy link
Contributor Author

I found that overriding accessibilityRoleDescription is working fine so far. I'll be submitting support for custom role descriptions as a separate PR. The trick, I think, is to call the method on the superclass if we don't have our own role description.

Still, I guess I should wait and merge this PR only once I've confirmed that this approach works for the text-related selectors.

@mwcampbell mwcampbell force-pushed the macos-expose-allowed-selectors branch from b38b319 to 85b7e3e Compare December 12, 2022 15:20
@mwcampbell
Copy link
Contributor Author

So far, my current approach, having a single node class and implementing isAccessibilitySelectorAllowed:, is working well, even after introducing text editing, so I'll merge this PR now.

@mwcampbell mwcampbell merged commit c4cbb23 into main Dec 14, 2022
@mwcampbell mwcampbell deleted the macos-expose-allowed-selectors branch December 14, 2022 13:34
@github-actions github-actions bot mentioned this pull request Dec 14, 2022
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