Repository navigation
[lexical-y] Chore: Vary the inline content of the paragraphs StructuralCommands acts on - #9613
Conversation
…alCommands acts on ## Description The structural command cells varied the container a command runs in, but the selected paragraph always held plain text, so a change that splits the paragraph's inline content when it turns it into another block passed every cell. Review of #9590 found one: `OverflowNode` returns false from `isInline()`, though the structure normalization treats it as inline, so with the caret inside an overflow, `$insertList` under `NodeStructureExtension` made a list of the overflow alone. `StructuralCommands.test.ts` gets a family for that dimension. The commands that turn the selected paragraph into another block (`$insertList` of each type, `$wrapNodes` into a quote, and the markdown shortcuts `# `, `- `, `> `) run on a paragraph `abc ` + an inline shape + ` end`, for each of the catalog's 23 inline shapes (text in its modes, tabs, line breaks, hashtags and other text subclasses, links, marks, a link inside a mark, overflow, inline decorators), with the caret before the inline content, the caret inside its own text (shapes that have one), or the whole paragraph selected (the shortcuts are typed at the start), in the root, a container that declares nothing, one that declares `children: 'block'` and a shadow root, unbound with `NodeStructureExtension` and bound with `@lexical/y`: 2,760 cells. Each checks the model rather than another tree: the block the command makes holds the paragraph's whole inline content, its text and the same inline nodes in order, with nothing split out; a shortcut that does not act (in a container that declares nothing, or a declared one without #9590) leaves the paragraph whole. Code highlight nodes live only in code blocks, so they are not among the shapes. The family finds a bug on main in the deprecated `$wrapNodes`: with the caret inside an overflow, or inside a link inside a mark (or the whole paragraph selected), it splits the paragraph in three and drops the overflow or the mark. Those 24 cells are `test.fails` naming #9612. Everything else passes on main. ## Test plan ``` pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/matrix/StructuralCommands.test.ts pnpm run ci-check ``` ### Before New cells. Without the `test.fails` marks, on main `be5afea`: ``` × $wrapNodes in a quote, caret inside it, on a paragraph with link/in-mark in root [unbound, NodeStructureExtension] (8 cells each: caret inside a link in a mark, the whole paragraph with one, caret inside an overflow) Tests 24 failed | 2736 passed (2760) ``` On #9590 at `f31df5d`, also the overflow cells of `$insertList` with the caret inside it (18). ### After On main `be5afea`: ``` Test Files 15 passed (15) Tests 6076 passed | 164 expected fail (6240) $ pnpm run ci-check Found 0 errors ```
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
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: |
… and skip only the cells that cannot apply
## Description
Review found that the "caret inside it" cells of the inline content family returned early, without running the command, for 64 cells that should have run: the caret was placed in a text node that held neither sentinel, but normalization merges plain text (`text/normal`) and the text around a line break into the sentinels' nodes, so no such node was found. The caret is now placed by offset, one character into the inline content, in the leaves as they are after normalization, and a cell where it cannot be placed fails instead of passing. The cells that cannot apply, the caret inside an inline decorator (which has no text node; 128 cells), are `test.skip` with the reason in their names. The whole-paragraph selection now runs from the paragraph's first text node to its last.
## Test plan
```
pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/matrix/StructuralCommands.test.ts
pnpm run ci-check
```
### Before
```
Tests 2736 passed | 24 expected fail (2760) (192 "caret inside" cells returned early)
```
### After
On main `be5afea`, the 64 cells that used to return early run and pass:
```
Tests 2608 passed | 24 expected fail | 128 skipped (2760)
```
On #9590 at `f31df5d`, the 18 overflow cells of `$insertList` still fail; at `0caffb6`, they pass.
```
$ pnpm run ci-check
Found 0 errors
```
![meta-code-review[bot]](https://avatars.githubusercontent.com/in/5334748?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks good to me. The new family covers a real gap, the selected paragraph's inline content. Its 24 test.fails cells are exactly the ones that fail on main because of #9612, and every other cell passes. A few optional, non-blocking suggestions:
1. Consider adding $setBlocksType to BLOCK_CMDS (StructuralCommands.test.ts:786-823). It is the non-deprecated replacement #9612 points to, and the playground toolbar's block format control calls it (packages/lexical-playground/src/plugins/ToolbarPlugin/utils.ts:211). With heading and quote entries added, 32 cells fail on main. All of them are overflow/text, with the caret inside the overflow or the whole paragraph selected. The paragraph becomes p("abc ") / heading("over the limit") / p(" end"), or three headings, and the OverflowNode is gone. INTERNAL_$isBlock (packages/lexical/src/LexicalUtils.ts:2911) treats the OverflowNode as a block because its isInline() is false. packages/lexical-y/AGENTS.md asks tests to cover the input space. src/__tests__/catalog/README.md asks for a known failure to be a test.fails naming the issue or pull request that fixes it, here #9454 or #9461. #9612 also says $setBlocksType keeps the content together on main, which may need a correction.
{made: n => $isHeadingNode(n), name: "$setBlocksType('h1')",
run: () => $setBlocksType($getSelection(), () => $createHeadingNode('h1'))},
{made: n => $isQuoteNode(n), name: '$setBlocksType(quote)',
run: () => $setBlocksType($getSelection(), () => $createQuoteNode())},2. Nit: $caretInside (line 747). The return false for a non-text leaf runs inside .some(walk), so the walk keeps going and selects offset 1 of " end". For every inline decorator the helper returns true, which contradicts its JSDoc, and expect(placed).toBe(true) cannot catch it. Today only the shape.kind skip keeps those cells out. Ending the walk there (for example offset = -2; return true;) makes the guard do what the comment says. The zero-width downstream/atom still needs the skip.
3. Nit: the Test plan. The description's After block (6076 passed | 164 expected fail (6240)) predates the second commit. The matrix directory now reports 5948 passed | 164 expected fail | 128 skipped (6240). AGENTS.md (Commits and Pull Requests) asks the Test plan to carry the real output.
A test-only change needs no changeset (CONTRIBUTING.md).
| const size = n.getTextContentSize(); | ||
| if (offset < size || (offset === size && $isTextNode(n))) { | ||
| if (!$isTextNode(n)) { | ||
| return false; |
There was a problem hiding this comment.
Nit (optional): this return false skips only this leaf inside .some(walk). The walk goes on with offset still 1 and selects offset 1 of " end", so $caretInside() returns true for every inline decorator. That contradicts the JSDoc above, and expect(placed).toBe(true) cannot catch it. Ending the walk here, for example offset = -2; return true;, returns false as documented. The zero-width downstream/atom still needs the skip.
| type?: string; | ||
| }; | ||
|
|
||
| const BLOCK_CMDS: BlockCmd[] = [ |
There was a problem hiding this comment.
Optional: consider adding $setBlocksType (heading and quote) to this list. On main it splits a paragraph around an OverflowNode when the caret is inside it or the whole paragraph is selected, for example p("abc ") / heading("over the limit") / p(" end"). That is 32 cells in this family, and the bug is the kind it was written to catch. Those cells would be test.fails naming #9454.
There was a problem hiding this comment.
This looks good to me. The new family checks the block each command makes against the paragraph's own text and inline node types rather than another tree, and it catches what it was written for: with the overflow handling in packages/lexical-list/src/formatList.ts put back to an isInline() check, exactly the 18 $insertList cells with the caret inside overflow/text fail, while at this head only the 24 $wrapNodes cells pinned to #9612 fail. The suggestions below are optional and non-blocking.
1. Consider adding $setBlocksType to BLOCK_CMDS (packages/lexical-y/src/__tests__/unit/matrix/StructuralCommands.test.ts:777). It is the replacement #9612 names and what the playground toolbar calls for headings and quotes, and under this family's check it fails 32 cells at this head. With the caret inside overflow/text or the whole paragraph selected, $setBlocksType(selection, () => $createHeadingNode('h1')) gives paragraph("abc ") / heading("over the limit") / paragraph(" end") with NodeStructureExtension or @lexical/y, and paragraph("abc " heading("over the limit") " end") without it; either way the OverflowNode is dropped. $setBlocksType finds the block with $findMatchingParent(..., INTERNAL_$isBlock) (packages/lexical-selection/src/range-selection.ts:154-155), which is true for the overflow because its isInline() is false. That also contradicts #9612's note that $setBlocksType keeps the content together on main. Two more commands (heading and quote), as test.fails naming a new issue, would pin it.
2. Consider requiring the shortcuts to act where the rule says they do (StructuralCommands.test.ts:909-918). An unchanged paragraph currently passes for a shortcut in every container. At this head # , - and > act in the root, declared and shadow containers for all 23 shapes and in none of the plain ones, which is what model() in the same file predicts, so asserting the made block there would catch a shortcut that stops firing on, say, a paragraph holding a link. The comment's "a declared one without the rule" no longer applies now that #9590 is on main.
3. Nit: $caretInside does not return false at an inline decorator (StructuralCommands.test.ts:726-738). Its JSDoc says it returns false where the character belongs to an inline decorator, but walk returns false into some() without consuming the offset, so the next child is tried and the caret lands at offset 1 of " end". The static skip keeps those cells from running, so nothing is wrong today; stopping the walk there (for example offset = -2; return true;) would let expect(placed).toBe(true) fire.
4. Nit: assert the binding's warnings in the bound cells (StructuralCommands.test.ts:903). The @lexical/y self-check reports editor/document mismatches through onWarn, which bindFresh collects in warnings, and only errors is asserted here. No bound cell warns at this head, so asserting warnings is empty for that mode costs nothing.
5. Nit: refresh the Test plan. AGENTS.md (Commits and Pull Requests) asks for the real output of the command you ran; the pasted output is from be5afea, before the inline-decorator skips and before #9590 merged. At this head the file reports Tests 3308 passed | 24 expected fail | 128 skipped (3460).
No changeset is needed for a test-only change (CONTRIBUTING.md, Changesets).
| type?: string; | ||
| }; | ||
|
|
||
| const BLOCK_CMDS: BlockCmd[] = [ |
There was a problem hiding this comment.
Optional: consider adding $setBlocksType (heading and quote) to this list. It is the replacement #9612 names and what the playground toolbar calls, and with this family's check it fails 32 cells at this head: the caret inside overflow/text or the whole paragraph selected gives paragraph("abc ") / heading("over the limit") / paragraph(" end") where structure is enforced, or paragraph("abc " heading("over the limit") " end") without it, dropping the OverflowNode. $findMatchingParent(..., INTERNAL_$isBlock) in range-selection.ts:154-155 picks the overflow because its isInline() is false. Pinning those cells as test.fails with a new issue would keep the family covering the API apps call.
| // Where a shortcut does not act (in a container that declares | ||
| // nothing, and in a declared one without the rule), the | ||
| // paragraph stays as it was. | ||
| // A command given a selection always makes its block. | ||
| const made = | ||
| cmd.made(block) || | ||
| cmd.made(block.getParent() as LexicalNode) || | ||
| (cmd.type !== undefined && $isParagraphNode(block)) | ||
| ? block | ||
| : null; |
There was a problem hiding this comment.
Optional: this accepts an unchanged paragraph for a shortcut in every container. At this head the shortcuts act in the root, declared and shadow containers for all 23 shapes and in none of the plain ones, as model() predicts, so requiring the made block where model says 'in place' would catch a shortcut that stops firing on a paragraph with inline content. The "declared one without the rule" clause is stale now that #9590 is on main.
| if (!$isTextNode(n)) { | ||
| return false; |
There was a problem hiding this comment.
Nit: returning false here does not stop some(): the walk moves on to the next child with offset unchanged, so for an inline decorator the caret ends up at offset 1 of " end" and $caretInside() returns true, not false as its JSDoc says. The static skip hides it today; offset = -2; return true; (so the offset === -1 check fails) would match the contract.
| ); | ||
| } | ||
| await Promise.resolve(); | ||
| expect(opened.errors).toEqual([]); |
There was a problem hiding this comment.
Nit: the @lexical/y self-check reports editor/document mismatches through onWarn, which bindFresh collects in warnings, not errors. None of the bound cells warns at this head, so asserting warnings is empty for the bound mode would make these cells check the Yjs side too.
Description
The structural command cells varied the container a command runs in, but the selected paragraph always held plain text, so a change that splits the paragraph's inline content when it turns it into another block passed every cell. Review of #9590 found one:
OverflowNodereturns false fromisInline(), though the structure normalization treats it as inline, so with the caret inside an overflow,$insertListunderNodeStructureExtensionmade a list of the overflow alone.StructuralCommands.test.tsgets a family for that dimension. The commands that turn the selected paragraph into another block ($insertListof each type,$wrapNodesinto a quote, and the markdown shortcuts#,-,>) run on a paragraphabc+ an inline shape +end, for each of the catalog's 23 inline shapes (text in its modes, tabs, line breaks, hashtags and other text subclasses, links, marks, a link inside a mark, overflow, inline decorators), with the caret before the inline content, the caret inside its own text (shapes that have one), or the whole paragraph selected (the shortcuts are typed at the start), in the root, a container that declares nothing, one that declareschildren: 'block'and a shadow root, unbound withNodeStructureExtensionand bound with@lexical/y: 2,760 cells. Each checks the model rather than another tree: the block the command makes holds the paragraph's whole inline content, its text and the same inline nodes in order, with nothing split out; a shortcut that does not act (in a container that declares nothing, or a declared one without #9590) leaves the paragraph whole. Code highlight nodes live only in code blocks, so they are not among the shapes.The family finds a bug on main in the deprecated
$wrapNodes: with the caret inside an overflow, or inside a link inside a mark (or the whole paragraph selected), it splits the paragraph in three and drops the overflow or the mark. Those 24 cells aretest.failsnaming #9612. Everything else passes on main.Test plan
Before
New cells. Without the
test.failsmarks, on mainbe5afea:On #9590 at
f31df5d, also the overflow cells of$insertListwith the caret inside it (18).After
On main
be5afea: