Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#10249 closed Bug (fixed)

Wrong undo/redo states at start

Reported by: Vladimir Kuchuk Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.1.1
Component: Core : Undo & Redo Version: 4.1
Keywords: Cc:

Description

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

Change History (13)

comment:1 Changed 6 years ago by Vladimir Kuchuk

Summary: UndoUndo/Redo buttons

comment:2 Changed 6 years ago by Jakub Ś

Status: newconfirmed

Problem reproducible from CKEditor 4.1 in all browsers.

comment:3 Changed 6 years ago by Piotrek Koszuliński

Most likely this is the result of #10072.

comment:4 Changed 6 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.1.1

comment:5 Changed 6 years ago by Olek Nowodziński

Owner: set to Olek Nowodziński
Status: confirmedassigned
Version 0, edited 6 years ago by Olek Nowodziński (next)

comment:6 Changed 6 years ago by Olek Nowodziński

Status: assignedreview

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

comment:7 Changed 6 years ago by 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.

comment:8 Changed 6 years ago by 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.

  1. Updated t/10249 branch.

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

  1. 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 ).

comment:9 Changed 6 years ago by Piotrek Koszuliński

Owner: changed from Olek Nowodziński to 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

comment:10 Changed 6 years ago by Piotrek Koszuliński

The t/10249b fixes also #10268.

comment:11 Changed 6 years ago by Olek Nowodziński

Status: reviewreview_passed

comment:12 Changed 6 years ago by Piotrek Koszuliński

Resolution: fixed
Status: review_passedclosed

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

comment:13 Changed 6 years ago by Olek Nowodziński

Summary: Undo/Redo buttonsWrong undo/redo states at start
Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy