Rewrite of the replace-fitting algorithm. Please help test

1

I’ve you’ve ever glanced into prosemirror-transform/src/replace.js, you’ve probably noticed the horrors of complexity that lurked in the step-fitting algorithm. I was recently looking into a problem with the code that I just didn’t manage to solve in the old codebase.

So I rewrote it, and the result is a lot easier to work with. But because this is still a subtle, tricky algorithm, I am a bit nervous about just assuming my new code is correct because the tests pass. If you have a custom schema and a bunch of automated tests, I’d be very grateful if you could run them with prosemirror-transform version 1.2.5-beta1 and let me know if you find any problems.

If no problems are found, I’ll tag this as a regular release in two weeks or so.

3 Likes
2

Dear @marijn,

We tested 1.2.5-beta1 on nextjournal and it’s looking good, thank you a lot!

With that version pasting open slices ending with lists in our title node now works, but we’re still incurring in a paste issue with our custom schema, namely code nodes:

    ...
    :code {:attrs {:id {:default nil}
                   :collapsed {:default false}}
           :content "inline*"
           :group "block"
           :marks ""
           ;; setting `code` to false to avoid being handled as plain text for copy & paste, see
           ;; https://github.com/ProseMirror/prosemirror-view/blob/d5f05ac6b99a417ab7faa7d31622b4c9b0a6fa03/src/clipboard.js#L39
           :code false
           :defining true
           :isolating true
           ...

Copying slices which start with a title and end with a code node are still not pasteable into a title. I will try to reproduce in a minimal schema based on the default one, but maybe in the meanwhile this rings the bell to you already…

Cheers and thank you again,

Andrea & Nextjournal’s Team

3

Is this a regression or something that didn’t work before either? There are limits to what the fitting algorithm can do—inserting something before a title, if the schema only allows one title, might just be impossible. But if you think there’s a reasonable behavior that’s failing to happen, and you have an example, do open an issue.

4

didn’t work before either. The scenario I’m referring to is a slice with a Title + contents + a final code node pasted into an empty document into the empty title placeholder. I’ll try to reproduce in a basic example.

5

Hi, @marijn !

I don’t know if this can help you, but I added the beta version as a resolution for prosemirror-transform in atlaskit repo. This is the environment (we are not up to date):

prosemirror-view@1.9.12
prosemirror-model@1.7.1
prosemirror-state@1.2.2

So far so good,

Test Suites: 6 failed, 268 passed, 274 total
Tests:       40 failed, 68 skipped, 3541 passed, 3649 total

Only 40 fails, all of them related to attributes been set the default value instead of the original one. For example:

Object {
    "attrs": Object {
-   "layout": "wrap-right", // expected value
+  "layout": "center", // default value
    "width": null,
},

I hope this information is useful!

Best

6

Yes, very! Could you try again with this patch?

7

Sure! Sadly I couldn’t find any new beta version on npm :frowning: . I will try with yalc tomorrow.

8

Ah, good point, there’s an 1.2.5-beta2 on npm now.

9

Same environment as before with the new beta2.

Test Suites: 2 failed, 272 passed, 274 total
Tests:       24 failed, 68 skipped, 3557 passed, 3649 total

These are the error this time:

  1. It looks that our own gap cursor implementation is not working anymore :frowning: (6 from 24)
Expected specified selections to match in both values.

      403 |                 expect(
      404 |                   editorInstance.editorView.state,
    > 405 |                 ).toEqualDocumentAndSelection(
          |                   ^
      406 |                   doc(panel()(p('onetwo'), p('three ')), hr(), '{<|gap>}'),
      407 |                 );
      408 |               });
  1. There is a recurrent error happening is a bit hard to explain. It’s happening within our insert nodes tests, I will try to share enough information. (18 from 24).

All the failing test are caused when you try to add a node in an invalid parent, with the beta version is adding the node in the wrong position.

  • Some times add an empty paragraph at the end for no reason.
  • Other times inject the node in the wrong position.
// Test
         it('start of paragraph', () => {
            const { editorView } = editor(doc(panel()(p('{<>}onetwo'))));
            insertDummyMedia(editorView);

            expect(editorView.state.doc).toEqualDocument(
              doc(
                mediaSingle({ layout: 'center' })(
                  temporaryMediaWithDimensions(),
                ),
                panel()(p('onetwo')),
              ),
            );
          });

// Output
// Inject the media after the paragraph node instead of before the paragraph.
// And add an empty paragraph at the end
// Like:
             doc(
                panel()(p('onetwo')),
                mediaSingle({ layout: 'center' })(
                  temporaryMediaWithDimensions(),
                ),
                p('')
              )
  • Also, there is a case where the node get replaced at all
// Test
           it('empty paragraph', () => {
              const editorInstance = editor(doc(p('{<>}')));
              insertAction(editorInstance);
              expect(
                editorInstance.editorView.state,
              ).toEqualDocumentAndSelection(doc(hr(), '{<|gap>}'));
            });
// Output
doc(p())
10

Thanks for the feedback. Could you tell a bit more about your schema and the actual Transform methods these tests end up calling? I am having trouble interpreting these results.

11

I think you can get access by unpkg:

Nodes: https://unpkg.com/browse/@atlaskit/adf-schema@6.0.0/dist/esm/schema/nodes/ Marks: https://unpkg.com/browse/@atlaskit/adf-schema@6.0.0/dist/esm/schema/marks

12

Mm deep in the code we are calling one of this:

tr.replaceWith($before), $after), content)
tr.insert($pos, content)
pmSafeInsert -> prosemirror-util.safeInsert

Without debugging is hard to know which one is calling. If you need I can do some debug tomorrow.

13

I looked through the relevant code, and this is indeed pretty deep—too deep for me to be able to derive a matching test case from it (I tried a bunch of possibilities, but none show behavior similar to the failure you are showing. I couldn’t get the tests to run at all locally.)

14

:frowning: to be fair our code is to complex and we are a couple of minor versions behind. If you could not reproduce maybe it is related to some of the other minors.

I might try to debug it tomorrow, but I don’t think I will have time tbh :sweat_smile:.

Best,

15

I’ve published this as prosemirror-transform 1.2.5. If you run into a regression, please file an issue report with reproduction instructions.