Bug: 🐛 introduit dans la version 7.2.0 avec l'ajout d'un focus sur collapse (DsfrAccordion)

[yvalentin]

Bonjour,

Lors de la tentative de mise à jour de la lib sur la version 7.2.0, nous nous apercevons d'un dysfonctionnement avec le composant dsfrAccordion. Un focus automatique fait scroller la page jusqu'a un tag html a contenu dans l'accordion.

Après recherche, nous détectons l'update de code suivant comme pouvant être en cause (j'ai testé avec la suppression du focus() et le comportement revient bien à la normal)

// src/composables/useCollapsable.ts
} else if (collapse.value) {
  collapse.value.querySelector('a')?.focus()
}

Ceci a été introduit lors du commit suivant
2d7018f#diff-20e22350480b8eef0a8afed726c806a2650e2b5bcb4c99ecec84bbe884aecf44R60

Une demo du bug peut être vu ici:
https://tee-preprod-pr1481.osc-fr1.scalingo.io/aides-entreprise/act-pas-a-pas

[laruiss]

Bonjour, merci, je vais le corriger bientôt.

[laruiss]

Alors, après des recherches, il se trouve que vous n’utilisez pas les accordéons comme recommandé par le DSFR :

• un accordéon doit être fermé par défaut
• un seul accordéon d’un groupe doit être ouvert à la fois

Maintenant à votre place, je me poserais 2 questions :

1. Quel est l’intérêt d’un point de vue UX d’avoir des accordéons s’ils sont par défaut ouverts ?
2. Quel est l’intérêt d’un point de vue UX d’avoir des accordéons qui sont tous ouverts ?

D’autre part, le focus est important pour l’a11y, je ne vais donc pas l’enlever.

[yvalentin]

Bonjour,

Merci pour le retour ! En faite, les accordéons sont fermés en version mobile et sinon effectivement ouverts.
Notre intérêt est donc de minimiser le contenu pour la version mobile.

Je vais voir quelle solution nous pouvons trouver avec notre designeuse

[MaitreManuel]

Bonsoir,

Je viens amener ma pierre à l'édifice.

J'ai constaté également ce bug sur les DsfrAccordion.vue lorsque le contenu du collapse possède un lien.
Sauf que le focus devrait rester sur le bouton qui toggle l'accordéon, sinon en terme d'accessibilité on est KO.

Dans mon cas, j'ai du contenu type texte avant le premier lien, ce qui fait que le focus sur ce dernier fait ignorer au lecteur d'écran tout le texte qui le précède.

Exemple :

Ici le focus est représenté par l'outline orange.

Pour ce qui est du composable onTransitionEnd, il faudrait ajouter un paramètre qui permettrait de focus ou non le premier lien du composant utilisant la fonction :

// src/composables/useCollapsable.ts ligne 56
const onTransitionEnd = (expanded: boolean, focusFirstAnchor: boolean = true): void => {
    collapsing.value = false
    if (focusFirstAnchor) {
      collapse.value?.querySelector('a')?.focus()
    }
    if (collapse.value && expanded === false) {
      collapse.value.style.removeProperty('--collapse-max-height')
    }
}

[...]

// src/components/DsfrAccordion/DsfrAccordion.vue ligne 79
@transitionend="onTransitionEnd(isActive, false)"

Ce qui corrige le problème :

Ma proposition de correction est ici

[ArnaudTA]

Problème identique de notre coté mais pas en utilisant le DsfrAccordion directement, notre SideMenuList peut ouvrir et fermer des sections en fonction d'interaction utilisateur. Cela nous pose problème car ces ouvertures/fermetures sont asynchrones et peuvent survenir par exemple lorsqu'un utilisateur est en train de remplir un Input.

Pour la Pr je suggérai que :

1. Le Sidemenu utilise @transitionend toujours à false
2. Changer le @transitionend pour qu'il prenne en paramètre un id d'element optionel sur lequel focus si renseigné

[MaitreManuel]

@ArnaudTA pour le Sidemenu, je suis pas sur que ça rentre dans le scope du composant d'avoir des champs texte quand tu regardes la doc.
J'ajouterai que "l'accordéon natif" en mode mobile devrait avoir ce comportement de focus le premier lien de la liste.

Et pour ton deuxième point ça implique d'ajouter la propagation de l'évènement @transitionend, donc il faudrait l'ajouter à chaque composant qui utilise le composable OU d'ajouter une props qui cible l'élément souhaité.

Dans les deux cas c'est un peu hors scope de ma PR

[ArnaudTA]

yep j'entends bien, de toutes façons on a patché la lib de notre coté en attendant cette PR ou une autre ça m'importe peu mais je te rejoint qu'il faut avoir plus de controle sur ce comportement

[laruiss]

Merci à tous pour vos commentaires et reproductions, animations et propositions !

Je ne suis pas l’auteur du composable useCollapsable(), il a été créé par un contributeur comme vous. Je ne me suis pas penché sur le sujet. Je vous fais confiance pour vos tests et découvertes.

@MaitreManuel j’espère que tu ne m’en voudras pas d’avoir pris le code de ta PR et de l’avoir utilisé dans la PR de @ArnaudTA... Pour une prochaine fois, veille bien à partir de la branche develop et à demander la fusion dans develop 😊 .

[github-actions]

🎉 This issue has been resolved in version 8.1.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[laruiss]

@yvalentin je veux bien votre retour sur cette correction :

• Est-ce le résultat que vous attendiez ?
• Est-ce que vous êtes sûr qu'il n'y a pas de régression au niveau de l'a11y ?

Et mes excuses pour avoir jugé trop rapidement qu'il n'y avait rien à corriger.

À bientôt

[yvalentin]

@laruiss Je testerais la montée de version sur la 8.1.0 la semaine prochaine. Je ferai un retour à ce moment là. Merci

[yvalentin]

@laruiss Pour nous, c'est tout bon. J'ai pu faire la monté de version en 8.1.1
Et je n'ai pas rencontre de regression ni UX ni d’accessibilité.

Merci