[whatwg] Feedback on UndoManager spec

[Ryosuke Niwa]

On Wed, Oct 26, 2011 at 10:57 AM, Aryeh Gregor <ayg at aryeh.name> wrote:

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

I'm not sure if we should allow that or not. I'm inclined towards not
allowing it for the sake of disallowing undoManager on a detached node.

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

I meant properties authors added to nodes. e.g.
span.myProperty = true;

Should span be removed by some automatic transaction, authors may expect it
to be restored on undo.

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

The assumption is transaction works like a regular object unless otherwise
stated. I guess I can cite your example though.

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


UAs don't do anything for manual transactions. They just call
unapply/reapply.

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

This is well defined in terms of DOM state. The spec says UAs should restore
the DOM state
so it all depends on how DOM state is defined.

I also vaguely remember Ehsan telling me Gecko might have a trouble
restoring the exactly same object
on undo/redo because of the way its undo and redo are implemented.

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

The same is true for having apply and reapply. Jonas wanted to get rid of
reapply altogether and just have
void apply(in boolean isReapply)

In this world, you could do
{ apply: function(isReapply) { return isReapply ? this.doApply() :
this.doReapply(); } }.

I didn't want this API because I'd expect apply and reapply to be
substantially different.

- Ryosuke