Opened 5 years ago

Closed 5 years ago

#8222 closed Bug (fixed)

IE: Selection bug when opening dialog on certain content

Reported by: damo Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.6.2
Component: Core : Selection Version: 3.6
Keywords: IBM Cc: satya

Description

There seems to be some odd behavior when opening a dialog in the editor without having focused inside the content area. The bug can manifest itself by deleting the HEAD element of the parent document. The loss of the parent document HEAD element does not occur usually on the nightly samples but does occur consistently in our application environment. My suspicion is that this is just due to some odd timeout order being fired as I have seen the issue occur in a nightly sample when stepping through the code with a debugger.

To reproduce:

  1. Open the AJAX sample
  2. Set the data like (the div is important):
    <div>
      <p>Test Paragraph</p>
      <p>Test Paragraph</p>
      <p>Test Paragraph</p>
    </div>
    
  3. Go back to WYSIWYG mode and press "Remove Editor"
  4. Press "Create Editor"
  5. Open the Find dialog from the toolbar, without focusing inside the editor body.

Expected Result: The dialog appears normally.

Actual Result: Although the dialog appears normally, an indication of the selection bug is visible. The Paragraph Format combo will indicate "Heading 1" as selected, this is in fact referring to the sample page heading! This erroneous value for Paragraph Format seems insignificant, but it is somehow related to our HEAD element being lost.

In addition, you can see that the 'data-cke-bookmark' span is appended to the H1 element at the top of the page.

Attachments (7)

Erroneous_H1_Selection.png (32.6 KB) - added by damo 5 years ago.
Missplaced_CKE_Bookmark.png (112.8 KB) - added by damo 5 years ago.
NightlySample_NoHead.png (76.5 KB) - added by damo 5 years ago.
The worst case affect
8222.html (2.0 KB) - added by damo 5 years ago.
Test case for 8222
8222.patch (2.0 KB) - added by garry.yao 5 years ago.
8222_2.patch (3.3 KB) - added by garry.yao 5 years ago.
8222_3.patch (3.3 KB) - added by garry.yao 5 years ago.

Download all attachments as: .zip

Change History (32)

Changed 5 years ago by damo

Changed 5 years ago by damo

Changed 5 years ago by damo

The worst case affect

comment:1 Changed 5 years ago by garry.yao

  • Status changed from new to pending

Looks serious but can u provide a reduced tc for the bug?

comment:2 Changed 5 years ago by damo

I've attached a TC to help reproduce some of the selection bugs. It is as reduced as I can get it for now. Once the selection bug is fixed I am pretty sure the disappearing HEAD issue will also get fixed.

Changed 5 years ago by damo

Test case for 8222

comment:3 Changed 5 years ago by damo

Fixed the instructions on the test case.

comment:5 in reply to: ↑ 4 Changed 5 years ago by satya

Replying to j.swiderski:

Based on provided TC, I can say that issue has been reproducible from CKEditor 3.0 in IE7-9

In IE7 JS error 'getParentEditor().getSelection()' is shown when clicking Find button for the first time.

IE8 behaves just like in the scenario described in the attached file. Extra spans are added to h1 and the second time you click Find button you get JS error:

Message: 'this.getParentEditor().getSelection()’ is nul lor not an object
Line: 857
URI: /3.6.1/ckeditor/_source/plugins/find/dialogs/find.js

IE9 adds only extra span tags to h1.

@j.swiderski we could only reproduce this issue starting in 3.6 & not in any previous versions. can you explain us how you have reproduced this version in 3.0?

comment:6 Changed 5 years ago by damo

Looks like the bug was introduced with rev 6698 to fix #6629.

comment:7 Changed 5 years ago by j.swiderski

May bad. Didn’t change the links when testing the sample.

Span tags have been added in IE8 and IE9 from CKEditor 3.6 rev [6904]

JS error has popped out in IE8 and IE7 from CKEditor 3.6.1 rev [6909]

Message: 'this.getParentEditor().getSelection()’ is nul lor not an object Line: 857 URI: /3.6.1/ckeditor/_source/plugins/find/dialogs/find.js

In IE7 JS error 'getParentEditor().getSelection()' is shown when clicking Find button for the first time.

IE8 behaves just like in the scenario described in the attached file. Extra spans are added to h1 and the second time you click Find button you get JS error.

IE9 adds only extra span tags to h1.

comment:8 Changed 5 years ago by j.swiderski

  • Version set to 3.6

comment:9 Changed 5 years ago by j.swiderski

  • Status changed from pending to confirmed

Changed 5 years ago by garry.yao

comment:10 Changed 5 years ago by garry.yao

  • Owner set to garry.yao
  • Status changed from confirmed to review

selection::lock() may leaks the selection to the outside of editor when editor doesn't actually have focus.

comment:11 Changed 5 years ago by damo

We've tried the patch out on our end and it seems to have solved the problem.

