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

Set default descendant when focusing on transcript and add keyboard focus indicators #4035

Merged
merged 47 commits into from
Sep 22, 2021

Conversation

compulim
Copy link
Contributor

@compulim compulim commented Aug 17, 2021

Related to #4018. Related to #4028. Related to #4029. Related to #4014.

Changelog Entry

Breaking changes

  • Style options are introduced to send button for improved accessibility:
    • suggestedActionBackground and suggestedActionXXXBackground are being deprecated in favor of suggestedActionBackgroundColor and suggestedActionBackgroundColorOnXXX respectively, for consistencies when porting to other platforms
    • suggestedActionDisabledXXX is being renamed to suggestedActionXXXOnDisabled, for consistencies with other style options
    • suggestedActionXXXOnActive, suggestedActionXXXOnFocus, suggestedActionXXXOnHover are introduced for styling per user gestures
    • suggestedActionKeyboardFocusIndicatorXXX are introduced for styling the "focus ring" when focused using a keyboard

Fixed

  • Fixes #4018. When using TAB or SHIFT + TAB key to focus on the transcript, it should select the last activity, by @compulim, in PR #4035
  • Fixes #4029. Added new keyboard focus indicator for suggested actions, by @compulim, in PR #4035
    • New style options are introduced: suggestedActionXXXOnActive, suggestedActionXXXOnFocus, suggestedActionXXXOnHover, suggestedActionKeyboardFocusIndicatorXXX
    • Style options are renamed: suggestedActionDisabledXXX become suggestedActionXXXOnDisabled
  • Fixes #4028. Added new keyboard focus indicator for send box buttons, by @compulim, in PR #4035
    • New style options are introduced: 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:

  • Disabled
  • Active (press-and-hold)
  • Hover
  • Focused
  • Normal

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 (via aria-disabled), it will be visible for accessibility.

Others

We also fixed some issues:

  • In our styleOptions.ts, we fixed a typo autoScrollSnapOnPageoffset (lowercase "offset")
    • This is not a breaking change, and don't need migrations
    • Internally, we are using the right name. We are just wrong on our typings, and our defaults
    • Web devs do not need to update any code on their side

We are also (slowly) updating our codebase to TypeScript. As we are updating, we might discover more type inconsistencies issues.

Specific Changes

  • Update associated codebase to TypeScript
    • Cleared some dead code while introducing some typings
  • Root package-lock.json updated itself after NPM install, looks like it was dirty before
  • Update <BasicTranscript> to have the last activity in the transcript as the default active descendant
    • Refactored the polyfilling scrollIntoView() out in a separate file scrollIntoViewWithBlockNearest.ts
  • Fix typos in defaultStyleOptions.ts and StyleOptions.ts
  • Test harness: added a new createRenderWebChatWithHook() helper so we can run hook without sending an invisible activity
  • Test snapshots updated as there must be a focusing activity as long as the transcript is not empty
  • Updated <SuggestedAction> to add :focus-visible and its polyfill via useFocusVisible(): [boolean]
    • Also modified <SuggestedActions> as we need to rearrange the DOM tree for :focus-visible
  • Updated <IconButton> to add :focus-visible and its polyfill
  • Updated style options for suggested actions based on the following rules:
    • The match other style options, sendButtonDisabledBorderColor was renamed to sendButtonBorderColorOnDisabled
    • To improve portability in future work, background is being deprecated in favor of backgroundColor
      • React Native does not support background image
  • Updated packages/test/page-object/.../clickSendButton.js to move mouse cursor away from send button after click
    • Today, after click, the cursor is still hovering on the button. When we do image snapshot afterward, we are capturing the hover state of the button as well
    • Tomorrow, after clicking, we will move the cursor offscreen, so we will not snapshot the hover state of the button
    • We need to update all snapshots as we no longer capture the hovering state
  • Improved test reliability for the following tests:
    • __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 clicking
  • 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 marked this pull request as ready for review August 27, 2021 05:12
@compulim compulim force-pushed the fix-4018-default-active-descendant branch from 6f94462 to 099f2f9 Compare August 27, 2021 05:13
@compulim compulim marked this pull request as draft September 2, 2021 22:18
@compulim compulim force-pushed the fix-4018-default-active-descendant branch 2 times, most recently from 5ba3f9a to b37d623 Compare September 9, 2021 22:04
@compulim compulim marked this pull request as ready for review September 9, 2021 23:00
@compulim 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
@compulim compulim force-pushed the fix-4018-default-active-descendant branch from e573d7f to d003ce2 Compare September 22, 2021 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants