Skip to content

cleanupFormattingGap handles non-primitive attribute values incorrectly #776

@12joan

Description

@12joan

Describe the bug
The cleanupFormattingGap function assumes that attribute values are primitives that can be meaningfully compared using ===, which can sometimes result in Yjs failing to delete ContentFormat items whose values are objects. In Yjs 13 (I was unable to reproduce this in Yjs 14), these ContentFormat items are sometimes* subsequently deleted by other code, but as part of a different transaction.

*This can happen when some code reads the event.changes getter property of some event object, which seems to have the side-effect of causing Yjs's internal data structure to be normalised.

This means that if an application is using an UndoManager scoped to a single transaction origin, performing an undo can succeed in restoring deleted text but fail to restore the ContentFormat item that was deleted in a separate transaction. This often results in text formatting being incorrectly extended to the end of the type.

Here's a demo of this happening in our application:

image.mp4

To Reproduce (Yjs 13.6.30)

import * as Y from 'yjs' // v13.6.30

const UNDO_ORIGIN = Symbol()

const yDoc = new Y.Doc()
const paragraph = yDoc.getText()

const undoManager = new Y.UndoManager(paragraph, {
  trackedOrigins: new Set([UNDO_ORIGIN]),
})

// It's important to apply the format in two steps so that the two
// { bold: { active: false } } objects are different
paragraph.insert(0, 'one two three', { bold: { active: false } })
paragraph.format(4, 9, { bold: { active: true } })
paragraph.format(7, 6, { bold: { active: false } })

paragraph.observe(event => {
  event.changes // Cause the internal data structure to be normalised
})

// The ContentFormat for { bold: { active: false } } is erroneously deleted as
// part of a separate transaction with an origin of null
yDoc.transact(() => paragraph.delete(2, 9), UNDO_ORIGIN)

// The UndoManager successfully undoes everything except the deletion of the
// ContentFormat for { bold: { active: false } }
undoManager.undo()

console.dir(paragraph.toDelta(), { depth: null })

Expected:

[
  { insert: 'one ', attributes: { bold: { active: false } } },
  { insert: 'two three', attributes: { bold: { active: true } } }
  { insert: ' three', attributes: { bold: { active: false } } }
]

Actual:

[
  { insert: 'one ', attributes: { bold: { active: false } } },
  { insert: 'two three', attributes: { bold: { active: true } } }
]

To reproduce (latest commit from main)

import * as Y from 'yjs'

const yDoc = new Y.Doc({ gc: false })
const paragraph = yDoc.get()

paragraph.insert(0, 'one two three', { bold: { active: false } })
paragraph.format(4, 9, { bold: { active: true } })
paragraph.format(7, 6, { bold: { active: false } })
paragraph.delete(2, 9)

for (let item = paragraph._start; item; item = item.right) {
  const description = item.content instanceof Y.ContentString
    ? JSON.stringify(item.content.str)
    : `${item.content.key} = ${JSON.stringify(item.content.value)}`
  const deletedLabel = item.deleted ? ' (deleted)' : ''
  console.log(`${item.id.clock}: ${description}${deletedLabel}`)
}

Expected:

0: bold = {"active":false}
1: "on"
3: "e " (deleted)
15: bold = {"active":true} (deleted)
5: "two" (deleted)
17: bold = {"active":false} (deleted)
8: " thr" (deleted)
12: "ee"
16: bold = {"active":false} (deleted)
14: bold = null

Actual:

0: bold = {"active":false}
1: "on"
3: "e " (deleted)
15: bold = {"active":true} (deleted)
5: "two" (deleted)
17: bold = {"active":false}
8: " thr" (deleted)
12: "ee"
16: bold = {"active":false} (deleted)
14: bold = null

Expected behavior
cleanupFormattingGap should mark the ContentFormat for { bold: { active: false } } as deleted even though the { active: false } object is not referentially equal to the { active: false } object of the preceding text.

Environment Information

  • The issue occurs in all tested browsers and recent Node.js versions.
  • Yjs version: See above.

Workaround
Users impacted by this can use their package manager or patch-package to resolve this issue by patching Yjs.

In the function cleanupFormattingGap, apply the following change, which uses the same equalAttrs comparison function that is used elsewhere in Yjs's code for comparing attribute values, which handles object attribute values correctly.

-        if (endFormats.get(key) !== content || startAttrValue === value) {
+        if (endFormats.get(key) !== content || equalAttrs(startAttrValue, value)) {
  • I'm a sponsor 💖
  • This issue is a blocker for my project.

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions