Skip to content

Improve screen reader and keyboard navigation experience for sound effect editor#11300

Open
microbit-robert wants to merge 1 commit into
microsoft:masterfrom
microbit-robert:sound-effect-sr
Open

Improve screen reader and keyboard navigation experience for sound effect editor#11300
microbit-robert wants to merge 1 commit into
microsoft:masterfrom
microbit-robert:sound-effect-sr

Conversation

@microbit-robert
Copy link
Copy Markdown
Contributor

This PR makes various keyboard navigation and screen reader improves to the sound effect field editor.

Editor tab:

  • Use the "p" key only to preview the sound effect when in the editor. Add a longer aria-label to explain this.
  • Radio group label added for waveforms
  • The duration input is given a combobox role and the down arrow opens the options and focuses the first option
  • The value and type for the effect and interpolation dropdowns is now output for screen reader
    • e.g., from "Tremolo, list box pop-up collapsed" to "Tremolo, Effect, list box pop-up collapsed"
  • Pressing Escape in the effect and interpolation lists closes them without closing the entire field editor
  • Improved frequency and volume slider labels

Gallery tab:

  • Change the gallery markup so it is a grid. This ensures that the preview buttons are discoverable by screen reader users.
    • The gallery wrapper now takes focus which uses aria-activedescendant to manage it's children. Previously, the children took focus directly.
  • Remove "Stop Sound Preview" text. This gets in the way of the user hearing the sound preview and the sounds are so short, there isn't really a need to stop them.
  • Focus the editor / gallery toggle after selecting a sound effect from the gallery - this matches the melody field editor

Copy link
Copy Markdown
Member

@riknoll riknoll left a comment

Choose a reason for hiding this comment

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

Minor nitpicks! LGTM otherwise!

outline: 3px solid var(--pxt-focus-border) !important;
outline-offset: 3px;
.common-button.sound-effect-play-button {
&[aria-selected="true"],&:focus-visible {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
&[aria-selected="true"],&:focus-visible {
&[aria-selected="true"], &:focus-visible {

const ref = React.useRef<HTMLDivElement>(null);

const handlePreviewButtonClick = (index: number) => {
ref.current?.focus({preventScroll: true});
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
ref.current?.focus({preventScroll: true});
ref.current?.focus({ preventScroll: true });

onFocus={focusSelectOrPlayElement}>
role="grid"
aria-activedescendant={activeDescendant}
onFocus={e => { setGridFocused(true); focusSelectOrPlayElement(e); }}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

don't like cramming multi-line handlers inline like this, let's pull it up into a variable. same for keydown below

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.

2 participants