diff --git a/.changeset/shared-inspector-select.md b/.changeset/shared-inspector-select.md new file mode 100644 index 000000000..d4d055b52 --- /dev/null +++ b/.changeset/shared-inspector-select.md @@ -0,0 +1,5 @@ +--- +'@open-slide/core': patch +--- + +Make the inspector selectable for elements rendered by imported/shared components. diff --git a/packages/core/e2e/fixture/components/shared.tsx b/packages/core/e2e/fixture/components/shared.tsx new file mode 100644 index 000000000..4a7347aba --- /dev/null +++ b/packages/core/e2e/fixture/components/shared.tsx @@ -0,0 +1,9 @@ +import type { ReactNode } from 'react'; + +export function Heading({ children }: { children: ReactNode }) { + return
{children}
; +} + +export function Card({ children }: { children: ReactNode }) { + return
{children}
; +} diff --git a/packages/core/e2e/fixture/slides/shared-select/index.tsx b/packages/core/e2e/fixture/slides/shared-select/index.tsx new file mode 100644 index 000000000..6320111eb --- /dev/null +++ b/packages/core/e2e/fixture/slides/shared-select/index.tsx @@ -0,0 +1,18 @@ +import type { Page, SlideMeta } from '@open-slide/core'; +import { Card, Heading } from '../../components/shared'; + +export const meta: SlideMeta = { + title: 'Shared Select', + createdAt: '2026-01-01T00:00:00.000Z', +}; + +const Only: Page = () => ( + <> + Shared heading click target + + Nested shared heading + + +); + +export default [Only] satisfies Page[]; diff --git a/packages/core/e2e/fixture/tsconfig.json b/packages/core/e2e/fixture/tsconfig.json index b2e8a3c8f..f33cfdd29 100644 --- a/packages/core/e2e/fixture/tsconfig.json +++ b/packages/core/e2e/fixture/tsconfig.json @@ -13,5 +13,5 @@ "skipLibCheck": true, "types": ["@open-slide/core/env"] }, - "include": ["slides/**/*", "open-slide.config.ts"] + "include": ["slides/**/*", "components/**/*", "open-slide.config.ts"] } diff --git a/packages/core/e2e/tests/inspector.spec.ts b/packages/core/e2e/tests/inspector.spec.ts index 53a18d748..f52bcf1a0 100644 --- a/packages/core/e2e/tests/inspector.spec.ts +++ b/packages/core/e2e/tests/inspector.spec.ts @@ -140,4 +140,24 @@ test.describe('inspector editing', () => { await editorCanvas(page).getByText('Editable headline').click(); await expect(page.locator('aside[data-inspector-ui]')).toBeVisible(); }); + + test('selecting text inside an imported shared component opens the panel', async ({ page }) => { + await openSlide(page, 'shared-select'); + await page.getByTitle('Inspect').click(); + await editorCanvas(page).getByText('Shared heading click target').click(); + + const panel = page.locator('aside[data-inspector-ui]'); + await expect(panel).toBeVisible(); + await expect(panel.getByPlaceholder('Element text')).toHaveValue('Shared heading click target'); + }); + + test('selecting text inside a nested shared component opens the panel', async ({ page }) => { + await openSlide(page, 'shared-select'); + await page.getByTitle('Inspect').click(); + await editorCanvas(page).getByText('Nested shared heading').click(); + + const panel = page.locator('aside[data-inspector-ui]'); + await expect(panel).toBeVisible(); + await expect(panel.getByPlaceholder('Element text')).toHaveValue('Nested shared heading'); + }); }); diff --git a/packages/core/src/app/components/inspector/inspect-overlay.tsx b/packages/core/src/app/components/inspector/inspect-overlay.tsx index 52bdfede5..02c78d262 100644 --- a/packages/core/src/app/components/inspector/inspect-overlay.tsx +++ b/packages/core/src/app/components/inspector/inspect-overlay.tsx @@ -38,7 +38,7 @@ export function InspectOverlay() { if (!isInspectableEventTarget(e.target)) return setHover(null); const el = pickInspectorTarget(pickElement(e.clientX, e.clientY)); if (!el) return setHover(null); - const hit = findSlideSource(el, slideId, { hostOnly: true }); + const hit = findSlideSource(el, slideId); if (!hit) return setHover(null); setHover({ hit }); }; @@ -47,7 +47,7 @@ export function InspectOverlay() { if (!isInspectableEventTarget(e.target)) return; const el = pickInspectorTarget(pickElement(e.clientX, e.clientY)); if (!el) return; - const hit = findSlideSource(el, slideId, { hostOnly: true }); + const hit = findSlideSource(el, slideId); if (!hit) return; e.preventDefault(); e.stopPropagation(); @@ -59,7 +59,7 @@ export function InspectOverlay() { if (!isInspectableEventTarget(e.target)) return; const el = pickInspectorTarget(pickElement(e.clientX, e.clientY)); if (!el) return; - const hit = findSlideSource(el, slideId, { hostOnly: true }); + const hit = findSlideSource(el, slideId); if (!hit) return; if (!(hit.anchor instanceof HTMLImageElement)) return; e.preventDefault(); diff --git a/packages/core/src/app/components/inspector/inspector-panel.tsx b/packages/core/src/app/components/inspector/inspector-panel.tsx index c63db13fa..cb53cfb82 100644 --- a/packages/core/src/app/components/inspector/inspector-panel.tsx +++ b/packages/core/src/app/components/inspector/inspector-panel.tsx @@ -62,7 +62,7 @@ type RangeStylePreview = { }; function resolveSelectedTarget(target: SelectedTarget, slideId: string): SelectedTarget { - const hit = findSlideSource(target.anchor, slideId, { hostOnly: true }); + const hit = findSlideSource(target.anchor, slideId); if (!hit) return target; if (hit.line === target.line && hit.column === target.column && hit.anchor === target.anchor) { return target; @@ -1093,17 +1093,37 @@ function round2(n: number): number { return Math.round(n * 100) / 100; } +/** + * Re-resolve a selection whose anchor has been detached, which happens on + * every HMR round that replaces the DOM node behind the selected element. + * + * The `data-slide-loc` query is exact and settles the common case. The scan + * below covers what that attribute cannot: imported and shared components + * carry their call site in the fiber tree rather than the DOM, so they are + * never tagged. + * + * That scan prefers a line *and* column hit before falling back to line alone. + * Several elements routinely originate on one source line, and dropping + * `hostOnly` from the lookup (needed so shared components stay selectable) + * widened the candidate set further, so a line-only match hands back whichever + * element the DOM walk happened to reach first. Keeping the line-only result + * as a last resort means an edit that shifts a column degrades to the previous + * behavior instead of dropping the selection outright. + */ function findElementByLine(slideId: string, line: number, column: number): HTMLElement | null { const root = document.querySelector('[data-inspector-root]'); if (!root) return null; const tagged = root.querySelector(`[data-slide-loc="${line}:${column}"]`); if (tagged) return tagged; const candidates = root.querySelectorAll('*'); + let lineOnly: HTMLElement | null = null; for (const el of candidates) { - const hit = findSlideSource(el, slideId, { hostOnly: true }); - if (hit && hit.line === line) return hit.anchor; + const hit = findSlideSource(el, slideId); + if (!hit || hit.line !== line) continue; + if (hit.column === column) return hit.anchor; + lineOnly ??= hit.anchor; } - return null; + return lineOnly; } function useReloadCounter(): number { diff --git a/packages/core/src/app/lib/inspector/fiber.test.ts b/packages/core/src/app/lib/inspector/fiber.test.ts index 11c2e356b..f3070196c 100644 --- a/packages/core/src/app/lib/inspector/fiber.test.ts +++ b/packages/core/src/app/lib/inspector/fiber.test.ts @@ -3,12 +3,12 @@ import { findSlideSource } from './fiber.ts'; class FakeHTMLElement { dataset: Record = {}; - private closestSelf: FakeHTMLElement | null = null; - setClosestSelfForSlideLoc() { - this.closestSelf = this; - } + parentElement: FakeHTMLElement | null = null; closest(selector: string): FakeHTMLElement | null { - if (selector === '[data-slide-loc]') return this.closestSelf; + if (selector !== '[data-slide-loc]') return null; + for (let cur: FakeHTMLElement | null = this; cur; cur = cur.parentElement) { + if (cur.dataset.slideLoc) return cur; + } return null; } } @@ -20,11 +20,15 @@ type FakeFiber = { _debugSource?: DebugSource; }; -function makeEl(opts: { slideLoc?: string; fiber?: FakeFiber } = {}): FakeHTMLElement { +function makeEl( + opts: { slideLoc?: string; fiber?: FakeFiber; parent?: FakeHTMLElement } = {}, +): FakeHTMLElement { const el = new FakeHTMLElement(); if (opts.slideLoc) { el.dataset.slideLoc = opts.slideLoc; - el.setClosestSelfForSlideLoc(); + } + if (opts.parent) { + el.parentElement = opts.parent; } if (opts.fiber) { (el as unknown as Record).__reactFiber$test = opts.fiber; @@ -37,15 +41,22 @@ function makeFiber(opts: { line?: number; column?: number; host?: boolean; + hostEl?: FakeHTMLElement; parent?: FakeFiber | null; }): FakeFiber { const source: DebugSource | undefined = opts.fileName !== undefined ? { fileName: opts.fileName, lineNumber: opts.line, columnNumber: opts.column } : undefined; + let stateNode: unknown; + if (opts.hostEl) { + stateNode = opts.hostEl; + } else if (opts.host) { + stateNode = new FakeHTMLElement(); + } return { return: opts.parent ?? null, - stateNode: opts.host ? new FakeHTMLElement() : undefined, + stateNode, _debugSource: source, }; } @@ -59,7 +70,7 @@ afterAll(() => { }); describe('findSlideSource primary path', () => { - it('reads line:column from data-slide-loc', () => { + it('reads line:column from data-slide-loc on the element itself', () => { const el = makeEl({ slideLoc: '42:7' }); const hit = findSlideSource(el as unknown as HTMLElement, 'cover'); expect(hit).not.toBeNull(); @@ -67,6 +78,16 @@ describe('findSlideSource primary path', () => { expect(hit?.column).toBe(7); expect(hit?.anchor).toBe(el as unknown as HTMLElement); }); + + it('falls back to a tagged ancestor when fiber debug source is missing', () => { + const wrapper = makeEl({ slideLoc: '5:2' }); + const el = makeEl({ parent: wrapper }); + const hit = findSlideSource(el as unknown as HTMLElement, 'cover'); + expect(hit).not.toBeNull(); + expect(hit?.line).toBe(5); + expect(hit?.column).toBe(2); + expect(hit?.anchor).toBe(wrapper as unknown as HTMLElement); + }); }); describe('findSlideSource fallback', () => { @@ -151,4 +172,156 @@ describe('findSlideSource fallback', () => { expect(hit?.line).toBe(99); expect(hit?.column).toBe(3); }); + + it('selects an imported component call site and keeps the host anchor', () => { + const hostEl = makeEl(); + const callSite = makeFiber({ + fileName: '/repo/slides/cover/index.tsx', + line: 20, + column: 4, + }); + const libraryHost = makeFiber({ + fileName: '/repo/components/Heading.tsx', + line: 3, + column: 2, + hostEl, + parent: callSite, + }); + const el = makeEl({ fiber: libraryHost }); + // Point the leaf fiber's stateNode at the clicked element itself. + libraryHost.stateNode = el; + + const hit = findSlideSource(el as unknown as HTMLElement, 'cover'); + expect(hit).not.toBeNull(); + expect(hit?.line).toBe(20); + expect(hit?.column).toBe(4); + expect(hit?.anchor).toBe(el as unknown as HTMLElement); + }); + + it('anchors an imported call site to the nearest host fiber, not the clicked node', () => { + const hostEl = makeEl(); + const callSite = makeFiber({ + fileName: '/repo/slides/cover/index.tsx', + line: 20, + column: 4, + }); + const libraryHost = makeFiber({ + fileName: '/repo/components/Heading.tsx', + line: 3, + column: 2, + hostEl, + parent: callSite, + }); + const leaf = makeFiber({ parent: libraryHost }); + const el = makeEl({ fiber: leaf }); + + const hit = findSlideSource(el as unknown as HTMLElement, 'cover'); + expect(hit).not.toBeNull(); + expect(hit?.line).toBe(20); + expect(hit?.column).toBe(4); + // Assert against hostEl rather than el. `anchor` is seeded with the clicked + // element, so the sibling test above would still pass if the walk never + // recognised libraryHost as a host fiber; only a distinct host element + // proves the `instanceof HTMLElement` branch actually ran. + expect(hit?.anchor).toBe(hostEl as unknown as HTMLElement); + expect(hit?.anchor).not.toBe(el as unknown as HTMLElement); + }); + + it('rejects imported call sites when hostOnly is set', () => { + const callSite = makeFiber({ + fileName: '/repo/slides/cover/index.tsx', + line: 20, + column: 4, + }); + const libraryHost = makeFiber({ + fileName: '/repo/components/Heading.tsx', + line: 3, + column: 2, + host: true, + parent: callSite, + }); + const el = makeEl({ fiber: libraryHost }); + libraryHost.stateNode = el; + + const hit = findSlideSource(el as unknown as HTMLElement, 'cover', { hostOnly: true }); + expect(hit).toBeNull(); + }); + + it('prefers a shared call site over a tagged slide ancestor', () => { + const wrapper = makeEl({ slideLoc: '8:2' }); + const callSite = makeFiber({ + fileName: '/repo/slides/cover/index.tsx', + line: 12, + column: 6, + }); + const libraryHost = makeFiber({ + fileName: '/repo/components/Heading.tsx', + line: 2, + column: 0, + host: true, + parent: callSite, + }); + const el = makeEl({ fiber: libraryHost, parent: wrapper }); + libraryHost.stateNode = el; + + const hit = findSlideSource(el as unknown as HTMLElement, 'cover'); + expect(hit).not.toBeNull(); + expect(hit?.line).toBe(12); + expect(hit?.column).toBe(6); + expect(hit?.anchor).toBe(el as unknown as HTMLElement); + }); + + it('resolves nested shared components to the nearest slide call site', () => { + const outerCall = makeFiber({ + fileName: '/repo/slides/cover/index.tsx', + line: 30, + column: 2, + }); + const innerCall = makeFiber({ + fileName: '/repo/slides/cover/index.tsx', + line: 31, + column: 4, + parent: outerCall, + }); + // Inner library host under , which is itself under . + const libraryHost = makeFiber({ + fileName: '/repo/components/Heading.tsx', + line: 2, + column: 0, + host: true, + parent: innerCall, + }); + const el = makeEl({ fiber: libraryHost }); + libraryHost.stateNode = el; + + const hit = findSlideSource(el as unknown as HTMLElement, 'cover'); + expect(hit).not.toBeNull(); + expect(hit?.line).toBe(31); + expect(hit?.column).toBe(4); + }); + + it('maps a text-bearing host under a shared component to the call site', () => { + // Mirrors clicking the text node "Click me" inside Click me: + // elementsFromPoint returns the host div; fiber points at the library file. + const callSite = makeFiber({ + fileName: '/repo/slides/demo/index.tsx', + line: 7, + column: 15, + }); + const libraryHost = makeFiber({ + fileName: '/repo/components/Heading.tsx', + line: 2, + column: 4, + host: true, + parent: callSite, + }); + const el = makeEl({ fiber: libraryHost }); + libraryHost.stateNode = el; + + const hit = findSlideSource(el as unknown as HTMLElement, 'demo'); + expect(hit).not.toBeNull(); + expect(hit?.line).toBe(7); + expect(hit?.column).toBe(15); + expect(hit?.anchor).toBe(el as unknown as HTMLElement); + }); }); diff --git a/packages/core/src/app/lib/inspector/fiber.ts b/packages/core/src/app/lib/inspector/fiber.ts index 1ec81e8f1..3f119211c 100644 --- a/packages/core/src/app/lib/inspector/fiber.ts +++ b/packages/core/src/app/lib/inspector/fiber.ts @@ -5,9 +5,10 @@ export type SlideSourceHit = { }; export type FindSlideSourceOptions = { - // Visual editor uses this: skip component-invocation JSX (``) - // since most components don't forward `style`. Comments leave it off - // so any JSX can be annotated. + // When true, only match host DOM fibers (`div`, `p`, …). Component + // call-site fibers (``) are skipped. Prefer leaving this off so + // imported/shared components remain selectable; their host children are + // authored outside `slides/` and never get `data-slide-loc`. hostOnly?: boolean; }; @@ -34,30 +35,47 @@ function normalizeDebugFileName(fileName: string): string { return fileName.split(/[?#]/)[0].replace(/\\/g, '/'); } -export function findSlideSource( +/** + * Read the `line:column` pair that the loc-tags Vite plugin writes into + * `data-slide-loc`. + * + * Every rejection path matters more than it looks: callers treat a returned + * object as an authoritative source position, so a malformed attribute has to + * degrade to "untagged" and let the fiber walk take over. Handing back a `NaN` + * line instead would point the inspector at a source location that does not + * exist. Returns null for a missing attribute, a missing or leading separator, + * or a non-finite half. + */ +function parseSlideLoc(el: HTMLElement): { line: number; column: number } | null { + const loc = el.dataset.slideLoc; + if (!loc) return null; + const idx = loc.indexOf(':'); + if (idx <= 0) return null; + const line = Number(loc.slice(0, idx)); + const column = Number(loc.slice(idx + 1)); + if (!Number.isFinite(line) || !Number.isFinite(column)) return null; + return { line, column }; +} + +/** + * Resolve a source position by walking the React fiber chain up from `el`. + * + * This exists because the loc-tags plugin only transforms files under + * `slides/`. JSX rendered from an imported or shared component carries no + * `data-slide-loc`, so the only record of where it was invoked lives in the + * fiber's debug source. The walk stops at the first ancestor whose debug file + * is the slide's own `index.tsx`, which is the call site the author can + * actually edit. + * + * `anchor` tracks the nearest host element seen so far rather than the matched + * fiber, because a component-invocation fiber has no DOM node of its own; the + * inspector still needs something on screen to outline and measure. + */ +function findViaFiber( el: HTMLElement, slideId: string, opts?: FindSlideSourceOptions, ): SlideSourceHit | null { - // Primary path: the `data-slide-loc` attribute injected by the - // loc-tags Vite plugin. Immune to HMR-stale fiber state. - const tagged = el.closest('[data-slide-loc]'); - if (tagged) { - const loc = tagged.dataset.slideLoc; - if (loc) { - const idx = loc.indexOf(':'); - if (idx > 0) { - const line = Number(loc.slice(0, idx)); - const column = Number(loc.slice(idx + 1)); - if (Number.isFinite(line) && Number.isFinite(column)) { - return { line, column, anchor: tagged }; - } - } - } - } - - // Fallback for JSX rendered from imported component files (which the - // loc-tags plugin doesn't transform). const needle = `/slides/${slideId}/index.tsx`; let fiber = getFiber(el); let anchor: HTMLElement = el; @@ -83,3 +101,44 @@ export function findSlideSource( } return null; } + +/** + * Map a clicked DOM element back to the JSX that produced it. + * + * The three strategies are ordered by how specific their answer is, not by how + * cheap they are. An exact tag on the element itself is unambiguous, so it + * wins outright. The fiber walk comes second because it is the only strategy + * that can see an imported or shared component's call site, and it has to + * outrank the tagged-ancestor lookup: a shared component nested inside tagged + * slide markup would otherwise resolve to its wrapper, and clicking the + * component would silently select the container around it (#327). The ancestor + * tag is the last resort for the case the fiber walk cannot cover, which is + * an HMR-stale or absent debug source. + */ +export function findSlideSource( + el: HTMLElement, + slideId: string, + opts?: FindSlideSourceOptions, +): SlideSourceHit | null { + // Exact tag on the clicked element (slide-authored host JSX). Immune to + // HMR-stale fiber state. + const own = parseSlideLoc(el); + if (own) { + return { ...own, anchor: el }; + } + + // Fiber walk resolves imported/shared component call sites. Host children + // rendered from another file are never tagged by loc-tags, and must win + // over a tagged slide ancestor (otherwise a wrapper steals the click). + const fiberHit = findViaFiber(el, slideId, opts); + if (fiberHit) return fiberHit; + + // Ancestor tag: last-resort mapping when fiber debug source is missing. + const tagged = el.closest('[data-slide-loc]'); + if (tagged) { + const loc = parseSlideLoc(tagged); + if (loc) return { ...loc, anchor: tagged }; + } + + return null; +}