a11y/security: scope Navbar ids + hover styles, Gallery lightbox focus trap
M-1: Navbar.toHtml emitted a fixed id="navbar-links" and unscoped .navbar-link/.navbar-cta :hover selectors -- two Navbars on one page collided on the duplicate id and cross-applied each other's hover colors (later <style> block wins in the cascade). Scope both on the Craft node id via scopeId(), matching the Menu/Tabs pattern: the links container gets a unique id, aria-controls/the hamburger toggle script reference it, and the hover rules are prefixed with a per-instance class on the <nav> root. M-2: Gallery lightbox had no focus management -- opening it left focus wherever it was (behind the now-visible overlay) and closing it never restored it. The inline script now stashes document.activeElement on open, moves focus to a new accessible close button, traps Tab on the close button while the dialog is open, and restores the saved focus on close (Escape, backdrop click, or the close button). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -5,28 +5,84 @@ const toHtml = (Navbar as any).toHtml;
|
||||
|
||||
describe('Navbar.toHtml hamburger accessibility (F2.3)', () => {
|
||||
test('mobile toggle button has an accessible name, aria-expanded, and aria-controls', () => {
|
||||
const { html } = toHtml({ showMobileMenu: true }, '');
|
||||
const { html } = toHtml({ showMobileMenu: true }, '', 'node-nav1');
|
||||
expect(html).toMatch(/class="navbar-hamburger"[^>]*aria-label="Toggle navigation menu"/);
|
||||
expect(html).toMatch(/aria-expanded="false"/);
|
||||
expect(html).toMatch(/aria-controls="navbar-links"/);
|
||||
expect(html).toMatch(/aria-controls="[^"]+"/);
|
||||
});
|
||||
|
||||
test('aria-controls target id exists on the links container', () => {
|
||||
const { html } = toHtml({ showMobileMenu: true }, '');
|
||||
expect(html).toContain('id="navbar-links"');
|
||||
const { html } = toHtml({ showMobileMenu: true }, '', 'node-nav1');
|
||||
const controls = html.match(/aria-controls="([^"]+)"/)![1];
|
||||
expect(html).toContain(`id="${controls}"`);
|
||||
});
|
||||
|
||||
test('toggle script flips aria-expanded on click', () => {
|
||||
const { html } = toHtml({ showMobileMenu: true }, '');
|
||||
const { html } = toHtml({ showMobileMenu: true }, '', 'node-nav1');
|
||||
expect(html).toMatch(/setAttribute\(['"]aria-expanded['"]/);
|
||||
});
|
||||
|
||||
test('no mobile menu: no hamburger button emitted', () => {
|
||||
const { html } = toHtml({ showMobileMenu: false }, '');
|
||||
const { html } = toHtml({ showMobileMenu: false }, '', 'node-nav1');
|
||||
expect(html).not.toContain('navbar-hamburger');
|
||||
});
|
||||
});
|
||||
|
||||
describe('Navbar.toHtml node-scoped ids/hover styles (M-1: two navbars must not collide)', () => {
|
||||
test('no bare unscoped id="navbar-links" is emitted', () => {
|
||||
const { html } = toHtml({ showMobileMenu: true }, '', 'node-nav1');
|
||||
expect(html).not.toContain('id="navbar-links"');
|
||||
});
|
||||
|
||||
test('two different node ids produce different links-container ids', () => {
|
||||
const { html: html1 } = toHtml({ showMobileMenu: true }, '', 'node-nav1');
|
||||
const { html: html2 } = toHtml({ showMobileMenu: true }, '', 'node-nav2');
|
||||
const id1 = html1.match(/id="([^"]+)"/)![1];
|
||||
const id2 = html2.match(/id="([^"]+)"/)![1];
|
||||
expect(id1).not.toBe(id2);
|
||||
});
|
||||
|
||||
test('aria-controls always equals the actual links-container id', () => {
|
||||
const { html } = toHtml({ showMobileMenu: true }, '', 'node-nav1');
|
||||
const controls = html.match(/aria-controls="([^"]+)"/)![1];
|
||||
const linksId = html.match(/id="([^"]+)"/)![1];
|
||||
expect(controls).toBe(linksId);
|
||||
});
|
||||
|
||||
test('hover style selectors are scoped per-instance, not bare .navbar-link/.navbar-cta', () => {
|
||||
const { html } = toHtml({ hoverColor: '#ff0000' }, '', 'node-nav1');
|
||||
// A selector rule that STARTS the line with .navbar-link:hover (i.e. not
|
||||
// preceded by a per-instance ancestor class) would be the old, unscoped,
|
||||
// globally-colliding form.
|
||||
expect(html).not.toMatch(/^\s*\.navbar-link:hover/m);
|
||||
expect(html).not.toMatch(/^\s*\.navbar-cta:hover/m);
|
||||
// still present, just scoped under a per-instance ancestor class
|
||||
expect(html).toMatch(/\.navbar-link:hover/);
|
||||
expect(html).toMatch(/\.[\w-]+ \.navbar-link:hover/);
|
||||
});
|
||||
|
||||
test('two navbars with different hoverColor do not leak style onto each other (scoped selectors differ)', () => {
|
||||
const { html: html1 } = toHtml({ hoverColor: '#ff0000' }, '', 'node-nav1');
|
||||
const { html: html2 } = toHtml({ hoverColor: '#00ff00' }, '', 'node-nav2');
|
||||
const scope1 = html1.match(/<style>\s*\.([\w-]+)\s/)![1];
|
||||
const scope2 = html2.match(/<style>\s*\.([\w-]+)\s/)![1];
|
||||
expect(scope1).not.toBe(scope2);
|
||||
expect(html1).toContain(`.${scope1} .navbar-link:hover`);
|
||||
expect(html2).toContain(`.${scope2} .navbar-link:hover`);
|
||||
});
|
||||
|
||||
test('a normal single navbar still renders its hover style (visual output preserved)', () => {
|
||||
const { html } = toHtml({ hoverColor: '#ff0000' }, '', 'node-nav1');
|
||||
expect(html).toMatch(/:hover\s*\{\s*color:\s*#ff0000/);
|
||||
});
|
||||
|
||||
test('same node id -> identical output across calls (deterministic)', () => {
|
||||
const { html: html1 } = toHtml({ showMobileMenu: true }, '', 'node-nav1');
|
||||
const { html: html2 } = toHtml({ showMobileMenu: true }, '', 'node-nav1');
|
||||
expect(html1).toBe(html2);
|
||||
});
|
||||
});
|
||||
|
||||
describe('Navbar.toHtml XSS hardening (hoverColor/backgroundColor/ctaColor into <style>)', () => {
|
||||
test('a hoverColor value containing </style><script> is neutralized in the hover <style> block', () => {
|
||||
const malicious = '#fff}</style><script>alert(1)</script><style>{';
|
||||
|
||||
@@ -2,7 +2,7 @@ import React, { CSSProperties, useState } from 'react';
|
||||
import { useNode, UserComponent } from '@craftjs/core';
|
||||
import { cssPropsToString } from '../../utils/style-helpers';
|
||||
import { useSiteDesign } from '../../state/SiteDesignContext';
|
||||
import { escapeHtml, escapeAttr, safeUrl, cssValue } from '../../utils/escape';
|
||||
import { escapeHtml, escapeAttr, safeUrl, cssValue, scopeId } from '../../utils/escape';
|
||||
|
||||
/* ---------- Types ---------- */
|
||||
|
||||
@@ -210,7 +210,7 @@ Navbar.craft = {
|
||||
|
||||
/* ---------- HTML export ---------- */
|
||||
|
||||
(Navbar as any).toHtml = (props: NavbarProps, _childrenHtml: string) => {
|
||||
(Navbar as any).toHtml = (props: NavbarProps, _childrenHtml: string, nodeId?: string) => {
|
||||
// Sanitized once here -- these are raw string-interpolation sinks below
|
||||
// (hoverCol/bgColor go into a <style> block, the worst case: </style>
|
||||
// breakout -> arbitrary <script>), see task-cssxss-brief.md.
|
||||
@@ -224,6 +224,19 @@ Navbar.craft = {
|
||||
const sticky = props.isSticky;
|
||||
const mobile = props.showMobileMenu;
|
||||
const logoUrl = props.logoUrl || '/';
|
||||
const links0 = props.links || defaultLinks;
|
||||
|
||||
// M-1: deterministic AND unique per-instance scope, keyed on the Craft
|
||||
// node id. Two Navbars on the same page previously emitted an identical
|
||||
// fixed id="navbar-links" (invalid duplicate-id HTML, ambiguous
|
||||
// aria-controls target) and unscoped `.navbar-link:hover`/`.navbar-cta:hover`
|
||||
// rules in each instance's own <style> block -- since both blocks target
|
||||
// the SAME global selector, the later one in the DOM silently overrides
|
||||
// the earlier one's hover color/behavior for BOTH navbars. Scoping the
|
||||
// links-container id and adding a per-instance class on the <nav> root
|
||||
// (used to prefix the hover selectors) eliminates both collisions.
|
||||
const scope = scopeId(nodeId, JSON.stringify(links0) + alignment + pad, 'nav');
|
||||
const linksId = `${scope}_links`;
|
||||
|
||||
const navStyle = cssPropsToString({
|
||||
display: 'flex',
|
||||
@@ -272,21 +285,23 @@ Navbar.craft = {
|
||||
// via aria-expanded, kept in sync with the .navbar-open class by the
|
||||
// inline onclick handler.
|
||||
const hamburgerHtml = mobile
|
||||
? `\n <button class="navbar-hamburger" aria-label="Toggle navigation menu" aria-expanded="false" aria-controls="navbar-links" onclick="var m=this.parentElement.querySelector('.navbar-links');var open=m.classList.toggle('navbar-open');this.setAttribute('aria-expanded', open ? 'true' : 'false');" style="display:none;background:none;border:none;cursor:pointer;padding:4px;flex-direction:column;gap:4px">
|
||||
? `\n <button class="navbar-hamburger" aria-label="Toggle navigation menu" aria-expanded="false" aria-controls="${escapeAttr(linksId)}" onclick="var m=document.getElementById('${linksId}');var open=m.classList.toggle('navbar-open');this.setAttribute('aria-expanded', open ? 'true' : 'false');" style="display:none;background:none;border:none;cursor:pointer;padding:4px;flex-direction:column;gap:4px">
|
||||
<span style="display:block;width:24px;height:2px;background-color:${escapeAttr(textCol)}"></span>
|
||||
<span style="display:block;width:24px;height:2px;background-color:${escapeAttr(textCol)}"></span>
|
||||
<span style="display:block;width:24px;height:2px;background-color:${escapeAttr(textCol)}"></span>
|
||||
</button>`
|
||||
: '';
|
||||
|
||||
// Hover CSS
|
||||
// Hover CSS -- scoped under `.${scope}` (a class on the <nav> root, added
|
||||
// below) so it can only ever match THIS instance's links/CTA, never bleed
|
||||
// into or get overridden by another Navbar instance's rules.
|
||||
const hoverCss = `<style>
|
||||
.navbar-link:hover { color: ${hoverCol} !important; }
|
||||
.navbar-cta:hover { filter: brightness(1.1); }${mobile ? `
|
||||
.${scope} .navbar-link:hover { color: ${hoverCol} !important; }
|
||||
.${scope} .navbar-cta:hover { filter: brightness(1.1); }${mobile ? `
|
||||
@media (max-width: 768px) {
|
||||
.navbar-hamburger { display: flex !important; }
|
||||
.navbar-links { display: none !important; position: absolute; top: 100%; left: 0; right: 0; flex-direction: column !important; background-color: ${bgColor}; padding: 12px 24px; gap: 12px !important; box-shadow: 0 4px 12px rgba(0,0,0,0.1); }
|
||||
.navbar-links.navbar-open { display: flex !important; }
|
||||
.${scope} .navbar-hamburger { display: flex !important; }
|
||||
.${scope} .navbar-links { display: none !important; position: absolute; top: 100%; left: 0; right: 0; flex-direction: column !important; background-color: ${bgColor}; padding: 12px 24px; gap: 12px !important; box-shadow: 0 4px 12px rgba(0,0,0,0.1); }
|
||||
.${scope} .navbar-links.navbar-open { display: flex !important; }
|
||||
}` : ''}
|
||||
</style>`;
|
||||
|
||||
@@ -309,9 +324,9 @@ Navbar.craft = {
|
||||
|
||||
return {
|
||||
html: `${hoverCss}
|
||||
<nav${navStyle ? ` style="${navStyle}${mobile ? ';position:relative' : ''}"` : ''}>
|
||||
<nav class="${scope}"${navStyle ? ` style="${navStyle}${mobile ? ';position:relative' : ''}"` : ''}>
|
||||
${logoHtml}${hamburgerHtml}
|
||||
<div class="navbar-links" id="navbar-links" style="display:flex;align-items:center;gap:24px">
|
||||
<div class="navbar-links" id="${linksId}" style="display:flex;align-items:center;gap:24px">
|
||||
${linksHtmlWithClass}
|
||||
</div>
|
||||
</nav>`,
|
||||
|
||||
@@ -93,3 +93,33 @@ describe('Gallery.toHtml deterministic + unique scope ids (thread node id, no Ma
|
||||
expect(html1).toBe(html2);
|
||||
});
|
||||
});
|
||||
|
||||
describe('Gallery.toHtml lightbox focus management (M-2)', () => {
|
||||
const props = { images: [{ src: '/a.jpg', alt: 'a' }], lightbox: true };
|
||||
|
||||
test('overlay includes a focusable close control with an accessible name and tabindex', () => {
|
||||
const { html } = toHtml(props, '', 'node-gal1');
|
||||
// A close control: a button (or the dialog container) with an accessible
|
||||
// name (aria-label) and an explicit tabindex so it's keyboard-focusable.
|
||||
expect(html).toMatch(/aria-label="[^"]*[Cc]lose[^"]*"[^>]*tabindex="-?\d+"|tabindex="-?\d+"[^>]*aria-label="[^"]*[Cc]lose[^"]*"/);
|
||||
});
|
||||
|
||||
test('script saves document.activeElement on open (for focus restore)', () => {
|
||||
const { html } = toHtml(props, '', 'node-gal1');
|
||||
expect(html).toMatch(/document\.activeElement/);
|
||||
});
|
||||
|
||||
test('script moves focus to the close control / dialog on open', () => {
|
||||
const { html } = toHtml(props, '', 'node-gal1');
|
||||
expect(html).toMatch(/\.focus\(\)/);
|
||||
});
|
||||
|
||||
test('script restores the previously-saved focus on close', () => {
|
||||
const { html } = toHtml(props, '', 'node-gal1');
|
||||
// The close function references a stored "last focused element" variable
|
||||
// and calls .focus() on it, not just moving focus INTO the dialog.
|
||||
const closeFnMatch = html.match(/function\s+\w+_close\s*\(\)\s*\{[^}]*\}/);
|
||||
expect(closeFnMatch).not.toBeNull();
|
||||
expect(closeFnMatch![0]).toMatch(/\.focus\(\)/);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -166,16 +166,37 @@ Gallery.craft = {
|
||||
let gridIdAttr = '';
|
||||
if (lightbox) {
|
||||
gridIdAttr = ` id="${galleryId}_grid"`;
|
||||
// M-2: focus management for the lightbox dialog.
|
||||
// - OPEN: stash `document.activeElement` (the thumbnail that triggered
|
||||
// the open) in a module-scoped var, then move focus onto the close
|
||||
// button -- so a screen-reader/keyboard user lands inside the dialog
|
||||
// instead of focus staying on (or silently falling back to <body>)
|
||||
// behind the now-visible overlay.
|
||||
// - Tab trap: while the overlay is open, every Tab keypress is
|
||||
// intercepted and refocuses the close button (the dialog's only
|
||||
// focusable control besides Escape/click-to-close), so focus can
|
||||
// never wander out into the page content hidden behind the overlay.
|
||||
// - CLOSE (Escape, backdrop click, or the close button): restore focus
|
||||
// to the element stashed on open.
|
||||
lightboxHtml = `
|
||||
<div id="${galleryId}_overlay" role="dialog" aria-modal="true" aria-label="Image preview" onclick="${galleryId}_close()" style="display:none;position:fixed;top:0;left:0;width:100%;height:100%;background:rgba(0,0,0,0.9);z-index:9999;justify-content:center;align-items:center;cursor:pointer">
|
||||
<button type="button" id="${galleryId}_closebtn" aria-label="Close preview" tabindex="-1" onclick="event.stopPropagation();${galleryId}_close()" style="position:absolute;top:16px;right:16px;width:36px;height:36px;border-radius:50%;border:none;background:rgba(255,255,255,0.15);color:#ffffff;font-size:20px;line-height:1;cursor:pointer;display:flex;align-items:center;justify-content:center">×</button>
|
||||
<img id="${galleryId}_img" src="" alt="" style="max-width:90%;max-height:90%;object-fit:contain;border-radius:8px" />
|
||||
</div>
|
||||
<script>
|
||||
function ${galleryId}_close(){document.getElementById('${galleryId}_overlay').style.display='none';}
|
||||
var ${galleryId}_lastFocus = null;
|
||||
function ${galleryId}_close(){
|
||||
document.getElementById('${galleryId}_overlay').style.display='none';
|
||||
if(${galleryId}_lastFocus && ${galleryId}_lastFocus.focus) ${galleryId}_lastFocus.focus();
|
||||
${galleryId}_lastFocus = null;
|
||||
}
|
||||
function ${galleryId}_open(src){
|
||||
${galleryId}_lastFocus = document.activeElement;
|
||||
var o = document.getElementById('${galleryId}_overlay');
|
||||
document.getElementById('${galleryId}_img').src = src;
|
||||
o.style.display = 'flex';
|
||||
var c = document.getElementById('${galleryId}_closebtn');
|
||||
if(c) c.focus();
|
||||
}
|
||||
document.getElementById('${galleryId}_grid').addEventListener('click', function(e){
|
||||
var t = e.target.closest('[data-lb-src]');
|
||||
@@ -190,7 +211,14 @@ document.getElementById('${galleryId}_grid').addEventListener('keydown', functio
|
||||
${galleryId}_open(t.getAttribute('data-lb-src'));
|
||||
});
|
||||
document.addEventListener('keydown', function(e){
|
||||
if(e.key==='Escape'){ ${galleryId}_close(); }
|
||||
var o = document.getElementById('${galleryId}_overlay');
|
||||
if(!o || o.style.display==='none') return;
|
||||
if(e.key==='Escape'){ ${galleryId}_close(); return; }
|
||||
if(e.key==='Tab'){
|
||||
e.preventDefault();
|
||||
var c = document.getElementById('${galleryId}_closebtn');
|
||||
if(c) c.focus();
|
||||
}
|
||||
});
|
||||
</script>`;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user