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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 Fix inserting newlines into formatted text #3582

Merged
merged 1 commit into from Jun 15, 2023

Conversation

alecgibson
Copy link
Contributor

Consider a document with a single piece of bold text. As a Delta, this
would be represented as:

const doc = new Delta().insert('a', {bold: true})

Now consider the following Delta being applied:

const delta = new Delta().insert('\n1')

The above Delta will:

  • prepend the document with a newline
  • follow that newline with an unformatted string

Indeed, using doc.compose(delta) yields:

const result = new Delta()
  .insert('\n1')
  .insert('a', {bold: true})

However, the same result is not reached when using the Quill
editor.applyDelta(), which instead results in bold formatting being
incorrectly applied to our unformatted string:

const badResult = new Delta()
  .insert('\n')
  .insert('1a', {bold: true})

This happens because Quill does an insert of the whole insertion, but
doesn't handle the line splitting.

This change fixes this issue by splitting ops on newlines, and handling
them separately, which applies the correct formatting.

@luin luin self-requested a review December 14, 2022 08:30
@alecgibson alecgibson force-pushed the newline-formats branch 2 times, most recently from c43ea58 to dbec0e9 Compare June 2, 2023 10:38
@luin
Copy link
Member

luin commented Jun 5, 2023

Hey @alecgibson 馃憢

Good catch! I just started working on this. Sorry for the delay! Haven't looked into the reason but it appears that the code fails the following test case:

it('#3582', function () {
  const editor = this.initialize(
    Editor,
    '<iframe class="ql-video" frameborder="0" allowfullscreen="true" src="#" width="100" height="300"></iframe><p><br></p>',
  );
  editor.applyDelta(
    new Delta()
      .insert('\n')
      .insert({ video: '#' }, { width: '100', height: '300' }),
  );
  expect(this.container).toEqualHTML(
    '<p><br></p><iframe class="ql-video" frameborder="0" allowfullscreen="true" src="#" width="100" height="300"></iframe><iframe class="ql-video" frameborder="0" allowfullscreen="true" src="#" width="100" height="300"></iframe><p><br></p>',
  );
});```

Consider a document with a single piece of bold text. As a `Delta`, this
would be represented as:

```js
const doc = new Delta().insert('a', {bold: true})
```

Now consider the following `Delta` being applied:

```js
const delta = new Delta().insert('\n1')
```

The above `Delta` will:

  - prepend the document with a newline
  - follow that newline with an **unformatted** string

Indeed, using `doc.compose(delta)` yields:

```js
const result = new Delta()
  .insert('\n1')
  .insert('a', {bold: true})
```

However, the same result is not reached when using the Quill
`editor.applyDelta()`, which instead results in bold formatting being
incorrectly applied to our unformatted string:

```js
const badResult = new Delta()
  .insert('\n')
  .insert('1a', {bold: true})
```

This happens because Quill does an insert of the whole insertion, but
doesn't handle the line splitting.

This change fixes this issue by splitting ops on newlines, and handling
them separately, which applies the correct formatting.
@alecgibson
Copy link
Contributor Author

@luin I've pushed a fix. Is there a reason that test case isn't committed?

@luin
Copy link
Member

luin commented Jun 6, 2023

@alecgibson Thanks! Functionality-wise it LGTM. Will consider if there are any opportunities for refactoring.

Is there a reason that test case isn't committed?

It was caught by fuzzing tests, and I just haven't had a chance to commit it.

@luin luin merged commit 792052b into quilljs:develop Jun 15, 2023
8 checks passed
@alecgibson alecgibson deleted the newline-formats branch February 9, 2024 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants