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

fix: convert content to a string before passing it to applyPasteRules meta value in insertContentAt.ts #5152

Conversation

gabemagee-ev
Copy link

@gabemagee-ev gabemagee-ev commented May 14, 2024

Changes Overview

In insertContentAt.ts cast content as a string in insertContentAt, since createNodeFromContent can return an Object when the content is complex, which prevents paste and input rules from being applied.

Implementation Approach

converting the object to a string prevents text.length from being NaN in PasteRule.ts:appendTransaction, which is necessary in order to process any paste or input events.

Testing Done

npm run test and testing the initial case that led me to the bug in the browser using developer tools

Verification Steps

Additional Notes

Checklist

  • I have renamed my PR according to the naming conventions. (e.g. feat: Implement new feature or chore(deps): Update dependencies)
  • 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

#5150

Copy link

netlify bot commented May 14, 2024

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit 12a46e4
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/665fe6383df4150008282d11
😎 Deploy Preview https://deploy-preview-5152--tiptap-embed.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@@ -127,11 +127,11 @@ export const insertContentAt: RawCommands['insertContentAt'] = (position, value,
}

if (options.applyInputRules) {
tr.setMeta('applyInputRules', { from, text: newContent })
tr.setMeta('applyInputRules', { from, text: newContent.toString() })
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I see why you are doing this. But I think it is probably better to do it in the else branches on line :119 and :114 instead since those are the branches that does not attempt to convert it to a string (really 119 is the one that matters 114 should be a string)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also tests would be greatly appreciated

Copy link
Author

Choose a reason for hiding this comment

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

that what I originally had, but I opted for this since I think we want newContent to retain it's original assignment for the tr.replaceWith(from, to, newContent) in line :121.

Copy link
Author

Choose a reason for hiding this comment

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

since this is my first contribution to this repo, I don't know where everything is. can you point me to where tests for a command or pasteRule would go, since I don't see any preexisting ones, or perhaps I am missing them. @nperez0111 thank you!

Copy link
Contributor

@nperez0111 nperez0111 May 15, 2024

Choose a reason for hiding this comment

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

@gabemagee-ev , the tests would be in this file:

context('/src/Commands/InsertContent/React/', () => {
before(() => {
cy.visit('/src/Commands/InsertContent/React/')
})
beforeEach(() => {
cy.get('.tiptap').type('{selectall}{backspace}')
})
it('should insert html content correctly', () => {
cy.get('button[data-test-id="html-content"]').click()
// check if the content html is correct
cy.get('.tiptap').should('contain.html', '<h1>Tiptap</h1><p><strong>Hello World</strong></p><p>This is a paragraph<br>with a break.</p><p>And this is some additional string content.</p>')
})
it('should keep spaces inbetween tags in html content', () => {
cy.get('.tiptap').then(([{ editor }]) => {
editor.commands.insertContent('<p><b>Hello</b> <i>World</i></p>')
cy.get('.tiptap').should('contain.html', '<p><strong>Hello</strong> <em>World</em></p>')
})
})
it('should keep empty spaces', () => {
cy.get('.tiptap').then(([{ editor }]) => {
editor.commands.insertContent(' ')
cy.get('.tiptap').should('contain.html', '<p> </p>')
})
})
it('should insert text content correctly', () => {
cy.get('button[data-test-id="text-content"]').click()
// check if the content html is correct
cy.get('.tiptap').should('contain.html', 'Hello World\nThis is content with a new line. Is this working?\n\nLets see if multiple new lines are inserted correctly')
})
it('should keep newlines in pre tag', () => {
cy.get('.tiptap').then(([{ editor }]) => {
editor.commands.insertContent('<pre><code>foo\nbar</code></pre>')
cy.get('.tiptap').should('contain.html', '<pre><code>foo\nbar</code></pre>')
})
})
})

You can follow this guide to get your environment set up:

## Set up the development environment
It’s not too hard to tinker around with the official repository. You’ll need [Git](https://github.com/git-guides/install-git), [Node and NPM](https://nodejs.org/en/download/) installed. Here is what you need to do then:
1. Copy the code to your local machine: `$ git clone [email protected]:ueberdosis/tiptap.git`
2. Install dependencies: `$ npm install`
3. Start the development environment: `$ npm run start`
4. Open http://localhost:3000 in your favorite browser.
5. Start playing around!

You are right about maintaining the call, I missed that.

I appreciate your contribution very much. If you need any help, I'm happy to 😄 !

@gabemagee-ev
Copy link
Author

after adding testing, I realized this wasn't a solution to the linked bug. Will work on a fix when I have time, likely next week. Sorry for the ping!

@nperez0111
Copy link
Contributor

after adding testing, I realized this wasn't a solution to the linked bug. Will work on a fix when I have time, likely next week. Sorry for the ping!

Hey @gabemagee-ev, I actually ran into this myself sometime last week and I started working on something to fix it but did not get the time to finish it. Would be great if you could take it over the finish-line, here is my branch for it (pointed at the only additional commit I made branch name is fix/simulated-paste-rules): fddfe31

The core of my idea here was to make the InputRules & PasteRules plugins do the work of accepting different content types and convert those to a format that can actually be used properly.

I think that my code works but I never got around to writing tests for it.

tests
@gabemagee-ev gabemagee-ev force-pushed the fix/cast-content-as-string-paste-rule branch from 9b0009e to 12a46e4 Compare June 5, 2024 04:14
@gabemagee-ev gabemagee-ev marked this pull request as draft June 5, 2024 04:15
@gabemagee-ev
Copy link
Author

@nperez0111 hey, I'm looking back into this. have you done any further work other than the commit you linked? thanks

@nperez0111
Copy link
Contributor

@nperez0111 hey, I'm looking back into this. have you done any further work other than the commit you linked? thanks

Nope just that branch (the one commit)

@nperez0111
Copy link
Contributor

I merged what I had

@nperez0111 nperez0111 closed this Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants