fix(core): recursively convert nested lists when toggling list type - #7995
fix(core): recursively convert nested lists when toggling list type#7995codewithsupra wants to merge 3 commits into
Conversation
toggleList's 'change list type' branch called tr.setNodeMarkup only on the top-level list node found by findParentNode. When the selection spanned a list containing nested sublists (e.g. a bulletList with nested bulletList items), only the outer list was converted — nested sublists kept their original type, breaking the visual nesting hierarchy. Fixes ueberdosis#7970 Add convertNestedLists() which walks the list subtree after the top-level conversion and converts any nested list node still on the original type. setNodeMarkup only changes a node's type/attrs (not its size), so captured positions remain valid without re-resolving after each conversion. Adds regression tests covering 2-level and 3-level nested list toggling, plus a changeset for the @tiptap/core patch bump.
✅ Deploy Preview for tiptap-embed ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
🦋 Changeset detectedLatest commit: 990312c The changes in this PR will be included in the next version bump. This PR includes changesets to release 72 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
arnaugomez
left a comment
arnaugomez
left a comment
There was a problem hiding this comment.
LGTM but I see that the fix only applies to bulletList and orderedList. Could a fix be included for taskList, too? The issue #7970 also mentions taskList.
@arnaugomez pointed out issue ueberdosis#7970 also names toggleTaskList, which this PR's first commit didn't cover — taskList wasn't tested at all. taskList has a different content model (taskItem+) than bulletList/orderedList (listItem+), so taskList <-> bulletList/ orderedList conversions go through toggleList's wrapInList + clearNodes fallback branch, not the setNodeMarkup 'change list type' branch the first commit fixed. That fallback had a separate, more severe bug: clearNodes() lifted every non-text node found by nodesBetween in document order, including wrapper nodes like listItem/ taskItem, not just textblocks. Lifting an outer wrapper before its still-to-be-visited nested children were processed mutated the document mid-iteration and corrupted the positions those later callbacks relied on. For a taskList nested inside a taskItem inside a taskList, this ejected the nested item's paragraph out of the list entirely, orphaning it as a bare top-level paragraph sibling — outright content loss, not just a wrong list type. clearNodes() now only lifts textblocks, processed in reverse document order so a deeper node's liftTarget is computed against a still-intact ancestor chain instead of one already reshaped by an earlier sibling's lift in the same pass. Container nodes are never lifted independently. This fixes the content-loss bug for all three commands ueberdosis#7970 named (toggleBulletList, toggleOrderedList, toggleTaskList) and top-level type conversion for taskList cross-family toggles now works correctly. Known remaining gap: when a cross-family conversion has a NESTED sublist of the other family (e.g. a taskList nested inside what's becoming a bulletList), the nested sublist itself keeps its original type/item-node type — only the outer list converts. taskItem and listItem are different node types with incompatible content models, so converting the nested sublist requires reconstructing it (new list + item nodes, not just setNodeMarkup), which this commit does not attempt. Flagged as a known limitation for now rather than risking an under-tested deeper rewrite; content-loss (the severe part) is fixed. Tests: packages/core/__tests__/clearNodes.spec.ts covers the fixed command directly; packages/extension-list/__tests__/ toggleListTaskListCrossFamily.spec.ts covers the taskList toggle scenarios named in ueberdosis#7970. Full monorepo suite (1368 tests) passes.
|
Good catch, thanks -- you're right, I only tested bulletList/orderedList and missed that the issue names toggleTaskList too. Pushed a second commit that covers it. taskList has a different content model (taskItem+) than bulletList/orderedList (listItem+), so taskList <-> bulletList/orderedList conversions go through toggleList's wrapInList + clearNodes fallback branch, not the setNodeMarkup branch the first commit fixed. Digging into that branch turned up something more serious than a wrong list type: clearNodes() was lifting every non-text node in the selection (including listItem/taskItem wrapper nodes, not just textblocks) in document order, using positions mapped against a doc that earlier iterations in the same pass had already mutated. For a taskList nested inside a taskItem inside a taskList, this actually ejected the nested item's paragraph out of the list entirely -- real content loss, not just a formatting mismatch. Fixed clearNodes() to only lift textblocks, processed in reverse document order so a deeper node's liftTarget is computed against a still-intact ancestor chain. This fixes the content-loss for all three commands named in #7970, and top-level type conversion now works correctly for taskList cross-family toggles. One gap I want to flag rather than paper over: when a cross-family conversion has a nested sublist of the other family (e.g. a taskList nested inside what's becoming a bulletList), the nested sublist itself keeps its original type -- only the outer list converts. taskItem and listItem have incompatible content models, so fixing that requires reconstructing the nested list (new list + item nodes), not just setNodeMarkup, and I didn't want to ship an under-tested version of that today. Happy to take that on as a follow-up if you'd like it in this PR rather than a separate one. Added packages/core/tests/clearNodes.spec.ts for the fixed command directly, and packages/extension-list/tests/toggleListTaskListCrossFamily.spec.ts for the taskList scenarios from #7970. Full monorepo suite (1368 tests) passes. |
Closes the gap flagged in the previous commit: nested sublists of a DIFFERENT list family (taskList inside what's becoming a bulletList, or vice versa) kept their original type because setNodeMarkup can only swap a node's own type — taskItem and listItem are incompatible node types, so the subtree has to be recreated node by node. convertNestedLists now classifies each nested sublist: - same family (bulletList <-> orderedList): wrapper type swapped in place via setNodeMarkup, walk keeps descending (unchanged behavior) - cross family: the whole subtree is rebuilt via rebuildListNode(), which recreates every item as the target item type and recursively converts deeper nested lists, then replaces the original in one size-preserving replaceWith (same child structure, only wrapper types change, so all positions collected up front stay valid) rebuildListNode() validates the rebuilt content against the target content models and bails (leaving the original untouched) rather than ever producing an invalid document. Family-specific item attributes with no equivalent on the target (taskItem's checked) are dropped — a bullet list has no checked state to carry them. The wrapInList fallback branches (taken for cross-family toggles of the top-level list) now also run the converter over every target-type list overlapping the selection, so sublists that survive inside the newly wrapped items get converted too. Tests upgraded from content-preservation-only to full-conversion assertions, plus new cases: cross-family sublist inside a same-family toggle, checked-state round-trip, and 3-level deep nesting. All 6 cross-family tests fail without this commit's source change (verified by reverting only toggleList.ts) and pass with it. Full monorepo suite: 1371 tests pass.
|
@arnaugomez the gap I flagged in my last comment is now closed — nested cross-family sublists convert fully. Latest commit adds rebuildListNode(): since taskItem and listItem are incompatible node types, setNodeMarkup can't convert across families, so the subtree is recreated node by node (items recreated as the target item type, deeper nested lists converted recursively) and swapped in with a single size-preserving replaceWith. The rebuild validates against the target content models and leaves the original untouched rather than ever producing an invalid document. convertNestedLists now classifies each nested sublist: same family (bulletList <-> orderedList) keeps the cheap setNodeMarkup path; cross family takes the rebuild path. The wrapInList fallback branches (used when the top-level toggle itself crosses families) run the same converter over the selection afterward, so sublists inside the newly wrapped items convert too. One deliberate behavior choice to flag for review: taskItem's checked state is dropped when converting away from the task family (a bullet list has no checked state to carry it), and converting back yields unchecked defaults. Covered by a test so the behavior is pinned either way — happy to change it if you'd prefer something else. Tests upgraded from content-preservation to full-conversion assertions, plus new cases: cross-family sublist nested inside a same-family toggle, checked-state round-trip, and 3-level deep nesting. All 6 fail with only the test change applied (verified by reverting just toggleList.ts) and pass with the fix. Full monorepo suite: 1371 tests green. This should now cover all three commands named in #7970 completely. |
|
Could you please not send in those huge LLM generated chunks of text? It's really hard to read and follow. There's no need to describe all tests by name or include all filenames in your updates. Otherwise I could just prompt my Claude to check this PR for updates. |
|
Fair feedback — I'll keep updates shorter and more to the point going forward. Thanks for flagging it. |
3 participants
Problem
Fixes #7970.
When converting between list types on a selection that spans a list containing nested sublists (e.g. toggleBulletList to toggleOrderedList), only the top-level list node gets its type changed. Nested sublists keep their original type, breaking the visual hierarchy — matches the repro in the issue exactly.
Root cause
In toggleList's "change list type" branch (packages/core/src/commands/toggleList.ts), tr.setNodeMarkup(currentList.pos, listType) is only called on the single node returned by findParentNode — the nearest matching ancestor list. It never descends into nested sublists inside the list items, so they keep their old node type.
Fix
Added convertNestedLists(), which walks the list subtree via node.descendants() after the top-level conversion and converts any nested list node still on the original type. Since setNodeMarkup only changes a node's type/attrs (not its size), positions captured before the walk stay valid throughout — no remapping needed between conversions.
Tests
Added packages/extension-list/tests/toggleListNestedStructure.spec.ts:
All three fail on main (reproducing the reported bug) and pass with this fix. Full extension-list + core suites (522 tests) pass with no regressions.
Changeset
Added a patch-level changeset for @tiptap/core.