Skip to content

[lexical-list] Fix: updating list type to/from check type updates child DOM elements#7800

Merged
etrepum merged 4 commits intofacebook:mainfrom
james-atticus:list-reconcile-children-yjs
Sep 8, 2025
Merged

[lexical-list] Fix: updating list type to/from check type updates child DOM elements#7800
etrepum merged 4 commits intofacebook:mainfrom
james-atticus:list-reconcile-children-yjs

Conversation

@james-atticus
Copy link
Copy Markdown
Contributor

@james-atticus james-atticus commented Sep 8, 2025

Description

ListItemNode reconciliation depends on the properties of the parent node. Specifically, attributes are added/removed depending on whether the list type is checked. If the type changes to/from checked but the children are not manually marked as dirty, then they are not updated.

One option is a ListNode node transform which marks the children as dirty, however that doesn't work for collab which sets node properties directly. (NB: the reason this isn't currently failing in collab tests is that $insertList replaces the parent list node which causes the entire list tree to be recreated in Yjs and on other clients.)

Another option, which I've included in this PR to demonstrate, is to add a function to LexicalNode which allows a node to indicate to the reconciler that it depends on its parent in some way. The reconciler can check this when iterating ElementNode children and reconcile such nodes.

Other options considered:

  • Some way to allow ElementNodes to mark their children as dirty when they're marked as dirty. Slightly bigger change but might be the better way to go.
  • Change ListItemNode to not depend on its parent. To do this I think we'd have to drop listitemUnchecked from the theme (could use :not(.listitemChecked) instead) as well as role/aria-checked/tabIndex from the li element.

Test plan

Added an example failing test case which passes with the reconciler updateDOM change.

@vercel
Copy link
Copy Markdown

vercel bot commented Sep 8, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
lexical Ready Ready Preview Comment Sep 8, 2025 7:28am
lexical-playground Ready Ready Preview Comment Sep 8, 2025 7:28am

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 8, 2025
@james-atticus james-atticus force-pushed the list-reconcile-children-yjs branch from f9bbff3 to 1f49427 Compare September 8, 2025 05:08
config: EditorConfig,
) {
const parent = this.getParent();
if ($isListNode(parent) && parent.getListType() === 'check') {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive-by: if the parent becomes not-check, then we should be removing the check attributes.

@etrepum
Copy link
Copy Markdown
Collaborator

etrepum commented Sep 8, 2025

I don't think it's more elegant but this dependency could also be expressed with a MutationListener or updateDOM change to ListNode that manually calls updateDOM on its existing children when checked changes. I think we might have some other ways to express this kind of thing once extensions land, so maybe we should avoid adding new code to the reconciler in the meantime?

@james-atticus
Copy link
Copy Markdown
Contributor Author

Forgot updateDOM could be used for this. It already had a prevNode.__tag !== this.__tag check so I've added a check for __listType too.

@james-atticus james-atticus changed the title RFC [lexical-list] Fix: updating list type to/from checked should update child DOM elements RFC [lexical-list] Fix: updating list type to/from check type updates child DOM elements Sep 8, 2025
@james-atticus james-atticus marked this pull request as ready for review September 8, 2025 07:28
@james-atticus james-atticus changed the title RFC [lexical-list] Fix: updating list type to/from check type updates child DOM elements [lexical-list] Fix: updating list type to/from check type updates child DOM elements Sep 8, 2025
test('Should update list children when switching from checklist to bullet', async () => {
const {editor} = testEnv;

await waitForReact(() => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw {discrete: true} or even just await should be sufficient for the updateDOM to have already occurred (since the deferred update was scheduled earlier on the microtask queue than the await), waitForReact is probably only useful for decorators

@etrepum etrepum added this pull request to the merge queue Sep 8, 2025
Merged via the queue into facebook:main with commit 9571a9e Sep 8, 2025
39 checks passed
This was referenced Sep 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants