fixing a bug in closeFragment

saranrapjs commented

2018-07-11 18:34:29 +02:00

(Migrated from github.com)

when fillBefore is called with toEnd=true, it won't return a fragment whose match isn't able to go to the end of the content expression. In this closeFragment code, we've seen a few cases where the code as it stands generates an exception. This should be a simple patch to fix this case, though unfortunately because it has come up in a production app as part of a paste, I don't have a great grasp on a test case that might be able to reproduce the conditions of the bug.

when [`fillBefore`](https://prosemirror.net/docs/ref/#model.ContentMatch.fillBefore) is called with `toEnd=true`, it won't return a fragment whose match isn't able to go to the end of the content expression. In this `closeFragment` code, we've seen a few cases where the code as it stands generates an exception. This should be a simple patch to fix this case, though unfortunately because it has come up in a production app as part of a paste, I don't have a great grasp on a test case that might be able to reproduce the conditions of the bug.

marijnh commented

2018-07-15 20:53:45 +02:00

(Migrated from github.com)

In principe, since the fragment passed to fillBefore comes from a node of type parent.type, it should always be possible to complete it. And even if we ignore that, not completing it just produces an invalid node, so that's no solution either.

As such, I really would be interested in the situation that produces this error, since the problem is likely elsewhere.

In principe, since the fragment passed to fillBefore comes from a node of type parent.type, it should always be possible to complete it. And even if we ignore that, not completing it just produces an invalid node, so that's no solution either. As such, I really would be interested in the situation that produces this error, since the problem is likely elsewhere.

saranrapjs commented

2018-07-24 21:18:08 +02:00

(Migrated from github.com)

I think I finally figured out the specific situation where this occurs: we were using the transformPasted hook, and inadvertently producing a schema-invalid Slice, which got inserted via doPaste.

I know that generally the philosophy of ProseMirror is that you should be responsible for checking schema validity as you go, but is this the right error to expect in a situation where the library is attempting to replace the current selection with a schema-invalid Slice? Should all pasted slices returning from transformPasted somehow be schema-validated? I'm happy if the answer is no, but in this case I think it might have been helpful if the error we'd been getting were specifically related to the fact that it stemmed from a schema issue.

I think I finally figured out the specific situation where this occurs: we were using the `transformPasted` hook, and inadvertently producing a schema-invalid Slice, which got inserted via `doPaste`. I know that generally the philosophy of ProseMirror is that you should be responsible for checking schema validity as you go, but is this the right error to expect in a situation where the library is attempting to replace the current selection with a schema-invalid Slice? Should all pasted slices returning from `transformPasted` somehow be schema-validated? I'm happy if the answer is no, but in this case I think it might have been helpful if the error we'd been getting were specifically related to the fact that it stemmed from a schema issue.

marijnh commented

2018-07-25 13:30:51 +02:00

(Migrated from github.com)

Should all pasted slices returning from transformPasted somehow be schema-validated?

The approach taken by the library is to assume that it only gets valid pieces of document. Re-checking is somewhat expensive (would have to walk the whole tree and run content expressions against node content), and doing that for everything that comes from user code, which would concern a lot of functions and methods, seems too wasteful.

As such, you can call check on nodes if you want to add assertions in your own code, but the library itself doesn't.

> Should all pasted slices returning from transformPasted somehow be schema-validated? The approach taken by the library is to assume that it only gets valid pieces of document. Re-checking is somewhat expensive (would have to walk the whole tree and run content expressions against node content), and doing that for _everything_ that comes from user code, which would concern a lot of functions and methods, seems too wasteful. As such, you can call [`check`](http://prosemirror.net/docs/ref/#model.Node.check) on nodes if you want to add assertions in your own code, but the library itself doesn't.

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Rows
Columns

fixing a bug in closeFragment #11

Pull request closed