Wrong undo/redo states at start

[Vladimir Kuchuk]

Undo button enabled at start in inline mode without making any changes.

[Jakub Ś]

Problem reproducible from CKEditor 4.1 in all browsers.

[Piotrek Koszuliński]

Most likely this is the result of #10072.

[Olek Nowodziński]

This issue affects both framed and inline editors.

[Olek Nowodziński]

The problem was introduced by #10103 (​git:a91dfb05f4de6a) that fixed other issues. It's reproducible when switching source<->wysiwyg and when starting a new editor instance. Both framed and inline.

This is how updateCommands() and undoManager.onChange() interact with the editor in various situations before and after #10103:

Before #10103                           +  After #10103
                                        |
Running the new instance +--------------+-------------------------------------------+
                                        |
updateCommands() on mode                |  undoManager.onChange() on mode
  \-> undo enable                       |    \-> undoManager.undoable() == false
  \-> redo enable                       |    \-> undoManager.redoable() == false
undoManager.onChange() on mode          |  updateCommands() on instanceReady
  \-> undoManager.undoable() == false   |    \-> undo enable
  \-> undoManager.redoable() == false   |    \-> redo enable
                                        |
Switch to source view +-----------------+-------------------------------------------+
                                        |
undoManager.onChange()                  |  undoManager.onChange()
  \-> undoManager.undoable() == false   |    \-> undoManager.undoable() == false
  \-> undoManager.redoable() == false   |    \-> undoManager.redoable() == false
updateCommands() on mode                |  undoManager.onChange() on mode
  \-> undo disable                      |    \-> undoManager.undoable() == false
  \-> redo disable                      |    \-> undoManager.redoable() == false
undoManager.onChange() on mode          |  updateCommands() on mode
  \-> undoManager.undoable() == false   |    \-> undo disable
  \-> undoManager.redoable() == false   |    \-> redo disable
                                        |
Switch back to wysiwyg +----------------+-------------------------------------------+
                                        |
updateCommands() on mode                |  undoManager.onChange() on mode
  \-> undo enable                       |    \-> undoManager.undoable() == false
  \-> redo enable                       |    \-> undoManager.redoable() == false
undoManager.onChange() on mode          |  updateCommands() on mode
  \-> undoManager.undoable() == false   |    \-> undo enable
  \-> undoManager.redoable() == false   |    \-> redo enable
                                        +

The problem is that after #10103 updateCommands() is executed after undoManager.onChange() and overwrites whatever the first one has set. The order matters.

Setting ​startDisabled property in command definitions doesn't solve the problem because in some cases undo/redo should be enabled when switching source->wysiwyg. I.e, if there was some undo image available before going wysiwyg->source. startDisabled violates this case and makes undo/redo unavailable regardless of real state.

There's a basic solution for this issue that executes existing ​procedure on both mode (to solve wysiwyg<->source issue) and instanceReady (to solve initial state issue) with lower priority so undoManager.onChange() always has the final word.

This solution is implemented in ​t/10249 branch (with tests).

[Piotrek Koszuliński]

Great job, Olek :) I love the table :P

When I'm looking now at ​git:a91dfb05 I think that I haven't done it correctly. Before that commit all commands were already updated before #instanceReady. So #10103 changed two things:

• commands are updated later than they were (what caused this issue),
• commands are updated after #instanceReady is fired, so when the editor should be already ready.

Therefore I would focus on fixing ​git:a91dfb05, because it wasn't my intention to change the order of things. We should avoid that because undo buttons may not be the only broken by this change.

So:

• all commands added before first editor#mode event should be updated on first editor#mode,
• all commands added later should be updated immediately.

But if this won't be easily doable we can go with the patch proposed in t/10249.

[Olek Nowodziński]

1. Updated test branch.

Added undo plugin's dt for entering/leaving the read-only mode (4.1, master). I discovered that undo states are also broken when leaving read-only mode.

2. Updated ​t/10249 branch.

To correctly handle the "new" read-only leaving issue.

3. Introduced ​t/10249b branch.

With another concept of fixing command state issue. In ​t/10249b, updateCommands() discovers that execution is performed on instanceReady and updates the new commands only. The new ones are those that came after first mode event but before instanceReady for some reason.

The main advantage of this solution is that it reverts mode listener:

this.on( 'mode', updateCommands );

to the pre - ​git:a91dfb05f4de6a era, outside of instanceReady callback. It also avoids double command state initialization thanks to command names cache. If accepted we can eventually rewrite this solution to have something like updateCommands( event, updateNewOnly ).

[Piotrek Koszuliński]

I pushed new tests to t/10249 which prove that:

• code preventing from updating commands two times during initialization doesn't work on t/10249b;
• on t/10249 commands' states are refreshed too late.

I also pushed a commit ​git:48001d8 which fixes both issues by pretty surprising simplification :D

[Piotrek Koszuliński]

The t/10249b fixes also #10268.

[Piotrek Koszuliński]

Fixed on ​git:1c7efa1 (dev) and 383f89b (tests).