ReviewOS

stacks/stx

x-for keyed reuse round-trips every row through a DocumentFragment, so a list cannot contain a focused input

#1882
Closed glennmichael123 opened this 11 days ago · {{ comments.length }} {{ comments.length === 1 ? 'comment' : 'comments' }}
11 days ago

x-for's keyed-reuse branch moves every surviving row through a DocumentFragment on each re-diff. Node identity is preserved, but removing a focused node from the document blurs it and resets activeElement — so a list may not contain a focused input. Any keystroke that touches the backing array kicks the user out of the field they're typing in.

Where

src/signals.ts:3711-3713, in the keyed-reuse path:

const fragment = document.createDocumentFragment()
liveNodes.forEach(groupNode => fragment.appendChild(groupNode))
parent.insertBefore(fragment, placeholder)

appendChild into a fragment detaches the node from the document first. That happens for every reused key, unconditionally — I read the whole branch (3695-3726) and there is no positional check, so a row that hasn't moved is still detached and re-inserted.

Two things make it worse:

  • signals.item.set(list[i]) at :3722 re-runs for every reused row, so any write to the list re-diffs all of them.
  • Without :key, getItemKey falls back to String(index) (:3556-3560), so the same path runs for unkeyed loops. There is no opt-out.

Why it matters

This is the single reason our composer's per-network override panel is written out nine times by hand.

The panel is nine near-identical 10-line blocks (composer.stx:87-176) that differ only in a provider slug and a label. Each contains a <textarea> the user types into while the shared post body is also being edited — so the row must survive re-renders with its caret, selection and undo history intact. Inside an x-for it doesn't. We fell back to nine static blocks with x-show, plus nine separate signals, plus a lookup map to recover the keyed access the template couldn't do.

That's ~90 lines of literal repetition and 9 signals where one loop would be 10 lines and one signal.

I verified node identity is preserved across an update (same textarea object before and after) — so this isn't a reuse bug. happy-dom doesn't emulate blur-on-detach, so it can't demonstrate the focus loss directly; the DOM spec behaviour is what makes it fatal, and it's why the workaround was necessary in a real browser.

Ask

A minimal-move patch: leave a reused node exactly where it is when its position is unchanged, and use parent.insertBefore(node, ref) — which moves without detaching — when it must move. Never a fragment round-trip.

With that, the composer collapses to:

<div x-for="row in variantRows()" :key="row.provider" x-show="row.visible">
  <textarea x-model="variants[row.provider]" @input="onVariantInput()"></textarea>
</div>

It'd also be worth stating the guarantee in the docs — "a row keeps focus, caret and undo history across updates" — rather than leaving it as an implementation detail. Right now nothing warns you; the field just loses focus mid-word and it reads like a mysterious app bug.

Related: the x-model lvalue limitation is the other half of why this collapsed into nine signals; filing separately.

Environment

@stacksjs/stx 0.2.153, Bun 1.3.1.

11 days ago

Fixed in 8189325, implemented exactly as the ask describes — minimal move, never a fragment.

A cursor walks the existing rendering in order. A node already at the cursor is left entirely alone; one that genuinely has to move is moved with insertBefore, which relocates without a detach/reattach cycle. A newly created row is inserted before the cursor rather than before the placeholder, so adding one in the middle no longer forces every row after it to be moved back into order.

Measured, before → after:

beforeafter
unchanged list of 33 insertions0
two-row reorder2 fragments0
three-row rotation3 insertions1

On testing the actual symptom: you were right that happy-dom does not emulate blur-on-detach, and I want to be explicit that the tests therefore do not assert focus loss. An activeElement assertion would pass against the broken runtime and prove nothing, which is worse than not having it. What they pin is the mechanism: no fragment is ever created, an unchanged list performs zero DOM insertions, and a reused node stays connected throughout. The focus, caret and undo-history behaviour follows from the DOM spec once those hold — but it is inference, not measurement, and someone should confirm the composer panel in a real browser before the nine hand-written blocks come out.

The other 8 of the 11 tests are reconciliation controls — insert in the middle, prepend, remove, reverse, wholesale replace, empty, node identity. They pass in both states deliberately: ordering was already correct, and the risk of this change is breaking it rather than failing to fix it.

Your related note is done too: the x-model lvalue limitation is fixed in 860a686 (#1883), so x-model="variants[row.provider]" now writes through the signal instead of over it. Between the two, the collapse to a single x-for you sketched should work — worth trying it before I claim it does.

I have not documented the guarantee in the docs yet, which was the last part of your ask. Worth stating as "a row keeps focus, caret and undo history across updates" once someone has confirmed it in a browser.

11 days ago

Claiming this one — working on it now.

11 days ago

Fixed in 81893251cf ("fix(signals): x-for leaves a row that has not moved alone").

A reused row is no longer round-tripped through a DocumentFragment when its position is unchanged, so a row that keeps its place keeps its focus, selection and any un-committed DOM state.

Pinned by packages/stx/test/signals/for-minimal-move.test.ts. Verified passing on current main.

Sign in to comment on this issue.