IE: Performance issue when typing inside an element with many children

[Tobiasz Cudnik]

There is a performance issue when typing inside an element with many children. Reproduced in IE8.

Steps

1. Paste content from the attachment into the editor
2. Place cursor after last character, but not outside of the paragraph.
3. Start typing

Expected: Text is typed as usual.

Actual: There is a serious hangup and CPU consumption.

[Garry Yao]

I consider this issue quite important as it's interfered editing experience, it's a regression caused by [5452], the slowness shown in the below stack rooted by 'compareEndPoints' which apparently we cannot avoid.

onkeydown -> saveSelection -> getSelection -> getBoundaryInformation -> IERange::compareEndPoints

[Garry Yao]

Well we also don't want to revert [5452], so the patch doesn't really resolve the issue but at least optimize such case, making the editor usable.

If the proposed approach is rejected, this ticket should be no longer be kept in 3.4.2.

[Sa'ar Zac Elias]

It's now definitely better, but we should investigate it some more, we may find a way to fix the bug rather than working around it.

[Garry Yao]

Ok, I've managed to simplify things quite a lot, yes, I'm not joking, Fred would be the best reviewer here.

[Frederico Caldeira Knabben]

Well... I'm always strongly afraid when we see part of our code being cut that way. For some good reason it's there.

I'm not reviewing it right now. I'm just leaving a warning notice for now.

[Frederico Caldeira Knabben]

The assumptions from my previous comment unfortunately became true. It was as easy as use SVN blame to check why we have those lines of code there. The patch causes a regression of #5778. Just the reproduction steps need attention:

1. Load an editor with enough content to make the scrollbar available in the editor.
2. Because of another editor issue, do not use the scrollbar to scroll-down, but do that with the mouse wheel.
3. Right-click at the last paragraph.

After patch results: the contents will scroll to the top.

[Garry Yao]

Indeed, I'd checked #5778 but as it's saying "affecting all IE" that I missed IE8 (the only affected in effect), but the major purpose is to revert part of [3329], so other parts are innocent.

[Frederico Caldeira Knabben]

In the current code, it's clear that we wanted to disable selection saving while either the mouse button or the keyboard key is down, re-enabling it when they're up. There must surely be a reason for that, but unfortunately I can't recall it (my fault for missing comments).

I think we should not proceed with the patch if we're not able to have the TC that originated that code, otherwise we'll be certainly introducing a regression. I'm removing the review just because of this, so we can give more thinking to this issue.

Couldn't we instead try to have better performance on the selection saving code?

[Garry Yao]

Replying to fredck:

In the current code, it's clear that we wanted to disable selection saving while either the mouse button or the keyboard key is down, re-enabling it when they're up. There must surely be a reason for that, but unfortunately I can't recall it (my fault for missing comments).

With the patch, selection saving happens now on editor blur (and it should), so it makes no sense of keeping the original logic.

Couldn't we instead try to have better performance on the selection saving code?

Haven't find any possibility yet.

[Frederico Caldeira Knabben]

Replying to garry.yao:

With the patch, selection saving happens now on editor blur (and it should), so it makes no sense of keeping the original logic.

That was the original idea when I've implemented the selection saving logic as well, but, if I remember it well, there are some situations where the related blur events are not fired, making the original logic necessary.

As I've said, this is all to be confirmed.

[Jay]

Replying to garry.yao:

Hello,

My company is close to being production ready with our first release built on top of CKEditor. Is this correction still targeted for 3.5.1 or do we need to adjust our timeline?

Thanks, Jay

[Garry Yao]

Proposing binary search for getBoundaryInformation reduce the complexity to O(logⁿ).

[Garry Yao]

Note that 6438_4.patch's NOT conflicted with 6438_3.patch, it would be better if both patches could be accepted.

[Sa'ar Zac Elias]

It's an R+ for me, but I think we need a third reviewer here. Also, it might be for the best to wait with that ticket to the next milestone, to have more time to test it and use the editor with it before releasing to the public.

[Frederico Caldeira Knabben]

There are a few things that could be simplified in the code, but it's generally acceptable. It's a great solution for the problem.

It would be nice to have a few comments to describe what's going on in the code, so it'll be easier to understand the logic. Other than that, there are a few points that need coding style attention.

Better to have a lot of attention on IE during the tests because of this one.

[Garry Yao]

Thanks for zhanyi for the original idea,fixed with [6337].

[Alfonso Martínez de Lizarrondo]

Is this patch the same one that was mentioned in #6837 ?

[Garry Yao]

Replying to alfonsoml:

Is this patch the same one that was mentioned in #6837 ?

Yes, originate from ​this forum post.