feat(studio): apply a style to a run of characters

[miguel-heygen]

What

The engine that applies a style to selected characters inside an element. Pure logic, no UI.

Why

Wrapping a DOM range in a span is three lines, and then every interesting case is a special case: recolouring nests spans that shadow each other, removing a style cannot reach the ancestor that set it, and styling across an existing run's boundary has to split it. Each fix is a new branch and the branches interact.

How

The element is read into a flat list of styled runs, the style is applied to a span of characters in that list, and the element is rebuilt from it. Replacing, removing, splitting and merging stop being cases: the rebuild emits one span per distinct run and cannot nest or duplicate, whatever was there before.

Selection offsets count UTF-16 units, so a boundary can land inside a visible character made from multiple code points; the applied range widens to whole grapheme clusters. A colour an ancestor overpaints is mirrored into the fill only for the affected run, because a colour that does not paint reads to the user as a colour that did not save.

Test plan

• 62 tests covering replace, remove, split, merge, grapheme boundaries, layer identity, sanitizer parity, nested line breaks, deep markup, editable boundaries, and per-run overpainted-fill behavior

The toolbar that drives this arrives in the next PR.

[vanceingalls]

Read the code (inlineTextStyleRange.ts + .test.ts in full at head 4d5913a7) and verified findings against the sanitizer allowlist from PR #3141. The read-flat-runs / rebuild approach is the right shape over range surgery, and the test file's naming ("flex containers", "data-hf-text-key identity", "break sentinels", "overpainted-fill") shows careful thinking about the real failure modes. Two correctness gaps stop me from stamping.

Blockers

1. Grapheme widening only handles UTF-16 surrogate pairs — ZWJ sequences, regional-indicator flags, skin-tone modifiers, and combining marks still split. isTrailingHalf at inlineTextStyleRange.ts:168-171:

function isTrailingHalf(text: string, offset: number): boolean {
  const code = text.charCodeAt(offset);
  return code >= 0xdc00 && code <= 0xdfff;
}

That widens the single-code-point emoji case (👍, tested at .test.ts:317 and :330). It does not widen:

• ZWJ family 👨‍👩‍👧 (5 code points joined by U+200D ZWJ) — a boundary at the ZWJ or between joined glyphs passes the surrogate check and splits the cluster.
• Regional-indicator flags 🇺🇸 (two surrogate pairs) — boundary at index 2 sits AFTER the trailing surrogate, isTrailingHalf returns false, flag halves land in different spans.
• Skin-tone 👍🏽 (base + modifier, both surrogate pairs) — boundary between them splits, giving base + orphaned modifier.
• Combining marks é (é) — both code units are BMP, no surrogate anywhere, isTrailingHalf returns false and base/combining land in different runs.

None of these appear in the test file — grepped for ZWJ / regional-indicator / skin-tone-modifier / combining-mark payloads and got zero matches. The docstring at :142 says "widened so they never fall inside a character" which reads as user-perceived character (extended grapheme cluster), but the code only implements code-point-pair. That's the whole reason the widening exists; shipping it as-is means the emoji-safety claim is only true for the tested subset. Intl.Segmenter with granularity: "grapheme" is available in every browser Studio targets — please widen to grapheme clusters and add tests for ZWJ / RI-flag / skin-tone / combining-mark.

2. Overpaint mirror over-applies: one overpainted subtree stamps -webkit-text-fill-color on every coloured run in the host. applyInlineStyle:100:

if (colourIsOverpainted(host)) render(host, next.map(mirrorFillColor));

colourIsOverpainted(host) at :119-131 scans host.querySelectorAll("span[style*='color']") and returns true if ANY span in the host is being overpainted by an ancestor. Then next.map(mirrorFillColor) (via mirrorFillColor at :135-139) stamps -webkit-text-fill-color: <run.color> on every run in the array that has a color. Not just the runs whose ancestor path actually overpaints.

In a document with two subtrees where only one has -webkit-text-fill-color on an ancestor, edits in the untouched subtree still stamp fill colors onto runs whose ancestors never overpainted. That's the stale-mirror problem: extra style bytes now win over any future ancestor overpaint change on the untouched subtree, breaking the "designer changes ancestor fill, all descendants follow" contract downstream. The stubbed test at .test.ts:538-545 exercises only the single-host case and can't catch this.

Two fixes worth considering: (a) evaluate overpaint per rendered span, only mirror the ones failing; (b) reassess mirror validity on subsequent edits (drop the mirror when the ancestor no longer overpaints).

