From 8029126ab7d3e29845daf277726d833553803ace Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 12 Jul 2026 14:35:12 -0700 Subject: [PATCH] deterministic + unique ids in 6 components (node-derived scope) Migrates Menu, ColumnLayout, Countdown, Gallery, Tabs, InputField, and TextareaField from Math.random()/content-hash scope ids to the threaded Craft node id (via scopeId()), keeping every emitted element id, aria-controls/aria-labelledby/for, and inline `, diff --git a/craft/src/components/sections/Gallery.toHtml.test.ts b/craft/src/components/sections/Gallery.toHtml.test.ts index c064c56..23d617a 100644 --- a/craft/src/components/sections/Gallery.toHtml.test.ts +++ b/craft/src/components/sections/Gallery.toHtml.test.ts @@ -61,3 +61,35 @@ describe('Gallery.toHtml lightbox accessibility (F1.3)', () => { expect(html).not.toContain('role="button"'); }); }); + +describe('Gallery.toHtml deterministic + unique scope ids (thread node id, no Math.random)', () => { + const props = { images: [{ src: '/a.jpg', alt: 'a' }], lightbox: true }; + + test('same node id -> identical output across calls (deterministic)', () => { + const { html: html1 } = toHtml(props, '', 'node-gal1'); + const { html: html2 } = toHtml(props, '', 'node-gal1'); + expect(html1).toBe(html2); + }); + + test('different node ids -> different, non-colliding gallery scope ids (identical images, no collision)', () => { + const { html: html1 } = toHtml(props, '', 'node-gal1'); + const { html: html2 } = toHtml(props, '', 'node-gal2'); + const id1 = html1.match(/id="([^"]+)_overlay"/)![1]; + const id2 = html2.match(/id="([^"]+)_overlay"/)![1]; + expect(id1).not.toBe(id2); + }); + + test('overlay/grid ids and the script function names all use the SAME scope', () => { + const { html } = toHtml(props, '', 'node-gal1'); + const scope = html.match(/id="([^"]+)_overlay"/)![1]; + expect(html).toContain(`id="${scope}_grid"`); + expect(html).toContain(`function ${scope}_close()`); + expect(html).toContain(`function ${scope}_open(`); + }); + + test('no nodeId (legacy 2-arg call): still deterministic across repeated calls, not random', () => { + const { html: html1 } = toHtml(props, ''); + const { html: html2 } = toHtml(props, ''); + expect(html1).toBe(html2); + }); +}); diff --git a/craft/src/components/sections/Gallery.tsx b/craft/src/components/sections/Gallery.tsx index 87dc18b..5504180 100644 --- a/craft/src/components/sections/Gallery.tsx +++ b/craft/src/components/sections/Gallery.tsx @@ -1,7 +1,7 @@ import React, { CSSProperties } from 'react'; import { useNode, UserComponent } from '@craftjs/core'; import { cssPropsToString } from '../../utils/style-helpers'; -import { escapeHtml, escapeAttr, safeUrl } from '../../utils/escape'; +import { escapeHtml, escapeAttr, safeUrl, scopeId } from '../../utils/escape'; interface GalleryImage { src: string; @@ -124,7 +124,7 @@ Gallery.craft = { /* ---------- HTML export ---------- */ -(Gallery as any).toHtml = (props: GalleryProps, _childrenHtml: string) => { +(Gallery as any).toHtml = (props: GalleryProps, _childrenHtml: string, nodeId?: string) => { const sectionStyle = cssPropsToString({ padding: '60px 20px', ...props.style, @@ -134,7 +134,11 @@ Gallery.craft = { const gap = props.gap || '16px'; const lightbox = props.lightbox || false; - const galleryId = 'gallery_' + Math.random().toString(36).slice(2, 8); + // Deterministic AND unique id, scoped on the Craft node id, for this + // gallery's overlay/grid element ids and inline-script function names -- + // so two Gallery instances (e.g. both left at default images) don't + // collide and end up sharing/clobbering one lightbox overlay. + const galleryId = scopeId(nodeId, JSON.stringify(images) + columns + gap, 'gallery'); const items = images.map((img) => { const caption = img.caption diff --git a/craft/src/components/sections/Tabs.toHtml.test.ts b/craft/src/components/sections/Tabs.toHtml.test.ts index 1503eb2..399ef46 100644 --- a/craft/src/components/sections/Tabs.toHtml.test.ts +++ b/craft/src/components/sections/Tabs.toHtml.test.ts @@ -51,3 +51,32 @@ describe('Tabs.toHtml accessibility (F1.2)', () => { expect(html).toMatch(/ArrowLeft/); }); }); + +describe('Tabs.toHtml deterministic + unique ids (thread node id, resolves id-collision finding)', () => { + test('same node id -> identical output across calls (deterministic, no Math.random)', () => { + const { html: html1 } = toHtml({ tabs }, '', 'node-tabs1'); + const { html: html2 } = toHtml({ tabs }, '', 'node-tabs1'); + expect(html1).toBe(html2); + }); + + test('two instances with IDENTICAL default tab content but different node ids do not collide', () => { + const { html: html1 } = toHtml({ tabs }, '', 'node-tabs1'); + const { html: html2 } = toHtml({ tabs }, '', 'node-tabs2'); + const id1 = html1.match(/role="tab"[^>]*aria-controls="([^"]+)"/)![1]; + const id2 = html2.match(/role="tab"[^>]*aria-controls="([^"]+)"/)![1]; + expect(id1).not.toBe(id2); + }); + + test('aria-controls still matches an existing panel id after the node-id change (internal consistency preserved)', () => { + const { html } = toHtml({ tabs }, '', 'node-tabs1'); + const controlsMatch = html.match(/role="tab"[^>]*aria-controls="([^"]+)"/); + expect(controlsMatch).toBeTruthy(); + expect(html).toContain(`id="${controlsMatch![1]}"`); + }); + + test('no nodeId (legacy 2-arg call): still deterministic across repeated calls, not random', () => { + const { html: html1 } = toHtml({ tabs }, ''); + const { html: html2 } = toHtml({ tabs }, ''); + expect(html1).toBe(html2); + }); +}); diff --git a/craft/src/components/sections/Tabs.tsx b/craft/src/components/sections/Tabs.tsx index 39df612..1231a8e 100644 --- a/craft/src/components/sections/Tabs.tsx +++ b/craft/src/components/sections/Tabs.tsx @@ -1,7 +1,7 @@ import React, { CSSProperties, useState } from 'react'; import { useNode, UserComponent } from '@craftjs/core'; import { cssPropsToString } from '../../utils/style-helpers'; -import { escapeHtml, escapeAttr } from '../../utils/escape'; +import { escapeHtml, escapeAttr, scopeId } from '../../utils/escape'; interface TabItem { label: string; @@ -124,20 +124,7 @@ Tabs.craft = { /* ---------- HTML export ---------- */ -/** - * Deterministic (non-cryptographic) string hash -- djb2 -- used to derive a - * stable id scope from existing prop data instead of Math.random/Date.now. - * Same input always produces the same output across export runs. - */ -function stableHash(seed: string): string { - let h = 5381; - for (let i = 0; i < seed.length; i++) { - h = ((h << 5) + h + seed.charCodeAt(i)) | 0; - } - return (h >>> 0).toString(36); -} - -(Tabs as any).toHtml = (props: TabsProps, _childrenHtml: string) => { +(Tabs as any).toHtml = (props: TabsProps, _childrenHtml: string, nodeId?: string) => { const sectionStyle = cssPropsToString({ padding: '60px 20px', ...props.style, @@ -151,13 +138,17 @@ function stableHash(seed: string): string { const contentBg = props.contentBg || '#ffffff'; // tabId scopes the functional wiring (onclick/getElementById) as well as - // the ARIA tab<->panel linking ids. It must be deterministic (no - // Math.random) because aria-controls/aria-labelledby need to reference - // the SAME id across repeated exports of the same content -- derived from - // anchorId when present, otherwise a stable hash of the tab labels (pure - // function of the props, not time/randomness). + // the ARIA tab<->panel linking ids. It must be BOTH deterministic (so + // aria-controls/aria-labelledby reference the SAME id across repeated + // exports of the same page) AND unique (so two Tabs instances with + // identical/default content -- e.g. both left at the default tab set -- + // don't collide and clobber each other's script globals / ARIA links). + // Scoping on the Craft node id gives both properties; when it's + // unavailable (legacy 2-arg call sites) we fall back to a stable hash of + // the tab content, matching the old (collision-prone but never random) + // behavior. const scopeSeed = props.anchorId || tabs.map((t) => t.label).join('|') + '::' + tabs.length; - const tabId = 'tabs_' + stableHash(scopeSeed); + const tabId = scopeId(nodeId, scopeSeed, 'tabs'); const tabButtons = tabs.map((tab, i) => { const isActive = i === 0;