[whatwg] Feedback on UndoManager spec

[Aryeh Gregor]

On Wed, Oct 26, 2011 at 1:13 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:
> 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.

Makes sense.

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

Well, if you do

  var span = document.createElement("span");
  span.undoScope = true;

what is the value of span.undoManager?  Per the current spec, it
should return an UndoManager that works just fine.  Is this desired,
or do you want to return null in this case?

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

Makes sense.

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

I don't understand what you mean here.  How can a Document node be
added or removed, since it can't be the child of anything?  If you add
or remove a doctype node, it should be a DOM change to the parent
Document because a child is added or removed, which is included in
what I said.

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

You mean IDL properties?  We can't include all of those in DOM state.
For instance, it would be extremely unexpected if undo were to undo
changes to document.cookie.  Or did you just mean custom IDL
properties that the author added via script, not built-in IDL
properties?

> Any object can implement Transaction interface.

I don't think that's how WebIDL works, but I'm not an expert in it.

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

This should be spelled out explicitly, IMO.

>> 12) Relatedly, does item() return references to Transactions, or copies?
>
> It returns the original object passed to transact.

This should be spelled out explicitly, IMO.

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

But this doesn't define what happens in the face of manual
transactions.  Also, it's not precise even if there are no manual
transactions.  If a node is removed from the DOM and undoing restores
it, does it restore the same object or a copy?  If a copy, does it
include custom properties that the author added or not?  I suspect
you'll say that this is deliberately undefined for the sake of
performance, but it's a potential interop issue.

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

I looked at the archives and didn't see any good arguments.  As far as
I can tell, if authors wanted behavior like with the isReapply
parameter, they could easily emulate it by changing

  { apply: f }

to

  { apply: function() { f(true) }, reapply: function() { f(false) } }

so the extra isReapply parameter doesn't give any extra control to
authors.  It just adds a second way to do the same thing, and
complicates the API.  It would be simpler and easier to understand if
authors just had to write the extra line or two of code and specify
separate functions for apply/reapply always, instead of being able to
specify either two functions or one that takes a boolean parameter to
achieve the same effect.