From adc6ebf6ceda669be03ffa472bb87ada94c7e349 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Fri, 7 Aug 2026 17:11:06 -0700 Subject: [PATCH] refactor(studio): split the inline styling steps that failed the audit gate The audit gate was already red on this branch before the selection-box commit landed, with four findings, all of them in the feature's own files: `restyle`, `runNodes`, `offsetOf` and the test's `rangeOver`. Split into named steps rather than suppressed. Two things worth saying about the split, because both are places it could have gone quietly wrong. `restyle` still walks UTF-16 units rather than code points: spreading the string would count a surrogate pair once and slide every index after an emoji. And `offsetOf` counts two different things depending on how the position was given, a place inside a text node or an index among the host's children, where a child element contributes everything inside it. Collapsing those two into one helper made bold stop toggling off, which the suite caught. --- .../editor/inlineTextStyleRange.test.ts | 53 ++++---- .../components/editor/inlineTextStyleRange.ts | 123 +++++++++++++----- 2 files changed, 113 insertions(+), 63 deletions(-) diff --git a/packages/studio/src/components/editor/inlineTextStyleRange.test.ts b/packages/studio/src/components/editor/inlineTextStyleRange.test.ts index e1b5eb332b..b377fbccd1 100644 --- a/packages/studio/src/components/editor/inlineTextStyleRange.test.ts +++ b/packages/studio/src/components/editor/inlineTextStyleRange.test.ts @@ -14,37 +14,36 @@ function mount(html: string): HTMLElement { } /** A range over the host's text, by character offsets across the whole element. */ -function rangeOver(host: HTMLElement, start: number, end: number): Range { - const range = document.createRange(); - // A line break counts as one character, the same way the module does. +/** Every text node and line break in order, with the offset each one starts at. */ +function charSpans(host: HTMLElement): Array<{ node: Node; from: number; length: number }> { + const spans: Array<{ node: Node; from: number; length: number }> = []; const walker = document.createTreeWalker(host, NodeFilter.SHOW_TEXT | NodeFilter.SHOW_ELEMENT); let seen = 0; - let startSet = false; - let node = walker.nextNode(); - while (node) { - if (node.nodeType === 1 && (node as Element).tagName !== "BR") { - node = walker.nextNode(); - continue; - } - const length = node.nodeType === 1 ? 1 : (node.textContent?.length ?? 0); - // A break is counted but never landed on: a boundary there belongs to the - // text beside it, which is where a real selection would put it too. - if (node.nodeType === 1) { - seen += length; - node = walker.nextNode(); - continue; - } - if (!startSet && seen + length >= start) { - range.setStart(node, start - seen); - startSet = true; - } - if (startSet && seen + length >= end) { - range.setEnd(node, end - seen); - return range; - } + for (let node = walker.nextNode(); node; node = walker.nextNode()) { + const isBreak = node.nodeType === 1 && (node as Element).tagName === "BR"; + if (node.nodeType === 1 && !isBreak) continue; + const length = isBreak ? 1 : (node.textContent?.length ?? 0); + spans.push({ node, from: seen, length }); seen += length; - node = walker.nextNode(); } + return spans; +} + +/** + * A range over the host's text, by character offsets across the whole element. + * + * A line break counts as one character, the same way the module does, but is + * never landed on: a boundary there belongs to the text beside it, which is + * where a real selection would put it too. + */ +function rangeOver(host: HTMLElement, start: number, end: number): Range { + const range = document.createRange(); + const text = charSpans(host).filter((span) => span.node.nodeType === 3); + const at = (offset: number) => text.find((span) => span.from + span.length >= offset); + const from = at(start); + const to = at(end); + if (from) range.setStart(from.node, start - from.from); + if (from && to) range.setEnd(to.node, end - to.from); return range; } diff --git a/packages/studio/src/components/editor/inlineTextStyleRange.ts b/packages/studio/src/components/editor/inlineTextStyleRange.ts index 39dfcec368..53ab11d75c 100644 --- a/packages/studio/src/components/editor/inlineTextStyleRange.ts +++ b/packages/studio/src/components/editor/inlineTextStyleRange.ts @@ -294,24 +294,50 @@ function restyle( const text = runs.map((run) => run.text).join(""); const perChar = charRuns(runs); const next: StyledRun[] = []; - + // Indexed by UTF-16 unit, not by code point: `perChar`, `start` and `end` all + // count units, and spreading the string would count a surrogate pair once and + // slide every index after an emoji. for (let index = 0; index < text.length; index += 1) { - const inside = index >= start && index < end; - const previous = perChar[index]?.style ?? {}; - const style = inside ? withDelta(previous, delta) : previous; - const origin = perChar[index]?.origin ?? null; - const identity = perChar[index]?.identity ?? ""; - const last = next[next.length - 1]; - // Merged as it is built, so equal neighbours never become two spans. Two - // that the design panel tracks as separate layers stay apart even when - // they now look identical, because merging them deletes one of them. - if (last && last.identity === identity && sameStyle(last.style, style)) - last.text += text[index]; - else next.push({ text: text[index] ?? "", style, origin, identity }); + appendChar( + next, + text[index] ?? "", + charAfter(perChar[index], index >= start && index < end, delta), + ); } return next; } +/** What one character is styled with once the delta has been applied to it. */ +function charAfter( + at: Omit | undefined, + inside: boolean, + delta: InlineStyleDelta, +): Omit { + const style = at?.style ?? {}; + return { + style: inside ? withDelta(style, delta) : style, + origin: at?.origin ?? null, + identity: at?.identity ?? "", + }; +} + +/** + * One character onto the run list, merged into the run before it when they + * belong together. + * + * Merged as it is built, so equal neighbours never become two spans. Two that + * the design panel tracks as separate layers stay apart even when they now look + * identical, because merging them deletes one of them. + */ +function appendChar(runs: StyledRun[], char: string, at: Omit): void { + const last = runs[runs.length - 1]; + if (last && last.identity === at.identity && sameStyle(last.style, at.style)) { + last.text += char; + return; + } + runs.push({ text: char, ...at }); +} + function withDelta(style: Record, delta: InlineStyleDelta): Record { const next = { ...style }; for (const [property, value] of Object.entries(delta)) { @@ -360,24 +386,35 @@ function runNodes(doc: Document, runs: StyledRun[]): Node[] { // origin is a new layer and is written as one. const claimed = new Set(); for (const run of runs) { - const key = styleKey(run.style); const carried = run.origin && !claimed.has(run.origin) ? preservedAttributes(run.origin) : new Map(); if (carried.size > 0 && run.origin) claimed.add(run.origin); - for (const [index, piece] of run.text.split(BREAK).entries()) { - if (index > 0) nodes.push(doc.createElement("br")); - if (!piece) continue; - if (!key && carried.size === 0) { - nodes.push(doc.createTextNode(piece)); - continue; - } - const span = doc.createElement("span"); - for (const [name, value] of carried) span.setAttribute(name, value); - if (key) span.setAttribute("style", key); - span.textContent = piece; - nodes.push(span); - carried.clear(); + nodes.push(...runNode(doc, run, carried)); + } + return nodes; +} + +/** + * One run's nodes: its line breaks as `
`, and its text as bare text when it + * has nothing to carry or a span when it has. The identity goes on the first + * piece only, so a run broken across lines does not claim it twice. + */ +function runNode(doc: Document, run: StyledRun, carried: Map): Node[] { + const key = styleKey(run.style); + const nodes: Node[] = []; + for (const [index, piece] of run.text.split(BREAK).entries()) { + if (index > 0) nodes.push(doc.createElement("br")); + if (!piece) continue; + if (!key && carried.size === 0) { + nodes.push(doc.createTextNode(piece)); + continue; } + const span = doc.createElement("span"); + for (const [name, value] of carried) span.setAttribute(name, value); + if (key) span.setAttribute("style", key); + span.textContent = piece; + nodes.push(span); + carried.clear(); } return nodes; } @@ -425,25 +462,39 @@ function laysOutItsChildren(host: Element): boolean { /** Where a DOM position falls, counted in characters from the element's start. */ function offsetOf(host: Element, container: Node, containerOffset: number): number | null { - let count = 0; - const walker = host.ownerDocument.createTreeWalker(host, NodeFilter.SHOW_TEXT | 1); + // A position between children, expressed as a child index. if (container === host) { - // A position between children, expressed as a child index. - for (const child of Array.from(host.childNodes).slice(0, containerOffset)) { - count += (child.textContent ?? "").length || (nodeName(child) === "BR" ? 1 : 0); - } - return count; + return Array.from(host.childNodes) + .slice(0, containerOffset) + .reduce((count, child) => count + subtreeCharLength(child), 0); } + let count = 0; + const walker = host.ownerDocument.createTreeWalker( + host, + NodeFilter.SHOW_TEXT | NodeFilter.SHOW_ELEMENT, + ); let node = walker.nextNode(); while (node) { if (node === container) return count + containerOffset; - if (node.nodeType === 3) count += (node.textContent ?? "").length; - else if (nodeName(node) === "BR") count += 1; + count += charLength(node); node = walker.nextNode(); } return null; } +/** How many character positions a node occupies itself: its own text, or one + * for a break. An element contributes nothing; the walk visits its text. */ +function charLength(node: Node): number { + if (node.nodeType === 3) return (node.textContent ?? "").length; + return nodeName(node) === "BR" ? 1 : 0; +} + +/** The same count for a child and everything inside it, for a position given as + * a child index rather than a place in a text node. */ +function subtreeCharLength(node: Node): number { + return (node.textContent ?? "").length || (nodeName(node) === "BR" ? 1 : 0); +} + function nodeName(node: Node): string { return node.nodeType === 1 ? (node as Element).tagName : ""; }