Feat: add a11y implementation#55
Conversation
… banner-type assumption Remove the always-true `if (bannerType !== "classic")` wrapper that was left behind after the early-return guard above it. The two lines of banner-focus-loop setup now run unconditionally for all non-classic types. Add a beforeAll in the focus-loop describe block that explicitly sets faz_banner_type to 'box' via setOption, making the test type-specific and proving the fix covers the previously excluded box banner type.
…ocument MutationObserver lifecycle
WalkthroughI cambiamenti introducono un sistema completo di accessibilità (a11y) per il banner dei cookie, aggiungendo un nuovo modulo JavaScript che implementa miglioramenti ARIA, gestione della tastiera (Escape, focus loop), e sincronizzazione dinamica degli attributi semantici, insieme a test end-to-end dedicati. Changes
Sequence Diagram(s)sequenceDiagram
participant Event as Banner Load Event
participant A11y as A11y Module
participant DOM as DOM Elements
participant Keyboard as Keyboard Handler
Event->>A11y: fazcookie_banner_loaded
A11y->>DOM: Restructure headings & IDs
A11y->>DOM: Add ARIA roles & attributes
A11y->>DOM: Wrap accordion in heading tags
A11y->>Keyboard: Attach Escape key listeners
A11y->>Keyboard: Setup focus loop handlers
A11y->>DOM: Sync aria-labels on checkbox state
Keyboard->>DOM: Close banner/modal on Escape/Tab
DOM->>DOM: Update aria-label on state change
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minuti Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
YvetteNikolov
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/js/a11y.js`:
- Around line 131-135: The addDescriptionWrapperId function (and the similar
blocks at the other ranges) only targets detail-* / show-desc-button /
hide-desc-button selectors, so CCPA/opt-out elements (optout-description,
optout-close, optout-cancel-button, optout-show-desc-button,
optout-hide-desc-button) are omitted; update addDescriptionWrapperId and the
other related routines in frontend/js/a11y.js to also query and handle elements
with the optout-* IDs/classes, set the same id/aria-controls values (e.g.,
ensure wrapper id 'faz-desc-content' is applied to optout-description too), and
include the optout-close/optout-cancel-button in the Escape-key close handler so
aria-controls and Escape-based closing behavior mirror the existing detail-*
flow.
In `@tests/e2e/global-setup.ts`:
- Around line 13-21: The preflight uses WP_LOGIN_PATH via the local variable
loginPath but other harness code still hardcodes '/wp-login.php', causing
misleading errors; create a small shared utility (e.g., getWpLoginPath or
wpLoginPath constant) used everywhere instead of the literal string, update the
fixtures that call loginAsAdmin to import and use that utility, and change the
thrown Error message that currently interpolates '/wp-login.php' to use the
loginPath variable so diagnostics reflect the actual path.
In `@tests/e2e/specs/a11y.spec.ts`:
- Around line 151-152: L'asserzione usa token inglesi fissi e fallisce con le
traduzioni; invece di expect(label).toMatch(/enabled|disabled/i) recupera
l'atteso localizzato da fazA11yConfig (le stringhe in
frontend/class-frontend.php) oppure verifica il nome della categoria più il
cambiamento di stato (es. leggi il label prima dell'azione, esegui il toggle su
checkbox.getAttribute('aria-label' o textContent) e confronta che lo stato è
invertito), aggiornando il test per usare i valori localizzati o la logica di
confronto stato/categoria invece di "enabled|disabled".
- Around line 175-181: The test dependency on fixture copy causes flakiness:
update the a11y.spec.ts test that checks the show-desc-button (selector
'[data-faz-tag="show-desc-button"]') so it no longer assumes the control always
exists; either (A) ensure the test setup injects a deliberately long banner
fixture so _fazSetShowMoreLess() will create the control, or (B) make the
assertion conditional by first checking for the presence/visibility of the
show-desc-button and only asserting its aria-controls when it exists (and
otherwise assert that the banner is short/unchanged). Modify the test setup or
the assertion flow around the show-desc-button locator accordingly.
- Around line 69-78: The test uses only button elements while the runtime
_fazLoopFocus() targets the first/last focusable among links, buttons and
elements with tabindex, so change the test to collect focusable elements the
same way _fazLoopFocus() does (use notice.locator with the same selector used in
script.js), then focus the last item from that collection and assert Tab cycles
to the first element; update references to
buttons.first()/buttons.last()/buttons accordingly so the test mirrors the
runtime behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 065a4b73-ef58-491e-9686-ebf1eee273d1
📒 Files selected for processing (7)
admin/modules/banners/includes/templates/6.0.0/template.jsonadmin/modules/banners/includes/templates/6.2.0/template.jsonfrontend/class-frontend.phpfrontend/js/a11y.jsfrontend/js/script.jstests/e2e/global-setup.tstests/e2e/specs/a11y.spec.ts
| function addDescriptionWrapperId() { | ||
| var wrapper = document.querySelector( '[data-faz-tag="detail-description"]' ); | ||
| if ( ! wrapper ) return; | ||
| wrapper.setAttribute( 'id', 'faz-desc-content' ); | ||
| } |
There was a problem hiding this comment.
Il popup CCPA/opt-out resta fuori da queste correzioni runtime.
Qui usi solo i selettori detail-* / show-desc-button / hide-desc-button, ma frontend/js/script.js e gli shortcode preparati in frontend/class-frontend.php gestiscono anche optout-description, optout-close, optout-cancel-button, optout-show-desc-button e optout-hide-desc-button. Nel flusso CCPA non verranno quindi applicati né aria-controls né la chiusura con Escape.
💡 Possibile correzione
- var wrapper = document.querySelector( '[data-faz-tag="detail-description"]' );
+ var wrapper = document.querySelector(
+ '[data-faz-tag="detail-description"], [data-faz-tag="optout-description"]'
+ );
if ( ! wrapper ) return;
wrapper.setAttribute( 'id', 'faz-desc-content' );
@@
- var closeBtn = modal.querySelector( '[data-faz-tag="detail-close"]' );
+ var closeBtn = modal.querySelector(
+ '[data-faz-tag="detail-close"], [data-faz-tag="optout-close"], [data-faz-tag="optout-cancel-button"]'
+ );
if ( closeBtn ) {
closeBtn.click();
}
@@
- var btn = wrapper.querySelector( '[data-faz-tag="show-desc-button"], [data-faz-tag="hide-desc-button"]' );
+ var btn = wrapper.querySelector(
+ '[data-faz-tag="show-desc-button"], [data-faz-tag="hide-desc-button"], [data-faz-tag="optout-show-desc-button"], [data-faz-tag="optout-hide-desc-button"]'
+ );
if ( btn ) btn.setAttribute( 'aria-controls', 'faz-desc-content' );Also applies to: 177-186, 231-245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/js/a11y.js` around lines 131 - 135, The addDescriptionWrapperId
function (and the similar blocks at the other ranges) only targets detail-* /
show-desc-button / hide-desc-button selectors, so CCPA/opt-out elements
(optout-description, optout-close, optout-cancel-button,
optout-show-desc-button, optout-hide-desc-button) are omitted; update
addDescriptionWrapperId and the other related routines in frontend/js/a11y.js to
also query and handle elements with the optout-* IDs/classes, set the same
id/aria-controls values (e.g., ensure wrapper id 'faz-desc-content' is applied
to optout-description too), and include the optout-close/optout-cancel-button in
the Escape-key close handler so aria-controls and Escape-based closing behavior
mirror the existing detail-* flow.
| const loginPath = process.env.WP_LOGIN_PATH ?? '/wp-login.php'; | ||
| const loginPage = await api.get(loginPath); | ||
| if (!loginPage.ok()) { | ||
| await api.dispose(); | ||
| throw new Error(`WordPress login page not reachable at ${baseURL}/wp-login.php (status ${loginPage.status()}).`); | ||
| } | ||
|
|
||
| // Verify credentials actually work before running the full suite. | ||
| const loginResponse = await api.post('/wp-login.php', { | ||
| const loginResponse = await api.post(loginPath, { |
There was a problem hiding this comment.
WP_LOGIN_PATH non è ancora propagato a tutto l'harness E2E.
Qui il preflight usa il nuovo env, ma tests/e2e/fixtures/wp-fixture.ts continua a navigare verso /wp-login.php e questo errore stampa ancora quel path hardcoded. Su Bedrock il setup può passare, ma gli spec che chiamano loginAsAdmin() continueranno a rompersi con diagnostica fuorviante. Porterei il path in una utility condivisa e userei loginPath anche nel messaggio di errore.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/global-setup.ts` around lines 13 - 21, The preflight uses
WP_LOGIN_PATH via the local variable loginPath but other harness code still
hardcodes '/wp-login.php', causing misleading errors; create a small shared
utility (e.g., getWpLoginPath or wpLoginPath constant) used everywhere instead
of the literal string, update the fixtures that call loginAsAdmin to import and
use that utility, and change the thrown Error message that currently
interpolates '/wp-login.php' to use the loginPath variable so diagnostics
reflect the actual path.
| // Collect all visible, non-disabled focusable elements in the notice. | ||
| const buttons = notice.locator('button:not([disabled])'); | ||
| await expect(buttons.first()).toBeVisible(); | ||
|
|
||
| // Focus the last button in the banner. | ||
| await buttons.last().focus(); | ||
|
|
||
| // Tab should loop back to the first button. | ||
| await page.keyboard.press('Tab'); | ||
| await expect(buttons.first()).toBeFocused(); |
There was a problem hiding this comment.
Il test del focus loop non usa lo stesso set di focusable del runtime.
_fazLoopFocus() in frontend/js/script.js si aggancia al primo/ultimo focusable tra link, button e [tabindex]; qui invece consideri solo i button. Se l'ultimo focusable reale del banner è un link o un elemento custom, questa asserzione fallisce o passa per il motivo sbagliato.
💡 Possibile correzione
- const buttons = notice.locator('button:not([disabled])');
- await expect(buttons.first()).toBeVisible();
+ const focusables = notice.locator(
+ 'a:not([disabled]), button:not([disabled]), [tabindex]:not([disabled]):not([tabindex="-1"])'
+ );
+ await expect(focusables.first()).toBeVisible();
@@
- await buttons.last().focus();
+ await focusables.last().focus();
@@
- await expect(buttons.first()).toBeFocused();
+ await expect(focusables.first()).toBeFocused();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/specs/a11y.spec.ts` around lines 69 - 78, The test uses only button
elements while the runtime _fazLoopFocus() targets the first/last focusable
among links, buttons and elements with tabindex, so change the test to collect
focusable elements the same way _fazLoopFocus() does (use notice.locator with
the same selector used in script.js), then focus the last item from that
collection and assert Tab cycles to the first element; update references to
buttons.first()/buttons.last()/buttons accordingly so the test mirrors the
runtime behavior.
| const label = await checkbox.getAttribute('aria-label'); | ||
| expect(label).toMatch(/enabled|disabled/i); |
There was a problem hiding this comment.
Questa asserzione rompe appena le traduzioni sono attive.
Le stringhe arrivano da fazA11yConfig e sono localizzate in frontend/class-frontend.php, quindi /enabled|disabled/i funziona solo su installazioni in inglese. Qui conviene derivare l'atteso dalla config localizzata oppure verificare il nome categoria e il cambio di stato, non i token inglesi.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/specs/a11y.spec.ts` around lines 151 - 152, L'asserzione usa token
inglesi fissi e fallisce con le traduzioni; invece di
expect(label).toMatch(/enabled|disabled/i) recupera l'atteso localizzato da
fazA11yConfig (le stringhe in frontend/class-frontend.php) oppure verifica il
nome della categoria più il cambiamento di stato (es. leggi il label prima
dell'azione, esegui il toggle su checkbox.getAttribute('aria-label' o
textContent) e confronta che lo stato è invertito), aggiornando il test per
usare i valori localizzati o la logica di confronto stato/categoria invece di
"enabled|disabled".
| test('show-more button has aria-controls="faz-desc-content"', async ({ page }) => { | ||
| await page.goto('/', { waitUntil: 'domcontentloaded' }); | ||
| await expect(page.locator('[data-faz-tag="notice"]')).toBeVisible(); | ||
| await page.locator('[data-faz-tag="settings-button"]').first().click(); | ||
|
|
||
| const showMoreBtn = page.locator('[data-faz-tag="show-desc-button"]'); | ||
| await expect(showMoreBtn).toHaveAttribute('aria-controls', 'faz-desc-content'); |
There was a problem hiding this comment.
Il test su show-desc-button dipende dal copy del fixture, non dalla fix a11y.
_fazSetShowMoreLess() crea quel controllo solo quando la descrizione supera la soglia ed è divisa in più paragrafi. Se il banner di test cambia lunghezza o markup, la suite fallirà anche con a11y.js corretto. Meglio fissare esplicitamente un banner lungo nel setup o rendere l'asserzione condizionata alla presenza del controllo.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/specs/a11y.spec.ts` around lines 175 - 181, The test dependency on
fixture copy causes flakiness: update the a11y.spec.ts test that checks the
show-desc-button (selector '[data-faz-tag="show-desc-button"]') so it no longer
assumes the control always exists; either (A) ensure the test setup injects a
deliberately long banner fixture so _fazSetShowMoreLess() will create the
control, or (B) make the assertion conditional by first checking for the
presence/visibility of the show-desc-button and only asserting its aria-controls
when it exists (and otherwise assert that the banner is short/unchanged). Modify
the test setup or the assertion flow around the show-desc-button locator
accordingly.
|
Hey @YvetteNikolov, great work on this! The design spec and implementation plan are really thorough, and the A few notes on overlap with PR #53 (which I'm about to merge): Already implemented in #53These fixes are already in my branch and will be in
Not in #53 — unique value from your PRThese are improvements we don't have and would love to integrate:
Suggested next stepI'm merging #53 now. Could you rebase your branch on the updated
Happy to help with the rebase if you need it. Really appreciate the contribution from Yard — this is exactly the WCAG work Wybe mentioned was coming! |
2 participants
Hello!
I am also a developer at Yard (https://yard.nl/). We manage a big number of municipality websites that undergo mandatory WCAG 2.2 audits. Until now we used CookieYes and maintained a separate npm package that includes accessibility fixes gathered from those audit reports.
We would like to migrate to your plugin. We'd need these improvements, hence this PR. What's changed:
a11y.js(big) - new file that applies all ARIA improvements at runtime afterfazcookie_banner_loadedscript.js(small) - focus loop now applies to box-type banners, not just popup.template.json(small) - minor CSS normalisation: heading margin-bottom and font-sizeWP_LOGIN_PATH.env variable because our Bedrock setup prepends/wp/to the login URLInitially, the structural fixes were applied by a dedicated PHP class at template-build time (9b66da4), baked into the cached template in the options table. I later moved everything into a11y.js (26a6ab1) so all accessibility logic lives in one place and is centrally maintained.
Summary by CodeRabbit
Release Notes
New Features
Tests