Skip to content

Conversation

@AlexandraGallipoliRodrigues
Copy link
Contributor

@AlexandraGallipoliRodrigues AlexandraGallipoliRodrigues commented Nov 18, 2025

ticket: https://jira.tid.es/browse/WEB-2283

Cambios

Añadir una “capa de accesibilidad para patrón acordeón con switch”:

  • aria-expanded
  • aria-controls
  • aria-live
  • aria-busy
  • label dinámico con “options below”.

Añadir estado de carga integrado en Row:

  • deshabilitar el switch cuando hay petición en curso (busy)

Y todo ello solo se activa si pasas controlDisclosure en ControlProps. Si no lo pasas, una Row con switch / checkbox / radio se sigue comportando como siempre.

Cambios que se deben hacer en webapp:

En pages/call-forwarding/ui/components/first-level-setting.tsx sustituir el SwitchRow por:

<Row
  title={texts.switchRow}
  switch={{
    name={`enable-${settingType.main}${settingType?.condition ?? ''}`},
    value: switchUiEnabled,
    onChange: handleChangeSettingStatus,
    controlDisclosure={{
      expanded: switchUiEnabled,
      'aria-live': 'assertive',
      'aria-busy': showChangeUndergoing === State.CHANGING,
      onLabelWhenExpanded: I18N.translate(''%(aria_label_switch_button_as_accordion_suffix_options_available_below)Options available below.''),
    }}
  }}
  dataAttributes={{ testid: switchTestId }}
/>

En pages/call-forwarding/ui/steps/main.tsx sustituir el SwitchRow por:

<Row
  title={I18N.translate('%(call_forwarding_main_forwarding_selector_switch)Call forwarding')}
  switch={{
    name: 'enable-forwarding',
    value: isForwardingEnabled,
    onChange: handleChangeForwardingSettings,
    controlDisclosure={{
      expanded: switchUiEnabled,
      'aria-live': 'assertive',
      'aria-busy': showChangeUndergoing === State.CHANGING,
      onLabelWhenExpanded: I18N.translate(''%(aria_label_switch_button_as_accordion_suffix_options_available_below)Options available below.''),
    }}
  }}
/>

Probado con voiceOver activado y en la brand de movistar:

test-local-expanded-rows.MP4

playroom

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds accessibility features for Row components with switches that act as accordion toggles, implementing ARIA patterns for disclosure widgets. It enables Row switches to announce their expanded state and optionally display loading indicators.

Key Changes:

  • Added SwitchDisclosure type with ARIA attributes (expanded, controls, live) and loading states (busy, showSpinner)
  • Integrated dynamic aria-label generation that appends "options below" text when expanded
  • Added loading state support with switch disabling and optional spinner display

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/list.tsx Outdated
const computedAriaLabel =
ariaLabel ??
(props.switchDisclosure?.expanded
? `${title} ${props.switchDisclosure?.onLabelWhenExpanded ?? 'Options available below.'}`
Copy link

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

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

The fallback text 'Options available below.' is hardcoded in English. This should be internationalized or derived from a translation function to support multiple languages, as the PR description shows usage with I18N.translate() in the webapp integration examples.

Copilot uses AI. Check for mistakes.
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 haven't found any I18N.translate() in this repo. How should I approach this?

Copy link
Member

Choose a reason for hiding this comment

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

Search in the repo for usage examples of: const {texts, t} = useTheme();

Texts are defined in the context provider and can be overriden, for example:

texts.formCreditCardNumberLabel || t(tokens.formCreditCardNumberLabel)
  • texts.formCreditCardNumberLabel is the user defined text override via provider (optional)
  • t(tokens.formCreditCardNumberLabel) calls the translate function for the token

src/list.tsx Outdated
</Box>
<div
aria-live={props.switchDisclosure?.live ?? 'off'}
aria-atomic
Copy link

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

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

The aria-atomic attribute is always set to true, but aria-live defaults to 'off'. When aria-live is 'off', aria-atomic has no effect. Consider only setting aria-atomic when aria-live is not 'off', or documenting why aria-atomic should always be present.

Suggested change
aria-atomic
{...(props.switchDisclosure?.live && props.switchDisclosure.live !== 'off' ? {'aria-atomic': true} : {})}

Copilot uses AI. Check for mistakes.
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 have adjusted aria-atomic so that it is only added when aria-live is not “off”.
In practice, our case always uses live: “assertive”, but we leave the type open to “polite” | “off” | “assertive”, so the wrapper behaves correctly in any of them

<div
aria-live={props.switchDisclosure?.live ?? 'off'}
aria-atomic
aria-busy={rowIsBusy || undefined}
Copy link

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

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

Using || undefined to conditionally render the attribute is unnecessary. The aria-busy attribute should be explicitly set to 'true' or 'false' as a string, or omitted entirely. Use aria-busy={rowIsBusy ? 'true' : undefined} or aria-busy={rowIsBusy ? true : undefined} instead.

Suggested change
aria-busy={rowIsBusy || undefined}
aria-busy={rowIsBusy ? 'true' : undefined}

Copilot uses AI. Check for mistakes.
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 thought is a type Booleanish = boolean | 'true' | 'false'; so booleans are also valid, right?

Copy link
Member

Choose a reason for hiding this comment

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

I think so

@github-actions
Copy link

github-actions bot commented Nov 18, 2025

Size stats

master this branch diff
Total JS 16.5 MB 16.5 MB +2.43 kB
JS without icons 2.36 MB 2.37 MB +2.43 kB
Lib overhead 86.7 kB 86.7 kB 0 B
Lib overhead (gzip) 19.1 kB 19.1 kB 0 B

@github-actions
Copy link

github-actions bot commented Nov 18, 2025

Accessibility report
✔️ No issues found

ℹ️ You can run this locally by executing yarn audit-accessibility.

@AlexandraGallipoliRodrigues
Copy link
Contributor Author

AlexandraGallipoliRodrigues commented Nov 18, 2025

sabeis como puedo probarlo en dev o en el propio flujo? Porque no veo en las github actions como desplegar mi snapshot o algo 🤨

nada, ya enconte el enlace ⬇️

@github-actions
Copy link

github-actions bot commented Nov 18, 2025

Deploy preview for mistica-web ready!

✅ Preview
https://mistica-r94kf82im-flows-projects-65bb050e.vercel.app

Built with commit 29943be.
This pull request is being automatically deployed with vercel-action

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@yceballost yceballost requested review from aweell and removed request for yceballost November 26, 2025 08:24
Copy link
Member

@pladaria pladaria left a comment

Choose a reason for hiding this comment

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

Please, implement a store (can be private) with an usage example of this feature.

A playroom snippet would also be nice

@pladaria pladaria self-requested a review November 27, 2025 09:43
src/list.tsx Outdated
atomicReading?: boolean;
}

