-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix accessibility issues in Adaptive Cards library #4335
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
compulim
requested review from
a-b-r-o-w-n,
cwhitten,
srinaath,
tdurnford,
tonyanziano and
beyackle
as code owners
July 6, 2022 04:48
compulim
changed the title
Fix accessibility in Adaptive Cards library
Fix accessibility issues in Adaptive Cards library
Jul 6, 2022
tdurnford
reviewed
Jul 18, 2022
...es/bundle/src/adaptiveCards/Attachment/AdaptiveCardHacks/private/useAdaptiveCardModEffect.ts
Show resolved
Hide resolved
tdurnford
reviewed
Jul 18, 2022
packages/bundle/src/adaptiveCards/Attachment/AdaptiveCardHacks/private/useValueRef.ts
Show resolved
Hide resolved
tdurnford
approved these changes
Jul 18, 2022
packages/bundle/src/adaptiveCards/Attachment/AdaptiveCardHacks/private/closest.ts
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changelog Entry
Fixed
role="button"
instead ofrole="menubar"
/role="menuitem"
, by @compulim, in PR #4263Description
ActionSet
/Action
in AC is currentlymenubar
/menuitem
respectively. For accessibility reasons, actions should bebutton
. This will help end-users who use screen reader to reach the action button via B/F keys.We should also consider updating the instruction text for indicating how to navigate. Currently, it is "Message is interactive. Press SHIFT TAB two to three times to go to the chat history. Then select the message".
As
ActionSet
/Action
are now buttons (form fields), we might be able to simplify it to "Message contains form fields."This pull request will also refactor existing hacks into a more consumable/pluggable pattern.
Design
We have isolated our DOM hacks to Adaptive Cards in a separate folder:
ActionSet
should not berole="menubar"
Action
should setaria-pushed
attributeWe added a new internal
useAdaptiveCardModEffect()
hook to apply/undo DOM hacks. These hacks, need to undo right before we re-render, and right after we mount it on the DOM:Action.ShowCard
Other designs
After Adaptive Card is rendered as an unmounted DOM tree, it could be walked using standard DOM traversal functions. When walking the DOM tree, we could programmatically turn it into React.
This approach leverage DOM reconciliation in React, which helps re-render.
However, for other hacks, we will need to modify the walking mechanism and apply the hack while walking the tree. This could be very difficult to add mods that requires traversing multiple nodes. The mod will be more-or-less similar to translating a DOM tree based purely on SAX interface, which could be very difficult to get done.
Thus, we are keeping our hacks as DOM-based, instead of React-based.
Specific Changes
adaptiveCards.js/disable card inputs
CHANGELOG.md
Review Checklist
CSS styles reviewed (minimal rules, noz-index
)Documents reviewed (docs, samples, live demo)Internationalization reviewed (strings, unit formatting)package.json
andpackage-lock.json
reviewedSecurity reviewed (no data URIs, check for nonce leak)