Skip to content

Make Spinner accessible #2416

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.

Sign up for GitHub

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

Open
rauboti wants to merge 3 commits into
base: master
Choose a base branch
from
+18 −7
Open

Make Spinner accessible #2416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
324d481
Add aria attributes
rauboti Aug 13, 2024
6dbf49f
Make aria-valuetext required
rauboti Aug 14, 2024
2db770f
Merge branch 'master' into accessible-spinner
rauboti Aug 28, 2024
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
2 changes: 1 addition & 1 deletion packages/ndla-ui/src/TreeStructure/TreeStructure.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ const NewFolder = ({ parentId, onCancel, structure, setStructure, onCreate }: Ne
</>
) : (
<FieldHelper>
<Spinner size="small" aria-label={t("loading")} />
<Spinner size="small" aria-valuetext={t("loading")} aria-label={t("loading")} />
</FieldHelper>
)}
</HStack>
Expand Down
9 changes: 5 additions & 4 deletions packages/primitives/src/Button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ export type ButtonVariantProps = { variant?: ButtonVariant } & RecipeVariantProp
interface BaseButtonProps extends HTMLArkProps<"button">, JsxStyleProps {
loading?: boolean;
loadingContent?: ReactNode;
loadingContentText?: string;
replaceContent?: boolean;
}

Expand Down Expand Up @@ -253,10 +254,10 @@ export const BaseButton = forwardRef<HTMLButtonElement, BaseButtonProps>(
);

export const Button = forwardRef<HTMLButtonElement, ButtonProps>(
({ variant, loadingContent, size, css: cssProp, ...props }, ref) => (
({ variant, loadingContent, loadingContentText, size, css: cssProp, ...props }, ref) => (
<BaseButton
{...props}
loadingContent={loadingContent ?? <Spinner size="small" />}
loadingContent={loadingContent ?? <Spinner size="small" aria-valuetext={loadingContentText ?? ""} />}
css={css.raw(buttonBaseRecipe.raw({ variant }), buttonRecipe.raw({ size }), cssProp)}
ref={ref}
/>
Expand All @@ -270,11 +271,11 @@ export type IconButtonVariantProps = { variant?: IconButtonVariant } & RecipeVar
export type IconButtonProps = BaseButtonProps & IconButtonVariantProps;

export const IconButton = forwardRef<HTMLButtonElement, IconButtonProps>(
({ variant, css: cssProp, loadingContent, size, replaceContent = true, ...props }, ref) => (
({ variant, css: cssProp, loadingContent, size, loadingContentText, replaceContent = true, ...props }, ref) => (
<BaseButton
{...props}
css={css.raw(buttonBaseRecipe.raw({ variant }), iconButtonRecipe.raw({ size }), cssProp)}
loadingContent={loadingContent ?? <Spinner size="small" />}
loadingContent={loadingContent ?? <Spinner size="small" aria-valuetext={loadingContentText ?? ""} />}
replaceContent={replaceContent}
ref={ref}
/>
Expand Down
14 changes: 12 additions & 2 deletions packages/primitives/src/Spinner.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ export const spinnerRecipe = cva({
borderBlockColor: "background.subtle",
borderInlineStartColor: "background.subtle",
borderInlineEndColor: "stroke.default",
"& [data-name='text']": {
display: "none",
},
_motionReduce: {
animationDuration: "2s",
},
Expand Down Expand Up @@ -51,10 +54,17 @@ export const spinnerRecipe = cva({

export type SpinnerVariantProps = RecipeVariantProps<typeof spinnerRecipe>;

export type SpinnerProps = HTMLArkProps<"div"> & JsxStyleProps & SpinnerVariantProps;
export type SpinnerProps = HTMLArkProps<"div"> & JsxStyleProps & SpinnerVariantProps & { "aria-valuetext": string };

const StyledSpinner = styled(ark.div, {}, { baseComponent: true });

export const Spinner = forwardRef<HTMLDivElement, SpinnerProps>(({ size, css: cssProp, ...props }, ref) => (
<StyledSpinner css={css.raw(spinnerRecipe.raw({ size }), cssProp)} {...props} ref={ref} />
<StyledSpinner
aria-busy="true"
aria-live="polite"
role="progressbar"
Copy link
Copy Markdown
Contributor

@Jonas-C Jonas-C Aug 14, 2024

Choose a reason for hiding this comment

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

Dette skal nok ikke være en progressbar, men heller en status. Vi har ikke noe progress på spinneren vår. Enten så eksisterer den eller så eksisterer den ikke. progressbar skal brukes til faktisk målbare distanser.

Copy link
Copy Markdown
Contributor Author

@rauboti rauboti Aug 14, 2024

Choose a reason for hiding this comment

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

Det er ikke helt presist. Progressbar kan fint brukes for ikke-definerte måledistanser, selv om det er mindre vanlig. Lasting er en status, men det er også en progresjon selv om vi ikke måler og viser den. Her er det gjort med vilje for å kunne støtte aria-valuetext. Men sida jeg også har valgt polite over assertive, så antar jeg at jeg kan bruke aria-label til samme info, og bare flatt sette aria-status.

Copy link
Copy Markdown
Contributor Author

@rauboti rauboti Aug 14, 2024

Choose a reason for hiding this comment

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

Forøvrig en sammenligning mellom role="status" (første bilde) og role="progressbar" (andre bildet) når jeg blar igjennom ei side som fortsatt har en spinner:
image
Og nr 2:
image

I den første, som har role="status", dukker kun teksten opp fordi jeg har lagt den som en paragraph; aria-label plukka den ikke opp. Nøkkelinfo er hhv nest siste og siste linje, teksten "Venter i spenning". Jeg prøvde også å bytte ut Spinner-komponenten med en vanlig div, i tilfelle det var noe mer ark (mer for å utelukke)
image

aria-hidden fordi jeg ikke vil lese opp bokstaven T. Og den hopper da glatt over det når jeg blar igjennom sida, og oppfatter ingenting. Er åpen for at jeg kanskje gjør noe feil, men det skal litt til å ødelegge speech vieweren, og jeg kan ikke se role="status" klarer å gi en skjermleser det samme som role="progressbar" her.

css={css.raw(spinnerRecipe.raw({ size }), cssProp)}
{...props}
ref={ref}
/>
));