Non-blockers (worth resolving in comment)

3. preservedAttributes (:1006-1013) copies every non-style, non-data-hf-id attribute. The sanitizer's FORMATTING_ATTRS allows only data-hf-text-key + data-hf-id. Anything else the origin carries (class, id, role, stray onclick from a paste) survives the rebuild in-editor but is stripped on save. Consider passing this through an allowlist that matches the sanitizer, so what the user sees mid-edit matches what persists.

4. editingHost selector .closest("[contenteditable]") (:777) matches contenteditable="false" too. If a caret ever lands inside a false subtree nested in a true editor, this rebuilds the false region. Use .isContentEditable or [contenteditable=""], [contenteditable="true"].

5. Overpaint detection span[style*='color'] (:122) substring-matches background-color/border-color/caret-color. Covered defensively by if (!span.style.color) continue, so nit — cleaner as [style*='color:'] or a full host.querySelectorAll('span') with the property check.

6. readInlineStyle collapsed-range semantics at :185-186: .slice(start, Math.max(end, start + 1)) reports the character after the caret for a collapsed range. Toolbars usually reflect the char before (last-typed state). Worth naming the choice in the docstring.

CI

The failing Test on run 31463831279 completed at 06:07:51Z — only 3 seconds after starting at 06:07:48Z, which is push-cancellation semantics (a newer commit was pushed at 06:07:34Z triggering a fresh run at 06:10:59Z currently IN_PROGRESS). The reported Test FAILURE is aggregation of the cancelled run, not a real regression. Re-check when the fresh run lands.

Verdict

REQUEST_CHANGES. The read-flat-runs approach is right, but the emoji-widening claim is only true for the single-code-point case (any composed emoji ships broken after an edit), and the overpaint mirror stamps redundant -webkit-text-fill-color bytes on every colored run in the host — inviting a downstream stale-mirror bug. Both are addressable with focused fixes; non-blockers are follow-up polish. Happy to re-review after the two blockers.

— Via

[james-russo-rames-d-jusso]

Reviewed at 4d5913a7.

Read this through the run-shape / cascade-correctness / downstream-contract-with-sanitizer / Unicode / adversarial-paste lens. Architecture is right: reading DOM to a flat list, applying delta, rebuilding, one span per distinct run — this replaces a class of DOM-range-surgery bugs with algebraic clarity, and the "outermost origin carries identity" rule threads the design-panel's layer tracking through cleanly. The -webkit-text-fill-color overpaint idea is correct in shape (see Via's blocker #2 for the scope bug).

Via has REQUEST_CHANGES with 2 blockers and 4 non-blockers. I strongly concur on both blockers and on 3 of the 4 non-blockers, plus have 4 additional findings inline that Via didn't call out.

Concurring with Via's blockers

