Michael[tm] Smith [:sideshowbarker] Reporter
	Description • 14 years ago

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:2.0b8pre) Gecko/20101207 Firefox/4.0b8pre
Build Identifier: 4.0b8pre

In the HTML5 spec, section 7.8, "Undo history", specifies a mechanism for
allowing Web applications to expose "undo" and "redo" actions to users, by providing a standard means for Web applications to manipulate an "undo transaction history", through the use of an "UndoManager" interface for adding and removing items from the undo transaction history, and "undo" and "redo" events that fire when a UA requests those actions from the Web app.

http://dev.w3.org/html5/spec/dnd.html#undo 

Reproducible: Always

	Alex Vincent [:WeirdAl]
	Comment 1 • 13 years ago

Ohhhh, I want this.  :)  I'm certain I could leverage existing TransactionManager code to implement the UndoManager interface.  I'm actually very familiar with what the txmgr does.

The big questions I have are about how and when we would build undo objects to add (i.e. DOM mutations, execCommand calls, etc.) and about the interfaces themselves.  I just sent a lengthy e-mail to whatwg mailing list on this very subject.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 2 • 13 years ago

It is not at all clear whether the UndoManager in HTML spec is good API.
So before implementing anything we need to review carefully the current
spec.

(I'll need to read your email...)

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 3 • 13 years ago

Marking this UNCONFIRMED for now.

	Daniel Glazman (:glazou) (not active in Mozilla any more)
	Comment 4 • 13 years ago

(In reply to comment #2)
> It is not at all clear whether the UndoManager in HTML spec is good API.
> So before implementing anything we need to review carefully the current
> spec.
> 
> (I'll need to read your email...)

100% agreed.

	Michael[tm] Smith [:sideshowbarker] Reporter
	Comment 5 • 13 years ago

I'd been hoping that UndoManager would get some review from implementors earlier so that, based on the feedback that came out of that, it could be refined in time for publication of the W3C Last Call draft of HTML5. But it didn't get that review and didn't get refined, and so was removed from the W3C HTML5 spec; see the following change:

http://html5.org/r/5735

So I'm not sure what to suggest as far as this bug goes. As the reporter, it's fine by me if the status is moved to resolved+wontfix. If/when UndoManager ever does get some more review and is improved to the point where it's actually worth implementing, then a new bug for this feature could be opened.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 13 years ago

Since the URL field in the bug points to WhatWG's HTML spec, I thought
this is about that spec, not W3C HTML5 (which, I admit, I rarely read)

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 7 • 13 years ago

(In reply to comment #5)
> So I'm not sure what to suggest as far as this bug goes. As the reporter, it's
> fine by me if the status is moved to resolved+wontfix. If/when UndoManager ever
> does get some more review and is improved to the point where it's actually
> worth implementing, then a new bug for this feature could be opened.

Part of implementing any new feature reviewing the specification. The fact that this feature hasn't been reviewed carefully at this point doesn't change anything about that, it just means that any assignee for this bug will need to spend more time on spec review than for the average bug. I don't know why we'd wontfix this bug before reviewing the specification and trying to improve it. (If the specification would turn out to be a train wreck that can't be saved, the situation would be different, but that seems not too likely.)

	Alex Vincent [:WeirdAl]
	Comment 8 • 13 years ago

I had no intention of implementing the API currently specified without review.  :)  I was just saying "This is a great idea, and I'd love to make it happen."

As I pointed out in said e-mail, I think there's room for improvement:
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-April/031191.html

	(no longer active)
	Comment 9 • 13 years ago

FWIW, I don't think that we should implement the API as currently specified.  Among other things that are wrong with it, the #1 thing that I don't like about it is how it mixes the notion of "DOM changes" and "undo objects".

I actually like our transaction manager APIs.  They're extremely powerful, and rather well designed.  I think something resembling that should be a better thing to expose to web content, but clear decisions need to be made on the scope of the functionality that needs to be covered by it (for example, should it support aggregate transactions, batch changes, listeners, etc.)

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 10 • 13 years ago

The thing I don't like about our transaction manager is that you have to do all mutations through the manager itself, rather than using normal DOM mutating methods. This has at least two problems:

1. You can't use libraries such as jQuery to do your mutations
2. You can't use various higher level APIs such as .innerHTML and
   range.surroundContents.

What I think we should have is something where you tell the transaction manager that you are starting a "undoable action", then do your modifications using the normal DOM, then tell the transaction manager that you are done with that action.

The transaction manager can use nsIMutationObserver notifications to record what modifications you are doing without the need to be directly notified.

	(no longer active)
	Comment 11 • 13 years ago

This seems like a very good suggestion.  And the good news that it's (rather) easily supportable with the current transaction manager, since it can handle any type of transaction.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 12 • 13 years ago

Here is the interface that I propose:

interface UndoManager {
  startTransaction(in optional DOMString title);
  endTransaction();

  readonly attribute unsigned long currentPosition;
  readonly attribute unsigned long length;
  DOMString? titleAt(in unsigned long index);

  void undo();
  void redo();

  void clearUndo();
  void clearRedo();
};


Alternatively, if we don't want the startTransaction/endTransaction API, which runs the risk that people forget to call endTransaction (for example if an exception is thrown), we can do something like:

interface UndoManager {
  transaction(in DOMString? title, Function callback);

  readonly attribute unsigned long currentPosition;
  readonly attribute unsigned long length;
  DOMString? titleAt(in unsigned long index);

  void undo();
  void redo();

  void clearUndo();
  void clearRedo();
};

The transaction function would synchronously call the callback, and any mutations that happen inside it form the body of the transaction.

	Alex Vincent [:WeirdAl]
	Comment 13 • 13 years ago

(In reply to comment #12)
> Alternatively, if we don't want the startTransaction/endTransaction API,
> which runs the risk that people forget to call endTransaction (for example
> if an exception is thrown), we can do something like:
> 
> interface UndoManager {
>   transaction(in DOMString? title, Function callback);
...
> };

I like this interface a lot, except that it doesn't allow for adding "custom" transactions.  For instance, as part of my changes to a document, I may want to store a property which isn't reflected as an attribute.  If I call undo, how do I ensure the property is reverted at the right time in the sequence of changes?

For XBL and friends, this might matter a bit:  that property could be reflecting a shadow element's attributes (<xul:textbox/> value, anyone?)

	Michael[tm] Smith [:sideshowbarker] Reporter
	Comment 14 • 13 years ago

Note that Ryosuke Niwa recently wrote a proposal for a significantly different UndoManager interface:

https://rniwa.com/editing/undomanager.html

See the related discussion on the whatwg mailing list:

http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2011-July/thread.html#32640
http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2011-August/thread.html#32763

Among other things, it formalizes the "transaction" concept and defines additional interfaces related to it.

	William Chen [:wchen] Assignee
	Comment 15 • 12 years ago

I've implemented the new UndoManager API without proper support for undoscope, in particular it's the interaction with contenteditable that is causing problems. The spec requires that each contenteditable element be able to maintain its own transaction history, however the way it works right now is that there is one transaction history for the document and this is something that needs to be fixed. I talked with ehsan about this and he suspects that it will probably be an large amount of work to get transaction history working properly with contenteditable.

So rather than letting my patches bitrot to the point where I would have to rewrite most of it, I would like to land my patch so that it is pref'ed off except for tests. When the above issues are fixed, I can update the implementation and expose it to the web.

bz: I heard that we have a way in the new bindings to pref off specific methods in an interface, can we do this with xpidl?

	Boris Zbarsky [:bzbarsky]
	Comment 16 • 12 years ago

> can we do this with xpidl?

Unfortunately, no.  We could try to add it...

Which interface are you dealing with here?

	William Chen [:wchen] Assignee
	Comment 17 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #16)
> > can we do this with xpidl?
> 
> Unfortunately, no.  We could try to add it...
> 
> Which interface are you dealing with here?

nsIDOMDocument.idl and nsIDOMHTMLElement.idl

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 12 years ago

Mmm.  Probably don't want to block on getting WebIDL bindings or those, though we may be close for Document.

Does this need to be a runtime pref?  Or could we just need a switch, with a build-time one being ok?

	William Chen [:wchen] Assignee
	Comment 19 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #18)
> Mmm.  Probably don't want to block on getting WebIDL bindings or those,
> though we may be close for Document.
> 
> Does this need to be a runtime pref?  Or could we just need a switch, with a
> build-time one being ok?

I want to enable the feature only for mochitests so I suspect that I will need this to be a runtime pref.

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 12 years ago

Oh, I see.

Yeah, we'd need to add something to xpconnect prototype setup to support this in xpidl.  :(

	(no longer active)
	Comment 21 • 12 years ago

(In reply to comment #20)
> Yeah, we'd need to add something to xpconnect prototype setup to support this
> in xpidl.  :(

How hard would that be?

	Boris Zbarsky [:bzbarsky]
	Comment 22 • 12 years ago

I'm not sure.  Presumably we'd need to store the pref stuff in the xpt, since that's what xpconnect has to work with.  I don't know enough about xpt files to know whether we can do that without a format change...

I suspect that if we can get the data into the xpt, using it during prototype setup is not that bad.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 23 • 12 years ago

Couldn't we add the method to a new xpidl interface and let nsDocument or nsHTMLDocument to
implement it. Then add conditional classinfo stuff.

	Boris Zbarsky [:bzbarsky]
	Comment 24 • 12 years ago

Oh, yes.  That would be the simple way to do it, indeed.  Good idea!

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 26 • 12 years ago

Comment on attachment 662572 [details] [diff] [review]
Implement HTML5 UndoManager. v1

Review of attachment 662572 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/interfaces/html/nsIDOMUndoManager.idl
@@ +8,5 @@
> +interface nsIVariant;
> +interface nsITransactionManager;
> +
> +[scriptable, uuid(71550a23-17e3-4de3-a546-ef990c4aeae6)]
> +interface nsIDOMUndoManager : nsISupports

This totally needs to use WebIDL

::: dom/tests/mochitest/general/test_interfaces.html
@@ +530,5 @@
>      "CameraManager",
>      "CSSSupportsRule",
>      "MozMobileCellInfo",
> +    "UndoManager",
> +    "UndoManagerDocument",

I don't like this :/

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 27 • 12 years ago

Use of WebIDL is nice to have, not must, IMHO.


But yeah, UndoManagerDocument shouldn't be in the global scope.
Renaming the interface to nsIUndoManagerDocument should be enough to fix the problem.

	Boris Zbarsky [:bzbarsky]
	Comment 28 • 12 years ago

The use of manual callbacks without going through our existing machinery for that stuff is almost certainly broken.  Likely broken for purposes of security, and even more likely to trigger compartment mismatch asserts.  :(

Is there a reason we can't use a callback interface for the transaction objects?

Is the relevant spec the one at http://dvcs.w3.org/hg/undomanager/raw-file/tip/undomanager.html ?

	William Chen [:wchen] Assignee
	Comment 29 • 12 years ago

Switched to WebIDL, and callback interface for DOMTransaction.

	Boris Zbarsky [:bzbarsky]
	Comment 30 • 12 years ago

Should the document have an undo manager independent of the root element?

	Boris Zbarsky [:bzbarsky]
	Comment 31 • 12 years ago

Also, the "origin of this IDL file" bits should link to the right spec.

	William Chen [:wchen] Assignee
	Comment 32 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #30)
> Should the document have an undo manager independent of the root element?

I don't think so but that's how it works in the spec. There was a bit of discussion a while back about this (http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-August/032770.html). We extend document with the undoManager property as a convenience for developers. If we don't want to let document have in independent undo manager (so that document.undoManager == document.documentElement.undoManager) then we should either get rid of document.undoManager or special case documentElement in the spec so that document.documentElement.undoscope defaults to true and that setting it will throw. I'm of the opinion that we should get rid of document.undoManager.

(In reply to Boris Zbarsky (:bz) from comment #31)
> Also, the "origin of this IDL file" bits should link to the right spec.

Done. Fixed locally.

	(no longer active)
	Comment 33 • 12 years ago

Comment on attachment 664107 [details] [diff] [review]
Implemented HTML5 UndoManager. v2

Review of attachment 664107 [details] [diff] [review]:
-----------------------------------------------------------------

(Apologies for the long delay here.)

::: content/html/content/src/UndoManager.cpp
@@ +122,5 @@
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(UndoAttrChanged)
> +  NS_INTERFACE_MAP_ENTRY(nsITransaction)
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +NS_INTERFACE_MAP_END

You should probably hook this stuff up to the new CC macros that we have.

@@ +134,5 @@
> +{
> +  mElement = aElement;
> +  mNameSpaceID = aNameSpaceID;
> +  mAttrAtom = aAttribute;
> +  mModType = aModType;

Please use initializer lists here and elsewhere in the patch.

@@ +211,5 @@
> +                  CharacterDataChangeInfo* aChange);
> +protected:
> +  void SaveRedoState();
> +  nsCOMPtr<nsIContent> mContent;
> +  CharacterDataChangeInfo mChange;

So it looks like you're never using CharacterDataChangeInfo::mDetails.  I think holding on to that can be risky since it has a weak content pointer.  I would really prefer if you created your own struct here which could get initialized from a CharacterDataChangeInfo.

@@ +544,5 @@
> +  NS_DECL_NSIMUTATIONOBSERVER_CHARACTERDATAWILLCHANGE
> +  NS_DECL_NSIMUTATIONOBSERVER_CONTENTAPPENDED
> +  NS_DECL_NSIMUTATIONOBSERVER_CONTENTINSERTED
> +  NS_DECL_NSIMUTATIONOBSERVER_CONTENTREMOVED
> +  UndoMutationObserver(nsITransactionManager* aTxnManager);

Nit: explicit.

@@ +549,5 @@
> +
> +protected:
> +  /**
> +   * Checks if |aContent| is within the undo scope of this
> +   * UndoMutationObser.

typo.

@@ +759,5 @@
> +namespace dom {
> +
> +class TxnScopeGuard {
> +public:
> +  TxnScopeGuard(UndoManager* aUndoManager)

explicit.

@@ +1141,5 @@
> +        keepAlive.AppendObject(txVariant);
> +        transactionItems.AppendElement(txVariant.get());
> +      }
> +    }
> +  }

Please get someone like bz look at this magic.  :-)

@@ +1209,5 @@
> +  if (!sDidCheckPref) {
> +    sDidCheckPref = true;
> +    sPrefValue = Preferences::GetBool("dom.undo_manager.enabled", false);
> +  }
> +  return sPrefValue;

Nit: you could do:

static bool sPrefValue = Preferences::...;
return sPrefValue;

::: content/html/content/src/UndoManager.h
@@ +30,5 @@
> +public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(UndoManager)
> +
> +  UndoManager(nsIContent* aNode);

Nit: please make the ctor explicit.

@@ +46,5 @@
> +  void ClearRedo(ErrorResult& aRv);
> +  static bool PrefEnabled();
> +  void Disconnect();
> +
> +  nsISupports* GetParentObject()

Nit: const.

::: editor/txmgr/idl/nsITransactionList.idl
@@ +48,5 @@
> +   * getData() returns the data (of type nsISupports array) associated with
> +   * the transaction list.
> +   */
> +  void getData(in long aIndex, [optional] out unsigned long aLength,
> +               [array, size_is(aLength), retval] out nsISupports aData);

Nit: please update the uuid of this interface.

::: editor/txmgr/idl/nsITransactionManager.idl
@@ +53,5 @@
>  
>    /**
> +   * Clears the undo stack only.
> +   */
> +  void clearUndoStack();

Nit: uuid.

	William Chen [:wchen] Assignee
	Comment 34 • 12 years ago

bz: Can you look over the block of code at line 1139 in UndoManager.cpp? I'm trying to put an array of callback interface objects into a DOM event.

	Boris Zbarsky [:bzbarsky]
	Comment 36 • 12 years ago

> I'm trying to put an array of callback interface objects into a DOM event.

I assume there's a reason that you have to use a variant for the arg to InitDOMTransactionEvent instead of using an actual array declared in the xpidl?

Even given that, why couldn't all that code in the referenced block be replaced with:

  nsCOMPtr<nsIWritableVariant> transactions = new nsVariant();
  transactions->SetAsArray(nsIDataType::VTYPE_INTERFACE_IS,
                           &NS_GET_IID(nsIUndoManagerTransaction),
                           items.Length(),
                           transactionItems.Elements());

?  Is the problem you're trying to solve that this ends up double-wrapping on the way back out to JS or something?

	William Chen [:wchen] Assignee
	Comment 37 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #36)
> > I'm trying to put an array of callback interface objects into a DOM event.
> 
> I assume there's a reason that you have to use a variant for the arg to
> InitDOMTransactionEvent instead of using an actual array declared in the
> xpidl?
> 
There were two reasons, the event generator doesn't support arrays. The second is that XPIDL doesn't support nsIVariant arrays.

> Even given that, why couldn't all that code in the referenced block be
> replaced with:
> 
>   nsCOMPtr<nsIWritableVariant> transactions = new nsVariant();
>   transactions->SetAsArray(nsIDataType::VTYPE_INTERFACE_IS,
>                            &NS_GET_IID(nsIUndoManagerTransaction),
>                            items.Length(),
>                            transactionItems.Elements());
> 
> ?  Is the problem you're trying to solve that this ends up double-wrapping
> on the way back out to JS or something?

I needed to use nsIVariant otherwise I wouldn't get the expandos on the transaction objects.

	Boris Zbarsky [:bzbarsky]
	Comment 38 • 12 years ago

> otherwise I wouldn't get the expandos on the transaction objects.

OK, so double-wrapping.  We could try to avoid that by hacking XPCConvert::NativeInterface2JSObject to expose the underlying object in this one case or something, but I'm not sure it's worth it.

Let's keep what you have for now and just get WebIDL codegen going for callback interfaces so we can stop the insanity.  One change needed, though: if JS_WrapValue returns false you have to rv.Throw(NS_ERROR_OUT_OF_MEMORY) and return immediately, not keep doing stuff on a JSContext that now has an exception pending.

	William Chen [:wchen] Assignee
	Comment 39 • 12 years ago

Here is the patch rebased. I think there were enough changes due to the WebIDLization of document and element that it warrants another quick look. I lost the interdiff but the changes I made are to Document and Element. WebIDL has let me remove the interfaces that were hidden behind a flag so there is much less nastiness now.

	William Chen [:wchen] Assignee
	Comment 40 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e251f2906f5

	Phil Ringnalda (:philor)
	Comment 41 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/6e251f2906f5

	Tim Down
	Comment 42 • 11 years ago

Firefox 20 release notes say UndoManager is supported, but it does not appear to be there (v20.0.1 on Windows 7).

	(no longer active)
	Comment 43 • 11 years ago

(In reply to comment #42)
> Firefox 20 release notes say UndoManager is supported, but it does not appear
> to be there (v20.0.1 on Windows 7).

It is disabled by default for now.

	Asaf Romano (gone)
	Comment 44 • 11 years ago

The year old draft API referenced here doesn't seem to mention what should |transact| do or not-do if merge is set to true whilst there are no undo entries. If one follows the algorithm blindly, it should throw because it would try to add transactions to a non-existing entry in the undo history.

"3. If merge is set to false, add an empty entry to the beginning of the undo transaction history.
4. Add the applied transaction to the beginning of the first entry in the undo transaction history."

Looking at the implementation though, it seems to be handled silently. First the transaction is applied, than NatchTopUndo is called, which, in turn, checks that there are least two item in the undo stack, and if not, just returns early and successfully.

Was this intended?