Opened 11 years ago

Closed 11 years ago

#3580 closed Bug (fixed)

IE: switching to Source in Ajax example raises an error

Reported by: Alfonso Martínez de Lizarrondo Owned by: Martin Kou
Priority: Must have (possibly next milestone) Milestone: CKEditor 3.0
Component: Core : Undo & Redo Version: SVN (CKEditor) - OLD
Keywords: IE Confirmed Review+ Cc:

Description

Load the Ajax sample in IE, switch to source mode and it raises an error.

The Image() method of the undo system is called twice and the second time editor.getSnapshot() returns false.

Attachments (3)

3580.patch (1.3 KB) - added by Martin Kou 11 years ago.
3580_2.patch (1.5 KB) - added by Martin Kou 11 years ago.
3580_3.patch (1.7 KB) - added by Martin Kou 11 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 11 years ago by Artur Formella

Keywords: Confirmed added
Milestone: CKEditor 3.0
Priority: NormalHigh

comment:2 Changed 11 years ago by Martin Kou

Owner: set to Martin Kou
Status: newassigned

comment:3 Changed 11 years ago by Martin Kou

Keywords: Review? added

Changed 11 years ago by Martin Kou

Attachment: 3580.patch added

comment:4 Changed 11 years ago by Garry Yao

Keywords: Review- added; Review? removed

A more simple fix would be found, as now we're saving snapshot for many commands which doesn't make sense for supporting undo, these are:

  1. Preview
  2. Copy
  3. Spell Check
  4. Find/Replace ( Each replacing is needing one, but not the dialog command )
  5. Select All
  6. Maximize
  7. Show Block
  8. About

We should give them a canUndo = false instead of introducing other specific flag.

comment:5 Changed 11 years ago by Martin Kou

Hmm... actually the undo systems seems broken in respect to editor mode changes. With the trunk, even if I've set canUndo to false - after switch to Source and back, I would have about 4 undo steps in the system. Some of them even have an extra empty paragraph before the first paragraph.

comment:6 in reply to:  4 Changed 11 years ago by Artur Formella

Saving snapshot before Source could make sense - this should be checked. Saving snapshot in other dialog aren't connected with the main issue - moved to #3621

Changed 11 years ago by Martin Kou

Attachment: 3580_2.patch added

comment:7 Changed 11 years ago by Martin Kou

Keywords: Review? added; Review- removed

It makes sense to save an undo snapshot when switching from Source to WYSIWYG mode - the document contents might have changed during Source mode. So if we just set canUndo = false the result would be wrong.

However, because editor.getSnapshot() might return null right after the Source command is executed, it doesn't make sense to save a snapshot right after Source command is invoked.

So, instead of saving snapshot at Source command invocation - we can save the snapshot whenever the editor mode has switched to WYSIWYG. This avoids the null error while still manages to save a snapshot if editor content is changed in Source mode.

comment:8 Changed 11 years ago by Artur Formella

Keywords: Review- added; Review? removed

Image is saved only when switching back to WYSIWYG so some undo information are lost.

I found one bug with this patch - steps to reproduce:

-Click "Create Editor"
-Write aaaaaa
-Go to Source mode
-Write bbbbbb
-Back to WYSIWYG
-Click Undo

Result: Editor is empty.
Expected result: aaaaaa in the editor.

To save image why not just add

if ( editor.mode == 'wysiwyg' )
	editor.fire( 'saveSnapshot' );

in source.exec() before setMode(...)

Changed 11 years ago by Martin Kou

Attachment: 3580_3.patch added

comment:9 Changed 11 years ago by Martin Kou

Keywords: Review? added; Review- removed

comment:10 Changed 11 years ago by Artur Formella

Keywords: Review+ added; Review? removed

comment:11 Changed 11 years ago by Martin Kou

Resolution: fixed
Status: assignedclosed

Fixed with [3599].

Click here for more info about our SVN system.

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