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

Added correct accessibility roles for select actions #5070

Merged
merged 10 commits into from
Dec 5, 2020

Conversation

jwoo-msft
Copy link
Member

@jwoo-msft jwoo-msft commented Nov 11, 2020

Related Issue

ADO https://microsoft.visualstudio.com/OS/_queries/edit/29254830/?triage=true

Description

Selection Actions on Image doesn't announce their accessibility roles, the fix adds the correct roles.

Sample Card

https://github.com/microsoft/AdaptiveCards/blob/main/samples/v1.0/Elements/Image.SelectAction.json

How Verified

How you verified the fix, including one or all of the following:

  1. New unit tests that were added if any. The issue is reproducible with the sample card above.
Microsoft Reviewers: Open in CodeFlow

@ghost ghost added the no-recent-activity label Nov 16, 2020
@ghost
Copy link

ghost commented Nov 16, 2020

Hi @jwoo-msft. This non-spec pull request has had no recent activity for the past 5 days . Please take the necessary actions (review, address feedback or commit if reviewed already) to move this along.

Copy link
Member

@dclaux dclaux left a comment

Choose a reason for hiding this comment

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

How does this work with custom actions implemented by developers? Why is the action itself not providing its UI accessibility trait?

@ghost ghost removed the no-recent-activity label Nov 16, 2020
@ghost
Copy link

ghost commented Nov 16, 2020

Hi @dclaux; Thanks for reviewing this previously stale pull request. Resetting staleness. @jwoo-msft FYI.

@jwoo-msft
Copy link
Member Author

jwoo-msft commented Nov 16, 2020

How does this work with custom actions implemented by developers? Why is the action itself not providing its UI accessibility trait?

@dclaux,
If you are referring the action to the one in the code below,
if (action.type == ACRSubmit || action.type == ACRToggleVisibility) {
that would work for custom action as well. Since that will be part of the public interface, do you want me to bring this to the wider discussion?
My proposal will be adding a platform-specific accessibility role tag to Adaptive Actions elements such Action.OpenURL, Action.Submit, and etc.
recipientView.accessibilityTraits |= action.accessibilityRole;

@dclaux
Copy link
Member

dclaux commented Nov 16, 2020

@jwoo-msft not sure I understand. Take Action.Execute; its accessibility trait should be "button". But with your code, its accessibility trait will simply not be set at all, because it doesn't meet any of the conditions in your if...else statement. And even if it did meet the condition, how would you guarantee that a custom action always gets the appropriate trait, unless the custom action itself "says" what trait it should have?

In JS, the base Action class declares a getAriaRole method that custom action classes can override.

@jwoo-msft
Copy link
Member Author

@jwoo-msft not sure I understand. Take Action.Execute; its accessibility trait should be "button". But with your code, its accessibility trait will simply not be set at all, because it doesn't meet any of the conditions in your if...else statement. And even if it did meet the condition, how would you guarantee that a custom action always gets the appropriate trait, unless the custom action itself "says" what trait it should have?

In JS, the base Action class declares a getAriaRole method that custom action classes can override.

@dclaux,
Thanks, David.
I forgot to mention that I appreciate your suggestion because I wasn't sure what you were suggesting initially. I agree with you on the change, and also I think we should make analogous changes to all platforms for consistency, and I believe that the role should be left as platform specific, e.g getAriaRole for JS, getAccessibilityTrait for iOS.

@ghost ghost added the no-recent-activity label Nov 23, 2020
@ghost ghost assigned paulcam206 Nov 23, 2020
@ghost
Copy link

ghost commented Nov 23, 2020

Hi @jwoo-msft. This non-spec pull request has had no recent activity for the past 5 days . Please take the necessary actions (review, address feedback or commit if reviewed already) to move this along.

@ghost ghost removed the no-recent-activity label Dec 2, 2020
@ghost
Copy link

ghost commented Dec 2, 2020

Hi @dclaux; Thanks for reviewing this previously stale pull request. Resetting staleness. @jwoo-msft FYI.

@jwoo-msft jwoo-msft merged commit 375acc0 into main Dec 5, 2020
@jwoo-msft jwoo-msft deleted the jwoo/ios-accessibility-4 branch December 5, 2020 01:49
@shalinijoshi19 shalinijoshi19 added this to the 20.11-12 milestone Dec 11, 2020
@ghost ghost added the AdaptiveCards v20.12 label Dec 11, 2020
rankush pushed a commit to rankush/AdaptiveCards that referenced this pull request May 8, 2024
* Added correct accessibility role for select actions

* updated accessibility trait assignment including custom action

* added custom action target support to Inline Action

* updated test
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.

None yet

4 participants