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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance degradation with DOM mutation events #1768

Closed
albertxing opened this issue Oct 13, 2017 · 36 comments
Closed

Performance degradation with DOM mutation events #1768

albertxing opened this issue Oct 13, 2017 · 36 comments

Comments

@albertxing
Copy link

The usage of DOM mutation events in Quill (specifically, DOMNodeInserted from c4d8978) significantly degrades the performance of DOM node insertions and removals on the entire document. Additionally, the API is deprecated.

More information about the performance effects of DOM mutation events can be found on MDN: https://developer.mozilla.org/en-US/docs/Web/Guide/Events/Mutation_events
In particular:

Adding DOM mutation listeners to a document profoundly degrades the performance of further DOM modifications to that document (making them 1.5 - 7 times slower!). Moreover, removing the listeners does not reverse the damage.

The listener was originally added to address #1437 — I'm not sure why adding mutation listeners would fix that bug, but it would be nice to find another solution.

I have tried substituting the mutation event listener with a MutationObserver, but using MutationObserver doesn't fix the bug.

Steps for Reproduction

  1. Visit https://quilljs.com
  2. Run the following code in console:
document.body.addEventListener('click', () => {
    console.time();
    for (let i = 0; i < 1000; i++) {
        const div = document.createElement('div');
        document.body.appendChild(div);
    }
    console.timeEnd();
});
  1. Click on the page and observe timing information
  2. Run the same code on another site, to compare

Expected behavior:
DOM node insertions are fast

Actual behavior:
DOM node insertions are painfully slow

Platforms:
This seems to affect all major browsers and platforms

Version:
>= 1.2.5, when the listener was added

@jhchen
Copy link
Member

jhchen commented Oct 16, 2017

Running on both quilljs.com and google.com results in < 5ms being reported on Chrome. Maybe the platform matters?

@albertxing
Copy link
Author

albertxing commented Oct 16, 2017

🤔 I think the difference is a lot more obvious when running the profiler as well -- though that could very well be the overhead associated with profiling events.

However if I use finer timing tools (e.g. performance.mark) to time only the appendChild, I see a slowdown of 2 - 3x

@pmdartus
Copy link

@albertxing I made a simple benchmark to reproduce the performance issue:
https://codepen.io/pmdartus/pen/NXrdgO?editors=1000

Here are the numbers I gathered:

  • Chrome (65): 3x slowdown
  • Firefox (56): ~20% slowdown
  • IE11: No impact

@saw
Copy link

saw commented Jan 10, 2018

How about only adding the DOMNodeInserted listener when IME input is detected? Add the listener when compositionstart is fired? It won't fix the bug, but it would limit the damage

@kevinhend
Copy link

There must be another way to fix #1437. Is there any explanation why adding the empty listener even fixes this?
At the moment we are experiencing issues with codeblocks because of c4d8978.
Try pasting around 100 lines of code into the quill playground, select all of it and toggle the code style. This takes less than a second without the listener but 4 or 5 seconds with the listener enabled. The listener is called 11000 times for the 100 lines...

@caleb531
Copy link

caleb531 commented Nov 5, 2018

@kevinhend @jhchen I propose we add a new boolean flag to the Quill constructor called compatibilityMode. When set to false, it bypasses adding the DOMNodeInserted listener. To preserve backwards-compatibility, the property should default to true, and require the developer to explicitly set it to false to regain that extra performance.

In the Quill source code:

if (config.compatibilityMode) {
  this.domNode.addEventListener('DOMNodeInserted', function() {});
}

When using Quill:

var editor = new Quill(document.querySelector('.editor'), {
  compatibilityMode: false,
  // ...
});

What do you think? If you'd like, I can submit a PR with my proposed implementation.

@paolavness
Copy link

Hello,
Wondering if there are any updates on this? I'm experiencing significant delays with deltas containing 5,000+ words and formatting.

@kevinhend
Copy link

I can see that the listener was removed again in 41a60fb#diff-64791d4c2bd2baa277845f35fac28055

As far as I can tell, this is not yet reflected in 1.3.6 but only in develop (for 2.0?)

@rafa-suagu
Copy link

Some news here?

@echan00
Copy link

echan00 commented Jul 29, 2019

Any plans to do something about this?

@usb248
Copy link

usb248 commented Aug 30, 2019

Project dead ?

@mateo666
Copy link

Is there a fix for this at the moment or not ?
Using version 1.3.7 can see this warning

polyfills.js:11152 [Violation] Added synchronous DOM mutation listener to a 'DOMNodeInserted' event. Consider using MutationObserver to make the page more responsive.

it causes perf issues especially when you have many quill editors on a page.

@DanielChuDC
Copy link

Any update?

@ac205
Copy link

ac205 commented Apr 9, 2020

Yes wondering if theres any updates also?

@Timebutt
Copy link

Timebutt commented Apr 19, 2020

I think @caleb531's solution is the one to go for here 👍

For everybody else that is facing this and wants to fix this in the meanwhile: I deleted the line

this.domNode.addEventListener('dragstart', e => this.handleDragStart(e));

everywhere I could find it in the quill folder in the node_modules of my project and then used https://github.com/ds300/patch-package to create a patch of these changes. The only files you really need to touch are ./dist/quill.js and ./dist/quill.min.js, depending on what you use in your project.

EDIT: ironically, if you want to create a patch for quill using latest patch-package (6.2.2), you'll first have to patch patch-package to be able to create the quill patch using this fix: ds300/patch-package#166.

@vinzzzb
Copy link

vinzzzb commented Apr 30, 2020

Any update?

@maurice85
Copy link

maurice85 commented Sep 23, 2020

Has this problem been fixed in a new update? I'm running angular 9 and i have this problem too.

@GitHubish
Copy link

Same problem, the report is important on my application. And impossible to find any viable way to solve it.

@reblim
Copy link

reblim commented Oct 18, 2020

Any updates regarding this issue?

The proposal from @caleb531 sounds good #1768 (comment)

@o0x2a
Copy link

o0x2a commented Oct 26, 2020

It would great of we can address this, I get this as warning:

[Violation] Added synchronous DOM mutation listener to a 'DOMNodeInserted' event. Consider using MutationObserver to make the page more responsive.

Screenshot 2020-10-26 at 16 54 12

@quroom
Copy link

quroom commented Apr 1, 2021

const _config = { attributes: true, childList: true, subtree: true };
const _observer = new MutationObserver(function(){});
_observer.observe(_this.domNode, _config);

This problem is related to DOMNodeInserted event.
We need to change it to MutationObserver for performance.

@21pg
Copy link

21pg commented Jun 17, 2021

any updates here to fix this ???

@usersina
Copy link

usersina commented Nov 7, 2021

Same issue with Angular 12 when creating <quill-editor> in an *ngFor.

@abacaj
Copy link

abacaj commented Nov 12, 2021

This was removed in 2018 but never made it into version 1.3.7 that is being served on NPM.

Removal PR in 2018: 41a60fb#diff-5f9943a0ada23637672023452b5f7e4c7d2a715487446dd346e4ebcd7c9d9ccbL16

1.3.7 that is being served on NPM (2017): https://github.com/quilljs/quill/blob/1.3.7/blots/scroll.js#L25

You can use develop branch instead of NPM package: https://github.com/quilljs/quill/blob/develop/blots/scroll.ts#L42
Or you can manually patch the library locally and remove the offending line.

@nagavadla
Copy link

Is there any update on this?

@mhraihan
Copy link

is there any solution?

@luin
Copy link
Member

luin commented Jan 5, 2023

Sorry for the late response. The listener has already been removed on develop branch and we are working on a new release for it. Closing for now.

@luin luin closed this as completed Jan 5, 2023
@Danielvandervelden
Copy link

Any news on when a new release will be available with this fix @luin ?

@ZayRTun
Copy link

ZayRTun commented Aug 13, 2023

Hi, any updates on this issue?

@MelkorNemesis
Copy link

Hello, I just started using quill and encountered the same warning. Any idea when this might get fixed?

@luin
Copy link
Member

luin commented Aug 14, 2023

Most changes planned for version 2.0 have already been implemented in develop branch. I'm going to update the documentation/website for version 2.0. Don't have a timeline to share but I hope we can release a RC version in two months.

@JennineHamilton
Copy link

JennineHamilton commented Sep 6, 2023

I can confirm removing the following from the minified js works. V 1.3.7

r.domNode.addEventListener("DOMNodeInserted",function(){}),

@leeobrum
Copy link

hello,
Is there still a long way to go before the project is updated to version 2.0? I am waiting

@marcbrame
Copy link

Sorry for the late response. The listener has already been removed on develop branch and we are working on a new release for it. Closing for now.

Epic comeback after PR open for 6 years :)

@ranran2121
Copy link

Using react-quill v2.0.0 and got both
-the Violation] Listener added for a synchronous 'DOMNodeInserted' DOM Mutation Event
-[Violation] Added non-passive event listener to a scroll-blocking 'touchstart'

Any idea about what to do?

@mk-pmb
Copy link

mk-pmb commented Jun 23, 2024

I'm also encountering "Use of Mutation Events is deprecated. Use MutationObserver instead." in one of my projects with Quill 1.3.7. I tried to upgrade to 2.0.2 but that breaks the entire project. Could you maybe release a 1.3.8 with the suggested skip option added?

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

No branches or pull requests