#709: Lighthouse check: Cookiebanner ARIA #40
Conversation
There was a problem hiding this comment.
Ha, fijn dat je hiermee bezig bent geweest. Ik heb nog wel wat vragen / opmerkingen, zoals je ziet in de comments.
Je begeleidend schrijven bij de PR was wel wat summier. Dus heb zelf een Lighthouse check uitgevoerd bij een willekeurig project (Melkweg) om te zien welke issues je nu weg aan het werken bent.
Ik denk dat nog niet alles klopt. Is er een manier waarop jij dit in de browser test?
Lighthouse kan soms wat lastig te interpreteren zijn. Lighthouse maakt gebruik van de "axe-core". Er is ook een eigen Axe Devtools, die ik wel eens gebruik. Die geeft soms net wat meer uitleg. (Chrome extension)
Daarnaast geeft Lighthouse ook nog wat "best practices". Het lijkt me goed om die ook mee te nemen. In dit geval gaat het vooral om een CSS aanpassing, dus die is relatief eenvoudig.
Best practices zijn in dit geval non-normatief, wat zoveel betekent als dat ze niet verplicht zijn volgens de guidelines. Maar wel goed om even mee te pakken indien mogelijk.
src/cookie-consent.mjs
Outdated
| @@ -122,10 +121,10 @@ export default class Dialog extends HTMLElement { | |||
| this.tabList.init(); | |||
|
|
|||
| const template = ` | |||
| <aside part="${this.config.prefix}" id="${this.config.prefix}" class="${this.config.prefix} js-cookie-bar" role="dialog" aria-live="polite" aria-describedby="${this.config.prefix}-description" aria-hidden="true" tabindex="0"> | |||
| <aside part="${this.config.prefix}" id="${this.config.prefix}" class="${this.config.prefix} js-cookie-bar" aria-role="dialog" aria-live="polite" aria-labelledby="${this.config.prefix}__title" aria-describedby="${this.config.prefix}-description" aria-hidden="true" tabindex="0"> | |||
There was a problem hiding this comment.
aria-role="dialog" Is dit correct? Ik denk dat dit role="dialog" moet blijven?
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/dialog_role
There was a problem hiding this comment.
Ik denk dat dit al verholpen zou moeten zijn door het toevoegen van de aria-labelledby
Binnen ARIA is het belangrijk dat iets een naam, rol en waarde heeft. Als 1 van de dingen ontbreekt dan snapt het algoritme dat de accessible naam bepaalt, het niet meer.
There was a problem hiding this comment.
Ah ja dit moest inderdaad role="dialog" blijven, ik blijf hier alleen wel deze melding voor krijgen met role="dialog" vandaar dat ik het een en ander aan het proberen was
src/dialog-tablist.mjs
Outdated
| @@ -42,7 +42,7 @@ const DialogTabList = (cookieInformation) => { | |||
| transform: scaleY(-1); | |||
| } | |||
| </style> | |||
| <li part="${PREFIX}__tab-list-item" role="presentation"> | |||
| <li part="${PREFIX}__tab-list-item" role="tab"> | |||
There was a problem hiding this comment.
Je voegt hier role="tab" toe, maar hij is ook nog aanwezig op het element met de class class="${PREFIX}__tab-toggle" , waardoor het dubbelop is. Denk dat dat voor problemen zorgt
There was a problem hiding this comment.
Ja, deze is eigenlijk echt allen toegevoegd zodat er geen aria warning verschijnt uit de lighthouse check. Als role="tablist" geen children heeft met role="tab" krijg je namelijk warning
There was a problem hiding this comment.
Eigenlijk dient de role='tab' maar op 1 niveau te worden toegevoegd. Nu heb je semantisch gezien een tab in een tab. (Als je hem niet op 1 van de 2 plekken verwijdert).
Ik denk dat de originele tabs-opzet binnen deze package eigenlijk niet ideaal is. Normaliter is de tablist hetgeen wat de tabjes representeert. En onder de tablist heb je dan verschillende tabpanelen.
Hier zitten de tabpanelen genest in de tablist.
Zie ook:
En op deze pagina zie je een standaard implementatie die eigenlijk gevolgd zou moeten worden:
https://www.w3.org/WAI/ARIA/apg/patterns/tabs/examples/tabs-manual/#tablist-1
Overigens ben ik niet getrouwd met het idee dat dit perse een tablist moet blijven, het kan waarschijnlijk ook versimpeld worden naar een lijst zoals je vaak ziet bij FAQ-items.
There was a problem hiding this comment.
Thanks voor dit voorbeeld ik raakte inderdaad erg in de wr van de bestaande code. Ik heb er nu bewust voor gekozen om het te laten voor wat het is omdat er de laatste tijd al erg veel aan deze package gewerkt is, en dit voor nu even de meest pragmatische oplossing leek.
There was a problem hiding this comment.
@CountNick Wat betekent dit effectief? Is het wat jou betreft dan bij deze afgerond en is deze PR dan weer ready for review? Laat me weten als ik weer moet kijken :)
There was a problem hiding this comment.
@MicheleNL Deze kan wat mij betreft nu inderdaad weer worden bekeken :)
src/dialog-tablist.mjs
Outdated
| href="#${PREFIX}-tabpanel-${index}" | ||
| aria-controls="${PREFIX}-tabpanel-${index}" | ||
| aria-owns="${PREFIX}__tab-list" |
There was a problem hiding this comment.
Heb je aria-owns hier nodig? Ik denk dat je nu een relatie probeert te leggen, omdat Lighthouse dat zegt, maar waarschijnlijk is dit een effect van de role="tab" die hier nog onterecht op staat.
There was a problem hiding this comment.
Lighthouse gaf aan dat role="tablist" children nodig heeft met een bepaalde role namelijk tab of tabpanel. Als er geen direct children zijn kan hier voor aria-owns gebruikt worden om toch die parent-child relatie te leggen. Ik denk dat het hele ding misschien gewoon even gecheckt moet worden op correcte aria-labels. Ik zal daar nog even verder naar kijken
There was a problem hiding this comment.
Afgaande op wat er op MDN staat denk ik toch niet dat deze role="tab" hier onterecht op staat: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/tab_role. Hier staat namelijk het volgende The ARIA tab role indicates an interactive element inside a tablist that, when activated, displays its associated tabpanel. Op zich is dit ook wel het interactieve element in de tablist die er voor zorgt dat de tabpanel word getoond
Ik test dit nu door lokaal een npm link op te zetten voor de package. Zo kan ik lokaal changes maken en snel weer een lighthouse test draaien. Excuus, ik ben inderdaad vergeten de exacte errors te beschrijven. Die styling op het input element moet binnen het project worden gedaan waar de popup is toegevoegd, dat kan helaas niet binnen deze PR |
src/dialog-tablist.mjs
Outdated
| @@ -42,7 +42,7 @@ const DialogTabList = (cookieInformation) => { | |||
| transform: scaleY(-1); | |||
| } | |||
| </style> | |||
| <li part="${PREFIX}__tab-list-item" role="presentation"> | |||
| <li part="${PREFIX}__tab-list-item" role="tab"> | |||
There was a problem hiding this comment.
Eigenlijk dient de role='tab' maar op 1 niveau te worden toegevoegd. Nu heb je semantisch gezien een tab in een tab. (Als je hem niet op 1 van de 2 plekken verwijdert).
Ik denk dat de originele tabs-opzet binnen deze package eigenlijk niet ideaal is. Normaliter is de tablist hetgeen wat de tabjes representeert. En onder de tablist heb je dan verschillende tabpanelen.
Hier zitten de tabpanelen genest in de tablist.
Zie ook:
En op deze pagina zie je een standaard implementatie die eigenlijk gevolgd zou moeten worden:
https://www.w3.org/WAI/ARIA/apg/patterns/tabs/examples/tabs-manual/#tablist-1
Overigens ben ik niet getrouwd met het idee dat dit perse een tablist moet blijven, het kan waarschijnlijk ook versimpeld worden naar een lijst zoals je vaak ziet bij FAQ-items.
There was a problem hiding this comment.
Volgens mij vind ik dit niet goed, ik heb dit lokaal bekeken en ik lijk nu te zien dat een element met de rol tabpanel genest is in een element met de rol tab. Dat is niet de bedoeling.
Het is in principe okay om pragmatisch te werk te gaan, maar ik ben er ook geen fan van om wijzigingen te doen om Lighthouse maar tevreden te stellen. Al zouden we het daarvoor doen, dan denk ik dat we hier niet zoveel mee opschieten, want Lighthouse lijkt nu ook nog steeds toegankelijkheidsfouten te rapporteren.
Mocht ik dit verkeerd hebben gezien, laat het me vooral weten. Als dit te maken heeft met beschikbare tijd, dan zou ik liever zien dat we dit op een tijdstip oppakken wanneer we met een gedegen oplossing kunnen komen.
Fixes the aria issues that were detected with lighthouse