• Grapheme widening (Via #1) — concur. My Unicode-exhaustiveness agent independently traced the identical failure modes: ZWJ family 👨‍👩‍👧 splits at any position inside the ZWJ chain (text[2] = U+200D, BMP, isTrailingHalf returns false, no widen); VS16 emoji presentation ☂️ splits between base and variation selector (text[1] = U+FE0F, BMP); combining marks é split base from mark (both BMP). Regional-indicator flags and skin-tone modifiers as Via described. The docstring at codePointBounds:142 says "widened so they never fall inside a character" which reads as grapheme cluster; the code implements code-point-pair only. Either the docstring narrows to "surrogate pair" (weak — the promised safety isn't the delivered one) or the code widens to grapheme via Intl.Segmenter({granularity: "grapheme"}) — the segmenter is available in every browser Studio ships to. Latter is the honest fix.

• Overpaint mirror over-application (Via #2) — concur, hadn't spotted it. colourIsOverpainted(host) at :119-131 returns a single boolean for the entire host; then next.map(mirrorFillColor) at :100 stamps -webkit-text-fill-color on every coloured run, not just the ones whose ancestor path actually overpaints. In a host containing two disjoint subtrees where only one has an overpainting ancestor, edits anywhere stamp mirror bytes onto runs whose ancestors never overpainted. Two downstream problems: (a) sanitizer keeps -webkit-text-fill-color (it's in FORMATTING_STYLE_PROPS), so the stale mirror persists to the file and future ancestor changes are silently overridden; (b) the composition file grows extra style bytes that never do anything except mask a future authoring change. Per-run overpaint detection (walk the rebuilt spans, check each getComputedStyle().webkitTextFillColor !== color) fixes both.

Concurring with Via's non-blockers

• .closest("[contenteditable]") matches contenteditable="false" subtrees (Via #4) — concur, I missed this. Selector doesn't distinguish; element.isContentEditable or the explicit selector [contenteditable=""], [contenteditable="true"] is the fix. Real reproducer: a nested <div contenteditable="false"> inside a live editor (thumbnails, image captions, decorative widgets) — if a caret lands inside it during a repeat-fire colour picker, editingHost returns the false subtree and render rewrites it.

• span[style*='color'] substring match (Via #5) — traced clean but agree with the cleanup. The if (!span.style.color) continue at :123 refines to only spans with an explicit color property, so no false positives from background-color / caret-color. Cleaner as [style*='color:'] and drops the guard.

• readInlineStyle collapsed-range char-after semantics (Via #6) — concur. The Math.max(end, start + 1) at :186 returns the character AFTER the caret. Toolbars conventionally reflect the last-typed character (before). Worth documenting the choice or flipping to char-before. Adjacent: when start === text.length the slice is empty → covered.length === 0 → returns {}; the toolbar shows no active styles at end-of-text even though the user is about to type into a styled context. Fall back to the last character when at EOT.

Additional findings (inline)

Four anchors Via didn't call out. See on-file comments:

1. walk:251 — non-styling tags flatten to <span> with their non-style attrs carried over. <a href="X">foo</a> becomes <span href="X" style="...">foo</span>: invalid HTML + dead link + sanitizer strips href on save, so the link is gone from the file too. Same class of loss for <sub>H</sub>2O, <mark>, <small>, <s>.

2. identityOf:265 — different attribute sets can serialize to the same identity string. data-x="1&data-y=2" (one attr with &/= in the value) collides with data-x="1" data-y="2" (two attrs). Two distinct tracked layers merge into one on rebuild.

3. subtreeCharLength:494 — undercounts by one per nested <br> when startContainer === host. textContent.length doesn't include a character for a nested <br>, but the flat-string model does. Any Range whose boundary is expressed as a child-index of the host slides one character per prior nested <br>. Not exercised by the tests (rangeOver always lands on text nodes) but reachable from real user gestures where a native selection is dragged from a text node out through the element boundary.

4. ownStyle:271 — engine preserves every inline style property on run origins; sanitizer keeps only 10. text-transform: uppercase, border: 1px solid, custom properties, etc. all preserved by the engine, stripped by filterStyle on save. User sees them in preview, they disappear on reload. The style-property twin of Via's #3 (which is about attrs).

Body-level nits (mine, not overlapping Via's)

• Recursion in walk (line 245-256) — same shape you just fixed in the sanitizer. HF#3141 R3 converted sanitizeRichTextChildren's recursion to an iterative post-order stack precisely because "adversarially deep pasted markup must not exhaust either the server or browser call stack." This engine has the identical shape. Not exploitable (client-side crash, not XSS), not currently reachable through happy-path use — but a paste from Google Docs at a few thousand levels of nested formatting would blow the browser stack here before it ever reaches the sanitizer. Cheap to convert to the same iterative walk pattern for symmetry with the sanitizer fix.

• TAG_STYLES (5 tags: B/STRONG/I/EM/U) and FORMATTING_TAGS (7 tags including SPAN/BR) must stay in sync. No test pins this. If a future PR adds <mark> or <s> to the sanitizer's FORMATTING_TAGS, the engine silently drops the semantic style on the first edit. One-liner check: for every non-SPAN, non-BR tag in FORMATTING_TAGS, the engine must have an entry in TAG_STYLES OR a test that pins its rebuild shape.

• BREAK = "�" marker (line 75). The comment says "the HTML parser replaces it with U+FFFD, so no document can contain one" — true for HTML parsing only. element.textContent = "�" from JS preserves the literal NUL. If any Studio path plants a NUL via textContent or createTextNode, it'd be split as a <br> on the first edit. Consider a PUA marker (e.g. ) or a defensive replace on read.

Positive verifications

• No XSS bypass and no invalid HTML emission for the styling engine's own output on well-formed input. Emits <span> and <br> only — both in the sanitizer's FORMATTING_TAGS.
• UTF-16 surrogate pair widening at codePointBounds is correct for the code-point case: high surrogate at start sits on a valid boundary (no widen needed); trailing surrogate at start or end widens the right direction. The gap Via flagged is grapheme-level.
• Flex/grid wrapper insertion — wrapper is a bare <span> with no attrs; survives sanitize; on re-read preservedAttributes(wrapper).size === 0 so it doesn't shadow real layers.
• data-hf-id correctly excluded from preservedAttributes and identity — the test at :457-466 pins this.
• Repeat-fire idempotence (5-round test at :383-391 + color-picker test at :487-499) holds.

🔮 Forward-looking (D4+ lens)

FL1 — Paste-handling path. This engine reads from the live DOM. When the D4+ paste handler lands and copies external HTML into the editor, the engine will read that adversarial content: deeply-nested spans (stack overflow via the recursion nit above), event handlers preserved by preservedAttributes (until Via's #3 lands), <template> elements the sanitizer would opaquely drop that this engine would flatten into wrapper spans. Check on D4+: does the paste path inert-parse + sanitize BEFORE inserting into the contenteditable? If not, this engine's shape hazards become adversarially reachable.

FL2 — Overpaint mirror across ancestor changes (adjacent to Via's #2). Once the mirror is per-run, the next question is stale-mirror lifecycle: when a designer changes an ancestor's -webkit-text-fill-color, the mirrored spans still carry the old fill. Check on D4+: if D4+ ships an ancestor-fill-change UI, does it re-evaluate mirrors on affected descendants? Or does the mirror get dropped on the next edit that touches the affected run?

What I didn't verify

• Did not run the 44 tests locally; trusting Miguel's 44/44 green + the sample I traced.
• Did not audit whether HF composition text elements currently contain <a> / <sub> / <mark> / <s> in the wild — concern #1's user impact depends on that. If never in practice, it's a scope-cleanup rather than a break.
• Did not run Intl.Segmenter against ZWJ / RI / skin-tone / VS16 payloads; Via's mechanism sketch is straightforward and the coverage story is what matters.

Overall: architecture is right, mechanism is right, 44 tests are unusually well-shaped for a foundational PR. Via's 2 blockers are the ones that gate merge; my 4 additional inlines + concurring non-blockers are worth folding into the R2 push.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

R2 verified at c6d4df89.

Every item Via flagged verifies clean. My R1 set is partially closed too — one narrow inline concern remains open, the rest were either directly addressed or resolved by side effect.

Via's blockers — both cleanly closed

• Grapheme widening. codePointBounds → graphemeBounds (inlineTextStyleRange.ts:151-181) using Intl.Segmenter({granularity: "grapheme"}). Traced: for a👨‍👩‍👧b with rangeOver(2, 8), boundaries = [0, 1, 9, 10], boundaryAtOrBefore(boundaries, 2) returns 1, boundaries.find(b >= 8) returns 9 → widened {start: 1, end: 9}, the full ZWJ family gets included. Test coverage added at .test.ts:347-357 for ZWJ / RI-flag / skin-tone / combining-mark. Coverage is it.each across all four so a regression on any one shows up as a distinct case name.

• Per-run overpaint reconciliation. colourIsOverpainted + mirrorFillColor replaced with reconcileFillColors(host) (:120-141). Per-span: (1) remove any existing mirror that equals the color (that's a generated mirror; take it off before probing so we reassess "would this run be overpainted without our own mirror in the way"); (2) read computed style; (3) if fill still differs from color, ancestor overpaints → add mirror; else no mirror. The remove-then-probe order is the load-bearing bit — makes the reconciliation stable across repeat-fire edits. Test at .test.ts:603-617 pins "only the overpainted subtree gets mirrored"; test at :619-629 pins "mirror is dropped when ancestor stops overpainting". Both are exactly the tests I'd have asked for.

Via's non-blockers — all cleanly closed

• preservedAttributes now allowlist-driven via isRichTextFormattingAttribute(name, value) (richTextSanitize.ts:102-104) which encapsulates FORMATTING_ATTRS.has(name.toLowerCase()) && SAFE_ATTR_VALUE.test(value) — the same predicate the sanitizer uses at :173. Engine now preserves exactly what will survive persistence. Test at .test.ts:490-500 drops aria-label and onclick; keeps data-hf-text-key. Nice extraction — the helper eliminates the two places where the same predicate could drift.

• contenteditable="false" filter at editingHost:221-223 — returns null when the closest editable is explicitly false. Test at .test.ts:502-510 proves the false subtree is not touched.

• span[style*='color'] → querySelectorAll("span") + property check at reconcileFillColors:123. The refinement guard was already correct; this cleans up the selector and drops the substring-match false-positive surface.

• readInlineStyle collapsed-caret char-before at :196-197 — slice(Math.max(0, start - 1), Math.max(1, start)) reads the character before the caret when collapsed, which is the toolbar convention. Test at .test.ts:282-289 pins char-before behavior.

My R1 set — status

• #1 walk:251 non-styling tag flattening — resolved by side effect. With preservedAttributes now allowlist-only, <a href="X">foo</a> no longer emits <span href="X">…</span> (href isn't allowlisted, so origin becomes null and href is dropped). The rebuild now silently drops the <a>'s semantics rather than emitting invalid HTML with a dead href. The residual question — "should HF text elements support links, i.e. should href be added to FORMATTING_ATTRS?" — is a persistence-vocabulary decision, not this PR's fix to make. Cleanly closed as far as the engine is concerned.

• #2 identityOf:265 collision — closed by the same side effect. Only data-hf-text-key (and data-hf-id, excluded) can now populate preservedAttributes, and SAFE_ATTR_VALUE = /^[A-Za-z0-9_:-]+$/ in the sanitizer forbids & and = in the value. Two-attribute vs one-attribute-with-delimiters collision is no longer constructible.

• #3 subtreeCharLength:494 undercount — still open at R2 head. subtreeCharLength is unchanged; still returns textContent.length || (BR ? 1 : 0), still undercounts by one per nested <br> when a Range boundary is expressed as (host, childIndex). Concrete trace unchanged from R1: for host <h1><span>abc<br>def</span>tail</h1> and Range {startContainer: host, startOffset: 1}, offsetOf returns 6, correct offset is 7. Not exercised by the tests (rangeOver lands on text nodes) but reachable from real user selection gestures. Low severity — narrow off-by-one, not a shape hazard — but a real bug.

• #4 ownStyle:284 non-allowlisted style preservation — still open. ownStyle reads every inline style property; FORMATTING_STYLE_PROPS keeps only 10. text-transform: uppercase on a pre-existing element (say from a composition authored before HF#3141 landed) is visible in preview and stripped on save. Reasonable to treat this as a documented one-time migration cost — pre-existing files are outside the write-path guarantee per HF#3141's PR body — but worth explicitly deciding. Options: (a) filter ownStyle through FORMATTING_STYLE_PROPS and match the sanitizer exactly; (b) leave as-is and accept the migration divergence; (c) widen FORMATTING_STYLE_PROPS if any properties are load-bearing for real HF text layers. Design decision — happy either way.

• Recursion nit — still open (walk at :237-266 is still recursive). Cheap fix for consistency with HF#3141 R3's iterative sanitizer, not a blocker.

CI

New head c6d4df89 just pushed; most checks pending. Preflight + Detect changes green. Recommend waiting on Tests, Tests on windows-latest, and the regression roll-up before merge.

Verdict

LGTM from my side on the R2 delta — Via's blockers are addressed with genuinely better mechanisms (not papered over), the allowlist-based preservedAttributes is a nicer shape than the two-item denylist, and the per-run overpaint reconciliation is more principled than the host-wide mirror. My one narrow concern (subtreeCharLength) is fine as a follow-up; the ownStyle question is worth naming a decision on.

Via's CHANGES_REQUESTED is still active on record — she's the fresh approver to look for once she re-reviews.

— Review by Rames D Jusso

[vanceingalls]

Reviewed delta 4d5913a7..c6d4df89c.

Blocker 1 (grapheme widening) — FIXED

isTrailingHalf/surrogate logic is gone. New graphemeBounds (inlineTextStyleRange.ts:151-164) uses new Intl.Segmenter(undefined, { granularity: "grapheme" }) (:158), enumerates segment end-boundaries via atEndOfSegment (:166-168), and snaps [start, end) to surrounding grapheme boundaries via boundaryAtOrBefore / boundaries.find(b >= end). This subsumes surrogate pairs, ZWJ, RI, skin-tone, and combining marks in one primitive.

Test coverage at .test.ts:347-359 (it.each) with four rows: "a zero-width-joiner family" (👨‍👩‍👧), "a regional-indicator flag" (🇺🇸), "an emoji with a skin-tone modifier" (👍🏽), "a combining-mark character" (é). Boundary is placed at 2, Math.max(3, grapheme.length) so at least one endpoint lands inside the cluster; assertion host.innerHTML === 'a<span style="color: red">${grapheme}</span>b' requires the cluster to end up whole in one span — impossible under the old surrogate-only widener for any of the four cases.

Blocker 2 (per-run mirror) — FIXED

Renamed to reconcileFillColors(host) (inlineTextStyleRange.ts:120-141), called after render (:101). Drop-and-reassess pass per the R1 ask:

• :128-131 — if existing -webkit-text-fill-color equals span.style.color (the shape of a previously generated mirror), remove it first so the computed fill reflects what would paint the run without the mirror.
• :132-139 — re-query computed.webkitTextFillColor + computed.color; only re-add the mirror when fill !== color.

Per-run check: yes — getComputedStyle(span) resolves that span's own ancestor path. Reassess-and-drop: yes — line 130 unambiguously.

Tests at .test.ts:603-617 ("mirrors only the run whose own ancestor path is overpainting") exercises a stubFill that returns overpaint for "left" and passthrough for "right"; asserts left['-webkit-text-fill-color'] === "blue" and right === "". .test.ts:619-631 ("drops a generated mirror when the ancestor stops overpainting") flips overpainted between two edits and asserts the mirror is gone after the second call.

Nit 3 (attribute allowlist) — FIXED

preservedAttributes (:451-459) filters via isRichTextFormattingAttribute imported from @hyperframes/core/rich-text-sanitize (which is FORMATTING_ATTRS = new Set(["data-hf-text-key", "data-hf-id"]) at richTextSanitize.ts:59). data-hf-id is additionally stripped in the editor via DERIVED_ATTR (:454), so only data-hf-text-key is carried. Test "does not carry attributes that the persistence sanitizer will remove" (.test.ts:490-500) asserts aria-label and onclick dropped.

Nit 4 (contenteditable=false) — FIXED (attribute check, not .isContentEditable)

editingHost (:218-229) still uses .closest("[contenteditable]") but explicitly bails when the resolved attribute value is "false" (:220-222): return editable.getAttribute("contenteditable")?.toLowerCase() === "false" ? null : editable;. Semantically equivalent to .isContentEditable for the R1 hazard, and correctly refuses a nested ce=false island (which [contenteditable=""], [contenteditable="true"] would have wrongly matched through). Test "does not edit through a contenteditable=false boundary" (.test.ts:502-510).

Nit 5 (exact color-property target) — FIXED

No span[style*='color'] in the file. reconcileFillColors iterates host.querySelectorAll<HTMLElement>("span") (:123) and filters via span.style.color — a property-specific CSSStyleDeclaration lookup that returns only the color value and can never bleed to background-color / border-color / caret-color. Substring hazard eliminated by construction.

Nit 6 (collapsed-caret char-before) — FIXED

readInlineStyle collapsed branch (:197-198): .slice(collapsed ? Math.max(0, start - 1) : start, collapsed ? Math.max(1, start) : end). For a collapsed caret at start > 0, slices [start-1, start) — the character before the caret. Test "reports the character before a collapsed caret at a style boundary" (.test.ts:283-289) mounts <span style="color: red">ab</span>cd, places caret at position 2, asserts styles.color === "red".

CI at head

Test/regression FAILURE entries at 06:23:34 + 06:23:46 were push-cancellation artifacts (3-4 second completions). Fresh runs superseded — new Test IN_PROGRESS at 06:27:12, fresh regression-shards 1-8 all in-flight. Live jobs at scan (Format, File size check, Semantic PR title, Detect changes, SDK) all green. No fresh failing signal.

Cross-reviewer convergence

Rames posted an independent R2 at c6d4df89 at 06:26:46Z: "Every item Via flagged verifies clean. My R1 set is partially closed too — one narrow inline concern remains open, the rest were either directly addressed or resolved by side effect." Two independent reviewers converge on both blockers fully closed at exact head.

New findings

None regressive. Small quality notes not worth blocking: Intl.Segmenter instantiated per-call in graphemeBounds (memoisable, negligible for element-scoped text); boundaryAtOrBefore's previous = offset initial is dead since boundaries always include 0. Docstrings match behavior.

Verdict

APPROVE at c6d4df89c. Both R1 blockers fully closed at their failing edges (Intl.Segmenter subsumes ZWJ/RI/skin-tone/combining; reconcileFillColors is per-span drop-and-reassess), and all four nits addressed with targeted tests. Rames converges independently. Ship on fresh CI landing green.

— Via

[james-russo-rames-d-jusso]

R3 verified at 9ee7b490.

All five of my R2-surviving concerns are closed with the same mechanism-not-papered-over quality as HF#3141's R3 pass. Miguel didn't just chase symptoms — every fix picked up the right primitive.

1. Non-styling tag flatten → sanitizer-parity unwrap (my R1 #1). childRunFrames at inlineTextStyleRange.ts:279-293 checks isRichTextFormattingTag(element.tagName). Non-formatting tags now walk their children with the parent's inherited style + origin — the sanitizer's unwrap semantics, mirrored in the engine. <a href="X" style="color: red">foo</a> → the <a>'s attrs and style are entirely ignored on the way in, so nothing is preserved to leak into the rebuild. Test at .test.ts:325-337 parametrizes across a / sub / sup / mark / s and pins the exact behavior. The comment on :290 ("Tags the sanitizer unwraps cannot own a layer") names the invariant.

2. Style-property preservation matches sanitizer (my R1 #4). New isRichTextFormattingStyle(property, value) in sanitizer at richTextSanitize.ts:107-113 — extracted from filterStyle at :195-201 which now uses it (same shared-predicate pattern as isRichTextFormattingAttribute). ownStyle at :301-311 now filters through this predicate. text-transform: uppercase on a pre-existing element → dropped by the engine before rebuild. Test at .test.ts:548-554 pins: <span style="color: red; text-transform: uppercase">abc</span> + font-weight:700 → <span style="color: red; font-weight: 700">abc</span>. Preview = save.

3. identityOf canonical encoding (my R1 #2 — already auto-closed at R2 by allowlist filter, now canonicalized explicitly). JSON.stringify([...preservedAttributes(element)].sort(...)) at :284. Unambiguous by construction — no &/= collision surface. Belt-and-braces alongside the allowlist filter that already made a colliding value inconstructible.

4. subtreeCharLength counts nested BRs (my R1 #3 — the narrow off-by-one). Now iterative subtree walk at :531-538, summing charLength(current) for every descendant node. Test at .test.ts:391-403 pins the exact scenario from my R1 anchor: host = a<span>b<br>c</span>d, range.setStart(host, 1); range.setEnd(host, 2) produces the correct styling on b and c separately (proving the BR was counted).

5. Iterative run traversal + 2000-nesting regression (my body-level recursion nit). readRuns at :236-248 now uses a RunWalkFrame[] stack — same iterative-with-typed-frames pattern as HF#3141 R3's sanitizer. Test at .test.ts:405-419 builds a 2000-deep span nest and asserts no stack growth (applyInlineStyle completes, output has 1 span with "x"). Symmetry with the sanitizer fix pattern is nice.

Positive verifications on R3

• Semantic consistency between engine and sanitizer. Both isRichTextFormattingAttribute and isRichTextFormattingStyle now shared predicates; unsupported tags are unwrap-not-flattened in both. Round-trip through engine → serialize → sanitize is now byte-stable for allowlisted content (modulo data-hf-id which is stamped fresh server-side each time).
• Traversal semantics preserved. The iterative visitRunFrame/childRunFrames reverses children before push so pops are in document order — same trick as HF#3141 R3's iterative sanitizer. Traced a simple <span>ab<b>cd</b>ef</span> and confirmed run order: ab, cd, ef (with the <b> in the middle contributing font-weight to cd).
• identityOf JSON encoding stays deterministic on preservedAttributes output (Map iteration order is insertion-order-stable in JS, and .sort([a],[b]) guarantees canonical order regardless of source). Two identical attr sets produce identical JSON.

What I didn't verify

• Did not run the 61 + 62 tests locally; trusting Miguel's green + the sample traces.
• Vance's APPROVED at R2 (c6d4df89, 06:28Z) may be stale under branch-protection's "dismiss stale reviews on new commits" — worth confirming her R3 re-approval before merge. If the HF ruleset dismisses on push, Miguel will need her fresh stamp.

Verdict

LGTM. This is one of the cleaner R3 passes I've seen this arc — every fix pulled the mechanism into alignment with the sanitizer rather than adding a workaround. The isRichTextFormattingStyle extraction is the tidy pattern-completion twin of isRichTextFormattingAttribute.

— Review by Rames D Jusso

[vanceingalls]

Reviewed delta c6d4df89c..9ee7b4900 — one commit, fix(studio): align inline styling with persistence, three files touched.

Rames #3 (subtreeCharLength nested <br>) — CLOSED

subtreeCharLength at inlineTextStyleRange.ts:530-538 now walks the subtree with an explicit stack, summing charLength(current) per node. charLength at :523-526 returns 1 for BR, so nested <br> inside (host, childIndex) ranges count correctly. Regression at .test.ts:391-405 mounts a<span>b<br>c</span>d, sets range.setStart(host, 1) / setEnd(host, 2), asserts two spans ["b","c"] — exactly the shape Rames flagged.

Rames #4 (ownStyle allowlist) — CLOSED

ownStyle at :300-309 now guards each declaration with isRichTextFormattingStyle(property, value), imported from @hyperframes/core/rich-text-sanitize (:27). New export at richTextSanitize.ts:107-114 reuses FORMATTING_STYLE_PROPS + UNSAFE_VALUE — the same allowlist filterStyle delegates to (:198-199). Test at .test.ts:548-554 mounts <span style="color: red; text-transform: uppercase">, applies font-weight:700, asserts text-transform stripped from final innerHTML — exactly the preview-vs-persistence divergence Rames named.

Iterative walk (Rames nit) — CLOSED

readRuns at :239-249 allocates pending: RunWalkFrame[] and drains via for (let frame = pending.pop(); frame; frame = pending.pop()). visitRunFrame (:251-265) and childRunFrames (:275-292) return frames instead of recursing. Test "walks deeply nested formatting without using the call stack" at .test.ts:406-419 builds 2,000 nested <span>s and asserts single-span final state — would blow the stack under the prior recursive walk.

Identity encoding canonical — CLOSED

identityOf at :295-298 returns JSON.stringify([...preservedAttributes(element)].sort(...)). Previous ${name}=${value} joined by & was collision-prone with values containing = / &; JSON-stringifying the sorted [[name, value], ...] tuple array is stable + escaped + collision-proof by construction.

Preview/persistence coverage — CLOSED

Two new alignment tests. Parametric it.each(["a","sub","sup","mark","s"]) at .test.ts:325-337 asserts unsupported tags are unwrapped exactly as isRichTextFormattingTag in the persistence sanitizer would. Enabled by childRunFrames:280-287 gating nextInherited / nextOrigin on isRichTextFormattingTag(element.tagName) — a non-formatting tag can no longer own a layer, matching what would survive persistence. Style-property counterpart at :548-554 closes the same loop for declarations.

R2 items regression check — CLEAN

• graphemeBounds still uses Intl.Segmenter at :159
• reconcileFillColors still per-span drop-and-reassess at :121-142 (drops mirror at :130-132, re-adds if fill ≠ color at :138-140)
• preservedAttributes still allowlist-driven via isRichTextFormattingAttribute at :477
• editingHost still bails on contenteditable="false" at :223
• readInlineStyle still reports char-before on collapsed caret at :198-200

CI at head

9ee7b4900: Preflight (lint + format) success, Detect changes success, Analyze (actions)/(python) success, CodeQL neutral, 12 SUCCESS total. Regression shards 1-9, perf (parity/fps/drift/load/scrub), Preview parity, Render/Tests on windows-latest, Analyze (javascript-typescript) all IN_PROGRESS (12). 11 CANCELLED are auto-superseded prior-head runs. Zero failures at head.

Verdict

APPROVE. Miguel's single delta commit closes all five items Rames flagged with mechanical, testable fixes: subtreeCharLength iterates the subtree so nested <br> count correctly; ownStyle filters through the sanitizer-owned isRichTextFormattingStyle; readRuns / visitRunFrame are stack-driven with a 2,000-level regression test; identityOf uses JSON canonicalization; and two new tests assert editor output equals what the sanitizer would keep for both unsupported tags and non-allowlisted style properties. None of my R2 approval items regressed. Merge on the remaining IN_PROGRESS checks landing green.

— Via

[vanceingalls]

Re-stamp at 32683fd86 — delta 9ee7b4900..32683fd86 is one commit test(studio): pin inline identity delimiters. Impl byte-identical to my R3 approval.

Delta is one file, +13/-0: new test "cannot merge identities through delimiter-bearing attribute values" at inlineTextStyleRange.test.ts:576-587. Constructs two spans where one has data-hf-text-key="a&data-hf-text-key=b" (a value containing the OLD &/= delimiters) and the other has data-hf-text-key="b". Applies a color style across both. Asserts:

• host.querySelectorAll("span") has length 2 (distinct identities preserved, not merged)
• exactly one [data-hf-text-key="b"] matches (no false collision hit)
• host.innerHTML does not contain "a&" (the delimiter payload never leaks into a merged key)

Would have failed under the pre-R3 ${name}=${value} joined by & encoding. JSON canonicalization at identityOf:295-298 makes it a hard fixed point.

CI at head: 0 failures. Merge is CLEAN pending remaining IN_PROGRESS checks.

— Via