[Bug]: Using clearNodes in the can() command will return incorrect results

[guanriyue]

Affected Packages

core, extension-bullet-list

Version(s)

2.8.0

Bug Description

My English isn't very good, so I use ChatGPT to help with translations. If there are any inaccuracies in my expressions, please let me know!

The can() command returns false when the command can actually be executed.

2024-10-12.10.37.46.mov

I tried debugging and fixing this issue, but in the end, it seems like it's actually something that needs to be handled by tiptap.

I have some hypotheses, but I can't confirm if they are correct. They might introduce new issues or cause breaking changes.

There are several related issues in tiptap's issues section, which we will mention in the following explanation.

• .can() returns false for extension-list even when the command is runnable #3229
• [Bug]: can command not working correctly with list #4057
• dispatch=undefined in createCan() results in dispatch attempts during can() #3025

——————————————————————————————————————————————————————————————————

The most immediate issue occurs in the clearNodes command

In the screen recording, we see that editor.can().toggleHeading({ level: 1 }) and editor.commands.toggleHeading({ level: 1 }) return different values, so let's first take a look at what's happening inside the toggleHeading command.

toggleHeading: attributes => ({ commands }) => {
  if (!this.options.levels.includes(attributes.level)) {
    return false
  }

  return commands.toggleNode(this.name, 'paragraph', attributes)
},

toggleNode calls setNode, so let's directly take a look at setNode.
The original code comments use //.
The comments below are my own additions, marked with //// for clarity.
We made two markers, A and B. The state at marker A is referred to as stateA, and the state at marker B is referred to as stateB (updatedState). As expected, stateA and stateB should be different.

export const setNode: RawCommands['setNode'] = (typeOrName, attributes = {}) => ({ state, dispatch, chain }) => {
  //// Skipping a small portion of unnecessary code.

  return (
    chain()
    // try to convert node to default node if needed
      .command(({ commands }) => {
        //// Mark A (StateA)
        const canSetBlock = setBlockType(type, attributes)(state)

        //// At this point, we attempt to determine whether it is allowed to directly modify list item 
        //// nodes into headings.
        //// Clearly, this is an invalid operation. So the value of canSetBlock is false
        if (canSetBlock) {
          return true
        }

        //// So we attempt to clear the list item nodes first. 
        //// According to the expected logic of clearNodes, they will be converted into paragraph nodes.
        return commands.clearNodes()
      })
      .command(({ state: updatedState }) => {
        //// Mark B (StateB)
        //// After the original nodes are converted into paragraph nodes,
        //// we attempt to convert the nodes into headings again using the latest state.
        //// Clearly, this is a valid operation, so the expected return result is `true`.
        return setBlockType(type, attributes)(updatedState, dispatch)
      })
      .run()
  )
}

After switching the list item node to a regular paragraph node, can().toggleHeading() still returns false.

This means that clearNodes did not execute as expected. Let's take a look at what happens inside clearNodes.

export const clearNodes: RawCommands['clearNodes'] = () => ({ state, tr, dispatch }) => {
  const { selection } = tr
  const { ranges } = selection

  if (!dispatch) {
    return true
  }

  //// Perform some operations on the current tr.

  return true
}

When the dispatch value is falsy (undefined), it will return true immediately.

can(), chain(), and commands all come from CommandManager. When executing a command, dispatch will be a function. However, if can() is used to check whether a command is allowed to be executed, dispatch will be undefined.

This means that when editor.can().toggleHeading({ level: 1 }) is executed, clearNodes does not make any changes to tr. Therefore, stateA and stateB are the same. At markers A and B, both the command and the parameters are identical. As a result, the outcome at marker B must also be the same as at marker A, which is false.

In other words, clearNodes did not perform any actions, leading to an incorrect final result for the can() command.
Therefore, we should remove the dispatch check in clearNodes and always allow tr to update in order to obtain a new state.

toggleBulletList and toggleOrderedList encounter the same issue, which is present in the toggleList code.

tiptap/packages/core/src/commands/toggleList.ts

Lines 122 to 128 in 4efd227

	const canWrapInList = can().wrapInList(listType, attributes)
	if (canWrapInList) {
	return true
	}
	return commands.clearNodes()

——————————————————————————————————————————————————————————————————

Directly removing the dispatch check is not the correct solution.

Removing the dispatch check will introduce another issue, which are related to issue #3025.
This issue is related to this segment of code in clearNodes:

tiptap/packages/core/src/commands/clearNodes.ts

Lines 42 to 46 in 4efd227

	if (node.type.isTextblock) {
	const { defaultType } = $mappedFrom.parent.contentMatchAt($mappedFrom.index())
	tr.setNodeMarkup(nodeRange.start, defaultType)
	}

Typically, we use paragraph nodes as the default node for the document, which is why tiptap sets the priority of paragraph nodes to 1000, while the default value of node priority is 100.
A paragraph node is a type of TextBlock node.

However, if defaultType is not a TextBlock node, then setNodeMarkType will throw an error when checking proseMirrorNode.validContent. This is the issue exemplified in issue #3025, which can be found in the provided codesandbox. It has a custom node called mycontainer, which has a higher priority than paragraph nodes, making it the default node for the document. This custom node only allows myinline (a TextBlock node) as its child nodes.

If the node in the code node.type.isTextblock is a paragraph (with inline child nodes), switching to mycontainer will result in an error.

Therefore, the code in clearNodes should include an additional check to ensure that defaultType is also a TextBlock before calling tr.setNodeMarkType.
Alternatively, we could attempt to find the first descendant node of defaultType that is of type TextBlock, recreate the node, and use tr.replaceRangeWith to switch the node.
I'm not sure if this approach is correct, but we definitely need to fix this issue.

——————————————————————————————————————————————————————————————————

Each command executed with can() should be wrapped in a try-catch block. If an exception occurs, the subsequent command checks should be terminated (in chainCommand), and false should be returned.

For issue #3025, it implemented a 'wrong' fix #3026.

This fix changes the shouldDispatch value from undefined (equivalent to true) to false inside CommandManager.createCan. After this fix was merged, it indeed resolved the issue of can().toggleBulletList() throwing an exception. However, executing commands.toggleBulletList() still throws an exception.

The root cause of this exception is triggered by the clearNodes command. When shouldDispatch is changed to false, the dispatch in commandProps becomes undefined, which then hits the condition if (!dispatch) inside clearNodes. As a result, can().toggleBulletList() does not throw an error and returns true, but the command still cannot be executed correctly.

There are many commands that may produce exceptions, as we cannot guarantee that the code we write is free of bugs. However, we must ensure that executing can() does not throw exceptions. This is because we typically call can() during the render phase to determine whether the corresponding toolbar buttons should be disabled. For example, in React, if an exception is thrown during the render phase, it can cause the application to 'crash,' resulting in a poor user experience.

Therefore, if the execution of a command throw an exception, then can() should always return false when checking that command. This is because executing that command can never yield any effect.

In order to provide a better development experience, when an exception is caught, we should use console to notify the developer. Or emit a "commandError" event.

——————————————————————————————————————————————————————————————————

For chainCommand, shouldDispatch should always be set to true to ensure that commandProps.dispatch is a function(a truth value) rather than undefined(a false value).

Let's think about how we define a command. I will use pseudocode to illustrate.

const myCommand = (props: CommandProps) => {
  const { state, tr, dispatch } = props;
  
  if (!ConditionsForExecutingTheCommandAreNotMet(state, tr)) {
    return false;
  }

  if (dispatch) {
    //// We only perform certain calculations when dispatch is needed; otherwise, we skip this section. 
    //// This can provide a slight performance boost."
    doSomeOperationsOnTr(tr);
  }
  
  return true; 
};

Taking the previously mentioned clearNodes as an example, the editor should be allowed to perform this operation in any state. This is also why we can directly return true when dispatch is undefined, skipping the subsequent operations on tr. As a result, executing editor.can().clearNodes() will always return true.

However, in chainCommand, we cannot skip the operations on tr.

Because in chainCommand, we record multiple command operations sequentially on a single tr. The later commands may depend on the earlier commands.

For example, in the initial case of stateA and stateB, stateB depends on the state derived after executing clearNodes on stateA. However, when executing clearNodes, we skipped the operations on tr, resulting in no changes to stateA. Consequently, stateB is identical to stateA, leading to an incorrect result.

We may need to revert the code involved in this fix #3026.

——————————————————————————————————————————————————————————————————

Ultimately, regardless of the solution, the issues with clearNodes should be addressed:

1. We cannot skip operations on tr.
2. We need to resolve the issues mentioned in dispatch=undefined in createCan() results in dispatch attempts during can() #3025.

Browser Used

Chrome

Code Example URL

No response

Expected Behavior

The execution of editor.can() returns the correct result.

Additional Context (Optional)

No response

Dependency Updates

• Yes, I've updated all my dependencies.