[whatwg] Feedback on UndoManager spec

[Aryeh Gregor]

Things I noticed while reading through it, leaving aside editorial
nitpicks that wouldn't improve clarity:

1) I was confused at first by the fact that undo goes backward in the
history, and redo goes forward.  I would have expected that new
transactions are added to the end of the list, not the beginning.
This way the list goes forward in time instead of backward.  Is there
some specific reason for why it's the other way around?  E.g., does
this match other platforms' undo APIs?  If you keep it this way,
should you change the section title "Undo: moving back in the undo
transaction history" to "Undo: moving forward in the undo transaction
history", and similarly for "Redo: moving forward in the undo
transaction history"?

2) What happens if you have an Element with the undoscope attribute
that doesn't descend from a Document?  Does undo management make any
sense in that case, or should the undoscope attribute have no effect
for detached elements?

3) It looks like there's no mention of the UA clearing old undo
entries.  If the UA is expected to remove old undo entries when the
undo history gets too long, this should be mentioned somewhere.

4) In the transact() method, step 2 is "Clear all transactions or
transaction groups between before the current undo position."  Should
"between" be removed?

5) You use step numbers in some places, like "go to step 8".  This is
risky, because if you add or remove a step from the algorithm you have
to track down all the references and change them manually, and if you
miss any your spec is incorrect.  Ian avoids this by using named
labels, and saying "go to the step in this algorithm labeled 'foo'".
I avoid it by not using gotos at all and using if/while/etc. instead,
although this results in a lot of sublist indentation sometimes.

6) In the definition of redo(), you say "(position is incremented by
1)".  I'm pretty sure you mean "decremented".

7) Where you say "The item(n) method must return the nth transaction's
associated data", "associated data" is a link to
#transaction-associated-data, but there's nothing with that id, and
the term "associated data" doesn't occur elsewhere.  Maybe you should
use a tool like anolis to write your spec, so that it will make sure
links like this are correct.  If you maintain these links manually,
probably there are other similar errors.

8) I'm confused by what role transaction groups play in UndoManager.
The prose says that an UndoManager has a list of transactions and
transaction groups, but item() only returns transactions.  What
happens if the nth item is a transaction group rather than a
transaction?  Does item(n) return an array instead of a Transaction?
Similarly, the non-normative description of undo() says "the
transaction or all transactions in the transaction group", but the
normative definition says just "the transaction" and doesn't mention
transaction groups.

Would it make more sense to say that UndoManager has a list of
transaction groups, and that some of the groups might just contain one
transaction, instead of having the list be a mix of transactions and
transaction groups?

9) In section 3.1 Mutations of DOM, you define "DOM changes" and "DOM
State" by reference to DOM 3.  It would be better if you gave explicit
lists, for clarity.  I think the only things that qualify as DOM
changes to a node are

* Changing the data of a text/comment/PI node
* Changing an attribute's name or value, for an element
* Adding or removing an attribute, for an element
* Inserting or removing a child
* Any DOM change to a child

And the DOM state of a node is its list of attributes (for elements),
its data (for text/comment/PI), and its list of children including all
their DOM state.

10) It's maybe not a big deal, but I think that you want to define
Transaction as a dictionary, not an interface, and remove the
[NoInterfaceObject]:

http://dev.w3.org/2006/webapi/WebIDL/#dfn-dictionary

If I understand correctly, this is what allows you to do transact()
with an object literal as its first argument.  If it were an
interface, you'd only be able to get Transaction objects by invoking
some method or attribute that returns them.  The difference doesn't
make a lot of sense in JavaScript, admittedly, and I might be
misunderstanding.

11) "Any changes made to the value of the isAutomatic attribute after
the transaction had been applied should not change the type of the
transaction."  What about changing other things about it?  If I do

  var transaction = { label: "x", apply: foo, unapply: bar, reapply:
baz, isAutomatic: false };
  document.undoManager.transact(transaction);
  transaction.unapply = quz;
  document.undoManager.undo();

which function is called, bar or quz?

12) Relatedly, does item() return references to Transactions, or copies?

13) "The highest node affecting an automatic transaction" is "the
editing host of the lowest common ancestor of nodes, inside the undo
scope associated with the UndoManager to which the transaction is
added, mutated while applying the transaction".  For "editing host
of", you might want to link to my definition:

http://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#editing-host-of

In particular, your current text isn't totally clear about what
happens if you have non-editable elements nested inside editable ones.
 If a transaction mutates "bar" in <div contenteditable>foo<span
contenteditable=false>bar</span>baz</div>, is the highest node the
div, or the text node "bar"?  I assume you mean the latter.

14) In section 3.3.2 Applying, Unapplying, and Reapplying Automatic
Transactions, you don't define any actual algorithm for how to
unapply/reapply transactions.  UAs won't be able to implement
automatic transaction interoperably based on this definition.  Do you
plan to eventually define this?  Obviously it will be very
complicated, but it will be critical for interop.

15) Is the isReapply parameter to apply() needed?  The only place I
see where it's used is if the author specifies a manual transaction
but leaves off a reapply() method.  In that case, why not just call
apply() with no extra parameter?  If the author wanted to have apply()
and reapply() behave differently, they could have specified a separate
reapply() method.

16) New event APIs shouldn't define init*Event().  Those are ugly and
awkward, and we keep them only for compatibility with old event types.
 Instead, you should use event constructors, like this:

http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#event-definitions-0

If I understand right, you'd initialize a TransactionEvent with new
TransactionEvent({type: foo, bubbles: true, cancelable: true}) instead
of initTransactionEvent(foo, true, true).  This doesn't rely on
argument order, so it's a lot easier to understand, and also you can
omit any of the arguments from the dictionary if you want their
default values.