Decoration Mapping Through Custom Replace Step Behavior

DanielFelixKim · April 17, 2020, 12:37am

Hello, I’m having trouble with mapping a decorationSet through a custom step. I created a step that extends ReplaceStep, where I replace a paragraph node with another node that has per word nodes. This is done by iterating through the textContent of the paragraph and then creating new nodes that wrap each word, and then doing the paragraph node replacement. I went this route because I found that the performance of creating a ReplaceAroundStep for each word, or a ReplaceAroundStep and then doing tr.split was worse for long paragraphs. Below is a visualization of the step.

<p>Some text here </p>

The <p> paragraph gets replaced with a new paragraph <np> that contains w>'s

<np><w>Some </w><w>text </w><w>here </w></np>

If I used the regular replace step and I had decorations in <p>, they would get dropped when they’re mapped through the regular replace step. Each decoration needs to be mapped by the number of node tokens added to each word. I provide this information in my custom step by overriding the getMap function to create the StepMap and passing it the ranges that describe the addition of the node tokens for each word. In this case, the step map is created with the following ranges: [1, 0, 1], [6, 0, 2], [11, 0, 2], [16, 0, 1]. I also created an inverse step that does the opposite, replacing <np> with <p>, which similarly has a set of ranges that remove the node tokens in the StepMap.

Problem: When I try to map a decoration set through a transaction that contains a StepMap with a range list like this one, the mapping is inconsistent and decorations are dropped/mapped wrong after going through several of these steps.

I can verify that my step ranges are correct because I’ve replaced the decorationSet.map in my apply of the Plugin with tr.mapping.map for each Decoration.from and Decoration.to in decorationSet.find() to create a new DecorationSet. The decorations always get mapped correctly with this patch fix, but this is less efficient since it maps every decoration position rather than using the tree structure of the set. I’m not sure if the problem is in how decorationSets behave with a long range list (>2) or if there is something else I need to add to my new replace step. Apologies for the long post!

marijn · April 17, 2020, 7:55am

None of the built-in steps create mappings like this, so there might be undiscovered bug there. Can you set up a minimal script that reproduces the issue?

DanielFelixKim · April 17, 2020, 9:20pm

Here’s a Glitch link with a minimal editor that implements the custom step. The setup has a paragraph in the first line and a “wordParagraph” with word nodes in the second line. Please see the schema and the enhanceTr and compressTr for more information about how the ranges are used in the custom step. You can see the rendering taking place if you inspect the elements in Chrome DevTools.

To reproduce the issue:

Type “text here” after “text” in the second line (the word paragraph)
The second line should now read “Some text text here here”
Click the Render button
Note that the last decoration is dropped

In contrast, refresh the page to reset the editor, and then repeat only step 1 & 2 and click on the Render with kludge map button. Note that the decoration is correctly mapped. I verified that the regular mapping drops the decoration by checking the decorationSet in the plugin before and after the mapping.

Interestingly, the regular mapping works fine without any typing first, but it starts incorrectly mapping only after a certain number of insertions at certain places. The reproduction steps above show the simplest case I found. However, the “kludge mapping” works fine with any edit and render. In the full implementation I’m working with, the editor will enhance a paragraph before any edits and compress the previous wordParagraph, so this rendering happens quite frequently.

marijn · April 20, 2020, 9:25pm

I’ve pushed a patch that should help with this.

But a word of caution—if your actual implementation also stores precreated step map ranges in your steps, that will break very much as soon as the steps are rebased (during collaborative editing or when using addToHistory: false transactions and then undoing), because you’ll still be publishing the outdated map locations from the mapped change. (Of course, the class in the glitch didn’t override map at all, so it’d degrade to a plain replace step on mapping, which would sidestep this issue.)

DanielFelixKim · April 21, 2020, 12:12am

Thank you for looking into it! I tried the patch, and while the behavior is better, there still seems to be some mapping issues after some edits. I’d be happy to get a set of reproducible steps, but the Glitch still has the old prosemirror-view package. I have it set up based off the examples on the website, where it’s using the require function you added that bundles the packages. Is there a way for me to update that, or do you need to update its sources? If there’s another way that you prefer to view a minimal script I could do that as well.

And thanks for the heads up! I think we’ll need to override the map function to then map the ranges on the Step correct?

marijn · April 21, 2020, 6:29am

Ah, thanks. The website rebuild process was broken. I’ve restarted it—the website should reflect the current version again now.

DanielFelixKim · April 22, 2020, 1:43am

I had replied thinking that the problem was actually fixed, but unfortunately I’m still seeing some decorations dropped during certain actions (hence the deleted post). I updated the Glitch to now show the new dropped decoration behavior. The following steps demonstrate how this happens.

Reproduction Steps

Click in the middle of the first word “Mollit”
Click the Insert Word in WordParagraph button
Click on the Render button
Note that the second decoration is now missing
If you click on Render again, you can see that the decoration wasn’t actually missing, but that it was mapped incorrectly and reappears at a different position

Comparison to Manual Position Mapping:

To compare against the kludge mapping which still works normally, refresh the page to reset the editor and repeat steps 1 and 2, then click on Render with kludge button. Note that the decorations are mapped correctly.

For more context regarding how the word insertion works, you can read its implementation on L96. Please let me know if you need any more information. This required quite some specific set up to be reproducible here since the demo is very barebones, so apologies for its clumsiness. In our implementation the incorrect mapping is still happening regularly.

marijn · May 1, 2020, 12:30pm

That’s not happening for me.

It would help a lot if you could…

Demonstrate the problem with a direct call to DecorationSet.map, without involving any editor views or plugins.
Simplify the inputs required to create the problem as much as possible, so that that boring task doesn’t fall on me.

peets · August 9, 2022, 7:35pm

Here’s a failing test case that illustrates this bug. I simplified it as much as I could, and it calls DecorationSet.map directly. I hope it helps find out what’s going on!

shinTSY · August 16, 2022, 7:17am

Hi, I still meet the problem in a table node when i used DecorationSet.map.

This is the DOM of my table node, and my widget decorations are rendered in paragraph.

<table>
  <tr>
      <td>
          <div>
            <div><p><myWidget/>some text</p></div>
          </div>
      </td>
    <td>
          <div>
            <div><p><myWidget/>some text</p></div>
          </div>
      </td>
  </tr>
<tr>
      <td>
          <div>
            <div><p><myWidget/>some text</p></div>
          </div>
      </td>
    <td>
          <div>
            <div><p><myWidget/>some text</p></div>
          </div>
      </td>
  </tr>
<tr>
      <td>
          <div>
            <div><p><myWidget/>some text</p></div>
          </div>
      </td>
    <td>
          <div>
            <div><p><myWidget/>some text</p></div>
          </div>
      </td>
  </tr>
<tr>
      <td>
          <div>
            <div><p><myWidget/>some text</p></div>
          </div>
      </td>
    <td>
          <div>
            <div><p><myWidget/>some text</p></div>
          </div>
      </td>
  </tr>
<tr>
     <td>
          <div>
            <div><p><myWidget/>some text</p></div>
          </div>
      </td>
   <td>
          <div>
            <div><p><myWidget/>some text</p></div>
          </div>
      </td>
  </tr>
<tr>
      <td>
          <div>
            <div><p><myWidget/>some text</p></div>
          </div>
      </td>
    <td>
          <div>
            <div><p><myWidget/>some text</p></div>
          </div>
      </td>
  </tr>
</table>

When I transform the second column’s every cells content to <div><div><p></p></div></div>, the position of the 5th row 1st col’s cell’s widget decoration is calculated wrong by DecorationSet.map. I tracked the code of Decoration.mapChildren, i think the problem is the line 559 and line 560 should use offset instead of oldOffset because the oldStart maybe is the pos after others steps.But the oldOffset is the pos before all the steps.

Is there a problem with comparing the positions of two different reference frames?

marijn · August 16, 2022, 2:00pm

Can you provide a reduced test case that demonstrates this issue? Using offset there definitely won’t work, but the issue may be that oldOffset should be adjusted for replaced ranges coming before the current range.

shinTSY · August 17, 2022, 3:52am

Hi Margin, thanks for your quickly reply.

Here is the link could reproduce this issure.

Link: ecstatic-ride-lg0gd8 - CodeSandbox

Reproduce steps:

Select second column like the image show.
Press ‘Delete’ key.
You will see some widgetDeco disappeared(mapped wrong).

Before:

After:

