[whatwg] Feedback on UndoManager spec

[Aryeh Gregor]

On Thu, Oct 27, 2011 at 6:10 PM, Jonas Sicking <jonas at sicking.cc> wrote:
> Why is it harder to remember one function name and one attribute name,
> than to remember to function names?

Because the function names in this case must be in your code and they
must be correct, or they won't work.  That means that when you
copy-paste code from another site, the name is guaranteed to be
correct and descriptive.  The name of a boolean parameter is only in
the spec, and authors don't read specs, so they're unlikely to ever
know the name in the first place.  They won't have a chance to
remember it.

This would be solved if the parameter weren't boolean -- it could be a
string that's equal to either "apply" or "reapply", for instance.
Boolean parameters are evil in general, as I've said before.  I also
don't like the second parameter of transact().

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?

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.

> To be able to mix automatic transactions and manual transactions. Consider
> an application that lets you edit both text and drawing on canvas. Such an
> app may implement drawing action on canvas as a manual transaction while
> adding some extra editing commands as automatic transactions and manage them
> all in the same undo manager.

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?

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

To get the equivalent of an automatic transaction, just don't specify
unapply/reapply functions.  To get the equivalent of a manual
transaction, do specify them.  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.

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.

On Thu, Oct 27, 2011 at 7:10 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:
> On my second thought, I'd rather not duplicate information already present
> on the DOM level 3 spec. Authors, implementors, and developers have spent a
> lot of time solidifying DOM level 3, and I'd rather defer the definition of
> what it means to mutuate DOM there.

Perhaps we should define this in DOM4 instead of your spec, but I
don't think your current definition is good enough.  It requires every
implementer or author to go through DOM 3 and make inferences.  It's
easy to make mistakes, or there might be things that are ambiguous.
It's much better for a specification to state things exactly, so that
it's as easy as possible to check that it's correct.  Defining things
imprecisely or indirectly shifts work from spec writers to spec
readers, which is a bad thing.  You should have a clear definition
that aims to exactly match DOM 3, and if it doesn't, that should be
treated as a spec bug and fixed.

> It seems like just referring to your spec solves the problem because the
> editing host is null if a node is not editable, in which case, we'll default
> to the the lowest common ancestor of nodes.

Yes, that's what I was thinking.