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

#709: Lighthouse check: Cookiebanner ARIA #40

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

#709: Lighthouse check: Cookiebanner ARIA #40

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7499634
Update roles
CountNick Jul 29, 2024
bba5f4b
Updat aria attributes
CountNick Jul 29, 2024
2af0d39
Update role
CountNick Jul 31, 2024
6873962
Update tabs
CountNick Aug 20, 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
11 changes: 4 additions & 7 deletions src/cookie-consent.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,7 @@ export default class Dialog extends HTMLElement {
saveButtonText: Config().get("labels.aria.button"),
defaultButtonLabel: Config().get("labels.button.default"),
acceptAllButton:
Config().get("acceptAllButton")
&& !Preferences().hasPreferences(),
Config().get("acceptAllButton") && !Preferences().hasPreferences(),
};
// custom content from data-attributes
const customContent = {
Expand Down Expand Up @@ -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">
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Screenshot 2024-07-31 at 13 44 03

<!--googleoff: all-->
<header part="${this.config.prefix}__header" class="${this.config.prefix}__header" id="${this.config.prefix}-description">
<h1 part="${this.config.prefix}__title">${this.data.title}</h1>
<h1 part="${this.config.prefix}__title" id="${this.config.prefix}__title">${this.data.title}</h1>
${this.data.description}
</header>
<form part="${this.config.prefix}__form">
Expand Down Expand Up @@ -186,9 +185,7 @@ export default class Dialog extends HTMLElement {
const checkedCount = values.filter((v) => v.accepted).length;
const userOptionsChecked = checkedCount >= requiredCount;
if (
this.data.acceptAllButton
&& this.config.type === "checkbox"
&& !userOptionsChecked
this.data.acceptAllButton && this.config.type === "checkbox" && !userOptionsChecked
) {
return values.map((value) => ({
...value,
Expand Down
7 changes: 4 additions & 3 deletions src/dialog-tablist.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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">
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Screenshot 2024-07-31 at 13 48 25

Copy link
Member

Choose a reason for hiding this comment

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

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:

image

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MicheleNL Deze kan wat mij betreft nu inderdaad weer worden bekeken :)

<header part="${PREFIX}__tab" class="${PREFIX}__tab">
<label part="${PREFIX}__option" class="${PREFIX}__option" data-required="${required}">
<input
Expand All @@ -57,9 +57,10 @@ const DialogTabList = (cookieInformation) => {
part="${PREFIX}__tab-toggle"
class="${PREFIX}__tab-toggle"
role="tab"
id="${PREFIX}-tab-${index}"
id="${PREFIX}-tab"
href="#${PREFIX}-tabpanel-${index}"
aria-controls="${PREFIX}-tabpanel-${index}"
aria-owns="${PREFIX}__tab-list"
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

aria-selected="false"
aria-label="${Config().get("labels.aria.tabToggle")}">
<svg part="${PREFIX}__tab-toggle-icon" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 25 16"><path d="M21.5.5l3 3.057-12 11.943L.5 3.557 3.5.5l9 9z"/></svg>
Expand Down Expand Up @@ -95,7 +96,7 @@ const DialogTabList = (cookieInformation) => {
: undefined,
}));
return `
<ul part="${PREFIX}__tab-list" class="${PREFIX}__tab-list" role="tablist" aria-label="${Config().get("labels.aria.tabList")}">
<ul part="${PREFIX}__tab-list" id="${PREFIX}__tab-list" class="${PREFIX}__tab-list" role="tablist" aria-owns="${PREFIX}-tab" aria-label="${Config().get("labels.aria.tabList")}">
${cookiesWithState.map(renderTab).join("")}
</ul>
`;
Expand Down
Loading