[whatwg] Feedback on UndoManager spec

[Ryosuke Niwa]

Thanks for a great feedback!

On Wed, Oct 26, 2011 at 9:42 AM, Aryeh Gregor <ayg at aryeh.name> wrote:
>
> 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?


This is so that the last transaction is always at position 0, and applying a
new transaction does not move the position. The position is non-zero only if
we have not applied any new transactions and have done undo.

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"?
>

I can change that.

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?
>

This is a debatable point. On one hand, allowing a node with undoManager to
be moved to another location in DOM seems nice but on the other hand, being
able to move a node with undoManager to a different document will be
problematic. And semantically, moving undoManager makes very little sense.

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

Yeah, I haven't put much thought into that. I'm thinking that we might need
to notify the content (e.g by firing some events) when UAs clear entries in
the undo transaction history.

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

Yes. The revised version is in its way.

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

Yes, I have a revised version which is going to be pushed soon.

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

That sentence is very out-of-date. Will update. FWIW, I do use anolis but
I've been ignoring all errors for some legacy reason.


> 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?
>

It'll be an array. I should probably revise that part.


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

Will revise.

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?
>

That might be a good idea actually.

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
>

Also, removing or adding document node, DOCTYPE node, etc...

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

I'm thinking whether DOM state should also include properties on the node or
not.

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

Any object can implement Transaction interface.

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?
>

quz.

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

It returns the original object passed to transact.

In particular, your current text isn't totally clear about what
> happens if you have non-editable elements nested inside editable ones.
>

Will revise.


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

If you're just replacing, inserting, or removing text inside the text node
"bar", then it's "bar". If the transaction anyway moves the node, or removes
the node even temporarily, then it's the span.

I should probably rewrite this definition as an algorithm. It's too
cumbersome to define it in a sentence.

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.


No. I did not specify that because the only requirement is that UAs restore
DOM states.
I specifically avoided to give any guarantee or implication as to in what
order things are restored
to allow optimizations.

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

There are good arguments made by Jonas on this topic.
Please look at whatwg archives on this topic.


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


Good idea. Will do.

- Ryosuke