Opened 7 years ago

Closed 7 years ago

#5530 closed Bug (fixed)

page break for printing can not be removed using Undo

Reported by: Satya Minnekanti Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.3
Component: General Version: 3.2
Keywords: IBM Confirmed Review+ Cc: Damian, joek

Description

To reproduce the defect:

  1. Open Ajax sample.
  1. Type some text and Click Insert Page break for Printing Icon.
  1. See that the page break for printing is inserted in the editor.
  1. Press Undo

Expected Result:

See that the Page break for Printing that we have inserted in Step 2 is removed

Actual Result:

Page break for printing is not removed no matter how many number of times we press Undo.

Tested against IE 6&7, FF3.6

Attachments (4)

5530.patch (2.6 KB) - added by Garry Yao 7 years ago.
5530_2.patch (2.8 KB) - added by Garry Yao 7 years ago.
5530_3.patch (2.8 KB) - added by Garry Yao 7 years ago.
5530_4.patch (2.8 KB) - added by Garry Yao 7 years ago.

Download all attachments as: .zip

Change History (21)

Changed 7 years ago by Garry Yao

Attachment: 5530.patch added

comment:1 Changed 7 years ago by Garry Yao

Keywords: Confirmed Review? added
Owner: set to Garry Yao
Status: newassigned

The undo system is trapped by "fix body" logics, the patch is targeting any undo system problems that caused by placing cursor directly under body after a command execution.

comment:2 Changed 7 years ago by Sa'ar Zac Elias

This patch fixed a bug regarding HRs and the Undo system, but introduced a new one in IE (tested with IE 8 and FF 3.6.3):

Steps to reproduce

  • Open a sample and set contentsLangDirection to rtl.
  • Place the caret somewhere in the document and insert two HRs.
  • Click Undo twice and then Redo twice.

On the second time, a JS error is thrown.

Should this be another ticket?

comment:3 Changed 7 years ago by Garry Yao

@Saare WFM in both IE and FF.

comment:4 in reply to:  3 Changed 7 years ago by Sa'ar Zac Elias

Replying to garry.yao:

@Saare WFM in both IE and FF.

The second step i presented was misleading, you should place the caret at the exact beginning of the document or more simply not to put the caret at all for this bug to occur.

comment:5 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review- added; Review? removed

The addition of editor.updateUndo is creating a dependency of the Undo plugin. It should use instead a new event like editor.fire( 'updateSnapshot' );

Changed 7 years ago by Garry Yao

Attachment: 5530_2.patch added

comment:6 Changed 7 years ago by Garry Yao

Keywords: Review? added; Review- removed

@alfonsoml Can you reproduce Saar's problem? It still WFM yet.

comment:7 Changed 7 years ago by Sa'ar Zac Elias

This bug is now occuring regardless the contentsLangDirection setting. I'm using the regualr replacebycode sample.

comment:8 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Yes, I can reproduce the bug in IE8. But I'm not sure if it's due to this patch, or the patch just made possible to discover it:

Without the patch, after inserting two HR (not page breaks), it's possible to remove only the last one. Now it's possible to remove both of them, and then when the Redo tries to insert the second one IE states that there's an error.

So I would like to be sure that this patch isn't really breaking anything, and then file this problem as a new ticket. If someone finds that applying a patch breaks something that previously worked then the patch must be corrected, if it's something that previously wasn't possible then I say to go ahead and apply the patch.

comment:9 Changed 7 years ago by Garry Yao

Thanks Saare, and Alfonso for the justification, now I confirm it an existing bug but not introduced by the patch here, for which I've opened #5614.

comment:10 Changed 7 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

I'm mostly of with the patch. The only problem I see is that in the event handle in line 133, you're calling "new Image()" and then calling the "update()", which creates again a "new Image()". Ideally only one image instance should be created in that case. Is it doable?

Suggestion: pass an *optional* parameter to update() for the image.

Changed 7 years ago by Garry Yao

Attachment: 5530_3.patch added

comment:11 Changed 7 years ago by Garry Yao

Keywords: Review? added; Review- removed

It's ok to make the param of undoManager::udpate mandatory due to its private scope.

comment:12 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review- added; Review? removed

That parameter isn't send at line 422

comment:13 Changed 7 years ago by Garry Yao

Keywords: Review? added; Review- removed

Ideally only one image instance should be created in that case. Is it doable?

Sorry I was mislead by Fred's suggestion, the update method should receive no arguments at all since it's by purpose that a fresh snapshot should be taken, which doesn't equal to the image at L135.

comment:14 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review- added; Review? removed

Did you forgot to upload a new patch?

According to the last comment it seems that you are describing an update function without parameters, and that it creates the snapshot inside of it, so that must be changed as well as the updateSnapshot listener.

Changed 7 years ago by Garry Yao

Attachment: 5530_4.patch added

comment:15 Changed 7 years ago by Garry Yao

Keywords: Review? added; Review- removed

Oops, actually 5530_4 is same with 5530_2.patch.

comment:16 Changed 7 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

comment:17 Changed 7 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [5461].

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