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 Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.3
Component: General Version: 3.2
Keywords: IBM Confirmed Review+ Cc: damo, 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

comment:1 Changed 7 years ago by garry.yao

  • Keywords Confirmed Review? added
  • Owner set to garry.yao
  • Status changed from new to assigned

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 Saare

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 follow-up: 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 Saare

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 alfonsoml

  • 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

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 Saare

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

comment:8 Changed 7 years ago by alfonsoml

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 fredck

  • 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

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 alfonsoml

  • 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 alfonsoml

  • 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

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 fredck

  • Keywords Review+ added; Review? removed

comment:17 Changed 7 years ago by garry.yao

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

Fixed with [5461].

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