Skip to content

Commit 0035476

Browse files
committed
fix(core): address review on the Collapsible extension
- `splitBlockTr` kept the children on the first half unconditionally, which at a start-of-title split stranded them on the now-empty block while the title moved away. They now follow the content, leaving the block intact with a new one above it, as Notion does. - `Shift-Enter` reached the collapsible Enter handler for blocks opting out of hard breaks with `hardBreakShortcut: "none"`, starting a child instead of splitting. Restricted to plain Enter. - `handleCollapsibleDrop` compared an undefined target id against itself and passed, letting a drop land at an unvalidated position. - The child-count side table was never pruned. The count now rides on the block's own decoration, so ProseMirror discards it with the block. - The chevron gets `aria-controls` naming the group it discloses. - `createBlockDecorationPlugin` takes `Omit<EditorProps, "decorations">`, so passing a `decorations` prop that would be silently dropped won't compile. - `isBlockCollapsible` takes only the schema, removing a cast in the exporter.
1 parent 6676aaa commit 0035476

9 files changed

Lines changed: 245 additions & 29 deletions

File tree

‎packages/core/src/api/blockManipulation/commands/splitBlock/splitBlock.test.ts‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,26 @@ describe("Test splitBlocks", () => {
101101
expect(newBlock.children).toEqual([]);
102102
});
103103