I am seeing the occasional "M is null" script error isolated to this call:

getSnapshotData:function(){return M.getFrameDocument().getBody().getHtml();}

We're still trying to determine if this is related to the fix or not. If not, we'll open a separate ticket.

comment:12 follow-up: Changed 5 years ago by satya

By applying patch to latest build, we come across issues with undo.

Open Ajax sample insert a link, click Undo

Expected Result: Link is removed.

Actual Result: Link not removed and we have to click undo twice to remove the link.

Same issues with images

comment:13 in reply to: ↑ 12 ; follow-up: Changed 5 years ago by fredck

Replying to satya:

By applying patch to latest build, we come across issues with undo.

I'm able to replicate an issue, but it happens also without the patch:

  1. Open the Ajax sample in Firefox.
  2. Without clicking inside the editor, click the Link button.
  3. Type any URL and confirm. The link is created.
  4. Click after the created link, to make the selection blink there. Note that the "auto-paragraph" feature is activated.
  5. Click the undo button once. ISSUE: The link is not removed, but the auto-paragraph is reverted.
  6. Click the undo button again. The link is finally removed.

I can't reproduce it with IE.

@satya, I would ask you to confirm two things:

  1. Does the above steps reflect the issue you were talking about?
  2. Can you confirm that the patch has nothing to do with it?

comment:14 in reply to: ↑ 13 Changed 5 years ago by satya

Replying to fredck:

Replying to satya:

By applying patch to latest build, we come across issues with undo.

I'm able to replicate an issue, but it happens also without the patch:

  1. Open the Ajax sample in Firefox.
  2. Without clicking inside the editor, click the Link button.
  3. Type any URL and confirm. The link is created.
  4. Click after the created link, to make the selection blink there. Note that the "auto-paragraph" feature is activated.
  5. Click the undo button once. ISSUE: The link is not removed, but the auto-paragraph is reverted.
  6. Click the undo button again. The link is finally removed.

I can't reproduce it with IE.

@satya, I would ask you to confirm two things:

  1. Does the above steps reflect the issue you were talking about?
  2. Can you confirm that the patch has nothing to do with it?

@fredck I don't need step 4 to reproduce my issue and i could confirm that i could reproduce my issue only after the patch applied. You could reproduce it by inserting images,flashes or IFrames. For links it's only happening in our internal builds, we will look in to it.

comment:15 Changed 5 years ago by fredck

@satya, I definitely can confirm the issues with image, iframe and flash, but it happens even before the patch. I have opened a new ticket for it: #8258.

I can't confirm any related issues with the patch on this ticket though. More info is needed.

comment:16 follow-up: Changed 5 years ago by damo

I used IE8 on Win 7, and followed the following steps:

  1. Extract trunk latest (rev. 7197)
  2. Open Ajax sample
  3. Without focusing in the editor, click the Image button, enter a url and click OK.
  4. Without focusing in the editor, click undo or use CTRL+Z.

Result: undo works as expected.

  1. Apply patch.
  2. Clear browser cache and reload same sample.
  3. Follow steps 3 & 4.

Result: undo must be clicked twice.

comment:17 in reply to: ↑ 16 Changed 5 years ago by fredck

  • Status changed from review to review_failed

Replying to damo:

Result: undo must be clicked twice.

I can confirm it as well, with IE9. Thanks for letting us know the browser information.

Changed 5 years ago by garry.yao

comment:18 Changed 5 years ago by garry.yao

  • Status changed from review_failed to review

New patch addressed the (duplicated) undo stack issue.

comment:19 Changed 5 years ago by garry.yao

  • Milestone set to CKEditor 3.6.2

I'm moving this one to milestone for causing other issues like #8275.

comment:20 Changed 5 years ago by fredck

  • Status changed from review to review_failed

Sorry but, while the change to the "updateSnapshot" behavior is a hack to workaround the issue, we have two problems: 1) it is counter-intuitive as one would expect the snapshot to be taken immediately and 2) it's a change to the "updateSnapshot" behavior.

Last edited 5 years ago by fredck (previous) (diff)

comment:21 Changed 5 years ago by garry.yao

  • Status changed from review_failed to review

comment:22 Changed 5 years ago by fredck

It was my mistake when reading the patches... in fact, my comment:20 precisely states that the proposed behavior change is the thing one would expect for it. So, I'm reconsidering patch 2.

Sorry for the mistake.

comment:23 Changed 5 years ago by fredck

  • Status changed from review to review_failed

comment:24 Changed 5 years ago by garry.yao

  • Status changed from review_failed to review

Changed 5 years ago by garry.yao

comment:25 Changed 5 years ago by fredck

  • Status changed from review to review_passed

comment:26 Changed 5 years ago by garry.yao

  • Resolution set to fixed
  • Status changed from review_passed to closed

Fixed with [7225].

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