[whatwg] Feedback on UndoManager spec

[Ryosuke Niwa]

On Fri, Oct 28, 2011 at 9:37 AM, Aryeh Gregor <ayg at aryeh.name> wrote:
>
> Would everyone be happy if there were no reapply() function, and
> apply() took a string argument that would be either "apply" or
> "reapply"?  If the functions are similar, it will be like the status
> quo except without a hard-to-remember boolean parameter and without
> the temptation to copy-paste code to separate functions.  If the
> functions are totally different, it's just an extra if/else.  Does
> that sound good to everyone?
>

I don't want string because then I'd have to do:
if (mode == 'reapply')
instead of
if (isReapply)
and the former is much more verbose.

On Thu, Oct 27, 2011 at 7:04 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:
> > Right. If authors decide to mess with DOM state of the highest node
> affected
> > by an automatic transaction, user agents can't do anything to fix that.
>
> It might cause compat issues if UAs don't handle such error cases
> interoperably, though.
>

Sure, but I don't think I can force UAs to implement unapply & reapply in
one way or another. There are too many cases to be considered (e.g. after
every DOM mutation, scripts inside mutation event listeners may have teared
down the DOM). I might say UAs may print a warning in console when DOM
states are inconsistent.

Ah, of course: you might want undo/redo to handle state other than
> just the DOM.  But why do we force the author to manage DOM state
> themselves just because they want to manage some non-DOM state?  Why
> don't we have the UA always manage the state of the DOM within the
> undoscope, but call any unapply/reapply functions in addition to that?
>

That won't work for a collaborative editor because undo/redo may need to
make DOM changes other than just restoring DOM states. For example, changes
made by other users can't be undone/redone by the local user, and those
changes may also have mutated the highest node affected by automatic
transactions. And authors still want to be able to sync the list of
undo/redo items with UAs (e.g. to enable "undo" item in "Edit" menu).

So how about this: get rid of the manual/automatic distinction
> entirely.  Just have one type of transaction, with the following
> behavior.  (I assume we have apply()/unapply()/reapply() with no
> parameters for any of them, for simplicity, but the same idea would
> work if we pass a parameter to apply().)
>
> * When the transaction is applied, call the apply() function, if any.
> Record all DOM changes within the undo scope.
> * When the transaction is unapplied, call the unapply() function, if
> any.  Then undo any DOM changes that unapply() caused that were within
> the undo scope.  Then undo the changes that were recorded from the
> original apply().
> * When the transaction is reapplied, call the reapply() function, if
> any.  Then undo any DOM changes that reapply() caused that were within
> the undo scope.  Then redo the changes that were recorded from the
> original apply().
>

For the said reason, this won't address all use cases for manual
transaction. But I've started to think that maybe calling unapply and
reapply after undo/redo is a good thing even for automatic
transactions. That'll let authors fix toolbar/widget status and maybe fix up
selection (to match Opera/WebKit on Mac).

The only limitation is that unapplying
> or reapplying a transaction can never affect the DOM within the undo
> scope except to undo or redo the original changes, but this seems like
> a good thing.  Authors will be able to make state changes in things
> like canvas or JS variables, and can change DOM state outside the undo
> scope however they want, but can't incorrectly change things in the
> undo scope.
>

Again, this won't work for a collaborative editor.

Is there any disadvantage to this compared to the current spec?  I
> really doubt authors will be able to get manual transactions right --
> it's just too easy for some complicated function to make some
> unexpected changes to the DOM for some reason.  The UA should silently
> fix these, not just let the DOM become inconsistent with the undo
> history.  Also, this means we don't have to leave behavior undefined
> for fixing up manual transactions that don't undo themselves properly.
>

But that's the whole point of manual transactions. It is there to let
authors make arbitrary changes in undo and redo.

- Ryosuke