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

Remove role="menubar/menuitem" from the container and siblings of selected card actions #3950

Merged
merged 9 commits into from
Jun 29, 2021

Conversation

compulim
Copy link
Contributor

@compulim compulim commented Jun 17, 2021

Fixes #3947.

Changelog Entry

Fixed

  • Fixes #3947. Adaptive Cards: all action sets (which has role="menubar") must have at least 1 or more role="menuitem", by @compulim, in PR #3950

Description

After clicked/performed a card action, for accessibility reasons, we did some manipulations:

  • Add aria-pressed="true" to indicate the button selected
  • Change to role="button" from role="menuitem", to fulfill the requirement for aria-pressed

Visually, it means:

<div class="ac-actionSet" role="menubar">
  <button class="ac-pushButton" role="menuitem">Clicked</button>
  <button class="ac-pushButton" role="menuitem">Not clicked</button>
</div>

Would become:

<div class="ac-actionSet" role="menubar">
  <button class="ac-pushButton" role="button" aria-pressed="true">Clicked</button>
  <button class="ac-pushButton" role="menuitem">Not clicked</button>
</div>

When the container only has 1 action, unfortunately, the container will be left with no descendants of role="menuitem", because we changed it to role="button".

Design

In this fix, we are going to:

  • After the user clicked on an action button
    1. Remove role="menubar" from the container;
    2. Remove role attribute from all non-selected card action, which was role="menuitem".

Visually, it means:

<div class="ac-actionSet" role="menubar">
  <button class="ac-pushButton" role="menuitem">Clicked</button>
  <button class="ac-pushButton" role="menuitem">Not clicked</button>
</div>

Would become:

<div class="ac-actionSet">
  <button class="ac-pushButton" role="button" aria-pressed="true">Clicked</button>
  <button class="ac-pushButton">Not clicked</button>
</div>

Since a single Adaptive Card could contains multiple ActionSet containers and one set of card actions, we are only touching those containers which selection has been made. And leave others untouched.

When we are working on this fix, we also discovered adaptivecards@2.9.0 is generating a DOM tree for "card action" which looks like:

<div class="ac-actionSet" role="menubar">
  <button class="ac-pushButton" role="button">...</button>
  <button class="ac-pushButton" role="button">...</button>
</div>

(This bug only applies to "card actions", but not "action set" containers.)

Note there are no menuitem roles under the menubar role. This is violating the WCAG rule and is filed as #3949.

Before waiting for a fix from Adaptive Cards team, we are hacking the DOM to change buttons' role to menuitem.

We believe this change is reasonable because:

  1. Adaptive Cards 2.9.0 applies role="menuitem" to elements inside ActionSet
    • ActionSet container is a special container for action buttons, multiple sets are allowed, can be put anywhere in the card including front
  2. Adaptive Cards 2.9.0 applies role="button" to elements inside card actions. But 2.5.0 was applying role="menuitem".
    • "Card actions" are action buttons for the whole card, analogy to "card submission", only one set is allowed, and must appear at the bottom of the card

Specific Changes

  • Updated AdaptiveCardRenderer
    • Refactored the original manipulation logic out
    • Added logics to:
      1. Remove role="menubar" from the container
      2. Remove role attribute from all sibling actions that are not selected
    • Added fixAccessibilityIssuesWithUndo:
      • After the card is rendered, make sure all button roles under menubar roles will become menuitem roles instead
  • I have added tests and executed them locally
  • I have updated CHANGELOG.md
  • I have updated documentation

Review Checklist

This section is for contributors to review your work.

  • Accessibility reviewed (tab order, content readability, alt text, color contrast)
  • Browser and platform compatibilities reviewed
  • CSS styles reviewed (minimal rules, no z-index)
  • Documents reviewed (docs, samples, live demo)
  • Internationalization reviewed (strings, unit formatting)
  • package.json and package-lock.json reviewed
  • Security reviewed (no data URIs, check for nonce leak)
  • Tests reviewed (coverage, legitimacy)

@compulim compulim merged commit ef147be into microsoft:main Jun 29, 2021
@compulim compulim deleted the fix-ac-broken-menubar branch June 29, 2021 20:04
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.

Adaptive Cards: After submitting, all [role="menubar"] must have 1 or more descendant of [role="menuitem"]
2 participants