[whatwg] Feedback on UndoManager spec

[Aryeh Gregor]

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

That sounds like the ideal behavior, unless it's too difficult to
implement.  If authors just used Node.dataset, that would solve their
problem, but it would be better to behave as expected to start with.

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

Some APIs copy objects in some way instead of maintaining a reference.
 For instance, in WebKit, Selection.getRangeAt() doesn't return the
same object you added with Selection.addRange() (although the spec
currently says it should).  It seems best to be explicit.

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

I mean, if there's a mix of automatic and manual transactions, and the
manual transactions don't restore the exact same DOM, nothing says how
the UA should unapply/reapply the automatic transactions.  It just
says "the user agent still must do its best effort to restore the DOM
state."

What's the use-case for manual transactions anyway?  I'm sure it's
been discussed, but your spec doesn't say.  It could use an example.

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

Well, at some point we want to make sure we have interop if possible,
unless it's really performance-critical that this be UA-dependent.

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

I think either one of those two APIs would be fine.  I don't think the
compromise is good, because it gives authors two ways to do the same
thing, which is confusing.  I don't know which API is better without
knowing the use-cases for manual transactions.  But Jonas' version is
more flexible, because if the two are substantially different you can
always just do

  { apply: function(isReapply) {
      if (isReapply) {
          // reapply logic
      } else {
          // totally separate apply logic
      }
  }, . . . }

which is really no harder to write than

  { apply: function() {
      // apply logic
  }, reapply: function() {
      // totally separate reapply logic
  }, . . . }

It's only one or two lines longer, and one level of indent greater.
So I don't think a separate reapply member of the dictionary is
useful.  It just makes things more complicated, even if most of the
time the logic will be totally separate.

On Thu, Oct 27, 2011 at 2:44 AM, Ryosuke Niwa <rniwa at webkit.org> wrote:
> Interesting. I've done some quick testing but maybe problems I had in mind
> no longer exist in WebKit. We do a poor job on node adoption and lifetime
> control so this might be a good opportunity to sort them out anyway.

FWIW, I've noticed WebKit doesn't always do adoptions correctly.  For
instance, Range.setStart() will throw if the node you pass is from a
different document than the range, when the spec says it should
succeed.  I think I've noticed this for some Node methods too.  The
general rule in the specs these days is anytime you reparent a node to
another document, it gets silently adopted.