Fixed issue where can() returned false when the command was executable #5744

guanriyue · 2024-10-21T04:00:23Z

Changes Overview

Fix the issue where can() check returns false even when the command is actually executable. And added cypress test cases.

Implementation Approach

When chainCommand is executed, multiple commands are run sequentially. The editor state used by later commands may depend on the preceding commands. Therefore, when executing can(), createChain must ensure that shouldDispatch is true to prevent skipping operations on tr.

This fix effectively reverts the code changes from the pull request #3026 and resolves the issue #3025 caused by the clearNodes error.

At the same time, it will also fix issue #3229 and issue #4057.

For more details, please refer to this issue #5721.

Testing Done

First, add the disabled attribute to the heading and list action buttons in the example default editor.

Before the fix, if the selection was on a heading node, the list action buttons would remain disabled, even though the commands were actually available. After the change, the list action buttons will appear enabled.

Additionally, corresponding Cypress test cases have been added.

Verification Steps

Access the default example editor at https://tiptap.dev/docs/examples/basics/default-text-editor or by executing cypress test

Additional Notes

Checklist

I have created a changeset for this PR if necessary.
My changes do not break the library.
I have added tests where applicable.
I have followed the project guidelines.
I have fixed any lint issues.

Related Issues

…le default editor.

…let list, ordered list, and heading are allowed to convert between each other.

…().run()` because the state required by later commands may depend on the commands executed earlier.

…e`, otherwise `clearNodes` will throw an exception during execution.

changeset-bot · 2024-10-21T04:00:26Z

⚠️ No Changeset found

Latest commit: 83e48ff

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

netlify · 2024-10-21T04:00:38Z

✅ Deploy Preview for tiptap-embed ready!

Name	Link
🔨 Latest commit	`83e48ff`
🔍 Latest deploy log	https://app.netlify.com/sites/tiptap-embed/deploys/671a34ee0bae970007fde2a1
😎 Deploy Preview	https://deploy-preview-5744--tiptap-embed.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

…o buttons, which will throw an exception and cause the test to fail.

nperez0111 · 2024-10-25T17:55:20Z

Hey @guanriyue I haven't reviewed this super thoroughly, because I think that this approach is wrong.

What can is meant for is to be able to allow commands to check whether they would be able to execute without actually modifying the document via transactions. At times, this can be sort of ambiguous, so sometimes a lazy implementation may not actually implement this properly. I think that clearNodes in this case is just implemented improperly where it should not have that early dispatch.

I do not believe that you need to actually change can in this case. I think that what needs to happen is there needs to be a better implementation of clearNodes.

nperez0111 · 2024-10-25T17:57:07Z

packages/core/src/CommandManager.ts

@@ -101,7 +101,7 @@ export class CommandManager {

  public createCan(startTr?: Transaction): CanCommands {
    const { rawCommands, state } = this
-    const dispatch = false
+    const dispatch = true


I believe this is wrong.

If dispatch is true, commands will think that they can modify the transaction when they should not be able to.

dispatch should probably be named shouldDispatch where it being false means you should not, as a command, actually apply your change to the editor.

nperez0111 · 2024-10-25T17:57:30Z

tests/cypress/integration/core/can.spec.ts

This test was correct. Please revert

nperez0111 · 2024-10-25T17:58:06Z

packages/core/src/commands/clearNodes.ts

The actual problem in this file is:

if (!dispatch) { return true }

Remove this section of code will resolve the current issue. However, it seems there is a deeper problem that needs to be addressed, which would involve a significant change.

I will take your advice and work to close this PR as soon as possible.

This is what the PR should be addressing. Everything else around this PR is incorrect with the intention behind can chains and how they should work

guanriyue · 2024-10-28T09:52:02Z

Have you looked at the description of this issue #5721? I explained why I believe dispatch should be a function in can.chain.

Perhaps it is a mistake; in fact, setting dispatch to true was part of the code logic before the last bug fix #3026. It's actually a bit of a long story.

Four years ago, at the very beginning, the dispatch variable was set to false.
7bfab46#diff-ac93a7861b6cdca5f5dd83ec422e904ebb145fd00a39343834384c6d241965c6R103

About five months later, it was changed to undefined for unclear reasons. When passed into buildProps, JavaScript treated it as true by default. These two changes come from the same author.
ca8d1a4#diff-ac93a7861b6cdca5f5dd83ec422e904ebb145fd00a39343834384c6d241965c6L87-R90

About a year and a half later, clearNodes caused an error, but the issue with clearNodes was not fixed, and executing command.clearNodes could still lead to problems, while the implementation of createCan was modified. dispatch was reset to false.
40f4ea3#diff-ac93a7861b6cdca5f5dd83ec422e904ebb145fd00a39343834384c6d241965c6L109-R111

After that fix, several new issues with can checks emerged.

I am now trying to change it back to true . And fix the real issue in clearNodes. It runs all the tests successfully on my local machine, except for the can.chain.dispatch.except.to.be.undefined, which is a test case added after the fix #3026.

If dispatch is true, commands will think that they can modify the transaction when they should not be able to.

Yes, you're right. However, in can.chain, it seems that we should not skip modifications on tr under any circumstances, as subsequent commands may depend on the state and tr after the preceding command is executed. Just like how skipping clearNodes caused issues. There are many commands similar to clearNodes. In our pursuit of higher performance and avoiding unnecessary code execution, we tend to favor lazy implementations.

dispatch should probably be named shouldDispatch where it being false means you should not, as a command, actually apply your change to the editor.

Yes, that is an inappropriate naming; it should be renamed to shouldDispatch.

nperez0111 · 2024-11-04T09:55:48Z

I'm sorry, but, I still believe that this is all misguided. Please refer to the documentation of Prosemirror https://prosemirror.net/docs/guide/#commands It specifically says that:

To figure out whether a selection can currently be deleted, you'd call deleteSelection(view.state, null), whereas to actually execute the command, you'd do something like deleteSelection(view.state, view.dispatch). A menu bar could use this to determine which menu items to gray out.

In this form, commands do not get access to the actual editor view—most commands don't need that, and in this way they can be applied and tested in settings that don't have a view available. But some commands do need to interact with the DOM—they might need to query whether a given position is at the end of a textblock, or want to open a dialog positioned relative to the view. For this purpose, most plugins that call commands will give them a third argument, which is the whole view.

function blinkView(_state, dispatch, view) {
if (dispatch) {
view.dom.style.background = "yellow"
setTimeout(() => view.dom.style.background = "", 1000)
}
return true
}
That (rather useless) example shows that commands don't have to dispatch a transaction—they are called for their side effect, which is usually to dispatch a transaction, but may also be something else, such as popping up a dialog.

It specifically says it should be called like deleteSelection(view.state, null), with the implementation as:

 function blinkView(_state, dispatch, view) {
  if (dispatch) {
    view.dom.style.background = "yellow"
    setTimeout(() => view.dom.style.background = "", 1000)
  }
  return true
 }

This is done purposefully, so that a command can be "tried" without actually committing the side-effect. If you pass true you defeat the purpose of it as you'll modify the editor content with the transaction or side-effect.

You are not solving the real problem which is the implementation of the command. You are attempting to change how can chains work to fit a wrong implementation, fix the implementation not how can chains work.

guanriyue added 4 commits October 21, 2024 10:52

feat: Add control to the heading and list action buttons in the examp…

e010e7b

…le default editor.

feat: Add test cases to the example default editor to ensure that bul…

cd28cfc

…let list, ordered list, and heading are allowed to convert between each other.

fix: shouldDispatch should remain true in `can().chain().commands…

cc6df2c

…().run()` because the state required by later commands may depend on the commands executed earlier.

fix: If defaultType.isTextBlock is false, skip `tr.setNodeMarkTyp…

c3d12d4

…e`, otherwise `clearNodes` will throw an exception during execution.

guanriyue requested review from bdbch and svenadlung as code owners October 21, 2024 04:00

guanriyue added 2 commits October 21, 2024 14:33

fix: Fix the issue in the test case, otherwise Cypress will detect tw…

adb6a81

…o buttons, which will throw an exception and cause the test to fail.

fix: In can.chain, dispatch should be guaranteed to be a function

6b08af8

guanriyue mentioned this pull request Oct 21, 2024

Fix the issue where can() check returns false even when the command can be executed. #5741

Closed

5 tasks

guanriyue and others added 2 commits October 21, 2024 19:22

Merge branch 'develop' into fix/can-commands

944f77b

Merge branch 'develop' into fix/can-commands

83e48ff

nperez0111 requested changes Oct 25, 2024

View reviewed changes

nperez0111 added the Info: Invalid The issue or pullrequest is invalid label Nov 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed issue where can() returned false when the command was executable #5744

Fixed issue where can() returned false when the command was executable #5744

guanriyue commented Oct 21, 2024

changeset-bot bot commented Oct 21, 2024 •

edited

Loading

netlify bot commented Oct 21, 2024 •

edited

Loading

nperez0111 commented Oct 25, 2024

nperez0111 Oct 25, 2024

nperez0111 Oct 25, 2024

nperez0111 Oct 25, 2024

guanriyue Oct 28, 2024

nperez0111 Nov 4, 2024

guanriyue commented Oct 28, 2024

nperez0111 commented Nov 4, 2024

Fixed issue where can() returned false when the command was executable #5744

Are you sure you want to change the base?

Fixed issue where can() returned false when the command was executable #5744

Conversation

guanriyue commented Oct 21, 2024

Changes Overview

Implementation Approach

Testing Done

Verification Steps

Additional Notes

Checklist

Related Issues

changeset-bot bot commented Oct 21, 2024 • edited Loading

⚠️ No Changeset found

netlify bot commented Oct 21, 2024 • edited Loading

✅ Deploy Preview for tiptap-embed ready!

nperez0111 commented Oct 25, 2024

nperez0111 Oct 25, 2024

Choose a reason for hiding this comment

nperez0111 Oct 25, 2024

Choose a reason for hiding this comment

nperez0111 Oct 25, 2024

Choose a reason for hiding this comment

guanriyue Oct 28, 2024

Choose a reason for hiding this comment

nperez0111 Nov 4, 2024

Choose a reason for hiding this comment

guanriyue commented Oct 28, 2024

nperez0111 commented Nov 4, 2024

changeset-bot bot commented Oct 21, 2024 •

edited

Loading

netlify bot commented Oct 21, 2024 •

edited

Loading