Bug 47428 – Redo in ReplaceNodeWithSpanCommand is broken

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

Created attachment 70277 [details]
demo

Redo in ReplaceNodeWithSpanCommand creates new element and as a result, subsequent redos that reply on the node added by ReplaceNodeWithSpanCommand fails.

[Ryosuke Niwa]

My previous analysis was (sort of) wrong.  redo fails because ReplaceNodeWithSpanCommand inherits from CompositeEditCommand instead of SimpleEditCommand.

[Darin Adler]

We may never get around to this, but long ago I wanted to change SimpleEditCommand and CompositeEditCommand to be completely separate concepts. The SimpleEditCommand would be more of an UndoStep object, with the sole responsibility of being an undoable operation. The CompositeEditCommand objects would be created only during the original editing operation and would be responsible for the more complex process of deciding what to do the first time around, but would then be deallocated and would no longer be around doing the undo/redo process.

[Ryosuke Niwa]

(In reply to comment #2)
> We may never get around to this, but long ago I wanted to change SimpleEditCommand and CompositeEditCommand to be completely separate concepts. The SimpleEditCommand would be more of an UndoStep object, with the sole responsibility of being an undoable operation. The CompositeEditCommand objects would be created only during the original editing operation and would be responsible for the more complex process of deciding what to do the first time around, but would then be deallocated and would no longer be around doing the undo/redo process.

I think the current implementation works almost like that.  Except CompositeEditCommand's doUnapply and doReapply of CompositeEditCommand still exist to as a stub / driver for SimpleEditCommands that compose the composite command. If we completely got rid of CompositeEditCommand, how do we determine which SimpleEditCommands correspond to which undo position?

e.g.
1. user bolden text
2. user italicize text
3. user undo italicize

In this case, each ApplyStyleCommand of step 1 and 2 will generate a bunch of SimpleEditCommands.  But we need some way to group commands together so that we can undo only SimpleEditCommands added by step 2 in step 3.  If we didn't have CompositeEditCommand at undo/redo, how do we determine that?

[Ryosuke Niwa]

Created attachment 70312 [details]
fixes the bug

[Darin Adler]

(In reply to comment #3)
> In this case, each ApplyStyleCommand of step 1 and 2 will generate a bunch of SimpleEditCommands.  But we need some way to group commands together so that we can undo only SimpleEditCommands added by step 2 in step 3.  If we didn't have CompositeEditCommand at undo/redo, how do we determine that?

The chain of undo steps would have markers to say where an end user command started. This could be another type of UndoStep object. This marker could also contain the user-visible name of the command.

[Darin Adler]

Comment on attachment 70312 [details]
fixes the bug

Is there any way to prevent us from making this mistake in the future? Are there any other cases of this mistake?

[Ryosuke Niwa]

(In reply to comment #5)
> The chain of undo steps would have markers to say where an end user command started. This could be another type of UndoStep object. This marker could also contain the user-visible name of the command.

But that seems to make it hard to support Undo Manager as it's proposed today on whatwg.  See
http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#undo seem

(In reply to comment #6)
> (From update of attachment 70312 [details])
> Is there any way to prevent us from making this mistake in the future? Are there any other cases of this mistake?

I remember the patch posted to the bug 35281 had a similar mistake.  We can probably prevent the majority of similar mistakes if we can prevent sub classes of CompositeEditCommand to override doUnapply and doReapply.  Can we make CompositeEditCommand's doUnapply and doReapply non-virtual?  Will that prevent sub classes to override them?

[Darin Adler]

(In reply to comment #7)
> (In reply to comment #5)
> > The chain of undo steps would have markers to say where an end user command started. This could be another type of UndoStep object. This marker could also contain the user-visible name of the command.
> 
> But that seems to make it hard to support Undo Manager as it's proposed today on whatwg.  See
> http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#undo seem

I think it would be find to have the UndoStep objects grouped into UndoableOperation objects. I just don’t think those should be the same CompositeEditCommand objects we have today. CompositeEditCommand objects have two separate jobs: 1) Doing the edit the first time. 2) Managing the undo process. And the data needed to do the edit the first time has no reason to hang around. I think the code would be easier to read if there was a separation of concerns.

[Ryosuke Niwa]

(In reply to comment #8)
> I think it would be find to have the UndoStep objects grouped into UndoableOperation objects. I just don’t think those should be the same CompositeEditCommand objects we have today. CompositeEditCommand objects have two separate jobs: 1) Doing the edit the first time. 2) Managing the undo process. And the data needed to do the edit the first time has no reason to hang around. I think the code would be easier to read if there was a separation of concerns.

I think that makes a lot of sense.  If we separate CompositeEditCommand into two classes as you suggest, then CompositeEditCommand doesn't have to derive from EditCommand any more, and we might be able to avoid making the same kind of mistakes in the future since there will be a clear distinction between CompositeEditCommand and SimpleEditCommand.  In fact, CompositeEditCommand doesn't even need to be a class in that case (although it's probably best to keep things together as a class).

[Ryosuke Niwa]

I did spent some time looking into how to separate CompositeEditCommand and typing command seems to be the key.  The way we implement right now reuses the last typing command if it's still "open," and we can't do that we if just replaced it with simple UndoStep.  One way to resolve this problem is by making TypingCommand inherit from both UndoStep and CompositeEditCommand but that's a bit ugly, and it seems to defeat the whole point of separating CompositeEditCommand into two classes.

On the other hand, typing command is a bit of mess right now, so maybe we can refactor it in such a way that we don't have to reuse it.  One way to accomplish is to create some subclass of UndoStep for typing command that stores some extra information needed to resume typing, and add a new constructor for TypingCommand that takes this special UndoStep object from which it can resume the typing.

[Ryosuke Niwa]

Committed r69453: <http://trac.webkit.org/changeset/69453>

[Ryosuke Niwa]

(In reply to comment #11)
> Committed r69453: <http://trac.webkit.org/changeset/69453>

Reverted unintentional changes to platform/*/editing in http://trac.webkit.org/changeset/69456.