add insertContent logic to setContent

[bdbch]

Please describe your changes

This PR removes some logical differences between setContent and insertContent or insertContentAt. setContent was using createDocument which is using a completely different way of parsing the content and creating an initial document.

Since setContent is never used on a non-existing document as the doc already exists as it is initialized while setting up the prosemirror editor inside the Editor class, we can just simply use insertContentAt to replace the whole document content instead of initializing a completely new doc to set.

This should help to make sure that in the future we only have one way of setting content into the editor removing the confusion regarding setContent and insertContent.

How did you accomplish your changes

I removed the old code from setContent and added a simple insertContent call to make sure all content commands are using the same way of content insertion logic.

How have you tested your changes

I created several tests for setContent and made sure they run in positive.

How can we verify your changes

Run the tests locally on your machine via npm run test:open and open the setContent spec. Otherwise you can also just run setContent in any editor.

Remarks

While I don't think this is a breaking change I think we should bump this change into a new minor version. In theory it does the same as the setContent logic (taking a document, setting the specified content as child content) - the only difference now is that escape characters \n or \t are now included while setting content and are not stripped out.

I'm open for this discussion on this one though.

Checklist

• The changes are not breaking the editor
• Added tests where possible
• Followed the guidelines
• Fixed linting issues

Related issues

• related to [Bug]: setContent breaks mentions when provided as html #4893 as I added a test for mentions via setContent
• related to [Bug]: 2.1.15 and up breaks dozens of our tests #4862 as I added a test for the case specified in this issue which now runs positive
• related to [Bug]: Difference in behavior of setContent and insertContent commands #4034 as this issue describes exactly what this fix is created for

[netlify]

✅ Deploy Preview for tiptap-embed ready!

Name	Link
🔨 Latest commit	beb8f05
🔍 Latest deploy log	https://app.netlify.com/sites/tiptap-embed/deploys/667ad3f73b49ce00080d768d
😎 Deploy Preview	https://deploy-preview-4895--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.

[Nantris]

Thanks again for your work on this!

This all seems sound. One concern though.

I think we should bump this change into a new minor version.

If that's the case, I think that the 2.1.x branch should have 2.1.15 reverted since people affected by #4862 can't upgrade past 2.1.15 without this patch being applied to the 2.1.x version. If waiting for the next minor version then this won't land until 2.3.x. I'm guessing 2.3.x is a long ways off, and currently we're blocked from even 2.2.0 by other issues.

That means we'd be stuck at 2.1.14 for potentially a very long time.

I don't know how long the team is planning to support the 2.1.x branch, but it seems quite possible that some fix will land on that branch that we need in the meanwhile.

[bdbch]

@slapbox could you point me to the issues which are blocking you from upgrading to 2.2.x right now? Not sure if we should start releasing or reverting patch versions of an older minor.

[Nantris]

Sure @bdbch - the only known blocker for us is #4704, but there's always a chance there's more blockers we haven't been able to get to yet because #4704 causes crashes.

[Nantris]

Thanks again for your work on this @bdbch. I'm wondering if you have any updates about how/when this change will be rolled out, or if 2.1.15 could be reverted? I think reverting 2.1.15 is the most appropriate approach, but if it's also incorporated into 2.2.x then I know that's a bit more complicated.

[Nantris]

Friendly bump.

Is there any way to provide some clarity on plans for this fix?

[bdbch]

I'll take a look at the failing tests today to see if they are coming from the changes introduced by this PR - otherwise I'll try to get another review.

[Nantris]

Ah yes I hadn't even realized there were failing tests. I expect the failures are unrelated based on it being complaining about editable vs non-editable, but maybe you'll find otherwise.

[Nantris]

2.3.0 seems to handle inserting /t properly, but not /n. If I insert /r/n it inserts a /n but it's not at all clear why this is necessary and it might have unintended consequences.

Inserting /n/n inserts two /n characters, leaving me all the more confused about why /n inserts zero of them.

Do you know why this might be? This is now the last known blocker for us to get off of the 2.1.x branch (so long as #5061 is also merged)

[Nantris]

@svenadlung any update on when this might be reviewed? I'm increasingly concerned we have no upgrade path and that it will only become more difficult to find one as this issue is baked into more and more versions of TipTap.

Patch releases should not break editors and when they do that should really be addressed. The lack of attention to fixing this unexpected breakage has been discouraging.

[nperez0111]

This was removed because commands.clearContent relies on setContent to be able to reset the document to the initial schema. This was early exiting and causing a no-op making clearContent not actually clear anything.

This is very old code that wasn't very well documented but after some testing, I'm confident in this not "leading to strange errors". insertContentAt is much more robust than when this was written.

I tested inserting empty content into a node and a block and it essentially resulted in a no-op.

[Nantris]

Thanks for reviewing @nperez0111. Will this be merged soon? We're still stuck on 2.1.x as 2.4.0 is released due to the underlying issue I hope this PR will resolve.

[nperez0111]

Thanks for reviewing @nperez0111. Will this be merged soon? We're still stuck on 2.1.x as 2.4.0 is released due to the underlying issue I hope this PR will resolve.

I understand your urgency. We are currently in discussion of whether this is actually a breaking change or not. We have discussed the possibility of releasing an rc to get feedback on whether it breaks user's workflows or not.

We will get back to you soon on this. We appreciate your patience.

[Nantris]

I noticed 2.5.0-beta.0 dropped today. I was wondering if this is planned for 2.5.x? This issue remains as the only one preventing us from upgrading past 2.1.13.

Could this be addressed by adding a flag to setContent and insertContent to enable either behavior as the end-developer requires? Otherwise I fear this is never going anywhere because it seems bound to be a breaking change for some people no matter what the final behavior. Many people have been developing with the new default behavior since November, whereas everyone on versions 2.1.13 and below is going to expect the old behavior.

Has any decision been reached? Thanks for reading!

---

PS: To clarify what exactly the problem the existing behavior causes for us:

2.1.13: inserting \n works fine, inserts a linebreak in code
2.1.14: inserting \n inserts nothing, but inserting \n\n inserts two linebreaks into code

[Nantris]

I had the idea for a workaround trying to remove one of the inserted \n characters in a chained command so we could unblock ourselves by inserting \n\n instead of \n in the meantime, but unfortunately this produces a mismatched transaction, so this indeed remains a blocker.

[nperez0111]

So, I've taken a slightly different approach with this one. I made it only ever apply the new "correct" logic when parseOptions.preserveWhitespace === 'full' || parseOptions.preserveWhitespace === true I did this because there is a weird mix between prosemirror parsing behaviors and tiptap parsing behaviors. Given that we already expose that property, we may as well respect what the developer intended.

preserveWhitespace is not entirely being respected in certain cases since tiptap does some whitespace removal of it's own on top. But with this additional it is at least a bit more in-line with prosemirror by keeping whitespace when the developer explicitly said that it should be kept entirely.

This therefore keeps backwards compatibility, while also exposing the correct behavior behind an existing option that now is respected.

Eventually this should be removed and we rely on insertContentAt's behaviors for everything, but I left only a comment to deal with that in the next major.

Thoughts @svenadlung & @bdbch ?

[Nantris]

Any update here? This has been broken for over half a year now.

[nperez0111]

@Nantris it is now part of v2.5.0-beta.5

Thanks for pushing this forward

[Nantris]

Many thanks to you all! Looks good!