marijn · August 18, 2022, 1:57pm

That’s not really a reduced test case, and I’m not motivated to try and manually derive one from it.

The patch below addresses my current understanding of the issue, but I’m not sure it actually solves the thing you’re seeing.

shinTSY · August 19, 2022, 2:53am

Thanks @marjin, that’s work.

When I was debugging map Children, I found that the problem was that the scope of the current decoration was outside the current mapping, but it still went into the branch of else if (newStart >= offset && dSize), which caused it not to The offset position is shifted.

Because the depth of the table is relatively deep, it is easy to reproduce this problem.

Can I try adding testcase to test-decoration.ts in the future?(imitation of table structure with blockquote and paragraph nesting)

marijn · August 19, 2022, 8:21am

Of course.

shinTSY · August 19, 2022, 2:03pm

Here’s the prosemirror-test-builder case could reproduce this issue in previous version which directly use oldOffset.

function buildMap(doc: Node, ...decorations: (DecoSpec | ((tr: Transform) => Transform))[]) {
  let f = decorations.pop()!
  let oldSet = build(doc, ...decorations as DecoSpec[])
  let tr = (f as any)(new Transform(doc))
  // I added a variable called updatedDoc 
  return {set: oldSet.map(tr.mapping, tr.doc), oldSet, updatedDoc: tr.doc}
}

it("correctly offset a deep structure", () => {
      // a structure like a 5 rows 2 cols table
      let d = doc(
          blockquote(/** table **/
              blockquote(/** table-row **/
                  blockquote(/** table-cell **/
                      p("1111111111")
                  ),
                  blockquote(/** table-cell **/
                      p("222222")
                  )
              ),blockquote(/** table-row **/
                  blockquote(/** table-cell **/
                      p("11111111111")
                  ),
                  blockquote(/** table-cell **/
                      p("2222222222")
                  )
              ),blockquote(/** table-row **/
                  blockquote(/** table-cell **/
                      p("1111111111")
                  ),
                  blockquote(/** table-cell **/
                      p("222222222")
                  )
              ),blockquote(/** table-row **/
                  blockquote(/** table-cell **/
                      p("1111111111")
                  ),
                  blockquote(/** table-cell **/
                      p("2222222222")
                  )
              ),blockquote(/** table-row **/
                  blockquote(/** table-cell **/
                      p("1111111111")
                  ),
                  blockquote(/** table-cell **/
                      p("222222222")
                  )
              )
          ))
      // first column pos array
      const decoPosArray: number[] = []
     // second column range array
      const replaceRangeArray: {start: number, end: number}[] = []
     // all paragraph nodes info array
      const paragraphPosArray: {pos: number, node: Node}[] = []
     // collect all paragraph node info
      d.descendants((node, pos) => {
        if (node.type.name === 'paragraph') {
          paragraphPosArray.push({pos, node})
          return false
        }
      })
      paragraphPosArray.forEach(({pos, node}, i) => {
        if (i % 2 === 0) {
          decoPosArray.push(pos + 1)
        } else {
          replaceRangeArray.push({start: pos, end: pos + node.nodeSize})
        }
      })
      let {oldSet, set, updatedDoc} = buildMap(d, ...decoPosArray.map(pos => ({pos})), tr => {
        replaceRangeArray.forEach(range => {
          tr.replaceWith(tr.mapping.map(range.start), tr.mapping.map(range.end, -1), p())
        })
        return tr
      })
      let oldSetStr = ''
      oldSet.find().forEach(deco => {
        if (d.nodeAt(deco.from)?.type.name === 'text') {
          oldSetStr += d.nodeAt(deco.from).text + '-'
        }
      })
      let newSetStr = ''
      set.find().forEach(deco => {
        if (!updatedDoc.nodeAt(deco.from)) {
          newSetStr += 'x'
          return
        }
        if (updatedDoc.nodeAt(deco.from)?.type.name === 'text') {
          newSetStr += updatedDoc.nodeAt(deco.from).text + '-'
        }
      })
      // if `newSetStr` === `oldSetStr` means offset succesfully.
      ist(newSetStr, oldSetStr)
    })

Does this test-case follow code conventions or code style? or is there any problem that needs to be corrected?

marijn · August 19, 2022, 3:38pm

Thanks! Added to the test suite in the patch below.