From eca624ee9d58494088a71dab51a729cae2806144 Mon Sep 17 00:00:00 2001 From: "bert.hausmans" Date: Wed, 3 Jun 2026 04:12:27 +0200 Subject: [PATCH] fix: suppress PrimeVue default drawer close button to fix focus a11y MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up (crewli-reviewer). With header:'!hidden' the default close button was still mounted (display:none); PrimeVue Drawer.focus() falls back to focusing it (no [autofocus] in our slots), a no-op that strands keyboard focus behind the overlay. Set :show-close-icon="false" so the button is never created — focus() no-ops cleanly and v-focustrap focuses the visible brand-row X. Also: flip the previously tautological showCloseIcon test to assert the decision (toBe(false)); align the mobile close aria-label to English 'Close menu' (matching 'Collapse sidebar') for locale consistency. Co-Authored-By: Claude Opus 4.8 --- .../src/components-v2/layout/AppSidebar.vue | 29 +++++++++++++------ .../components-v2/layout/SidebarHeader.vue | 4 +-- .../layout/__tests__/AppSidebar.spec.ts | 9 +++--- .../layout/__tests__/SidebarHeader.spec.ts | 6 ++-- dev-docs/BACKLOG.md | 11 ++++--- 5 files changed, 37 insertions(+), 22 deletions(-) diff --git a/apps/app/src/components-v2/layout/AppSidebar.vue b/apps/app/src/components-v2/layout/AppSidebar.vue index 70e328d6..36cdddcc 100644 --- a/apps/app/src/components-v2/layout/AppSidebar.vue +++ b/apps/app/src/components-v2/layout/AppSidebar.vue @@ -115,15 +115,25 @@ const mobileVisible = computed({ render-fn) — the 3-component block is short and the duplication is preferable to the indirection of a dynamic component or slot wiring. - Close control (MOBILE-SHELL-PARITY defect 2): PrimeVue's default header - close-X is force-hidden via `header: '!hidden'`. Plain `hidden` lost to - PrimeVue's base `.p-drawer-header` display in stylesheet order, so the - default X leaked through and overlapped the brand row (the reported - defect). The important variant (`!hidden`, matching this file's existing - `!w-64`) wins. The mobile close affordance instead lives in - SidebarHeader's brand row, which renders an X ("Sluit menu") on mobile — - a single, non-overlapping control in the same top-right slot the desktop - sidebar uses for its collapse chevron. No duplicate, no overlap. + Close control (MOBILE-SHELL-PARITY defect 2): the mobile close affordance + lives in SidebarHeader's brand row, which renders an X ("Close menu") on + mobile — a single, non-overlapping control in the same top-right slot the + desktop sidebar uses for its collapse chevron. + + PrimeVue's OWN default header close-X is suppressed two ways, by design: + - `:show-close-icon="false"` stops PrimeVue mounting the default close + button at all. This is required for a11y, not just visuals: PrimeVue's + Drawer.focus() (no [autofocus] in our slots) falls back to focusing + `this.closeButton`; if that button is merely display:none (the earlier + `header: 'hidden'` approach), focus() is a no-op and keyboard focus is + stranded behind the overlay. With showCloseIcon=false the button is + never created, focus() no-ops cleanly, and v-focustrap focuses the + first focusable element — the visible brand-row X. + - `header: '!hidden'` then collapses the now-empty header band so it adds + no space above the content. The important variant beats PrimeVue's base + `.p-drawer-header` display (plain `hidden` lost to it in stylesheet + order — the original leak that let the default X overlap the brand row). + No duplicate control, no overlap, no stranded focus. Bottom anchoring (MOBILE-SHELL-PARITY defect 3 — WorkspaceSwitcher): the content pt is a full-height flex COLUMN (`flex flex-col h-full @@ -148,6 +158,7 @@ const mobileVisible = computed({ v-model:visible="mobileVisible" position="left" class="!w-64" + :show-close-icon="false" :pt="{ content: { class: 'flex flex-col p-0 overflow-hidden h-full min-h-0' }, header: { class: '!hidden' }, diff --git a/apps/app/src/components-v2/layout/SidebarHeader.vue b/apps/app/src/components-v2/layout/SidebarHeader.vue index eaa13e0e..32573901 100644 --- a/apps/app/src/components-v2/layout/SidebarHeader.vue +++ b/apps/app/src/components-v2/layout/SidebarHeader.vue @@ -133,7 +133,7 @@ function handleCollapseClick(): void { Desktop (>=lg): collapse chevron (◀) → toggleSidebar(). Mobile ( diff --git a/apps/app/src/components-v2/layout/__tests__/AppSidebar.spec.ts b/apps/app/src/components-v2/layout/__tests__/AppSidebar.spec.ts index d23ebcda..77f8dcb9 100644 --- a/apps/app/src/components-v2/layout/__tests__/AppSidebar.spec.ts +++ b/apps/app/src/components-v2/layout/__tests__/AppSidebar.spec.ts @@ -177,16 +177,17 @@ describe('AppSidebar', () => { expect(pt.content.class).toContain('min-h-0') }) - it('mobile: drawer does NOT suppress the close affordance via showCloseIcon=false', async () => { + it('mobile: drawer suppresses the PrimeVue default close button (showCloseIcon=false) to avoid a stranded focus target', async () => { mockIsMobileRef.value = true const wrapper = mountSidebar() await wrapper.vm.$nextTick() - // The mobile close control lives in SidebarHeader's brand row; the Drawer - // is left at its default showCloseIcon (never forced to false). - expect(wrapper.findComponent(DrawerStub).props('showCloseIcon')).not.toBe(false) + // showCloseIcon=false stops PrimeVue mounting the default close button, so + // Drawer.focus() never falls back to a display:none button and leaves focus + // stranded. The explicit close control is SidebarHeader's brand-row X. + expect(wrapper.findComponent(DrawerStub).props('showCloseIcon')).toBe(false) }) it('mobile: WorkspaceSwitcher renders inside the drawer', async () => { diff --git a/apps/app/src/components-v2/layout/__tests__/SidebarHeader.spec.ts b/apps/app/src/components-v2/layout/__tests__/SidebarHeader.spec.ts index fa071f84..7e789fb4 100644 --- a/apps/app/src/components-v2/layout/__tests__/SidebarHeader.spec.ts +++ b/apps/app/src/components-v2/layout/__tests__/SidebarHeader.spec.ts @@ -200,13 +200,13 @@ describe('SidebarHeader', () => { // header (defect 1) // ------------------------------------------------------------------------- - it('mobile: brand-row control is an explicit close (aria-label "Sluit menu", X icon)', () => { + it('mobile: brand-row control is an explicit close (aria-label "Close menu", X icon)', () => { mockIsMobileRef.value = true const wrapper = mountHeader() const btn = wrapper.find('button[aria-label]') - expect(btn.attributes('aria-label')).toBe('Sluit menu') + expect(btn.attributes('aria-label')).toBe('Close menu') expect(btn.find('.icon-stub').attributes('data-icon')).toBe('tabler-x') }) @@ -237,7 +237,7 @@ describe('SidebarHeader', () => { const buttons = wrapper.findAll('button[aria-label]') expect(buttons).toHaveLength(1) - expect(buttons[0].attributes('aria-label')).toBe('Sluit menu') + expect(buttons[0].attributes('aria-label')).toBe('Close menu') }) // P6-styling-fix — the brand row is ALWAYS left-aligned with constant diff --git a/dev-docs/BACKLOG.md b/dev-docs/BACKLOG.md index b67c18a3..7a858ca4 100644 --- a/dev-docs/BACKLOG.md +++ b/dev-docs/BACKLOG.md @@ -697,10 +697,13 @@ Resolved on branch `feat/mobile-shell-parity`: forces the header expanded on mobile. - **Extraneous X (defect 2):** PrimeVue's default drawer header X leaked through the non-important `header: 'hidden'` (it lost to the base `.p-drawer-header` - display in stylesheet order) and overlapped the brand row. Force-hidden with - `header: '!hidden'`; the mobile close affordance now lives in `SidebarHeader`'s - brand row (an X / "Sluit menu", reusing the existing handler) — a single - non-overlapping control mirroring the desktop collapse-chevron slot. + display in stylesheet order) and overlapped the brand row. Now suppressed at + the source with `:show-close-icon="false"` (also avoids an a11y trap: with the + button merely display:none, `Drawer.focus()` falls back to focusing it and + strands keyboard focus) plus `header: '!hidden'` to collapse the empty band. + The mobile close affordance lives in `SidebarHeader`'s brand row (an X / + "Close menu", reusing the existing handler) — a single non-overlapping control + mirroring the desktop collapse-chevron slot. - **WorkspaceSwitcher (defect 3):** static audit (code + `@primeuix` base CSS) showed the drawer content is already a correct full-height flex column (`.p-drawer-left .p-drawer-content { height:100% }` + `flex-grow:1`), so the