docs(block-editor): spec for the embedded-contentlet selection guard (#36985)

[rjvelazco]

Proposed Changes

Spec-Kit PR 1 of 2 for the reopened #36985 — specification only, no implementation.

Clicking an embedded contentlet card in the new Block Editor does create a NodeSelection. A byte-wise change-guard downstream then rebuilds the whole document and destroys it, so the card shows no selection ring and the toolbar's edit / delete / reorder actions stay disabled.

Why it looked random

The guard compares JSON.stringify of the incoming value against JSON.stringify of editor.getJSON(). TipTap normalizes on parse — fills schema defaults, drops undeclared attrs, reorders keys — so any stored document that does not round-trip byte-identically through the current schema fails the check permanently. It is a per-row data condition, not per content type or per site, which is exactly what authors were reporting.

Three confirmed triggers, all measured

Trigger	Cause
chartCount typo	The legacy editor wrote SetDocAttrStep('chartCount', …) until 3cd5549d35 (#26025, 2023-09-11). stripDocStats strips charCount, not chartCount.
Missing indent	IndentExtension declares indent (default 0) on heading / paragraph / blockquote. Older content has no such key; the editor adds it on parse.
Unknown node types	The string branch skips preserveUnknownNodesInDocument, so a custom block is compared placeholder-against-original.

Supplying a missing key is not enough — it has to be in the right position. That is the measured Case B in the spec, and it is what makes this a byte-comparison bug rather than a missing-attribute bug.

Why only one screen

The new Angular Edit Content screen binds with [formControlName], so the value arrives once through writeValue and the effect short-circuits forever after. The legacy JSP contentlet editor assigns blockEditor.value, so its effect is the loader and stays live — the only host where a re-push can reach setContent after load. The spec carries the full host-to-branch map.

What's in this PR

File	Contents
specs/36985-block-editor-selection-guard/spec.md	Problem, reproduction (server-free and local-instance), root cause, fix scope, non-goals, regression risk, AC-001…AC-008
specs/36985-block-editor-selection-guard/data-model.md	Value shapes and the normalization Node.fromJSON applies
specs/36985-block-editor-selection-guard/contracts/content-match.contract.md	The comparator's input→verdict table — the source for the Red-phase tests

plan.md, research.md, quickstart.md and checklists/ are Spec-Kit working artifacts and are gitignored by design (.gitignore:229-233).

Reproduction

No customer data required. The spec gives two recipes: a schema round-trip that needs no server, and an API-seeded BROKEN/CONTROL pair on a local instance. The legacy JSP contentlet editor is required — the content type needs CONTENT_EDITOR2_ENABLED=false, and the new Angular screen will not reproduce it.

A convenience seeding script is attached to the QA note on the issue rather than committed; the spec's steps are the source of truth.

Still open, recorded in the spec rather than hidden

• The local CONTROL click has not been performed. The host reasoning rests on customer-environment evidence plus the routing in dot-contentlet-editor.service.ts:204. If CONTROL also fails locally, "Why only one screen is affected" needs rewriting before anyone implements against it.
• What re-pushes the value in the legacy host is unidentified. commitEditor after remote customBlocks extensions is excluded for the local fixture. This does not block the fix — once the comparison is structural the verdict becomes "unchanged", so a re-push is harmless rather than merely suppressed.

Checklist

• Spec validated against specs/36985-block-editor-selection-guard/checklists/requirements.md
• No [NEEDS CLARIFICATION] markers
• No customer data — environment, hostnames and content identifiers replaced with a local fixture
• Every claim measured against the real schema via getSchema(createEditorExtensions(...)), not inferred
• Approval from another dev — required before the implementation PR proceeds

Notes for reviewers

• The spec deliberately keeps the Spec-Kit template's mandatory H2 headings, with the readable framing inside them, so /speckit-plan and /speckit-tasks keep resolving.
• Non-goals worth confirming: the click handler is not touched, no chartCount → charCount data migration, no port of click-to-select to the legacy editor (contentlet-block.node.ts:23-24 has none, so the FEATURE_FLAG_NEW_BLOCK_EDITOR=false workaround still reproduces the original symptom — separate ticket), and no apply-once memo in the effect while the re-push is unidentified.
• AC-005 is a behaviour change, not a regression guard. A bare array is measured false today and must be true — expect it to fail at Red.

Spec for #36985

🤖 Generated with Claude Code

This PR fixes: #36985

[claude]

Claude finished @rjvelazco's task in 1m 34s —— View job

---

Spec review — #36985 selection-guard

This is a documentation-only PR (three Spec-Kit artifacts, no implementation). I reviewed it for technical accuracy against the current source and internal consistency, since a wrong claim here propagates into the implementation PR.

• Read the three spec documents
• Verify code references cited in the spec against the actual source
• Check internal consistency (line numbers, AC/contract mapping)
• Post review

Verification results

Every load-bearing code claim I could check against the repo holds:

Spec claim	Source	Verdict
Faulty check at editor.component.ts:137-155	editorContentMatchesParsed spans 137–155	✅
Comparator is JSON.stringify(stripDocStats(incoming)) === JSON.stringify(stripDocStats(editor.getJSON()))	:143 + editorDocumentJsonText (:63-65)	✅
String branch (:143) skips preserveUnknownNodesInDocument; object branch (:152) applies it	confirmed	✅
Value effect :619-632, short-circuits on if (!v) return	:619-632, guard at :621	✅
Other callers commitEditor:498, writeValue:747	both confirmed	✅
stripDocStats strips only charCount/wordCount/readingTime, not chartCount	DOC_STAT_ATTRS = ['charCount','wordCount','readingTime']	✅
IndentExtension declares indent on heading/paragraph/blockquote	INDENT_TYPES (indent.extension.ts:19)	✅
withDocStats emit at :713	confirmed	✅

Notably, the existing docstring on stripDocStats (doc-stats.utils.ts:6-14) already names #36985 as its motivation and describes the exact "guard permanently false → setContent rebuilds doc → clobbers just-clicked contentlet selection" chain — which independently corroborates the spec's root-cause narrative.

Notes (non-blocking)

• 🟡 Minor: spec.md:228 — the "Where the code lives" table points stripDocStats to utils/doc-stats.utils.ts:4, but line 4 is the DOC_STAT_ATTRS constant; the stripDocStats function itself is at :19. Trivial, but worth correcting given the spec's stated precision bar ("every claim measured against the real schema").
• The spec is admirably explicit about what has not been verified — the local CONTROL click in the legacy JSP host, and the identity of the re-push. Both are called out in "Still pending" and "One thing we still don't know" rather than papered over. That's the right call; the reviewer approving this should read those two sections as the real open risk, since the "Why only one screen is affected" section rests on them.
• Contract requirement Test Branch and Commit #2 (schema.nodes.doc.spec.attrs is null, so Fragment.eq and Node.eq are equivalent today but must not be treated as interchangeable) is the sharpest part of the design and gives the implementer a clear, defensible instruction. Good.
• AC/contract mapping is coherent: contract rows 1, 3, 6, 8 (the four Red-phase behaviour changes) line up with AC-001/002/005/007, and the "row 4 or row 7 failing at Red means the fixtures are wrong" backstop is a nice guard against an over-permissive fix.

Verdict

No blocking issues. The spec is accurate against the code, internally consistent, and honest about its unverified assumptions — appropriate for a PR-1 spec gated on dev approval. The one factual nit (:4 vs :19) is optional to fix.

No issues found. (spec-only; the single note above is a documentation nit, not a defect.)

· issue-36985-...-spec

[rjvelazco]

Analyze my comments; don't take anything for granted, be critical, and respond as concisely as possible.

[rjvelazco]

Why is the JSON different? Is there a change in the Schema? It's because of a change in the way we add the charCount, wordCount and readingTime? See:

core/core-web/libs/new-block-editor/src/lib/editor/utils/doc-stats.utils.ts

Lines 19 to 38 in 3192816

	export function stripDocStats(json: JSONContent): JSONContent {
	const attrs = json?.attrs;
	if (!attrs) {
	return json;
	}
	const rest = { ...attrs };
	for (const key of DOC_STAT_ATTRS) {
	delete rest[key];
	}
	const next = { ...json };
	if (Object.keys(rest).length > 0) {
	next.attrs = rest;
	} else {
	delete next.attrs;
	}
	return next;
	}

and

core/core-web/libs/block-editor/src/lib/components/dot-block-editor/dot-block-editor.component.ts

Lines 438 to 450 in 3192816

	private updateCharCount(): void {
	const tr = this.editor.state.tr.setMeta('addToHistory', false);
	if (this.characterCount.characters() != 0) {
	tr.step(new SetDocAttrStep('charCount', this.characterCount.characters()))
	.step(new SetDocAttrStep('wordCount', this.characterCount.words()))
	.step(new SetDocAttrStep('readingTime', this.readingTime));
	} else {
	// If the content is empty, we need to remove the attributes
	tr.step(new RestoreDefaultDOMAttrs());
	}
	this.editor.view.dispatch(tr);

We should always preserve the same schema so if something is failing because of this, we should fix the schema.

[rjvelazco]

Is there a script we can add to the editor/JSP so we can ensure the Block Editor is rebuilding? This can be done locally while testing when you read this comment. Post here the result of the check

[rjvelazco]

Looks like this is a manual confirmation that we should do in the browser console which it's not the best practice. Is there a script we can add either in the block editor or in the JSP?

[rjvelazco]

Do we have a list of attr/marks/node/extension that were dropped? For compatibility we should support what the old editor supported when possible

[rjvelazco]

Why we allowed JSP to re-push the value/setContent? I think content should be loaded once because after that the Block Editor has control over the content and, if I'm not wrong (we should double check), JSP or the legacy form shouldn't modify the Block editor content outside itself.

[rjvelazco]

How heavy can this be? Considering that the Block Editor is a Tree object and can has a lot of nesting?

If this happens to be a heavy check, can we skip comparison and always send the new object to the JSP? If we do that, how can we skip temporal state that we don't want to push? Ex: The highlight we add on text selection or the class we add to the dotContentlet?

[rjvelazco]

Is there a ProseMirror/Tiptap util for this?

[rjvelazco]

Before going with the fix you propose, I want you to check this: Why we keep listening to the this.value();? Is there a way the content of the block editor changes outside the block editor? Is there any issue/ticket that back up this behavior?

[rjvelazco]

Another way to reproduce this issue without the script:

1. Set the FEATURE_FLAG_NEW_BLOCK_EDITOR to false
2. Create a Block Editor content with this structure:

# Heading 1

text testing

[Insert a `dotContent`]

Text testing

3. Save and Publish
4. Verify that the dotContent is clickable
5. Remove the FEATURE_FLAG_NEW_BLOCK_EDITOR or set it to true
6. Go back to the just created content
7. See the error when attending to click the dotContent