-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Set default descendant when focusing on transcript and add keyboard focus indicators #4035
Merged
compulim
merged 47 commits into
microsoft:main
from
compulim:fix-4018-default-active-descendant
Sep 22, 2021
Merged
Set default descendant when focusing on transcript and add keyboard focus indicators #4035
compulim
merged 47 commits into
microsoft:main
from
compulim:fix-4018-default-active-descendant
Sep 22, 2021
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,
beyackle,
cwhitten,
srinaath,
tdurnford and
tonyanziano
as code owners
August 27, 2021 05:12
compulim
force-pushed
the
fix-4018-default-active-descendant
branch
from
August 27, 2021 05:13
6f94462
to
099f2f9
Compare
compulim
force-pushed
the
fix-4018-default-active-descendant
branch
2 times, most recently
from
September 9, 2021 22:04
5ba3f9a
to
b37d623
Compare
compulim
changed the title
Set default descendant when focusing on transcript
Set default descendant when focusing on transcript and add keyboard focus indicators
Sep 17, 2021
cwhitten
approved these changes
Sep 20, 2021
compulim
force-pushed
the
fix-4018-default-active-descendant
branch
from
September 22, 2021 17:56
e573d7f
to
d003ce2
Compare
This was referenced Sep 27, 2021
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
Breaking changes
suggestedActionBackground
andsuggestedActionXXXBackground
are being deprecated in favor ofsuggestedActionBackgroundColor
andsuggestedActionBackgroundColorOnXXX
respectively, for consistencies when porting to other platformssuggestedActionDisabledXXX
is being renamed tosuggestedActionXXXOnDisabled
, for consistencies with other style optionssuggestedActionXXXOnActive
,suggestedActionXXXOnFocus
,suggestedActionXXXOnHover
are introduced for styling per user gesturessuggestedActionKeyboardFocusIndicatorXXX
are introduced for styling the "focus ring" when focused using a keyboardFixed
suggestedActionXXXOnActive
,suggestedActionXXXOnFocus
,suggestedActionXXXOnHover
,suggestedActionKeyboardFocusIndicatorXXX
suggestedActionDisabledXXX
becomesuggestedActionXXXOnDisabled
sendBoxButtonXXXOnActive
,sendBoxButtonXXXOnFocus
,sendBoxButtonXXXOnHover
,sendBoxButtonKeyboardFocusIndicatorXXX
Description
Resolving inconsistencies on TAB and SHIFT + TAB
Unless the transcript is empty, there should always be one activity focused (a.k.a. one active descendant).
When using SHIFT + TAB to focus from the send box to the transcript, if no activities were selected (a.k.a. no active descendant), we will select the last activity as the active descendant.
When using TAB to focus from top of the page to the transcript, if no active descendant were set, we should select the last activity as well.
This PR is to select the default active descendant.
While touching the codebase, we are also updating portions to TypeScript to reduce our debts.
Improving focus indicators
Added keyboard focus indicators for suggested actions and send box buttons. Per discussion with design team, the design is adopted from Fluent UI.
We also updated our
styleOptions
to make sure web devs can style both widgets with an accessible design.Design
In order to resolve this issue without introducing UX kinks, we need to differentiate the focus based on modality. We should scroll the focusing activity into the view only if the transcript is focused via keyboard (i.e. TAB or SHIFT + TAB).
The standard way is to use
:focus-visible
pseudo class. But it was introduced recently in Chrome/Edge 86+, and not supported in Safari.We ported the pseudo class polyfill from WICG into React and conditionally polyfill based on capability detection.
Focus indicators
Every button should have 5 different UI states for styling. This is the order of preferences:
When styling, these states may conflict with each other. For example, the button can be both hover and focused at the same time. Based on interactivity and user's intention, we ranked the states in the order listed above. E.g. when a button is disabled and hover, the disabled styling will be applied. Note the normal state will always apply to the button.
A new keyboard focus indicator (
:focus-visible
) is added. Since it is appear as a "focus ring" on top of the shade/background of the widget, it do not conflict with the 5 states mentioned above and can co-exist with others. Even the button is disabled (viaaria-disabled
), it will be visible for accessibility.Others
We also fixed some issues:
styleOptions.ts
, we fixed a typoautoScrollSnapOnPageoffset
(lowercase "offset")We are also (slowly) updating our codebase to TypeScript. As we are updating, we might discover more type inconsistencies issues.
Specific Changes
package-lock.json
updated itself after NPM install, looks like it was dirty before<BasicTranscript>
to have the last activity in the transcript as the default active descendantscrollIntoView()
out in a separate filescrollIntoViewWithBlockNearest.ts
defaultStyleOptions.ts
andStyleOptions.ts
createRenderWebChatWithHook()
helper so we can run hook without sending an invisible activity<SuggestedAction>
to add:focus-visible
and its polyfill viauseFocusVisible(): [boolean]
<SuggestedActions>
as we need to rearrange the DOM tree for:focus-visible
<IconButton>
to add:focus-visible
and its polyfillsendButtonDisabledBorderColor
was renamed tosendButtonBorderColorOnDisabled
background
is being deprecated in favor ofbackgroundColor
packages/test/page-object/.../clickSendButton.js
to move mouse cursor away from send button after click__tests__/html/accessibility.adaptiveCard.withoutTapAction.html
was wrongly grabbing<select>
element from the screen reader version of the transcript__tests__/toaster.js
: after action is dispatched, should wait until the rendering is completed before taking snapshot__tests__/basic.js
should wait for "UI connected"packages/directlinespeech/__tests__
temporarily disable tests related to "internal HTTP support", will re-enable in Re-enable Direct Line Speech "internal HTTP" tests #4053__tests__/adaptiveCards.js
should wait for next render after updating props before clickingCHANGELOG.md
I have updated documentationReview 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)