type SwitchDisclosure = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider a more open approach rather than the row with switch only?

I was reviewing requests from design teams and in this case the control used is a radio instead of a switch:

Telefonica/mistica-design#2172

Can (or should) this approach be abstracted to work with any type of control the list allows?

Copy link
Contributor

Choose a reason for hiding this comment

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

agree, I think we should make this more generic

src/list.tsx Outdated
Comment on lines 283 to 284
busy?: boolean; // blocks UI and sets aria-busy
showSpinner?: boolean; // shows spinner on the right
Copy link
Contributor

Choose a reason for hiding this comment

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

showSpinner and busy aren't the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

one was for showing the spinner the other for setting aria-busy, but as the spinner is not in the spec, i'll delete the showSpinner

src/list.tsx Outdated
atomicReading?: boolean;
}

type SwitchDisclosure = {
Copy link
Contributor

Choose a reason for hiding this comment

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

agree, I think we should make this more generic

src/list.tsx Outdated
}

type SwitchDisclosure = {
expanded: boolean; // is the related content expanded
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want comments to be shown by vscode as jsdoc, you need to write them in jsdoc format:

{
    /** some doc about foo */
    foo: string;
}

src/list.tsx Outdated
Comment on lines 281 to 283
controlsId?: string; // id of the related content (if any)
live?: 'off' | 'polite' | 'assertive'; // announcement channel when changing expanded
busy?: boolean; // blocks UI and sets aria-busy
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer using aria-xxx naming

src/list.tsx Outdated
live?: 'off' | 'polite' | 'assertive'; // announcement channel when changing expanded
busy?: boolean; // blocks UI and sets aria-busy
showSpinner?: boolean; // shows spinner on the right
onLabelWhenExpanded?: string; // message when expanded = true
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need a different label for the expanded state? can't this be controlled by Row users by changing the label when needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for changing the announcement of the SR, in case you wanted something diffferent form "Options below"

src/list.tsx Outdated
trackingEvent?: TrackingEvent | ReadonlyArray<TrackingEvent>;

switch: ControlProps | undefined;
switchDisclosure?: SwitchDisclosure;
Copy link
Contributor

Choose a reason for hiding this comment

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

why a different switchDisclosure prop? shouldn't most of that props be part of the ControlProps type?

src/list.tsx Outdated
Comment on lines 597 to 598
aria-controls={props.switchDisclosure?.controlsId}
aria-expanded={props.switchDisclosure?.expanded}
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't work. You need to add these aria props in Switch component. TS doesn't warn you because there is a TS bug that allows you passing any aria-xxx prop to any component

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oki, but for the role row it seems that aria doesn't support aria-expanded
image

i'll added inside the div before the RadioButton

Comment on lines 586 to 589
<div
aria-live={props.switchDisclosure?.live ?? 'off'}
aria-atomic={props.switchDisclosure?.live !== 'off'}
aria-busy={rowIsBusy || undefined}
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this aria-live/atomic/busy part not needed in the renderRowWithDoubleInteraction case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's needed as well, i'll added it

src/list.tsx Outdated
</Box>
)}
/>
{showSpinner && (
Copy link
Contributor

Choose a reason for hiding this comment

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

is this spinner in the spec?

Copy link
Contributor

Choose a reason for hiding this comment

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

I recall the spinner was part of other conversation rather than expand/collapse states.

Telefonica/mistica-design#2151

The row is also using a switch but not necessarily expands or collapses content. If we want to cover coth cases i can update specs.

https://teams.microsoft.com/l/message/19:68854aea147e4787803aff0e380e1208@thread.tacv2/1739980344078?tenantId=9744600e-3e04-492e-baa1-25ec245c6f10&groupId=e265fe99-929f-45d1-8154-699649674a40&parentMessageId=1739980344078&teamName=M%C3%ADstica&channelName=Help&createdTime=1739980344078

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you’re right, the spinner is not part of the task, it comes from the original call-forwarding loading states (CHANGING / LENGTHY), which is a separate matter.

I’ll remove showSpinner and the built–in spinner rendering from Row

@AlexandraGallipoliRodrigues
Copy link
Contributor Author

Please, implement a store (can be private) with an usage example of this feature.

private story

A playroom snippet would also be nice

dev playroom

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.

5 participants