Opened 13 years ago
Closed 13 years ago
#8222 closed Bug (fixed)
IE: Selection bug when opening dialog on certain content
Reported by: | Damian | Owned by: | Garry Yao |
---|---|---|---|
Priority: | Normal | Milestone: | CKEditor 3.6.2 |
Component: | Core : Selection | Version: | 3.6 |
Keywords: | IBM | Cc: | Satya Minnekanti |
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:
- Open the AJAX sample
- Set the data like (the div is important):
<div> <p>Test Paragraph</p> <p>Test Paragraph</p> <p>Test Paragraph</p> </div>
- Go back to WYSIWYG mode and press "Remove Editor"
- Press "Create Editor"
- 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)
Change History (32)
Changed 13 years ago by
Attachment: | Erroneous_H1_Selection.png added |
---|
Changed 13 years ago by
Attachment: | Missplaced_CKE_Bookmark.png added |
---|
Changed 13 years ago by
Attachment: | NightlySample_NoHead.png added |
---|
comment:1 Changed 13 years ago by
Status: | new → pending |
---|
Looks serious but can u provide a reduced tc for the bug?
comment:2 Changed 13 years ago by
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.
comment:5 Changed 13 years ago by
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:7 Changed 13 years ago by
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 13 years ago by
Version: | → 3.6 |
---|
comment:9 Changed 13 years ago by
Status: | pending → confirmed |
---|
Changed 13 years ago by
Attachment: | 8222.patch added |
---|
comment:10 Changed 13 years ago by
Owner: | set to Garry Yao |
---|---|
Status: | confirmed → review |
selection::lock() may leaks the selection to the outside of editor when editor doesn't actually have focus.
comment:11 Changed 13 years ago by
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: 13 Changed 13 years ago by
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 follow-up: 14 Changed 13 years ago by
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:
- Open the Ajax sample in Firefox.
- Without clicking inside the editor, click the Link button.
- Type any URL and confirm. The link is created.
- Click after the created link, to make the selection blink there. Note that the "auto-paragraph" feature is activated.
- Click the undo button once. ISSUE: The link is not removed, but the auto-paragraph is reverted.
- 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:
- Does the above steps reflect the issue you were talking about?
- Can you confirm that the patch has nothing to do with it?
comment:14 Changed 13 years ago by
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:
- Open the Ajax sample in Firefox.
- Without clicking inside the editor, click the Link button.
- Type any URL and confirm. The link is created.
- Click after the created link, to make the selection blink there. Note that the "auto-paragraph" feature is activated.
- Click the undo button once. ISSUE: The link is not removed, but the auto-paragraph is reverted.
- 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:
- Does the above steps reflect the issue you were talking about?
- 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 13 years ago by
@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: 17 Changed 13 years ago by
I used IE8 on Win 7, and followed the following steps:
- Extract trunk latest (rev. 7197)
- Open Ajax sample
- Without focusing in the editor, click the Image button, enter a url and click OK.
- Without focusing in the editor, click undo or use CTRL+Z.
Result: undo works as expected.
- Apply patch.
- Clear browser cache and reload same sample.
- Follow steps 3 & 4.
Result: undo must be clicked twice.
comment:17 Changed 13 years ago by
Status: | review → 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 13 years ago by
Attachment: | 8222_2.patch added |
---|
comment:18 Changed 13 years ago by
Status: | review_failed → review |
---|
New patch addressed the (duplicated) undo stack issue.
comment:19 Changed 13 years ago by
Milestone: | → CKEditor 3.6.2 |
---|
I'm moving this one to milestone for causing other issues like #8275.
comment:20 Changed 13 years ago by
Status: | review → 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.
comment:21 Changed 13 years ago by
Status: | review_failed → review |
---|
comment:22 Changed 13 years ago by
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 13 years ago by
Status: | review → review_failed |
---|
I have failures on FF and IE after patch 2:
http://ckeditor.t/dt/plugins/list/list.html
http://ckeditor.t/tt/4196/1.html
http://ckeditor.t/tt/4385/1.html
Changed 13 years ago by
Attachment: | 8222_3.patch added |
---|
comment:25 Changed 13 years ago by
Status: | review → review_passed |
---|
comment:26 Changed 13 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
Fixed with [7225].
The worst case affect