Repository navigation
[lexical-list][lexical-mdast] Bug Fix: Keep GFM task items in bullet and number lists - #9475
conghuydever wants to merge 12 commits into
Conversation
… trip
## Description
GFM decides per item whether a list item is a task. Plain items and task items can share a list, and an ordered list can hold task items. GitHub renders both that way:
| Markdown | GitHub renders |
| --- | --- |
| `- plain` / `- [x] done` | a checkbox on `done` only |
| `1. [ ] todo` / `2. [x] done` | a numbered list with a checkbox on each item |
In Lexical only a check list has boxes, every item of a check list has one, and a check list can't be ordered. `@lexical/mdast` maps these lists onto that model, and the export can't tell what was written. So a round trip with no edits changes the Markdown:
| Markdown | Exported today |
| --- | --- |
| `- plain\n- [x] done\n- [ ] todo` | `- [ ] plain\n- [x] done\n- [ ] todo` |
| `1. [ ] todo\n2. [x] done` | `1. todo\n2. done` |
| `3. [x] three\n4. [ ] four` | `3. three\n4. four` |
The first case adds a box the author did not write. The other two lose the boxes and the ticks. An ordered list becomes a number list, and `ListItemNode` drops `checked` from items that are not in a check list.
This change adds `taskListItemState` to `state.ts`, next to the other states that keep the exact Markdown syntax. It is set on import only where the Lexical list can't hold the box:
- `'none'` on an item without a box in a check list. The export writes no box for it while it is unticked. Once ticked it gets `[x]`, so a tick is never lost.
- `'unchecked'` / `'checked'` on an item with a box in an ordered list. The list stays a number list, so the editor looks the same as today, and the export writes the box back.
Items created in the editor have no state, so they export as before: every item of a check list gets a box. That includes a bullet list turned into a check list, and an item added with Enter after a plain item (the state uses `resetOnCopyNode`). There are tests for these cases.
An ordered list with task items is written with `.`. The GFM task list writer (`mdast-util-gfm-task-list-item` 2.0.0) only adds a box after `1.`, so with `1)` the boxes were dropped even when `checked` was set.
Out of scope: the editor still shows a checkbox on a plain item in a check list, and no checkbox in an ordered list. Drawing those would need changes to `@lexical/list`. This PR only stops the Markdown from changing.
Alternative considered: import an ordered task list as a check list, so its boxes can be ticked in the editor. A check list has no numbers, so the editor would stop showing them, and the imported list type would change for everyone. Keeping the box in a state changes nothing the editor shows.
AI disclosure: the code, tests and this description were prepared with AI assistance.
## Test plan
New cases in `packages/lexical-mdast/src/__tests__/unit/MdastImportExport.test.ts` ("task lists"): round trips of mixed, ordered, nested and custom-start task lists, `1)` written as `1.`, an ordered task list still imported as a number list, a ticked plain item, an item added after a plain item, a bullet list turned into a check list, and a check list made in the editor.
### Before
```
$ pnpm exec vitest run packages/lexical-mdast/src/__tests__/unit/MdastImportExport.test.ts -t 'task lists'
× plain items next to task items
× a task item followed by a plain item
× an ordered task list
× an ordered task list with a custom start
× an ordered list with plain and task items
× a mixed task list nested in a task list
× an ordered task list nested in a bullet list
× writes an ordered task list with "." so the boxes are kept
× writes a box on an item added after a plain item
Tests 9 failed | 4 passed | 108 skipped (121)
```
The 4 that pass before guard against regressions: a ticked plain item, a bullet list turned into a check list, a check list made in the editor, and the ordered task list staying a number list.
### After
```
$ pnpm exec vitest run packages/lexical-mdast packages/lexical-markdown
Test Files 16 passed (16)
Tests 945 passed (945)
```
`pnpm run tsc` is clean, `npx eslint` and `npx prettier --check` on the changed files are clean, and `scripts/__tests__/unit/treeShakingSource.test.ts` passes.
Tested on macOS with Node 24.21.0. Browser, E2E, builds and Flow were not run.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
…and number lists
## Description
GFM decides per item whether a list item is a task. Plain items and task items can share a list, and an ordered list can hold task items. GitHub renders both that way:
| Markdown | GitHub renders |
| --- | --- |
| `- plain` / `- [x] done` | a checkbox on `done` only |
| `1. [ ] todo` / `2. [x] done` | a numbered list with a checkbox on each item |
In Lexical only a check list has checkboxes, every item of a check list has one, and a check list can't be ordered. `ListItemNode` drops `checked` from items that are not in a check list. So with `@lexical/mdast`:
| Markdown | Editor shows | Exported with no edits |
| --- | --- | --- |
| `- plain\n- [x] done` | a checkbox on both items | `- [ ] plain\n- [x] done` |
| `1. [ ] todo\n2. [x] done` | numbers, no checkboxes | `1. todo\n2. done` |
The first case adds a box the author did not write. The second loses the boxes and the ticks.
This change makes a task a property of the item, as in GFM.
`@lexical/list`:
- A `ListItemNode` with a checked state is a task item in any list. In a bullet or number list, `getChecked()` returns it, and `undefined` for a plain item. In a check list it returns a boolean as before.
- The `ListItemNode` and `ListNode` transforms no longer clear `checked` outside a check list.
- A task item gets the checkbox `role`, `aria-checked` and the `listitemChecked` / `listitemUnchecked` theme classes in any list, and a click on its checkbox toggles it. A plain item gets none of these. The Space and arrow key handlers use the same check as the click (an item of a check list, or an item with `aria-checked`), but I have no test for them.
- A list command (`$insertList`) still makes the whole list one kind: a bullet or number list makes every item plain, a check list makes every item a task and keeps the ticks.
- Enter copies the item, so a plain item is followed by a plain item and a task by an unticked task.
`@lexical/mdast`:
- Only an unordered list whose items are all tasks becomes a check list. Any other list keeps its kind, and its task items keep `checked`.
- The export writes `checked` per item, so plain items stay plain and task items keep their boxes.
- An ordered list with task items is written with `.`. The GFM task list writer (`mdast-util-gfm-task-list-item` 2.0.0) only adds a box after `1.`, so with `1)` the boxes were dropped.
Behavior changes:
- `getChecked()` can now be a boolean on an item of a bullet or number list. Before it was always `undefined` there.
- An item that has `checked` keeps it when it ends up in a bullet or number list, because the transforms no longer clear it.
- A GFM list that mixes plain items and task items imports as a bullet list with task items, not as a check list.
- Saved editor states should load as before: `exportJSON` writes `getChecked()`, which was `undefined` outside a check list, so older states have no `checked` there. I did not test an old state.
The playground theme styles the checked and unchecked classes in any list (`list-style-type: none`), so there a task item shows its checkbox in place of its bullet or number. I did not change the theme.
Out of scope: `@lexical/markdown` and its `CHECK_LIST` transformer are unchanged.
Alternative considered: keep the Lexical model and store the box an item was written with in a `@lexical/mdast` node state. That fixes the export, but the editor still shows a checkbox on plain items and none in ordered lists, and the state goes stale when a list command changes the list's type. An earlier version of this PR did that.
AI disclosure: the code, tests and this description were prepared with AI assistance.
## Test plan
New `packages/lexical-list/src/__tests__/unit/TaskListItems.test.tsx`: for a bullet and a number list holding a plain item and two task items, it checks `getChecked()`, the checkbox attributes and theme classes, a click on each checkbox, Enter after a plain item and after a task, and a saved editor state. It also checks that the three list commands make the whole list one kind.
New cases in `packages/lexical-mdast/src/__tests__/unit/MdastImportExport.test.ts` ("task lists"): round trips of mixed, ordered, nested and custom-start task lists, `1)` written as `1.`, the list type and `checked` each list imports as, an item made a task, Enter after a plain item and after a task, and check lists made in the editor.
### Before
The new tests on unchanged `main` (7bc4baa):
```
$ pnpm exec vitest run packages/lexical-list/src/__tests__/unit/TaskListItems.test.tsx packages/lexical-mdast/src/__tests__/unit/MdastImportExport.test.ts -t task
Tests 22 failed | 7 passed | 107 skipped (136)
```
The 7 that pass before guard against regressions: a bullet or number list command makes every item plain, a list of task items only is a check list, an item made a task, a bullet list turned into a check list, a check list made in the editor, and the `task list` round trip.
### After
```
$ pnpm exec vitest run packages/lexical-list/src/__tests__/unit/TaskListItems.test.tsx packages/lexical-mdast/src/__tests__/unit/MdastImportExport.test.ts -t task
Tests 29 passed | 107 skipped (136)
$ pnpm run test-unit
Tests 10094 passed | 4 expected fail | 2 skipped (10100)
```
`pnpm run tsc` is clean, and `npx eslint` and `npx prettier --check` on `packages/lexical-list/src` and `packages/lexical-mdast/src` are clean.
Tested on macOS with Node 24.21.0. Flow, browser tests, E2E and the playground were not run.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
conghuydever
changed the title
…and number lists
## Description
GFM decides per item whether a list item is a task. Plain items and task items can share a list, and an ordered list can hold task items. GitHub renders both that way:
| Markdown | GitHub renders |
| --- | --- |
| `- plain` / `- [x] done` | a checkbox on `done` only |
| `1. [ ] todo` / `2. [x] done` | a numbered list with a checkbox on each item |
In Lexical only a check list has checkboxes, every item of a check list has one, and a check list can't be ordered. `ListItemNode` drops `checked` from items that are not in a check list. So with `@lexical/mdast`:
| Markdown | Editor shows | Exported with no edits |
| --- | --- | --- |
| `- plain\n- [x] done` | a checkbox on both items | `- [ ] plain\n- [x] done` |
| `1. [ ] todo\n2. [x] done` | numbers, no checkboxes | `1. todo\n2. done` |
The first case adds a box the author did not write. The second loses the boxes and the ticks.
This change lets an item of a bullet or number list be a task, without changing what existing code sees.
`@lexical/list`:
- New `$setTaskListItemChecked(listItem, checked)` makes an item a task (checked or not), or plain with `undefined`. In a check list it is `setChecked`. In a bullet or number list it keeps the checkbox in a NodeState, so `checked` and `getChecked()` behave exactly as before.
- New `$getTaskListItemChecked(listItem)` reads it: `getChecked()` in a check list, the task item state in a bullet or number list, `undefined` for a plain item.
- A task item gets the checkbox `role`, `aria-checked` and the `listitemChecked` / `listitemUnchecked` theme classes in any list, and a click on its checkbox toggles it. Space and the arrow keys use the same check as the click (an item of a check list, or an item with `aria-checked`); I have no test for them.
- Enter after a checked task adds an unchecked task, as in a check list. Enter after a plain item adds a plain item.
- A list command (`$insertList`) still makes the whole list one kind: a bullet or number list makes its task items plain, a check list makes every item a task and keeps the ticks.
- A task item that ends up in a check list keeps its tick there.
`@lexical/mdast`:
- Only an unordered list whose items are all tasks becomes a check list. Any other list keeps its kind, and its task items become task items with `$setTaskListItemChecked`.
- The export writes `$getTaskListItemChecked` per item, so plain items stay plain and task items keep their boxes.
- An ordered list with task items is written with `.`. The GFM task list writer (`mdast-util-gfm-task-list-item` 2.0.0) only adds a box after `1.`, so with `1)` the boxes were dropped.
Compatibility:
- No existing API changes: `getChecked()`, `setChecked()`, `toggleChecked()`, `$createListItemNode()` and the clearing of `checked` outside a check list work as before. There are tests for `getChecked()` and `setChecked()` on items of a bullet and a number list.
- Nothing sets the task item state except the new helper, so apps that don't call it see no change, and saved editor states load as before.
- The one behavior change is in `@lexical/mdast` (`@experimental`): a GFM list that mixes plain items and task items now imports as a bullet list with task items, not as a check list.
- The playground theme styles the checked and unchecked classes in any list (`list-style-type: none`), so there a task item shows its checkbox in place of its bullet or number. I did not change the theme.
Out of scope: `@lexical/markdown` and its `CHECK_LIST` transformer are unchanged.
Alternatives considered:
- Keep the box an item was written with in a `@lexical/mdast` node state, and leave `@lexical/list` alone. That fixes the export only: the editor still shows a checkbox on plain items and none in ordered lists. An earlier version of this PR did that.
- Let `checked` itself mark a task item in any list. That is simpler, but it changes what `getChecked()` returns and stops clearing `checked` outside a check list, which existing code may rely on. The previous commit of this PR did that.
AI disclosure: the code, tests and this description were prepared with AI assistance.
## Test plan
New `packages/lexical-list/src/__tests__/unit/TaskListItems.test.tsx`: for a bullet and a number list holding a plain item and two task items, it checks `$getTaskListItemChecked`, that `getChecked()` and `setChecked()` behave as before, the checkbox attributes and theme classes, a click on each checkbox, Enter after a plain item and after a task, and a saved editor state. It also checks that the three list commands make the whole list one kind, and that a task item moved into a check list keeps its tick.
New cases in `packages/lexical-mdast/src/__tests__/unit/MdastImportExport.test.ts` ("task lists"): round trips of mixed, ordered, nested and custom-start task lists, `1)` written as `1.`, the list type and task items each list imports as, an item made a task, Enter after a plain item and after a task, and check lists made in the editor.
### Before
The new tests use the new helpers, so they can't run on `main`. The round-trip cases alone, through the public API, on `main` (7bc4baa):
```
× plain items next to task items
× a task item followed by a plain item
× an ordered task list
× an ordered task list with a custom start
× an ordered list with plain and task items
× a mixed task list nested in a task list
× an ordered task list nested in a bullet list
× an ordered task list written with ")"
Tests 8 failed (8)
```
For example `- plain\n- [x] done\n- [ ] todo` came back as `- [ ] plain\n- [x] done\n- [ ] todo`, and `1. [ ] todo\n2. [x] done` as `1. todo\n2. done`.
### After
```
$ pnpm exec vitest run packages/lexical-list/src/__tests__/unit/TaskListItems.test.tsx packages/lexical-mdast/src/__tests__/unit/MdastImportExport.test.ts -t task
Tests 32 passed | 107 skipped (139)
$ pnpm run test-unit
Tests 10097 passed | 4 expected fail | 2 skipped (10103)
$ pnpm run flow
Found 0 errors
```
`pnpm run tsc` is clean, and `npx eslint` and `npx prettier --check` on `packages/lexical-list` and `packages/lexical-mdast/src` are clean.
Tested on macOS with Node 24.21.0. Browser tests, E2E and the playground were not run.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![meta-code-review[bot]](https://avatars.githubusercontent.com/in/5334748?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for the detailed write-up and tests. Deciding task-ness per item matches the GFM spec, which defines a task list item as a list item whose first paragraph begins with a task list item marker, whatever the kind of list; the new round-trip cases in MdastImportExport.test.ts fail on main and pass on this branch, and getChecked(), setChecked() and toggleChecked() keep their behavior. Three things before this can land:
1. Typing a task marker in an imported mixed list unticks the other task items (must fix). With MdastShortcutsExtension, import - plain\n- [x] done, press Enter after plain and type [x] new: on main the result is - [ ] plain\n- [x] new\n- [x] done, on this branch - [ ] plain\n- [x] new\n- [ ] done. Typing [ ] at the start of plain unticks done the same way. $tryCheckbox (packages/lexical-mdast/src/MdastShortcuts.ts:255) converts the list with list.setListType('check'), which dirties only the ListNode, so the migration in the ListItemNode transform (packages/lexical-list/src/LexicalListItemNode.ts:213-221) never runs for the other items, and in the check list getChecked() reads a __checked that was never set. Any setListType('check') call loses the ticks this way, and the leftover state comes back if the list is set to bullet again. Suggested fix: do the migration in the list's own transform, updateChildrenListItemValue in formatList.ts, which runs whenever the list is dirty. Optionally, also make $tryCheckbox work per item with $setTaskListItemChecked, switching to a check list only when every item is a task, as $listTypeFromMdast now does; that would also let the shortcut work in number lists. Please add a test for the shortcut case.
2. A changeset is needed. CONTRIBUTING.md ("Changesets") asks for one for every user-facing change. This adds public API and Flow types to @lexical/list and changes what @lexical/mdast imports and exports, so .changeset/ needs an entry naming both packages with patch, plus a Verify: paragraph for the import change (see "Behavior changes" in .changeset/README.md). For example:
---
'@lexical/list': patch
'@lexical/mdast': patch
---
[fix] Keep GFM task items in bullet and number lists in `@lexical/mdast`, using the new `$getTaskListItemChecked` and `$setTaskListItemChecked` from `@lexical/list`
Verify: Markdown that mixes plain and task items in one list now imports as a bullet list whose task items have checkboxes, not as a check list; ordered task lists keep their checkboxes and are written with `.`.3. HTML does not round-trip a task item (worth considering). exportDOM writes a task item as <li role="checkbox" aria-checked> inside a plain <ul> or <ol>. On import, isDomChecklist (LexicalListNode.ts:430-447, and its copy in ListImportExtension.ts:40-46) makes such a <ul> a check list, and in an <ol> the attribute only sets __checked, which the number list clears. So $generateHtmlFromNodes followed by $generateNodesFromDOM turns - plain\n- [x] done back into a check list and 1. [ ] todo\n2. [x] done into a numbered list without boxes. Copy and paste inside one editor is fine, since the JSON clipboard carries NodeState. Importing li[aria-checked] as a task item when the list carries no check-list marker and not every item has aria-checked would mirror $listTypeFromMdast.
Smaller notes:
- In the playground theme an ordered task list looks exactly like a check list, because the checked and unchecked classes set
list-style-type: noneanddisplay: block(PlaygroundEditorTheme.css:526-537). If task items in a number list should show their numbers, as your table expects, the theme needs a rule forol > litask items. - Space and the arrow keys on task items next to plain items behave like a check list in Chromium. A browser test under
packages/**/__tests__/browser(see "Testing Strategy" in AGENTS.md) would lock that in, since jsdom cannot.
| if (parent.getListType() === 'check') { | ||
| const task = $getState(node, taskListItemState); | ||
| if (task !== '') { | ||
| if (node.getLatest().__checked == null) { | ||
| node.setChecked(task === 'checked'); | ||
| } | ||
| $setState(node, taskListItemState, ''); | ||
| } | ||
| } |
There was a problem hiding this comment.
This migration only runs when the item itself is dirty. ListNode.setListType('check') dirties only the list, and updateChildrenListItemValue writes an item only to renumber it, so the other items keep their listTaskItem state while $getTaskListItemChecked reads getChecked(), which is Boolean(__checked) and so false. $tryCheckbox in packages/lexical-mdast/src/MdastShortcuts.ts:255 takes exactly this path: import - plain\n- [x] done, press Enter after plain, type [x] new, and done ends unchecked (it stays checked on main). Moving the migration into the list's own transform (updateChildrenListItemValue, when the list is a check list) would cover setListType, the shortcut and any other caller.
There was a problem hiding this comment.
Fixed in 94773ca. The step that turns a task item into a check list item is now $adoptTaskListItem (internal, not exported), and the ListNode transform calls it for every item of a check list, so setListType('check'), the shortcut and any other caller keep the ticks. It also clears the task state, so nothing comes back when the list is set to bullet again. I put it in the list's $transform rather than in updateChildrenListItemValue, because calling a $ function from that non-$ helper makes @lexical/rules-of-lexical warn.
New tests: the two shortcut cases from your review in MdastShortcuts.test.ts ([x] new on a new line, and [ ] at the start of plain), and setListType('check') then setListType('bullet') in TaskListItems.test.tsx. All three fail on 9108a40 and pass now.
I left $tryCheckbox as it is: with the fix it gives what main gives (- [ ] plain\n- [x] new\n- [x] done), while making it per item would change what the shortcut does in a plain bullet list.
| 'aria-checked', | ||
| listItemNode.getChecked() ? 'true' : 'false', | ||
| ); | ||
| dom.setAttribute('aria-checked', checked ? 'true' : 'false'); |
There was a problem hiding this comment.
This now writes role="checkbox" aria-checked on a task item inside a plain <ul> or <ol>, and exportDOM reuses it, but the importers do not read it back: isDomChecklist (LexicalListNode.ts:430-447, ListImportExtension.ts:40-46) makes any <ul> with an li[aria-checked] child a check list, and in an <ol> the attribute only sets __checked, which the number list clears. $generateHtmlFromNodes followed by $generateNodesFromDOM turns - plain\n- [x] done into a check list and 1. [ ] todo\n2. [x] done into a list without boxes. Consider importing li[aria-checked] as a task item with $setTaskListItemChecked when the list carries no check-list marker and not every item has aria-checked.
There was a problem hiding this comment.
Agreed that HTML doesn't round-trip a task item yet. I'd like to leave it for a separate PR: making isDomChecklist need aria-checked on every item, and importing the rest as task items, changes HTML paste for every app (Google Docs and other sources included), not only @lexical/mdast users. It isn't a regression either, since on main the same Markdown already becomes a check list or loses its boxes. The description now lists it under "Out of scope".
…hen a list becomes a check list
## Description
- setListType('check') marks only the list, so the ListItemNode transform that turns a task item into a check list item never ran for the other items, and they lost their ticks. The mdast checkbox shortcut takes this path: in an imported `- plain` / `- [x] done`, typing `[x] ` on a new line or `[ ] ` at the start of `plain` unticked `done`. The ListNode transform now does this for every item of a check list, and leaves no task state behind.
- A click or Space on a check list item calls toggleChecked() again, as on main, so a ListItemNode subclass that overrides it still sees the toggle.
- Add a changeset for @lexical/list and @lexical/mdast.
- Add a browser test for ArrowLeft and Space on a task item next to a plain item.
## Changeset
.changeset/mdast-task-list-items.md
## Test plan
### Before
$ pnpm exec vitest run packages/lexical-list/src/__tests__/unit/TaskListItems.test.tsx packages/lexical-mdast/src/__tests__/unit/MdastShortcuts.test.ts
× setListType keeps the ticks in a check list, and leaves none behind
× a click on a check list item still goes through toggleChecked()
× [x] on a new line keeps the ticks of the other task items
× [ ] at the start of a plain item keeps the ticks of the other task items
Tests 4 failed | 43 passed (47)
### After
$ pnpm exec vitest run packages/lexical-list/src/__tests__/unit/TaskListItems.test.tsx packages/lexical-mdast/src/__tests__/unit/MdastShortcuts.test.ts packages/lexical-mdast/src/__tests__/unit/MdastImportExport.test.ts
Tests 166 passed (166)
$ VITEST_BROWSER=chromium,firefox,webkit pnpm exec vitest run --project "browser*" packages/lexical-list
Test Files 3 passed (3)
Tests 6 passed (6)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the review. Handled in 94773ca:
On the smaller notes:
While checking for breaking changes I also found that a click or Space on a check list item now called With this branch merged with |
There was a problem hiding this comment.
Thanks, this looks good. The tick loss from the last round is fixed: the ListNode transform now moves a task item's state into checked for every item of a check list, so setListType('check') and the [x] shortcut keep the other ticks, and the changeset and the browser test are in. Deciding per item matches the GFM task list extension, which defines a task list item as "a list item where the first block in it is a paragraph which begins with a task list item marker", and the new round trips fail on main and pass here. Everything below is optional and non-blocking.
1. Keep isCheckListItem scoped to Lexical's own lists (packages/lexical-list/src/checkList.ts:526-527). It now accepts any li with aria-checked, and the check list handlers use it to decide where the focus belongs: after a press, on keys other than Space, the arrows and Escape, and on beforeinput, they move the focus back to the editor root when the focused element passes this check. A decorator that renders focusable li[aria-checked] elements inside the editor (a multi-select listbox of role="option" items, an inline menuitemcheckbox menu) now loses its focus to the editor when the user clicks an option, and pressing Enter or typing on a focused option moves the focus first, so the keystroke edits the document (Enter inserts an empty paragraph); on main the option keeps the focus and the document is unchanged. Of the suggestions here, this is the one I would do before merging. Requiring the parent to be a Lexical list keeps task items working and restores that:
(parentNode.__lexicalListType === 'check' ||
// @ts-ignore internal field
(parentNode.__lexicalListType != null &&
dom.hasAttribute('aria-checked')))A browser test next to TaskListItemKeys.test.ts that clicks such an element in a decorator and checks that it keeps the focus would lock this in (AGENTS.md, "Testing Strategy": behavior that needs real focus belongs in __tests__/browser).
2. JSDoc of $setTaskListItemChecked (packages/lexical-list/src/LexicalListItemNode.ts:915). In a check list, undefined calls setChecked(undefined) and the item stays an unchecked task, since every item of a check list is one. A sentence saying so would keep "or a plain item with undefined" from reading as if it worked in every list.
3. Smaller notes
- A read in the same update as
setListType('check'), before the transforms run, still seesfalsefor a task item, because$getTaskListItemCheckedreturnsgetChecked()there. Falling back to the task state while__checkedis unset would make the helper agree with what the transform produces. - Indenting a task item copies its state onto the new wrapper item (
$handleIndentbuilds it with$copyNode,packages/lexical-list/src/formatList.ts:630), so the wrapper stores"listTaskItem": "unchecked"in its JSON and gets the unchecked theme class. Nothing visible breaks; clearing the state on the wrapper would keep stored JSON clean. - The ARIA table in
packages/lexical-website/docs/concepts/keyboard-accessibility.mdlistscheckboxandaria-checkedfor the "check list leaf" only; a task item of a bullet or number list now gets them too.
The HTML round trip and the playground theme are out of scope, as the description says; a follow-up issue for isDomChecklist would keep the HTML case on the list. The fork's CI workflows have not run on this head yet and need a maintainer's approval to start.
| (parentNode.__lexicalListType === 'check' || | ||
| dom.hasAttribute('aria-checked')) |
There was a problem hiding this comment.
Optional: this accepts any li with aria-checked, and returnFocusToRoot, getActiveCheckListItem and handleSelectDefaults all decide through it. A decorator that renders focusable li[aria-checked] elements inside the editor (a multi-select listbox of role="option" items, an inline menuitemcheckbox menu) now loses its focus to the editor root on a click, and an Enter or a letter pressed on a focused option edits the document instead, where main leaves both alone. Requiring the parent to be a Lexical list keeps task items working:
(parentNode.__lexicalListType === 'check' ||
// @ts-ignore internal field
(parentNode.__lexicalListType != null &&
dom.hasAttribute('aria-checked')))| } | ||
|
|
||
| /** | ||
| * Makes a list item a task, checked or not, or a plain item with `undefined`. |
There was a problem hiding this comment.
Nit: in a check list, undefined goes to setChecked(undefined) and the item stays an unchecked task (every check list item is one). A sentence saying so would keep this line from reading as if undefined makes an item plain in every list.
|
Checked this for markdown losslessness — round-tripping is strictly better than
if (listItem.getLatest().__checked == null) {
listItem.setChecked(task === 'checked');
}The guard assumes if (parent.getListType() !== 'check' && node.getChecked() != null)
Repro, public API only:
Dropping the guard fixes it: task state is only ever set outside a check list, where Relatedly, the test "A checked state set the old way is still dropped outside a check list" passes because Follow-up, not blocking: Review is from reading the source; I did not run the suite. |
lexical
@lexical/a11y
@lexical/clipboard
@lexical/code
@lexical/code-core
@lexical/code-prism
@lexical/code-shiki
@lexical/compiler
@lexical/devtools-core
@lexical/dragon
@lexical/eslint-plugin
@lexical/extension
@lexical/fast-check
@lexical/file
@lexical/hashtag
@lexical/headless
@lexical/history
@lexical/html
@lexical/internal
@lexical/link
@lexical/list
@lexical/mark
@lexical/markdown
@lexical/mdast
@lexical/offset
@lexical/overflow
@lexical/plain-text
@lexical/react
@lexical/rich-text
@lexical/selection
@lexical/table
@lexical/tailwind
@lexical/text
@lexical/utils
@lexical/y
@lexical/yjs
commit: |
… beats its task state ## Description Follow-up to the review in meta#9475 (comment). Outside a check list, `setChecked()` only hides `__checked`: `getChecked()` returns `undefined` there, and the ListNode transform clears it only when the list itself changes. So an item can still hold a checked state when `$setTaskListItemChecked` makes it a task in a bullet or number list. When the list then became a check list, `$adoptTaskListItem` kept that old value and dropped the task state. Two ways to get there: - `item.setChecked(true)` on a bullet item, then `$setTaskListItemChecked(item, false)`, then `setListType('check')`: the item came back checked. - In one update, a checked item of a check list, `setListType('bullet')`, `$setTaskListItemChecked(item, false)`, `setListType('check')`: the same. `$setTaskListItemChecked` now clears `__checked` when it sets the task state in a bullet or number list, so the two never live on one item. The check in `$adoptTaskListItem` stays. It lets a checked state set in the check list win over the task state in the same update. The `[x] ` shortcut does this: on an unchecked task item of `- [ ] todo` / `- plain`, it makes the list a check list and checks the item, before the transforms run. Without the check, the transform would set `todo` back to unchecked. New tests cover it, at the list level and through the shortcut. The test comment that said a checked state set the old way is "dropped" outside a check list now says it does not show, which is what the test checks. ## Test plan ### Before $ pnpm exec vitest run packages/lexical-list/src/__tests__/unit/TaskListItems.test.tsx packages/lexical-mdast/src/__tests__/unit/MdastShortcuts.test.ts × when the list becomes a check list > the task state wins over a checked state set the old way expected [ false, true, true ] to deeply equal [ false, false, true ] × when the list becomes a check list > the task state wins over the tick of a check list left in the same update expected [ false, false, true ] to deeply equal [ false, false, false ] Tests 2 failed | 49 passed (51) ### After $ pnpm exec vitest run packages/lexical-list/src/__tests__/unit/TaskListItems.test.tsx packages/lexical-mdast/src/__tests__/unit/MdastShortcuts.test.ts packages/lexical-mdast/src/__tests__/unit/MdastImportExport.test.ts Tests 170 passed (170) $ pnpm exec vitest run --project unit packages/lexical-list packages/lexical-mdast Test Files 26 passed (26) Tests 492 passed (492) $ VITEST_BROWSER=chromium,firefox,webkit pnpm exec vitest run --project "browser*" packages/lexical-list Test Files 3 passed (3) Tests 6 passed (6) `pnpm run tsc`, `pnpm run tsc-test` and `pnpm run flow` give 0 errors, and eslint and prettier are clean on the changed files. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@potatowagon Thanks for the careful read. Fixed in 068ac30, but in a different place than you suggested. You're right that the The bug is real when only the item changes, or within one update:
I kept the guard. It lets a checked state set in the check list win over the task state in the same update, and the Instead,
I also reworded the test comment: the value set the old way is hidden, not dropped. I left the dead branch in the item transform as it is, since it's on Agreed on
|
There was a problem hiding this comment.
Thanks, this looks good. Deciding task-ness per item matches the GFM task list extension, which defines a task list item as "a list item where the first block in it is a paragraph which begins with a task list item marker"; the new round trips fail on main and pass here, and 068ac30d closes the stale-checked case from the last round: $setTaskListItemChecked now clears __checked outside a check list, and both cases from the discussion are pinned in TaskListItems.test.tsx. Everything below is optional and non-blocking.
1. Keep isCheckListItem scoped to Lexical's own lists (packages/lexical-list/src/checkList.ts:526-527). It now accepts any li with aria-checked, and the check list handlers use it to decide when to hand the focus back to the editor root (after a press on content, on most keys, on beforeinput). With CheckListExtension registered, a decorator that renders focusable li[aria-checked] items inside the editor (a multi-select listbox, a menuitemcheckbox menu) now loses its focus to the root when an option is clicked, and Enter on a focused option can land in the document; on main the option keeps the focus. AGENTS.md ("Backwards Compatibility") asks that existing behavior stay intact, and requiring the parent to be a Lexical list keeps task items working:
(parentNode.__lexicalListType === 'check' ||
// @ts-ignore internal field
(parentNode.__lexicalListType != null &&
dom.hasAttribute('aria-checked')))A browser test next to TaskListItemKeys.test.ts would lock it in (AGENTS.md, "Testing Strategy": behavior that needs a real focus engine belongs in __tests__/browser).
2. Clear the task state on indent wrappers (packages/lexical-list/src/formatList.ts:630). $handleIndent builds the wrapper item with $copyNode(listItemNode), which carries the listTaskItem NodeState, so the wrapper stores "listTaskItem": "unchecked". Enter on an empty nested item then builds the new outer item as a copy of that wrapper ($handleListInsertParagraph, formatList.ts:825): from - plain\n- [ ] todo, indenting todo, pressing Enter twice and typing next exports - plain\n - [ ] todo\n- [ ] next, while the same nesting imported from Markdown gives a plain - next. Clearing the task state on the new wrapper (for example $setTaskListItemChecked(newListItem, undefined) before it is inserted), and on the wrapper copies in $handleOutdent, would fix both the stored JSON and the Enter case.
3. Smaller notes
- JSDoc of
$setTaskListItemChecked(packages/lexical-list/src/LexicalListItemNode.ts:919): in a check list,undefinedleaves the item an unchecked task. A sentence saying so would keep "or a plain item withundefined" from reading as if it worked in every list. - The ARIA table in
packages/lexical-website/docs/concepts/keyboard-accessibility.mdlistscheckboxandaria-checkedfor the "check list leaf" only; task items of bullet and number lists now get them too. - The HTML round trip and a per-item
$tryCheckboxare out of scope as agreed; a follow-up issue forisDomChecklistwould keep the HTML case tracked.
| (parentNode.__lexicalListType === 'check' || | ||
| dom.hasAttribute('aria-checked')) |
There was a problem hiding this comment.
Optional: this accepts any li with aria-checked, and returnFocusToRoot, getActiveCheckListItem and handleSelectDefaults all decide through it. With CheckListExtension registered, a decorator that renders focusable li[aria-checked] items inside the editor (a multi-select listbox, a menuitemcheckbox menu) now loses its focus to the editor root when an option is clicked, where main leaves it on the option. Requiring the parent to be a Lexical list keeps task items working:
(parentNode.__lexicalListType === 'check' ||
// @ts-ignore internal field
(parentNode.__lexicalListType != null &&
dom.hasAttribute('aria-checked')))| // The same for a task item of a bullet or number list: a copy of it (the | ||
| // next item, after Enter) is an unchecked task. | ||
| if ($getState(original, taskListItemState) === 'checked') { | ||
| $setState(this, taskListItemState, 'unchecked'); | ||
| } |
There was a problem hiding this comment.
Optional: $copyNode also builds the wrapper item when a task item is indented ($handleIndent, formatList.ts:630), so the wrapper keeps listTaskItem: "unchecked" in its JSON, and Enter on an empty nested item later copies that wrapper (formatList.ts:825): from - plain\n- [ ] todo, indent todo, Enter, Enter, type next gives - plain\n - [ ] todo\n- [ ] next. Clearing the task state on the new wrapper in $handleIndent (and on the wrapper copies in $handleOutdent) would avoid that box.
| } | ||
|
|
||
| /** | ||
| * Makes a list item a task, checked or not, or a plain item with `undefined`. |
There was a problem hiding this comment.
Nit: in a check list, undefined goes to setChecked(undefined) and the item stays an unchecked task (every check list item is one). A sentence saying so would keep this line from reading as if undefined makes an item plain in every list.
There was a problem hiding this comment.
Thanks, this looks good. The per-item task model matches the GFM task list rule, existing check lists keep getChecked() and toggleChecked(), the new NodeState leaves existing JSON untouched, and the mixed, ordered and nested round trips now come back as written. The suggestions below are optional and non-blocking.
Suggestions (optional)
- Limit the widened checkbox check to Lexical list items (
packages/lexical-list/src/checkList.ts:519-529).isCheckListItemnow accepts anyliwitharia-checked, so a widget inside the editor that uses one (for example a multi-select listbox ofli[role=option][aria-checked]rendered by a decorator) has its focus moved to the editor root on a click and on most keys, and in Chromium Enter on a focused option inserts a paragraph into the document. Requiring a Lexical list parent keeps task items working:A browser test beside(parentNode.__lexicalListType === 'check' || (parentNode.__lexicalListType != null && dom.hasAttribute('aria-checked')))
TaskListItemKeys.test.tswould cover it; AGENTS.md ("Testing Strategy") puts real focus behavior in__tests__/browser. - Don't copy the task state onto indent wrappers (
packages/lexical-list/src/formatList.ts:630, and the wrapper copies at:708,:714and:747).$copyNode(listItemNode)carries thelistTaskItemstate onto the item that wraps the nested list, and Enter on an empty nested item copies that wrapper, so- plain/- [ ] todo, then indentingtodo, pressing Enter twice and typingnext, exports- [ ] next. Calling$setTaskListItemChecked(copy, undefined)on those copies gives- next. - JSDoc (
packages/lexical-list/src/LexicalListItemNode.ts:919). In a check list,$setTaskListItemChecked(item, undefined)leaves an unchecked task, since every check list item is a task; one sentence saying so would avoid the surprise. - Docs (
packages/lexical-website/docs/concepts/keyboard-accessibility.md:59). The ARIA table listscheckboxandaria-checkedfor the check list leaf only; task items of bullet and number lists now get them too. - Changeset (
.changeset/mdast-task-list-items.md). The entry is[fix]; a separate[feature]entry for@lexical/listwould announce$getTaskListItemCheckedand$setTaskListItemCheckedto people upgrading (categories in CONTRIBUTING.md, "Changesets").
The HTML round trip and a per-item [ ] shortcut are reasonable follow-ups, as discussed in the thread. The CI workflows for this head are still waiting for a maintainer to approve them.
| (parentNode.__lexicalListType === 'check' || | ||
| dom.hasAttribute('aria-checked')) |
There was a problem hiding this comment.
Optional: this now accepts any li[aria-checked] inside the editor, so a widget that renders one (for example a decorator's multi-select listbox of li[role=option][aria-checked]) has its focus moved to the editor root by the check list handlers. Requiring a Lexical list parent keeps task items working:
(parentNode.__lexicalListType === 'check' ||
(parentNode.__lexicalListType != null &&
dom.hasAttribute('aria-checked')))| if ($getState(original, taskListItemState) === 'checked') { | ||
| $setState(this, taskListItemState, 'unchecked'); | ||
| } |
There was a problem hiding this comment.
Optional: $copyNode also builds the wrapper items that indent and outdent create (formatList.ts:630, :708, :714, :747), so the wrapper keeps an unchecked task state, and Enter out of the nested list copies it onto the new outer item (- [ ] next where - next was typed). Clearing the state on those wrapper copies with $setTaskListItemChecked(copy, undefined) avoids it.
| } | ||
|
|
||
| /** | ||
| * Makes a list item a task, checked or not, or a plain item with `undefined`. |
There was a problem hiding this comment.
Nit: in a check list, undefined leaves an unchecked task (every check list item is a task); one sentence here saying so would prevent the surprise.
…-round-trip # Conflicts: # packages/lexical-mdast/src/__tests__/unit/MdastShortcuts.test.ts
Description
GFM decides per item whether a list item is a task. Plain items and task items can share a list, and an ordered list can hold task items. GitHub renders both that way:
- plain/- [x] donedoneonly1. [ ] todo/2. [x] doneIn Lexical only a check list has checkboxes, every item of a check list has one, and a check list can't be ordered.
ListItemNodedropscheckedfrom items that are not in a check list. So with@lexical/mdast:- plain\n- [x] done- [ ] plain\n- [x] done1. [ ] todo\n2. [x] done1. todo\n2. doneThe first case adds a box the author did not write. The second loses the boxes and the ticks.
This change lets an item of a bullet or number list be a task, without changing what existing code sees.
@lexical/list:$setTaskListItemChecked(listItem, checked)makes an item a task (checked or not), or plain withundefined. In a check list it issetChecked. In a bullet or number list it keeps the checkbox in a NodeState, socheckedandgetChecked()behave exactly as before.$getTaskListItemChecked(listItem)reads it:getChecked()in a check list, the task item state in a bullet or number list,undefinedfor a plain item.role,aria-checkedand thelistitemChecked/listitemUncheckedtheme classes in any list, and a click on its checkbox toggles it. Space and the arrow keys use the same check as the click (an item of a check list, or an item witharia-checked). A click or Space on a check list item still callstoggleChecked(), so aListItemNodesubclass that overrides it still sees the toggle.$insertList) still makes the whole list one kind: a bullet or number list makes its task items plain, a check list makes every item a task and keeps the ticks.setListType('check'). TheListNodetransform does this for every item of a check list, sincesetListTypemarks only the list.@lexical/mdast:$setTaskListItemChecked.$getTaskListItemCheckedper item, so plain items stay plain and task items keep their boxes... The GFM task list writer (mdast-util-gfm-task-list-item2.0.0) only adds a box after1., so with1)the boxes were dropped.[ ]/[x]shortcut in a list that has task items keeps the ticks of the other items, as onmain.Compatibility:
getChecked(),setChecked(),toggleChecked(),$createListItemNode()and the clearing ofcheckedoutside a check list work as before. There are tests forgetChecked()andsetChecked()on items of a bullet and a number list, and for the click on a check list item going throughtoggleChecked().@lexical/mdast(@experimental): a GFM list that mixes plain items and task items now imports as a bullet list with task items, not as a check list.list-style-type: none), so there a task item shows its checkbox in place of its bullet or number. I did not change the theme.Out of scope:
@lexical/markdownand itsCHECK_LISTtransformer are unchanged.isDomChecklistmakes a<ul>with anyli[aria-checked]a check list, and in an<ol>the attribute is dropped. Importing such items as task items changes HTML paste for every app, so it fits better in its own PR. Copy and paste inside one editor keeps them, since the JSON clipboard carries NodeState.Alternatives considered:
@lexical/mdastnode state, and leave@lexical/listalone. That fixes the export only: the editor still shows a checkbox on plain items and none in ordered lists. An earlier version of this PR did that.checkeditself mark a task item in any list. That is simpler, but it changes whatgetChecked()returns and stops clearingcheckedoutside a check list, which existing code may rely on. An earlier commit of this PR did that.AI disclosure: the code, tests and this description were prepared with AI assistance.
Changeset
.changeset/mdast-task-list-items.md:@lexical/listand@lexical/mdastpatch,[fix], with aVerify:paragraph.Test plan
New
packages/lexical-list/src/__tests__/unit/TaskListItems.test.tsx: for a bullet and a number list holding a plain item and two task items, it checks$getTaskListItemChecked, thatgetChecked()andsetChecked()behave as before, the checkbox attributes and theme classes, a click on each checkbox, Enter after a plain item and after a task, and a saved editor state. It also checks that the three list commands make the whole list one kind, that a task item moved into a check list keeps its tick, thatsetListType('check')keeps the ticks andsetListType('bullet')leaves no task state behind, and that a click on a check list item goes throughtoggleChecked().New
packages/lexical-list/src/__tests__/browser/TaskListItemKeys.test.ts: in a real browser, ArrowLeft at the start of a task item next to a plain item focuses its checkbox and Space ticks it; ArrowLeft at the start of the plain item leaves the focus in the editor.New cases in
packages/lexical-mdast/src/__tests__/unit/MdastImportExport.test.ts("task lists"): round trips of mixed, ordered, nested and custom-start task lists,1)written as1., the list type and task items each list imports as, an item made a task, Enter after a plain item and after a task, and check lists made in the editor.New cases in
packages/lexical-mdast/src/__tests__/unit/MdastShortcuts.test.ts: in an imported- plain/- [x] done,[x]typed on a new line and[ ]typed at the start ofplainkeepdoneticked.Before
The new tests use the new helpers, so they can't run on
main. The round-trip cases alone, through the public API, onmain(7bc4baa):For example
- plain\n- [x] done\n- [ ] todocame back as- [ ] plain\n- [x] done\n- [ ] todo, and1. [ ] todo\n2. [x] doneas1. todo\n2. done.The tests added in the last commit, with the
@lexical/listsource of the previous head (9108a40), test file paths left out:After
With this branch merged with
main(ff45cf6):That merged run had everything in this branch except the
toggleChecked()change and its test. With those added,pnpm exec vitest run packages/lexical-list packages/lexical-mdastgives 490 passed (490), thepackages/lexical-listbrowser tests 6 passed in the three browsers, andpnpm run tsc,pnpm run flow, eslint and prettier are clean.Tested on macOS with Node 24.21.0, browser tests in Chromium, Firefox and WebKit through Playwright. E2E and the playground were not run.
🤖 Generated with Claude Code