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.

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

Make Spinner accessible #2416

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Make Spinner accessible #2416

Changes from 1 commit
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
13 changes: 12 additions & 1 deletion 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 @@ -56,5 +59,13 @@ export type SpinnerProps = HTMLArkProps<"div"> & JsxStyleProps & SpinnerVariantP
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"
aria-valuetext="Loading"
Copy link
Member

Choose a reason for hiding this comment

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

Bør vel puttes inn i oversettelser.

Copy link
Contributor

@Jonas-C Jonas-C Aug 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Nei. Vi støtter ikke oversettelser i primitives, og det synes jeg ikke vi skal heller. Dette burde generelt sett settes der den brukes, for å gi mer nøyaktige oversettelser.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Må innrømme jeg hadde over gjennomsnittet lyst te å legge inn oversettelser her, men sida denne valuetexten KAN være noe forskjellig avhengig av hvor spinneren brukes (trenger ikke være, men kan), ble det hakket mindre viktig.
Denne fungerer derfor kun som en fallbackverdi. Jo flere ting man må huske å sette på komponentene, jo lettere er det for at noe går i glømmeboka. Spesielt attributter som brukes mindre jevnlig er utsatt for å kunne glemmes. For meg er det argument godt nok til å beholde den her, som en fallback. Men den bør definitivt overskrives der komponenten brukes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Denne må vel overstyres alle steder, da hovedspråket på siden er norsk. Da er det like greit å gjøre den required v.h.a typescript.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Da liker jeg faktisk oversettelser bedre, ev. under egen gruppering composites eller noe, dersom primitives skal være fri for slikt. Jeg mener det ikke er sannsynlig at den tekstverdien kommer til å ende opp med å være forskjellig på alle steder, og da mener jeg det er tungvint å skulle legge den inn hvert sted komponenten brukes hvis det kan unngås (og heller la den være overstyrbar de steder den må overstyres).
Jeg tar den ut og legger den som required for typescript nå (så kan det heller gjøres om senere), men ber om at vi gjør en vurdering av om vi kan håndtere slikt bedre.

role="progressbar"
Copy link
Contributor

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
Contributor Author

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
Contributor Author

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}
/>
));
Loading