Last Comment Bug 617532 - implement the HTML5 "undo history" feature (UndoManager interface)
: implement the HTML5 "undo history" feature (UndoManager interface)
Status: RESOLVED FIXED
: dev-doc-needed, html5
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- enhancement with 4 votes (vote)
: mozilla20
Assigned To: William Chen [:wchen]
:
Mentors:
https://dvcs.w3.org/hg/undomanager/ra...
Depends on: 792890 827028 827086 827426 840877 872870 880892
Blocks: html5 827100 827102 827132
  Show dependency treegraph
 
Reported: 2010-12-07 22:48 PST by Michael[tm] Smith
Modified: 2013-07-17 01:18 PDT (History)
28 users (show)
wchen: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implement HTML5 UndoManager. v1 (113.39 KB, patch)
2012-09-19 08:27 PDT, William Chen [:wchen]
no flags Details | Diff | Review
Implemented HTML5 UndoManager. v2 (114.61 KB, patch)
2012-09-24 10:25 PDT, William Chen [:wchen]
ehsan: review+
Details | Diff | Review
Implemented HTML5 UndoManager. v3 (147.80 KB, patch)
2012-12-12 13:57 PST, William Chen [:wchen]
bzbarsky: feedback+
Details | Diff | Review
v2 diff v3 (12.85 KB, patch)
2012-12-12 13:57 PST, William Chen [:wchen]
no flags Details | Diff | Review
Implemented HTML5 UndoManager. v4 (141.09 KB, patch)
2012-12-31 12:15 PST, William Chen [:wchen]
ehsan: review+
Details | Diff | Review

Description Michael[tm] Smith 2010-12-07 22:48:22 PST
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
Comment 1 Alex Vincent [:WeirdAl] 2011-04-09 19:27:03 PDT
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.
Comment 2 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-04-09 22:56:49 PDT
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...)
Comment 3 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-04-09 22:57:37 PDT
Marking this UNCONFIRMED for now.
Comment 4 Daniel Glazman (:glazou) 2011-04-10 00:24:10 PDT
(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.
Comment 5 Michael[tm] Smith 2011-04-10 03:31:42 PDT
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.
Comment 6 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-04-10 08:37:54 PDT
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)
Comment 7 :Ms2ger 2011-04-10 09:41:08 PDT
(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.)
Comment 8 Alex Vincent [:WeirdAl] 2011-04-10 10:12:35 PDT
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
Comment 9 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-15 17:02:46 PDT
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.)
Comment 10 Jonas Sicking (:sicking) PTO Until July 5th 2011-04-15 19:44:43 PDT
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.
Comment 11 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-18 16:34:25 PDT
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.
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2011-07-19 15:01:23 PDT
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.
Comment 13 Alex Vincent [:WeirdAl] 2011-07-22 22:57:48 PDT
(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?)
Comment 14 Michael[tm] Smith 2011-08-06 01:58:19 PDT
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.
Comment 15 William Chen [:wchen] 2012-09-12 08:00:16 PDT
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?
Comment 16 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-09-12 09:26:46 PDT
> can we do this with xpidl?

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

Which interface are you dealing with here?
Comment 17 William Chen [:wchen] 2012-09-12 09:29:43 PDT
(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
Comment 18 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-09-12 09:37:48 PDT
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?
Comment 19 William Chen [:wchen] 2012-09-12 10:17:38 PDT
(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.
Comment 20 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-09-12 10:45:03 PDT
Oh, I see.

Yeah, we'd need to add something to xpconnect prototype setup to support this in xpidl.  :(
Comment 21 :Ehsan Akhgari (busy, don't ask for review please) 2012-09-12 11:10:36 PDT
(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?
Comment 22 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-09-12 13:36:52 PDT
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.
Comment 23 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-09-12 14:19:08 PDT
Couldn't we add the method to a new xpidl interface and let nsDocument or nsHTMLDocument to
implement it. Then add conditional classinfo stuff.
Comment 24 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-09-12 14:22:49 PDT
Oh, yes.  That would be the simple way to do it, indeed.  Good idea!
Comment 25 William Chen [:wchen] 2012-09-19 08:27:24 PDT
Created attachment 662572 [details] [diff] [review]
Implement HTML5 UndoManager. v1
Comment 26 :Ms2ger 2012-09-19 09:50:17 PDT
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 :/
Comment 27 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-09-19 09:53:15 PDT
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.
Comment 28 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-09-19 10:02:24 PDT
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 ?
Comment 29 William Chen [:wchen] 2012-09-24 10:25:49 PDT
Created attachment 664107 [details] [diff] [review]
Implemented HTML5 UndoManager. v2

Switched to WebIDL, and callback interface for DOMTransaction.
Comment 30 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-09-24 10:44:52 PDT
Should the document have an undo manager independent of the root element?
Comment 31 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-09-24 10:45:30 PDT
Also, the "origin of this IDL file" bits should link to the right spec.
Comment 32 William Chen [:wchen] 2012-09-24 11:33:27 PDT
(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.
Comment 33 :Ehsan Akhgari (busy, don't ask for review please) 2012-12-04 14:19:10 PST
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.
Comment 34 William Chen [:wchen] 2012-12-12 13:57:15 PST
Created attachment 691513 [details] [diff] [review]
Implemented HTML5 UndoManager. v3

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.
Comment 35 William Chen [:wchen] 2012-12-12 13:57:38 PST
Created attachment 691514 [details] [diff] [review]
v2 diff v3
Comment 36 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-12-12 19:02:19 PST
> 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?
Comment 37 William Chen [:wchen] 2012-12-13 16:38:52 PST
(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.
Comment 38 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-12-13 18:40:17 PST
> 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.
Comment 39 William Chen [:wchen] 2012-12-31 12:15:23 PST
Created attachment 696780 [details] [diff] [review]
Implemented HTML5 UndoManager. v4

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.
Comment 41 Phil Ringnalda (:philor) 2013-01-05 16:31:24 PST
https://hg.mozilla.org/mozilla-central/rev/6e251f2906f5
Comment 42 Tim Down 2013-04-26 02:44:03 PDT
Firefox 20 release notes say UndoManager is supported, but it does not appear to be there (v20.0.1 on Windows 7).
Comment 43 :Ehsan Akhgari (busy, don't ask for review please) 2013-04-26 06:31:31 PDT
(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.
Comment 44 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-07-17 01:18:10 PDT
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?

Note You need to log in before you can comment on or make changes to this bug.