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

[Bug]: chained splitBlock commands split at unexpected points #4376

Open
1 of 2 tasks
Nantris opened this issue Aug 24, 2023 · 9 comments
Open
1 of 2 tasks

[Bug]: chained splitBlock commands split at unexpected points #4376

Nantris opened this issue Aug 24, 2023 · 9 comments
Labels
Category: Open Source The issue or pull reuqest is related to the open source packages of Tiptap. Type: Bug The issue or pullrequest is related to a bug

Comments

@Nantris
Copy link
Contributor

Nantris commented Aug 24, 2023

Which packages did you experience the bug in?

core

What Tiptap version are you using?

2.1.3

What’s the bug you are facing?

chain().splitBlock().splitBlock().splitBlock().run() produces very unexpected results, possibly due to improper mapping. This issue also affects the base splitBlock ProseMirror command but it's not suitable to file it there because ProseMirror commands are not designed to be chained.

What browser are you using?

Chrome

Code example

No response

What did you expect to happen?

I expected the same results from chain().splitBlock().splitBlock().splitBlock().run() as from commands.splitBlock(); commands.splitBlock(); commands.splitBlock();

Anything to add? (optional)

chained-split-block-commands.mp4
singular-split-block-commands.mp4

Did you update your dependencies?

  • Yes, I’ve updated my dependencies to use the latest version of all packages.

Are you sponsoring us?

  • Yes, I’m a sponsor. 💖
@Nantris Nantris added Category: Open Source The issue or pull reuqest is related to the open source packages of Tiptap. Type: Bug The issue or pullrequest is related to a bug labels Aug 24, 2023
@Nantris
Copy link
Contributor Author

Nantris commented Aug 24, 2023

This appears to be another of those edge cases introduced by chaining. - unfortunately this is the first time I've encountered an issue like this outside of lists, so the problems may be more pervasive than I believed.

I confirmed that removing all uses of tr.mapping.map in the splitBlock function resolves this, but I'd suspect that breaks something? somewhere?

As far as detecting these cases more broadly, I'd suggest a new suite of tests that compare the results of:

  • editor.commands.someCommand(); editor.commands.someCommand(); editor.commands.someCommand();
    against
  • editor.chain().someCommand().someCommand().someCommand().run()

and the expectation would be that they produce identical outputs. @bdbch @svenadlung

@Nantris
Copy link
Contributor Author

Nantris commented Oct 8, 2023

Encountered this again with a simple chain like editor.chain().deleteSelection().moveSelectionBy(2).splitBlock().run() at the start of a codeblock without any lists involved. moveSelection uses the tr.selection and not the state.selection so this really shouldn't be an issue, but it is.

This issue really breaks the promise of TipTap and locks users into exhausting searches for solutions to things that are supposed to be simple because ProseMirror just can't support what TipTap tries to make it do out of the box. Some patches are necessary, or at least warnings on the chain documentation mentioning this. We've spent probably almost a month working around these sorts of issues and it's getting extremely discouraging.

Any comment on a possible plan for this? If there's no plan to address this then I would have to recommend against new projects using TipTap, because it sets you out on the path of trying to resolve something using chain so you spend hours doing it just to find it's impossible for undocumented reasons and then you have to redo the work in pure ProseMirror. I cannot stress enough how much time this is wasting and our only apparent mistake is expecting the APIs to work as documented.

@bdbch
Copy link
Contributor

bdbch commented Oct 10, 2023

I'll be available for work tomorrow morning and I'll check out the issue described here to totally understand what exactly goes wrong. As far as I understood this edge case occurs as soon as you do multiple modifications with fixed positions which don't point to the correct positions after the first chained transaction?

@bdbch
Copy link
Contributor

bdbch commented Oct 10, 2023

Can't give a 100% plan for this right now, just that we're going to look into this.

@Nantris
Copy link
Contributor Author

Nantris commented Oct 10, 2023

Thanks for the reply @bdbch. Unfortunately it's still not entirely clear to me what causes the issues. Most of the manifestations have been in the listItems so that's made it hard to pick up on generalizable patterns.

I don't think it related to fixed positions per se.

My best theory for the latest issue I faced would be that split operations after delete operations mangle the contents due to ProseMirror's internal logic not expecting such events to be chained together. Based on the nature of the previously reported list issues, I'm guessing it's some failure in the mapping logic in cases where they're chained together that unfortunately will not be trivial to fix.

@bdbch
Copy link
Contributor

bdbch commented Oct 10, 2023

I'm pretty sure it's because the chain uses the same transaction and state before dispatching which means positions are not mapped automatically between chain steps.

I did a quick test yesterday and was able to get the splitBlock command to work with chaining via adding Transaction Mapping in my command.

I still think about a good solution that would solve this automatically but from what I've seen as for now users need to map positions manually. Means we also need to go through our commands and fix this.

@bdbch
Copy link
Contributor

bdbch commented Oct 12, 2023

For the first draft I'd opt into adding a bit into the documentation about the requirement to map manually in commands.

Second draft will be to fix missing mappings of positions in Tiptap's core commands

Third draft would be to find a solution at some point that automatically maps positions between chained commands if that is even possible with how chains are working right now without refactoring the whole thing.

Is that okay with you @slapbox?

@Nantris
Copy link
Contributor Author

Nantris commented Oct 12, 2023

Thanks for the attention on this @bdbch. That sounds like a reasonable path. Some of these issues derive from inside of ProseMirror's internal functions though and will likely not be able to be fixed with that approach, and there will likely need to be a fourth draft which would involve patching ProseMirror itself. The hardest part of doing that will likely be coming up with a list of broken chain commands without the community having to find out about each one the hard way - so it does seem rather daunting even if the actual work required may not be extensive, just because it's so enigmatic.

Worse still, some of the bugs may derive from issues with ProseMirror's mapping calculations themselves, at which point this becomes quite arduous to fix.

I've nearly got our editor's first release finished so I'm hopeful that I personally won't find myself stuck in the mud again any time soon, but there will always be new features and bugs to fix so I may soon eat those words.

@bdbch
Copy link
Contributor

bdbch commented Oct 13, 2023

I already added going through our core and pro commands to find potentially broken commands to my task list, just need to find time to prioritize this in, hopefully next week.

The commands delivered by Tiptap & the documentation should make this work and understandable for users though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Open Source The issue or pull reuqest is related to the open source packages of Tiptap. Type: Bug The issue or pullrequest is related to a bug
Projects
Status: Triage open
Development

No branches or pull requests

3 participants