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

Make Suggested Actions accessible again! #2613

Merged
merged 14 commits into from
Nov 20, 2019

Conversation

corinagum
Copy link
Contributor

@corinagum corinagum commented Nov 19, 2019

(jk they were never accessible)

Fixes #1780
Fixes #2277
Fixes #2285

Changelog Entry

  • Fixes #1780, #2277, and #2285. Make Suggested Actions accessible, Fix Markdown card in carousel being read multiple times, and label widgets of Connectivity Status and Suggested Actions containers, by @corinagum in PR #2613

Note: How to test Suggested Actions container on Mock Bot

  • use command emptycard or
  • use command suggested-actions

Description

  • Make Suggested Actions container accessible. See Browser Discrepancies below.
  • Fix Markdown card in carousel being read multiple times - previously I recorded this as a nesting issue but was actually a problem with the attachment types inside. This has been resolved with aria-live=" " and aria-hidden={true} inside the Carousel component
  • Connectivity Status: AT will now read "Connectivity Status: Connected/Unable to connect/etc" when it reaches the (Connectivity Status container) - in the UI this component only shows when not connected. Changes in connectivity status will also be announced by the AT, since this is a status widget.
  • Suggested Actions container: This component is hidden until buttons are added inside. At such time, the AT will announce "Suggested Actions container: has content" regardless of where the user is focused on the app, due to this component being a status widget. When the Suggested Actions container is empty, AT will announce "Suggested Actions container: is empty"
  • Japanese localization file updated to latest
  • Please note, "Upload file text" in localization files has been modified. See en-us for changes.

Browser discrepancies

Previously, Suggested Actions were ignored by the AT. This PR fixes that, HOWEVER, there are some idiosyncrasies across browsers that are documented below:

  1. On Chrome & NVDA: When the Suggested Actions container changes, it will read "Suggested actions container: has content. Blue Red Green" twice. I've gone over and over the code with @compulim as my sanity check and I'm confident that this is caused by a Chrome error, not Web Chat's code.
  2. Firefox & NVDA: Throughout the app, the the AT will randomly read extra letters that are not in the aria-label or visual on the UI. For example, it will say "Suggested Actions: has content. ED".
  3. Edge & Narrator: AT will read the change in Suggested Actions container "Suggested Actions: has content. Contains three items", but it will NOT read the buttons inside without navigating to that container. Other browsers will say "Suggestesd Actions container: has content. Blue red Green".
  4. Safari & VoiceOver: AT will read the Connectivity Status and SuggestedActions labels/containers multiple times on start up.

  • Testing Added

@corinagum corinagum marked this pull request as ready for review November 20, 2019 21:27
@coveralls
Copy link

coveralls commented Nov 20, 2019

Coverage Status

Coverage increased (+0.2%) to 64.01% when pulling 1388c4f on corinagum:1780-suggestedActionsA11y into 39ab54e on microsoft:master.

@corinagum corinagum changed the title 1780 suggested actions a11y Make Suggested Actions Accessible again! Nov 20, 2019
@corinagum corinagum changed the title Make Suggested Actions Accessible again! Make Suggested Actions accessible again! Nov 20, 2019
Copy link
Contributor

@compulim compulim left a comment

Choose a reason for hiding this comment

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

Automatic approve if everything commented is fixed.

packages/component/src/Activity/StackedLayout.js Outdated Show resolved Hide resolved
packages/component/src/Activity/StackedLayout.js Outdated Show resolved Hide resolved
packages/component/src/Activity/StackedLayout.js Outdated Show resolved Hide resolved
packages/component/src/Activity/CarouselFilmStrip.js Outdated Show resolved Hide resolved
packages/component/src/Attachment/TextContent.js Outdated Show resolved Hide resolved
packages/component/src/SendBox/SuggestedActions.js Outdated Show resolved Hide resolved
packages/component/src/SendBox/SuggestedActions.js Outdated Show resolved Hide resolved
packages/component/src/Utils/AbsoluteTime.js Outdated Show resolved Hide resolved
packages/component/src/Utils/AbsoluteTime.js Outdated Show resolved Hide resolved
packages/core/src/reducers/suggestedActions.js Outdated Show resolved Hide resolved
corinagum and others added 4 commits November 20, 2019 14:13
Co-Authored-By: William Wong <compulim@users.noreply.github.com>
Co-Authored-By: William Wong <compulim@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants