On Wed, Oct 26, 2011 at 10:57 AM, Aryeh Gregor <a...@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