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

Accessibility compliance: Improve share dialog accessibility #9989

Merged
merged 16 commits into from
May 9, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion react-common/components/controls/EditorToggle.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,11 @@ const EditorToggleAccessibleMenu = (props: EditorToggleProps) => {
next.push({...current});

// The selected item will always be a top-level option, not in a dropdown
if (selected === index) next[next.length - 1].selected = true;
if (selected === index) {
next[next.length - 1].selected = true;
} else {
next[next.length - 1].selected = false;
}

if (isDropdownItem(current)) {
next.push(...current.items.filter(i => i.focusable))
Expand Down
54 changes: 10 additions & 44 deletions react-common/components/controls/FocusList.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as React from "react";
import { ContainerProps } from "../util";
import { ContainerProps, findNextFocusableElement } from "../util";

export interface FocusListProps extends ContainerProps {
role: string;
Expand Down Expand Up @@ -41,7 +41,9 @@ export const FocusList = (props: FocusListProps) => {
focusableElements = [];

for (const element of focusable.values()) {
focusableElements.push(element as HTMLElement);
if (getComputedStyle(element).display !== "none") {
focusableElements.push(element as HTMLElement);
}

// Remove them from the tab order, menu items are navigable using the arrow keys
element.setAttribute("tabindex", "-1");
Expand All @@ -57,42 +59,6 @@ export const FocusList = (props: FocusListProps) => {
}
}

const isFocusable = (e: HTMLElement) => {
return e.getAttribute("data-isfocusable") === "true"
riknoll marked this conversation as resolved.
Show resolved Hide resolved
&& getComputedStyle(e).display !== "none";
}

const firstFocusableElement = () => {
return focusableElements.find(e => isFocusable(e))
}

const lastFocusableElement = () => {
for (let i = 0; i < focusableElements.length; i++) {
if (isFocusable(focusableElements[focusableElements.length - 1 - i])) {
return focusableElements[focusableElements.length - 1 - i];
}
}

return focusableElements[0];
}

const nextFocusableElement = (index: number, forwards: boolean) => {
let current: HTMLElement
for (let i = 1; i < focusableElements.length; i++) {
if (forwards) {
current = focusableElements[(index + i) % focusableElements.length];
}
else {
current = focusableElements[(index + focusableElements.length - i) % focusableElements.length];
}

if (isFocusable(current)) {
return current;
}
}
return focusableElements[0];
}

const onKeyDown = (e: React.KeyboardEvent<HTMLElement>) => {
if (!focusableElements?.length) return;

Expand Down Expand Up @@ -120,31 +86,31 @@ export const FocusList = (props: FocusListProps) => {
}
else if (e.key === (useUpAndDownArrowKeys ? "ArrowDown" : "ArrowRight")) {
if (index === focusableElements.length - 1 || target === focusList) {
focus(firstFocusableElement());
focus(findNextFocusableElement(focusableElements, index, 0, true));
}
else {
focus(nextFocusableElement(index, true));
focus(findNextFocusableElement(focusableElements, index, index + 1, true));
}
e.preventDefault();
e.stopPropagation();
}
else if (e.key === (useUpAndDownArrowKeys ? "ArrowUp" : "ArrowLeft")) {
if (index === 0 || target === focusList) {
focus(lastFocusableElement());
focus(findNextFocusableElement(focusableElements, index, focusableElements.length - 1, false));
}
else {
focus(nextFocusableElement(index, false));
focus(findNextFocusableElement(focusableElements, index, index - 1, false));
}
e.preventDefault();
e.stopPropagation();
}
else if (e.key === "Home") {
focus(firstFocusableElement());
focus(findNextFocusableElement(focusableElements, index, 0, true));
e.preventDefault();
e.stopPropagation();
}
else if (e.key === "End") {
focus(lastFocusableElement());
focus(findNextFocusableElement(focusableElements, index, focusableElements.length - 1, true));
e.preventDefault();
e.stopPropagation();
}
Expand Down
14 changes: 7 additions & 7 deletions react-common/components/controls/FocusTrap.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as React from "react";
import { classList, nodeListToArray } from "../util";
import { classList, nodeListToArray, findNextFocusableElement } from "../util";

export interface FocusTrapProps extends React.PropsWithChildren<{}> {
onEscape: () => void;
Expand Down Expand Up @@ -58,24 +58,24 @@ export const FocusTrap = (props: FocusTrapProps) => {

if (forward) {
if (goToEnd) {
focusable[focusable.length - 1].focus();
findNextFocusableElement(focusable, index, focusable.length - 1, forward).focus();
}
else if (index === focusable.length - 1) {
focusable[0].focus();
findNextFocusableElement(focusable, index, 0, forward).focus();
}
else {
focusable[index + 1].focus();
findNextFocusableElement(focusable, index, index + 1, forward).focus();
}
}
else {
if (goToEnd) {
focusable[0].focus();
findNextFocusableElement(focusable, index, 0, forward).focus();
}
else if (index === 0) {
focusable[focusable.length - 1].focus();
findNextFocusableElement(focusable, index, focusable.length - 1, forward).focus();
}
else {
focusable[Math.max(index - 1, 0)].focus();
findNextFocusableElement(focusable, index, Math.max(index - 1, 0), forward).focus();
}
}

Expand Down
13 changes: 8 additions & 5 deletions react-common/components/share/ShareInfo.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export interface ShareInfoProps {
onClose: () => void;
}


export const ShareInfo = (props: ShareInfoProps) => {
const {
projectName,
Expand Down Expand Up @@ -323,7 +324,7 @@ export const ShareInfo = (props: ShareInfoProps) => {
{showSimulator && shareState !== "gifrecord" &&
<div className="project-share-thumbnail">
{thumbnailUri
? <img src={thumbnailUri} />
? <img src={thumbnailUri} alt={lf("Preview of your code running on a micro:bit")} aria-label={lf("Micro:bit simulator preview")}/>
riknoll marked this conversation as resolved.
Show resolved Hide resolved
: <div className="project-thumbnail-placeholder">
<div className="common-spinner" />
</div>
Expand Down Expand Up @@ -406,6 +407,7 @@ export const ShareInfo = (props: ShareInfoProps) => {
<div className="common-input-attached-button">
<Input
ariaDescribedBy="share-input-title"
ariaLabel={lf("Your shareable project link")}
handleInputRef={handleInputRef}
initialValue={shareData.url}
readOnly={true}
Expand Down Expand Up @@ -462,7 +464,7 @@ export const ShareInfo = (props: ShareInfoProps) => {
className="menu-button project-qrcode"
buttonRef={handleQRCodeButtonRef}
title={lf("Show QR Code")}
label={<img className="qrcode-image" src={shareData?.qr} />}
label={<img className="qrcode-image" src={shareData?.qr} alt={lf("QR code to access your project")} aria-label={lf("Project share link QR code")}/>}
onClick={handleQRCodeClick}
/>
</div>
Expand All @@ -475,7 +477,8 @@ export const ShareInfo = (props: ShareInfoProps) => {
selected={embedOptions.findIndex(i => i.name === embedState)} />
<Textarea readOnly={true}
rows={5}
initialValue={shareData?.embed[embedState]} />
initialValue={shareData?.embed[embedState]}
ariaLabel={lf("Embed code textarea")} />
</div>}
{kioskState &&
<div>
Expand Down Expand Up @@ -511,9 +514,9 @@ export const ShareInfo = (props: ShareInfoProps) => {
</div>

{showQRCode &&
<Modal title={lf("QR Code")} onClose={handleQRCodeModalClose}>
<Modal title={lf("QR Code")} onClose={handleQRCodeModalClose} ariaLabel={lf("QR Code modal")} >
<div className="qrcode-modal-body">
<img className="qrcode-image" src={shareData?.qr} />
<img className="qrcode-image" src={shareData?.qr} alt={lf("QR code to access your project")} aria-label={lf("Project share link QR code enlarged")} />
</div>
</Modal>
}
Expand Down
21 changes: 21 additions & 0 deletions react-common/components/util.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -91,4 +91,25 @@ export function screenToSVGCoord(ref: SVGSVGElement, coord: ClientCoordinates) {
screenCoord.x = coord.clientX;
screenCoord.y = coord.clientY;
return screenCoord.matrixTransform(ref.getScreenCTM().inverse());
}

export function findNextFocusableElement(elements: HTMLElement[], focusedIndex: number, index: number, forward: boolean): HTMLElement {
Copy link
Member

Choose a reason for hiding this comment

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

nit: you can eliminate the focusedIndex argument by using the iterative implementation above instead of the recursive one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer using recursion for this, but would like to hear more of your thoughts if you feel strongly about implementing it with iteration.

Copy link
Member

Choose a reason for hiding this comment

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

the iterative implementation doesn't require passing in focusedIndex. I just also generally prefer loops to tail recursion, personally (but feel free to ignore)

const increment = forward ? 1 : -1;
const element = elements[index];
// in this case, there are no focusable elements
if (focusedIndex === index) {
return element;
}
if (getComputedStyle(element).display !== "none") {
return element;
} else {
if (index + increment >= elements.length) {
index = 0;
} else if (index + increment < 0) {
index = elements.length - 1;
} else {
index += increment;
}
}
return findNextFocusableElement(elements, focusedIndex, index, forward);
}
4 changes: 3 additions & 1 deletion react-common/styles/controls/Modal.less
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@
.common-modal {
width: 50%;
max-width: 40rem;
max-height: 100%;
border-radius: .285rem;
overflow: hidden;
overflow-x: hidden;
overflow-y: auto;
}

.wide > .common-modal {
Expand Down
2 changes: 1 addition & 1 deletion react-common/styles/react-common-variables.less
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@
* EditorToggle *
****************************************************/

@editorToggleBackgroundColor: rgba(52,73,94,.4);
@editorToggleBackgroundColor: rgba(52,73,94,.8);
@editorToggleBorderColor: rgba(52,73,94,.2);
@editorToggleBorderWidth: 3px;

Expand Down
6 changes: 6 additions & 0 deletions react-common/styles/share/share.less
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@
margin-top: 1rem;
position: relative;
margin-bottom: 2rem;
justify-content: space-between;
}

.project-share-social {
Expand Down Expand Up @@ -312,6 +313,11 @@
.gif-recorder-content .thumbnail-controls {
padding: 0 2rem;
}

.embed.mobile-portrait-hidden {
color: #323130 !important;
riknoll marked this conversation as resolved.
Show resolved Hide resolved
background: #e0e1e2 !important;
}
}

@media @mobileAndBelow {
Expand Down