104+
it("Block has children, split at start of content", () => {
105+
getEditor().transact((tr) => {
106+
setSelectionWithOffset(tr.doc, "paragraph-with-children", 0);
107+
});
108+
109+
splitBlock(getEditor().transact((tr) => tr.selection.anchor));
110+
111+
// The first half keeps no content, so the children go with the second one,
112+
// which does — rather than being stranded on an empty block.
113+
const original = getEditor().document.find(
114+
(block) => block.id === "paragraph-with-children",
115+
)!;
116+
expect(original.children).toEqual([]);
117+
118+
const newBlock = getEditor().document.find((block) => block.id === "0")!;
119+
expect(newBlock.children.map((child) => child.id)).toEqual([
120+
"nested-paragraph-0",
121+
]);
122+
});
123+
104124
it("Keep type", () => {
105125
getEditor().transact((tr) => {
106126
setSelectionWithOffset(tr.doc, "heading-0", 4);

‎packages/core/src/api/blockManipulation/commands/splitBlock/splitBlock.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,13 @@ export const splitBlockTr = (
5858
// To avoid that, the group is detached before the split and put back on the
5959
// original block afterwards. Both happen in the same transaction, so this is
6060
// still a single undo step.
61-
const childContainer = info.childContainer;
61+
//
62+
// Splitting at the very start is the exception: there the first half keeps no
63+
// content at all and the whole of it moves to the second, so the children
64+
// follow it rather than being left behind on an empty block. That's what a
65+
// plain `tr.split` already does.
66+
const splitAtStart = posInBlock === info.blockContent.beforePos + 1;
67+
const childContainer = splitAtStart ? undefined : info.childContainer;
6268

6369
if (childContainer) {
6470
// The group sits after `posInBlock`, so deleting it doesn't shift the split

‎packages/core/src/api/exporters/html/util/serializeBlocksInternalHTML.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,7 @@ function serializeBlock<
204204
// `CollapsibleExtension` doesn't run when exporting, so the chevron is added
205205
// here to keep the layout identical to the editor's. It can't fold anything,
206206
// so it's rendered inert.
207-
if (isBlockCollapsible(editor as any, block.type as string, props)) {
207+
if (isBlockCollapsible(editor, block.type as string, props)) {
208208
const collapseButton = createCollapseButton(true);
209209
collapseButton.setAttribute("aria-hidden", "true");
210210
collapseButton.setAttribute("tabindex", "-1");

‎packages/core/src/editor/Block.css‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -339,9 +339,16 @@ NESTED BLOCKS
339339
padding: 3px;
340340
}
341341

342-
/* Headings pad 18px at the top rather than 3px (see HEADINGS above), which
343-
lifts the centred chevron off the text; this pushes it back down by half the
344-
difference. */
342+
/* Headings pad 18px at the top (see HEADINGS above), so centring the chevron
343+
against the whole content box leaves it sitting above the text. This pushes it
344+
back down.
345+
346+
The value is empirical, and only right for a single-line heading around the
347+
default level — the correct offset depends on the heading's line box, which
348+
scales with `--level`, and that custom property is set on the content element,
349+
a sibling the button can't read it from. Not visible in the screenshot tests
350+
either, which mask `.bn-collapse-button`. Wants a proper pass: align to
351+
`flex-start` and offset by the first line's height. */
345352
.bn-block-outer[data-collapsible="true"]:has(
346353
> .bn-block > .bn-block-content[data-content-type="heading"]
347354
)

‎packages/core/src/extensions/Collapsible/Collapsible.test.ts‎

Lines changed: 120 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,13 @@
1+
import { TextSelection } from "prosemirror-state";
12
import { afterEach, beforeEach, describe, expect, it } from "vite-plus/test";
23

3-
import type { PartialBlock } from "../../blocks/defaultBlocks.js";
4+
import { BlockNoteSchema } from "../../blocks/BlockNoteSchema.js";
5+
import {
6+
defaultBlockSpecs,
7+
type PartialBlock,
8+
} from "../../blocks/defaultBlocks.js";
49
import { BlockNoteEditor } from "../../editor/BlockNoteEditor.js";
10+
import { createBlockSpec } from "../../schema/index.js";
511
import { CollapsibleExtension } from "./Collapsible.js";
612

713
/**
@@ -26,8 +32,16 @@ afterEach(() => {
2632
window.localStorage.clear();
2733
});
2834

29-
function createEditor(initialContent: PartialBlock<any, any, any>[]) {
30-
const editor = BlockNoteEditor.create({ initialContent });
35+
function createEditor(
36+
initialContent: PartialBlock<any, any, any>[],
37+
schema?: BlockNoteSchema<any, any, any>,
38+
): BlockNoteEditor<any, any, any> {
39+
// Spread rather than passed as `undefined`, which isn't the same as absent
40+
// here — it overrides the default schema with nothing.
41+
const editor = BlockNoteEditor.create({
42+
initialContent,
43+
...(schema ? { schema } : {}),
44+
} as any);
3145
editor.mount(document.createElement("div"));
3246
activeEditors.push(editor);
3347

@@ -67,18 +81,44 @@ function addChildButton(editor: BlockNoteEditor<any, any, any>, id: string) {
6781
* equivalent: it replays only the *steps* a handler dispatched, so a selection
6882
* the handler set is dropped.
6983
*/
70-
function pressEnter(editor: BlockNoteEditor<any, any, any>) {
84+
function pressEnter(
85+
editor: BlockNoteEditor<any, any, any>,
86+
{ shift = false }: { shift?: boolean } = {},
87+
) {
7188
const view = editor.prosemirrorView;
7289
const event = new KeyboardEvent("keydown", {
7390
key: "Enter",
7491
code: "Enter",
92+
shiftKey: shift,
7593
bubbles: true,
7694
cancelable: true,
7795
});
7896

7997
view.someProp("handleKeyDown", (handler) => handler(view, event));
8098
}
8199

100+
/** A collapsible block that takes no hard breaks, so Shift-Enter falls through. */
101+
const NO_HARD_BREAK_SCHEMA = BlockNoteSchema.create({
102+
blockSpecs: {
103+
...defaultBlockSpecs,
104+
collapsibleNoHardBreak: createBlockSpec(
105+
{
106+
type: "collapsibleNoHardBreak",
107+
propSchema: {},
108+
content: "inline",
109+
},
110+
{
111+
meta: { collapsible: true, hardBreakShortcut: "none" },
112+
render: () => {
113+
const dom = document.createElement("p");
114+
115+
return { dom, contentDOM: dom };
116+
},
117+
},
118+
)(),
119+
},
120+
});
121+
82122
const TOGGLE_HEADING: PartialBlock<any, any, any> = {
83123
id: "toggle-heading",
84124
type: "heading",
@@ -171,6 +211,26 @@ describe("CollapsibleExtension", () => {
171211
expect(chevron(editor, "toggle-heading")).not.toBe(null);
172212
});
173213

214+
it("points the chevron at the group it discloses, for screen readers", () => {
215+
const editor = createEditor(TOGGLE_HEADING_WITH_CHILD);
216+
const button = chevron(editor, "toggle-heading")!;
217+
218+
const controls = button.getAttribute("aria-controls");
219+
expect(controls).toBe("bn-collapse-children-toggle-heading");
220+
// The link has to actually resolve, or it's worse than saying nothing.
221+
expect(
222+
editor.prosemirrorView.dom.querySelector(`#${controls}`)?.className,
223+
).toContain("bn-block-group");
224+
225+
// Nothing to point at while the block has no children.
226+
const childless = createEditor([
227+
{ id: "toggle", type: "toggleListItem", content: "Toggle" },
228+
]);
229+
expect(chevron(childless, "toggle")!.hasAttribute("aria-controls")).toBe(
230+
false,
231+
);
232+
});
233+
174234
// #2124
175235
it("drops the chevron when a toggle heading is converted to a plain heading", () => {
176236
const editor = createEditor(TOGGLE_HEADING_WITH_CHILD);
@@ -397,11 +457,66 @@ describe("CollapsibleExtension", () => {
397457
expect(editor.document[0].type).toBe("paragraph");
398458
expect(editor.document[0].children).toHaveLength(0);
399459

400-
// A cursor part-way through the title splits it, keeping the children.
460+
// A cursor part-way through the title splits it, keeping the children on
461+
// the half that keeps the title.
401462
editor.setTextCursorPosition("toggle", "start");
463+
editor.transact((tr) =>
464+
tr.setSelection(TextSelection.create(tr.doc, tr.selection.from + 3)),
465+
);
402466
pressEnter(editor);
403467
const toggle = editor.document.find((block) => block.id === "toggle")!;
404468
expect(toggle.children.map((child) => child.id)).toEqual(["child"]);
405469
});
470+
471+
// At the very start there's no title left on the original half to keep the
472+
// children company, so they go with the text instead of being stranded on an
473+
// empty block. Notion likewise leaves the toggle intact and puts the new
474+
// block above it.
475+
it("moves the children with the title when splitting at the start", () => {
476+
const editor = createEditor([
477+
{
478+
id: "toggle",
479+
type: "toggleListItem",
480+
content: "Toggle",
481+
children: [{ id: "child", type: "paragraph", content: "Child" }],
482+
},
483+
]);
484+
collapsible(editor).setCollapsed({ id: "toggle" }, false);
485+
486+
editor.setTextCursorPosition("toggle", "start");
487+
pressEnter(editor);
488+
489+
const [empty, withTitle] = editor.document;
490+
expect(empty.content).toEqual([]);
491+
expect(empty.children).toHaveLength(0);
492+
expect(withTitle.content).toEqual([
493+
{ type: "text", text: "Toggle", styles: {} },
494+
]);
495+
expect(withTitle.children.map((child) => child.id)).toEqual(["child"]);
496+
});
497+
498+
// A block that opts out of hard breaks lets Shift-Enter fall through to the
499+
// default Enter chain. It should still split there rather than start a
500+
// child, the way it did before this handler existed. Needs a block with
501+
// `hardBreakShortcut: "none"`: with the default, the hard-break handler runs
502+
// first and this never gets the chance to go wrong.
503+
it("ignores Shift-Enter", () => {
504+
const editor = createEditor(
505+
[{ id: "toggle", type: "collapsibleNoHardBreak", content: "Toggle" }],
506+
NO_HARD_BREAK_SCHEMA,
507+
);
508+
collapsible(editor).setCollapsed({ id: "toggle" }, false);
509+
editor.setTextCursorPosition("toggle", "end");
510+
511+
pressEnter(editor, { shift: true });
512+
513+
expect(editor.document).toHaveLength(2);
514+
expect(editor.document[0].children).toHaveLength(0);
515+
516+
// ...whereas plain Enter still starts a child.
517+
editor.setTextCursorPosition("toggle", "end");
518+
pressEnter(editor);
519+
expect(editor.document[0].children).toHaveLength(1);
520+
});
406521
});
407522
});

‎packages/core/src/extensions/Collapsible/Collapsible.ts‎

Lines changed: 58 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -96,10 +96,12 @@ export type CollapsibleOptions = {
9696
* Whether a block of `type` with `props` declares itself collapsible.
9797
*
9898
* Shared with the HTML exporter, which has to reproduce what the extension
99-
* renders without an editor view to read decorations from.
99+
* renders without an editor view to read decorations from. It takes only the
100+
* schema rather than a whole editor, so an editor with a concrete block schema
101+
* can be passed without a cast.
100102
*/
101103
export function isBlockCollapsible(
102-
editor: BlockNoteEditor<any, any, any>,
104+
editor: Pick<BlockNoteEditor<any, any, any>, "schema">,
103105
type: string,
104106
props: Record<string, any>,
105107
): boolean {
@@ -111,15 +113,28 @@ export function isBlockCollapsible(
111113
: !!collapsible;
112114
}
113115

116+
/** The DOM id given to a collapsible block's child group, for `aria-controls`. */
117+
export function collapsibleChildrenId(blockId: string): string {
118+
return `bn-collapse-children-${blockId}`;
119+
}
120+
114121
/**
115122
* The chevron shown to the left of a collapsible block's content. It's the
116-
* disclosure control for the block's children, so it carries `aria-expanded`.
123+
* disclosure control for the block's children, so it carries `aria-expanded`,
124+
* and `aria-controls` naming what it discloses where there is anything to name —
125+
* a childless block has no group to point at.
117126
*/
118-
export function createCollapseButton(expanded: boolean): HTMLButtonElement {
127+
export function createCollapseButton(
128+
expanded: boolean,
129+
controls?: string,
130+
): HTMLButtonElement {
119131
const button = document.createElement("button");
120132
button.className = "bn-collapse-button";
121133
button.type = "button";
122134
button.setAttribute("aria-expanded", expanded ? "true" : "false");
135+
if (controls) {
136+
button.setAttribute("aria-controls", controls);
137+
}
123138
button.innerHTML =
124139
// https://fonts.google.com/icons?selected=Material+Symbols+Rounded:chevron_right:FILL@0;wght@700;GRAD@0;opsz@24&icon.query=chevron&icon.style=Rounded&icon.size=24&icon.color=%23e8eaed
125140
'<svg xmlns="http://www.w3.org/2000/svg" height="24px" viewBox="0 -960 960 960" width="24px" fill="CURRENTCOLOR"><path d="M320-200v-560l440 280-440 280Z"/></svg>';
@@ -147,10 +162,6 @@ export const CollapsibleExtension = createExtension(
147162

148163
const isExpanded = (id: string) => toggledState.get({ id });
149164

150-
// Child counts as of each block's last decoration, so that a block which
151-
// gains one can expand rather than appear to swallow it.
152-
const childCounts = new Map<string, number>();
153-
154165
/** Redraws the decorations, for state that isn't in the document. */
155166
function invalidate() {
156167
// A headless editor has no view to redraw.
@@ -176,8 +187,16 @@ export const CollapsibleExtension = createExtension(
176187
}
177188
});
178189

179-
function createChevron(id: string, expanded: boolean, disabled: boolean) {
180-
const button = createCollapseButton(expanded);
190+
function createChevron(
191+
id: string,
192+
expanded: boolean,
193+
disabled: boolean,
194+
hasChildren: boolean,
195+
) {
196+
const button = createCollapseButton(
197+
expanded,
198+
hasChildren ? collapsibleChildrenId(id) : undefined,
199+
);
181200
button.disabled = disabled;
182201
// Keeps the editor's selection (and focus) where it was.
183202
button.addEventListener("mousedown", (event) => event.preventDefault());
@@ -227,17 +246,26 @@ export const CollapsibleExtension = createExtension(
227246

228247
const collapsiblePlugin = createBlockDecorationPlugin(
229248
collapsiblePluginKey,
230-
(info, pos, id) => {
249+
(info, pos, id, previous) => {
231250
const props = info.blockContent.node.attrs;
232251
if (!isBlockCollapsible(editor, info.blockNoteType, props)) {
233252
return [];
234253
}
235254

236255
const childCount = info.childContainer?.node.childCount ?? 0;
237-
if (childCount > (childCounts.get(id) ?? childCount)) {
256+
// The count as of the last time this block was decorated, carried on the
257+
// decoration so it lives and dies with the block. A block that has just
258+
// gained a child expands, rather than appearing to swallow it — note
259+
// that this writes collapse state from inside `apply`, which a remote
260+
// insert can therefore trigger. It's idempotent, and the write is to
261+
// per-user storage rather than the document.
262+
const lastChildCount = previous.find(
263+
(decoration) => decoration.spec.childCount !== undefined,
264+
)?.spec.childCount;
265+
266+
if (childCount > (lastChildCount ?? childCount)) {
238267
toggledState.set({ id }, true);
239268
}
240-
childCounts.set(id, childCount);
241269

242270
const expanded = isExpanded(id);
243271
// Nothing to reveal, and no "add a block" button either.
@@ -252,21 +280,35 @@ export const CollapsibleExtension = createExtension(
252280
"data-collapsible": "true",
253281
...(expanded ? {} : { "data-collapsed": "true" }),
254282
},
255-
{ blockId: id },
283+
{ blockId: id, childCount },
256284
),
257285
Decoration.widget(
258286
pos + 1,
259-
() => createChevron(id, expanded, disabled),
287+
() => createChevron(id, expanded, disabled, childCount > 0),
260288
{
261289
blockId: id,
262290
side: -1,
263291
// Everything the button's DOM depends on, so ProseMirror reuses
264292
// it until one of them changes.
265-
key: `bn-collapse-button:${id}:${expanded}:${disabled}`,
293+
key: `bn-collapse-button:${id}:${expanded}:${disabled}:${
294+
childCount > 0
295+
}`,
266296
},
267297
),
268298
];
269299

300+
// Names the group the chevron discloses, for `aria-controls`.
301+
if (info.childContainer) {
302+
decorations.push(
303+
Decoration.node(
304+
info.childContainer.beforePos,
305+
info.childContainer.afterPos,
306+
{ id: collapsibleChildrenId(id) },
307+
{ blockId: id },
308+
),
309+
);
310+
}
311+
270312
// An expanded block with no children gets an "add a block" button, so
271313
// its chevron has something to reveal.
272314
if (expanded && editor.isEditable && childCount === 0) {

0 commit comments

Comments
 (0)