From 0cbc58f8d1ecc87dd904a6e8ce2bb42e918ac167 Mon Sep 17 00:00:00 2001
From: Josh Knapp <jknapp85@gmail.com>
Date: Sun, 12 Jul 2026 18:30:22 -0700
Subject: [PATCH] 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>
---
 .../components/basic/Navbar.toHtml.test.ts    | 68 +++++++++++++++++--
 craft/src/components/basic/Navbar.tsx         | 37 +++++++---
 .../sections/Gallery.toHtml.test.ts           | 30 ++++++++
 craft/src/components/sections/Gallery.tsx     | 32 ++++++++-
 4 files changed, 148 insertions(+), 19 deletions(-)

diff --git a/craft/src/components/basic/Navbar.toHtml.test.ts b/craft/src/components/basic/Navbar.toHtml.test.ts
index a9aca89..cc27a6a 100644
--- a/craft/src/components/basic/Navbar.toHtml.test.ts
+++ b/craft/src/components/basic/Navbar.toHtml.test.ts
@@ -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>{';
diff --git a/craft/src/components/basic/Navbar.tsx b/craft/src/components/basic/Navbar.tsx
index 6c789c8..7eaf233 100644
--- a/craft/src/components/basic/Navbar.tsx
+++ b/craft/src/components/basic/Navbar.tsx
@@ -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>`,
diff --git a/craft/src/components/sections/Gallery.toHtml.test.ts b/craft/src/components/sections/Gallery.toHtml.test.ts
index 23d617a..1f55cbb 100644
--- a/craft/src/components/sections/Gallery.toHtml.test.ts
+++ b/craft/src/components/sections/Gallery.toHtml.test.ts
@@ -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\(\)/);
+  });
+});
diff --git a/craft/src/components/sections/Gallery.tsx b/craft/src/components/sections/Gallery.tsx
index c1c99c4..035d699 100644
--- a/craft/src/components/sections/Gallery.tsx
+++ b/craft/src/components/sections/Gallery.tsx
@@ -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">&times;</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>`;
   }