Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#6438 closed Bug (fixed)

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

Reported by: Tobiasz Cudnik Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.5.1
Component: General Version: 3.0
Keywords: IE Cc: zhanyi1022@…

Description

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.

Attachments (5)

content.txt (7.3 KB) - added by Tobiasz Cudnik 10 years ago.
6438.patch (1.7 KB) - added by Garry Yao 10 years ago.
6438_2.patch (2.5 KB) - added by Garry Yao 10 years ago.
6438_3.patch (1.7 KB) - added by Garry Yao 10 years ago.
6438_4.patch (5.4 KB) - added by Garry Yao 10 years ago.

Download all attachments as: .zip

Change History (28)

Changed 10 years ago by Tobiasz Cudnik

Attachment: content.txt added

comment:1 Changed 10 years ago by Tobiasz Cudnik

Status: newconfirmed

comment:2 Changed 10 years ago by 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

Changed 10 years ago by Garry Yao

Attachment: 6438.patch added

comment:3 Changed 10 years ago by Garry Yao

Owner: set to Garry Yao
Status: confirmedreview

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.

comment:4 Changed 10 years ago by Sa'ar Zac Elias

Status: reviewreview_failed

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.

comment:5 Changed 10 years ago by Garry Yao

Milestone: CKEditor 3.4.2CKEditor 3.5

comment:6 Changed 10 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.5CKeditor 3.4.3

Changed 10 years ago by Garry Yao

Attachment: 6438_2.patch added

comment:7 Changed 10 years ago by Garry Yao

Status: review_failedreview

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

comment:8 Changed 10 years ago by 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.

comment:9 Changed 10 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

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.

comment:10 Changed 10 years ago by Garry Yao

Status: review_failedreview

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.

Changed 10 years ago by Garry Yao

Attachment: 6438_3.patch added

comment:11 Changed 10 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.4.3CKEditor 3.5.1
Status: reviewreview_failed
Version: 3.4.13.0

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?

comment:12 in reply to:  11 ; Changed 10 years ago by 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.

comment:13 in reply to:  12 Changed 10 years ago by 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.

comment:14 Changed 10 years ago by Garry Yao

Status: review_failedassigned

comment:15 in reply to:  14 Changed 10 years ago by 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

Changed 10 years ago by Garry Yao

Attachment: 6438_4.patch added

comment:16 Changed 10 years ago by Garry Yao

Status: assignedreview

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

comment:17 Changed 10 years ago by Garry Yao

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

comment:18 Changed 10 years ago by 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.

comment:19 Changed 10 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

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.

comment:20 Changed 10 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed

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

comment:21 Changed 10 years ago by Garry Yao

Cc: zhanyi1022@… added

comment:22 Changed 10 years ago by Alfonso Martínez de Lizarrondo

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

comment:23 in reply to:  22 Changed 10 years ago by Garry Yao

Replying to alfonsoml:

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

Yes, originate from this forum post.

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy