Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
8 changes: 8 additions & 0 deletions demos/src/Examples/Default/React/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -84,48 +84,56 @@ const MenuBar = () => {
<button
onClick={() => editor.chain().focus().toggleHeading({ level: 1 }).run()}
className={editor.isActive('heading', { level: 1 }) ? 'is-active' : ''}
disabled={!editor.can().toggleHeading({ level: 1 })}
>
H1
</button>
<button
onClick={() => editor.chain().focus().toggleHeading({ level: 2 }).run()}
className={editor.isActive('heading', { level: 2 }) ? 'is-active' : ''}
disabled={!editor.can().toggleHeading({ level: 2 })}
>
H2
</button>
<button
onClick={() => editor.chain().focus().toggleHeading({ level: 3 }).run()}
className={editor.isActive('heading', { level: 3 }) ? 'is-active' : ''}
disabled={!editor.can().toggleHeading({ level: 3 })}
>
H3
</button>
<button
onClick={() => editor.chain().focus().toggleHeading({ level: 4 }).run()}
className={editor.isActive('heading', { level: 4 }) ? 'is-active' : ''}
disabled={!editor.can().toggleHeading({ level: 4 })}
>
H4
</button>
<button
onClick={() => editor.chain().focus().toggleHeading({ level: 5 }).run()}
className={editor.isActive('heading', { level: 5 }) ? 'is-active' : ''}
disabled={!editor.can().toggleHeading({ level: 5 })}
>
H5
</button>
<button
onClick={() => editor.chain().focus().toggleHeading({ level: 6 }).run()}
className={editor.isActive('heading', { level: 6 }) ? 'is-active' : ''}
disabled={!editor.can().toggleHeading({ level: 6 })}
>
H6
</button>
<button
onClick={() => editor.chain().focus().toggleBulletList().run()}
className={editor.isActive('bulletList') ? 'is-active' : ''}
disabled={!editor.can().toggleBulletList()}
>
Bullet list
</button>
<button
onClick={() => editor.chain().focus().toggleOrderedList().run()}
className={editor.isActive('orderedList') ? 'is-active' : ''}
disabled={!editor.can().toggleOrderedList()}
>
Ordered list
</button>
Expand Down
22 changes: 22 additions & 0 deletions demos/src/Examples/Default/React/index.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,28 @@ context('/src/Examples/Default/React/', () => {
})
})

it('Heading, bulletList, and orderedList should be allowed to convert between each other.', () => {
cy.get('.tiptap h1').should('exist')
cy.get('.tiptap ul').should('not.exist')
cy.get('.tiptap ol').should('not.exist')
cy.get('button').contains('Bullet list').should('not.be.disabled')
cy.get('button').contains('Ordered list').should('not.be.disabled')

cy.get('button').contains('Bullet list').click()
cy.get('.tiptap h1').should('not.exist')
cy.get('.tiptap ul').should('exist')
cy.get('.tiptap ol').should('not.exist')
cy.get('button').contains('H1').should('not.be.disabled')
cy.get('button').contains('Ordered list').should('not.be.disabled')

cy.get('button').contains('Ordered list').click()
cy.get('.tiptap h1').should('not.exist')
cy.get('.tiptap ul').should('not.exist')
cy.get('.tiptap ol').should('exist')
cy.get('button').contains('H1').should('not.be.disabled')
cy.get('button').contains('Bullet list').should('not.be.disabled')
})

it('should apply the paragraph style when the keyboard shortcut is pressed', () => {
cy.get('.tiptap h1').should('exist')
cy.get('.tiptap p').should('not.exist')
Expand Down
2 changes: 2 additions & 0 deletions demos/src/Examples/Transition/Vue/index.spec.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
/// <reference types="cypress" />

context('/src/Examples/Transition/Vue/', () => {
beforeEach(() => {
cy.visit('/src/Examples/Transition/Vue/')
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/CommandManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ export class CommandManager {

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

Choose a reason for hiding this comment

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

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.

const tr = startTr || state.tr
const props = this.buildProps(tr, dispatch)
const formattedCommands = Object.fromEntries(
Expand Down
4 changes: 3 additions & 1 deletion packages/core/src/commands/clearNodes.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

The actual problem in this file is:

  if (!dispatch) {
    return true
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ export const clearNodes: RawCommands['clearNodes'] = () => ({ state, tr, dispatc
if (node.type.isTextblock) {
const { defaultType } = $mappedFrom.parent.contentMatchAt($mappedFrom.index())

tr.setNodeMarkup(nodeRange.start, defaultType)
if (defaultType && defaultType.isTextblock) {
tr.setNodeMarkup(nodeRange.start, defaultType)
}
}

if (targetLiftDepth || targetLiftDepth === 0) {
Expand Down
6 changes: 3 additions & 3 deletions tests/cypress/integration/core/can.spec.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

This test was correct. Please revert

Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ describe('can', () => {
expect(canSetMarkToBold).to.eq(true)
})

it('builds and passes down an undefined dispatch for nested "can" chain', () => {
it('builds and passes down a function dispatch for nested "can" chain', () => {
const editor = new Editor({
extensions: [Document, Paragraph, Text, History],
})
Expand All @@ -191,8 +191,8 @@ describe('can', () => {
.run()

// eslint-disable-next-line no-unused-expressions
expect(capturedOuterDispatch).to.be.undefined
expect(capturedOuterDispatch).to.be.not.undefined
// eslint-disable-next-line no-unused-expressions
expect(capturedInnerDispatch).to.be.undefined
expect(capturedInnerDispatch).to.be.not.undefined
})
})