page break for printing can not be removed using Undo

[Satya Minnekanti]

To reproduce the defect:

1. Open Ajax sample.

2. Type some text and Click Insert Page break for Printing Icon.

3. See that the page break for printing is inserted in the editor.

4. 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

[Garry Yao]

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.

[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?

[Garry Yao]

@Saare WFM in both IE and FF.

[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.

[Alfonso Martínez de Lizarrondo]

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

[Garry Yao]

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

[Sa'ar Zac Elias]

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

[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.

[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.

[Frederico Caldeira Knabben]

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.

[Garry Yao]

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

[Alfonso Martínez de Lizarrondo]

That parameter isn't send at line 422

[Garry Yao]

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.

[Alfonso Martínez de Lizarrondo]

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.

[Garry Yao]

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

[Garry Yao]

Fixed with [5461].