[whatwg] Feedback on UndoManager spec

[Aryeh Gregor]

On Fri, Oct 28, 2011 at 12:59 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:
> 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.

It's a few extra characters.  I really think the increase in clarity
is worth it.  Boolean parameters are much more confusing, and should
be avoided wherever possible.

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

Okay, good.  That's another use-case.  I think it would be helpful if
you added these use-cases to the spec so that it's easier to see why
it's designed the way it is.

Now I suggest that automatic transactions behave as I described, also
running reapply/unapply but undoing any DOM changes they do.  Thus for
things like tracking canvas state, the author can still use them
safely.  For collaborative editing, I suggest that we avoid the
problems due to mixing manual and automatic transactions by making it
per-UndoManager instead of per-transaction.  So instead of an
isAutomatic property for transactions, have methods on UndoManager,
maybe enableManualTransactions() and disableManualTransactions().
Then when one of these is called, clear the transaction history.  That
way, we don't have to worry about undefined behavior when manual and
automatic transactions are mixed, and we still satisfy all use-cases.
Collaborative editors won't want to use automatic transactions, right?

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

I think this is a good idea.

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

Right, but we need to be careful not to give authors more power than
necessary.  We should only be exposing functionality justified by the
use-cases.  Otherwise we make things more complicated for everyone.
If we can't think of any good reason authors would want to mix manual
and automatic transactions, for instance, we shouldn't allow it if we
can avoid complication that way.  If we later find that authors really
do want to mix them, we can add new features at that point, but we
shouldn't before then.


Something that just occurred to me: the specification needs to handle
textareas and inputs.  Their values aren't part of the DOM, but they
need to be tracked in the undo history.  Their values should be part
of DOM state for our purposes, even though changing them doesn't fire
mutation events.