Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

undo manager #209

Open
johanneswilm opened this issue Sep 14, 2019 · 22 comments
Open

undo manager #209

johanneswilm opened this issue Sep 14, 2019 · 22 comments

Comments

@johanneswilm
Copy link
Contributor

It has been proposed to give JavaScript a way to manipulate the browser's undo stack [1]. I think it would be useful for JavaScript editors if in addition to what the explainer [2] mentions, it can handle two situations:

  1. Collaborative editing: User A (local) and user B (remote) are collaborating on a document that contains two paragraphs. User A writes 2 words in paragraph one. The size of the undo stack is 2. User A writes 3 more words in paragraph two. The size of the undo stack is now 5. User B deletes paragraph two. The size of the undo stack should now be 2 (the two words in paragraph one). Ideally: If user B now undoes the deletion of paragraph two, the size of the undo stack should be 5 again.

  2. Separate undo stacks for specific elements controlled by JS: Many editors accept text input in other places than in the main editor. This can for example be in a dialog box that is presented as a layover (see below). While the focus is on any text input element in there, the user should not accidentally be changing the text in the main editor by hitting the undo key combination.

Untitled document - Google Docs

[1] https://rniwa.github.io/undo-api
[2] https://whsieh.github.io/UndoManager

@whsieh
Copy link

whsieh commented Sep 15, 2019

First, thanks for opening this issue!

With regards to (2), I believe that the proposed undoScope attribute ought to help here, by setting undoScope=true on the modal. This way, undoing in the text field in the example above would add to the modal dialog’s undo stack, rather than the undo stack of the page, and further attempts to undo while the modal (or one of its children) is still focused would result in a no-op.

@johanneswilm
Copy link
Contributor Author

johanneswilm commented Sep 15, 2019

@whsieh Ok, that is good to know. What threw me off was the note saying:

The user agent may decide to have a separate undo history per frame. For this proposal allows WebKit's behavior, which is to have a single undo history for all browsing contexts under a single top-level browsing context.

That made it sound as if it was up to the UA whether or not it would allow more than one undo history on the same page.

How about point 1? When an undo history history is kept in JavaScript, it will basically do the following:

A. Keep a history of all document changes, whether each of the changes is remote or local, possibly up to some max number of changes.

B. When the undo button is updated, the JS editor goes back in the undo history to see if it can find at least one local change that still could be undone. If that is the case, the button will be enabled, otherwise disabled.

C. If an enabled undo button is pressed, the JS editor goes back in the undo history and undoes the last local change that could be undone.

D. If a user undoes a change and there are remote collaborators, the user sends the change via websocket to collaborators specifying that the change Y is to cancel out previous change number X. That way the remote users know that they can skip both change X and change Y when doing B and C.

I understand that this is probably way to much complexity for the browser to engage in, but maybe there is a way to fake it? To let the JavaScript editor maintain the history and o enable/disable the undo/redo buttons whenever they need to?

@Reinmar
Copy link

Reinmar commented Sep 15, 2019

I've been thinking about real-time collab (RTC) editing and selective undo and, if I get the proposal right, it should be fine. Maybe not ideal, but fine.

The problem in RTC editing is that you don't know precisely what will have to be done upon undoing, but you know that an undo step was created. Your editor must have its own internal undo manager which knows when an undo step is created and it allows performing undo/redo operations on the editor.

For CKEditor 5 and integration between its internal undo manager and the browser's undo manager would look more or less like this:

editor.plugins.get( 'Undo' ).on( 'newItem', () => {
    document.undoManager.addItem( new UndoItem( {
        label: 'Editing',
        undo: () => editor.execute( 'undo' ),
        redo: () => editor.execute( 'redo' )
    } ) );
} );

And as long as the number of undo steps that are available in the editor's internal data model is equal to the number of undo items added to the browser's undo manager, this integration should work for undoing.

The same should be true for redoing because, again, the number of browser redo steps must simply match the number of redo steps available in the editor's undo manager. As long as in both implementations (browser's and editor's) there's always just one redo step for one undo step, both mechanisms should stay in sync.


BTW, I found one typo in https://rniwa.github.io/undo-api/#undomanager-interface (I can't find the GH repo for it):

image

@johanneswilm
Copy link
Contributor Author

johanneswilm commented Sep 15, 2019

@Reinmar Are you saying you would just add/delete entries to the undo stack to always have the right amount of undo/redo items in the browser's stack? What about the label for the undo item? I assume that is going to be used somewhere in the UI so that it can say things like "Undo adding two words".

If you are not using the label anyway, would it not be enough to always have zero or one item in the undo/redo stacks to either enable or disable to button?

@Reinmar
Copy link

Reinmar commented Sep 15, 2019

Are you saying you would just add/delete entries to the undo stack to always have the right amount of undo/redo items in the browser's stack?

Do you mean the depth of the stack? Yep, we could do that.

Regarding deleting redo steps if a user undid some changes and then e.g. typed a letter which meant that a new item appeared so addItem() was called – that's described in addItem()'s algorithm that it will clear all items with indexes greater than the current state:

  1. Clear redo items on hisotry represented by undoManager.

So, I think that it shouldn't be a problem to keep both stacks in sync.

Regarding labels – I don't think we'd ever use any other label than "Editing" (or no label, so just "Undo"/"Redo"). The ROI of naming all the actions that the user can do is close to zero for me. AFAIU, this label would only visible when using the context menu or the top menu – I'd say that those are the least common ways to undo things in our case. So, too much problem for us for too little profit.

@johanneswilm
Copy link
Contributor Author

Let me try again:
In collaborative editing you will have a shifting number of undoable items. It can be 5 at one time, then 2 then again 5 without the local user having done anything in-between all just because of actions of remote users. Are you saying that you will be adding and removing items from the local undo stack to get the number to be right at all times? So for example to go from 5 to 2, you just add three more items to the stack?

If additionally you don't use the label and I assume the number of items doesn't show up in the UI - wouldn't it be enough to make sure to always just keep one or zero items on the undo/redo stacks to enable and disable those buttons? Why would you want/need to keep 2 or 5 or 10 items on the stack?

I would agree that this would work - but it's kind of a hack and labeling all edits just "Editing" is probably not what they had in mind when they added the label.

@Reinmar
Copy link

Reinmar commented Sep 15, 2019

In collaborative editing you will have a shifting number of undoable items. It can be 5 at one time, then 2 then again 5 without the local user having done anything in-between all just because of actions of remote users.

Oh right... I assumed that since a user can undo only his/her changes, the number of undo steps is constant (doesn't change when the other users edit the content).

But you're right that there are scenarios which lead to an undo step becoming "empty". For example if you changed a color of text which was later removed by some other user, you can't undo changing its color.

Interestingly, I think that we can't tell without performing expensive computations whether other user changes nulled one of your undo steps. @scofalik, what do you think about this?


Why would you want/need to keep 2 or 5 or 10 items on the stack?

Yup, that'd be completely fine. That's what we do – we just enable/disable the undo/redo commands on our side and that affect the disable/enable state of our undo/redo buttons. We're only interested whether there's at least one undoable/redoable item out there.

I would agree that this would work - but it's kind of a hack and labeling all edits just "Editing" is probably not what they had in mind when they added the label.

I think that there may be some cases fo the label. For instance, you may have a graphics editor and a close set of operations that the user can perform. That would allow labeling them and that may be somehow useful.

So, while I wouldn't use it, I can't say that it's definitely completely useless.

BTW, I think that only Safari names its undo/redo options. Other browsers always display "Undo"/"Redo".

@whsieh
Copy link

whsieh commented Sep 16, 2019

A reminder for all who are interested: there will be a breakout session for the UndoManager API at 17:30 on Wednesday — https://w3c.github.io/tpac-breakouts/sessions.html#undomanager.

@scofalik
Copy link

scofalik commented Sep 16, 2019

Firstmost, excuse me, but I haven't read undo manager / undo API draft, so I might be missing something.

Answering @Reinmar question: yes, in CKE5, we would have to do some potentially time-consuming evaluations to know if undo execution is actually going to make any changes in the content. AFAIR, other popular editors also have this "problem" that sometimes undo does nothing.

However, this does not have to be true for all editors. Maybe there's an RTC solution, based on something other than OT (and probably less complex than what we have in CKE5) that does not need that much evaluation.

By the way, reading this thread one thing got my attention: why are we talking about real-time collaboration? Real-time collaboration problem solving is very specific and varies between editors, so I am not sure what kind of features would be possible to bring in the API that would help developers. Adding/removing/referencing undo steps is probably all that is needed and all that we can have?

EDIT: Okay, after re-reading for the 5th time I think I finally got what @johanneswilm meant :). Yes, well, you could have "a shifting number of undo steps". You meant the same thing that me and @Reinmar when describing "empty" undo steps. And yes, it could happen that you have:

  • 5 undo steps,
  • then 2 (content-changing) undo steps,
  • and then 5 undo steps again (after client B undid their changes).

However, I think that this should be controlled by an application, not a browser (so, the application should use the browser API to add/remove steps not the browser firing events to be handled by an application).

Still, as I wrote above, in CKE5 we would probably not use this feature in this case as it would be too time-consuming given the low value of the enhancement (not having empty steps).

@johanneswilm
Copy link
Contributor Author

@scofalik I think we are discussing realtime editors here to see if this proposal will work for that usecase because it is sort of the most complex thing we are doing in relation to undo/redo, I think. The conclusion so far seems to be that - yes, even for that usecase it will work, and the spec will represent a significant improvement over the current situation (we would just add either zero or one undo/redo events so that the buttons enable/disable as needed), but it only works because we would be using the new features to enable/disable the undo/redo buttons as we need them and not as the spec intends us to do. If the browser makers are ok with (ab)using it like that and don't just want to give us a more direct way to enable/disable then I think we should support this.

As for the shifting number of undo steps - yes, you did finally get me. Building an editor where you can undo the changes of your collaborator (or enemy) could also be a fun challenge, but probably not something for realworld usage. :)

Even if this is not used as intended for the main editor, it may be an improvement for the various side editors that web apps can have. Like a modal with a form with five different form fields can have it's own unique undo history that goes across input/textarea.

@johanneswilm
Copy link
Contributor Author

To note also: even if an editor doesn't use collaborative editing 99% of the time, or maybe even doesn't have the backend for using collaborative editing in 95% of cases, most editor libraries that do hope to support collaborative editing at least in some cases some day will likely revert to the kind of behavior that @Reinmar @scofalik and me are describing here. I could image a small number of inhouse editors of really big companies that have the resources to build their own JS editor without using any frameworks and know that they will never ever want to do anything with collaborative editing would just use this feature as intended.

@Dalzhim
Copy link

Dalzhim commented Dec 2, 2019

The proposal seems to enforce a model where undo and redo are two seperate actions as evidenced by the distinct UndoItemCallbacks on every UndoItem.
I usually consider both undo and redo actions as being the same generic function that defaults to a different stack and where every UndoItem is invertible. Here is some code to make this easier to understand:

const undoStack = Array();
const redoStack = Array();

function applyAction(originalStack, oppositeStack)
{
    const item = originalStack.pop();
    oppositeStack.push(invertAction(item));
    item.apply();
}

function undo()
{
    applyAction(undoStack, redoStack);
}

function redo()
{
    applyAction(redoStack, undoStack);
}

In my experience, it is easier to manage complexity when reasoning about invertible actions as opposed to implementing undo and redo as two separate actions. Should the UndoManager really push developers toward this direction?

@johanneswilm
Copy link
Contributor Author

@Dalzhim As I read the proposal, it simply lets you connect two function calls for when the browser native menus want to evoke either redo or undo. It does not tell you how to deal with either action or how to organize your editor internal undo/redo stacks - it merely connects the browser native UI with your stacks.

Do you have an alternative proposal how the browser could communicate to your app to please either redo or undo? Or did I completely misunderstand either you or the initial proposal?

@Dalzhim
Copy link

Dalzhim commented Dec 2, 2019

@johanneswilm The § 2. Definition section defines the undo history which seems to be considered an implementation detail for the browser implementers. It suggests managing it as an ordered list with a caret pointing at the location where the redo stack ends and the undo stack starts.

Unless I am misinterpreting something, I do not see how this model allows user code to manage the stacks. Here is a relevant quote from the proposal, emphasis is mine:

This specification tries to address above issues by providing ways to define undo scopes, add items to user agent's native undo history, and create a sequence of DOM changes that can be automatically undone or redone by user agents.

It also seems to mean that in order to use the apply/invert model with the undo/redo callbacks, every UndoItem's redo callback will have to be registered to a function like the following.

function redoCallback(item)
{
    applyAction(invertAction(item));
}

The current UndoItemCallback is a void function and doesn't provide the required parameter. It has to be captured beforehand and so a new handler has to be instantiated for every single UndoItem.

Alternative proposal 1
Modify the signature of UndoItemCallback so that it provides the UndoItem being operated upon as a parameter.

Alternative proposal 2
Make it possible for user code to provide its own UndoHistory implementation as opposed to enforcing it on the browser's side, accomodating both use cases (undo/redo as two different actions vs apply/invert on generic actions).

Alternative proposal 3
Enforce the apply/invert model by modifying the definition for the undo history.

@johanneswilm
Copy link
Contributor Author

johanneswilm commented Dec 2, 2019

Unless I am misinterpreting something, I do not see how this model allows user code to manage the stacks.

@Dalzhim I think what the proposal describes is a model which works well in cases where you have a common undo history for one element and all its descendant elements in which some of these descendant elements let the browser's native implementation create undo items and others do this by means of JavaScript. Imagine for example a <form>-element with one common history which includes several <input type="text'> fields which let the browser handle it's history and one contenteditable element for which JavaScript creates undo/redo entries.

If you have an element that is to have its entirely own undo/redo stack that is JS controlled and you don't want the browser to add/remove items on its internal stack (for example because you are working in a collaborative editor where the stack size may change), the way you can achieve that is by means of a few tricks:

  • If you need only the undo menu enabled, flush the stack, then add one item to the undo stack. Listen to the beforeinput event for undo and cancel it to undo something.

  • If you need only the redo menu enabled, flush the stack, then add one item to the undo stack and then undo it. Listen to the beforeinput event for redo and cancel it to redo something.

  • If you need both redo and undo enabled, flush the stack, add two items to the undo stack and then undo one of them. Listen for beforeinput event for both undo and redo and cancel both of them so that both menus stay activated.

  • In Safari there are labels on what redo and undo actually do. If you want those to be correct, flush the stack after every edit action and repopulate as mentioned in the above three points with correct labels.

That should take care of your proposal 2, right?

As for your alternatives: currently the proposal doesn't actually register the undoitem itself nor needs to know what form that is stored in so and you could just register something generic like:

document.undoManager.addItem(new UndoItem({
    label: "Edit",
    undo: () => myEditor.undo(),
    redo: () => myEditor.redo()
}));

Or if you want to keep your stack and the browser's stack in sync and the label correct, etc. something like

document.undoManager.addItem(new UndoItem({
    label: "Split paragraph",
    undo: () => myEditor.undo({historyItem: 123}),
    redo: () => myEditor.redo({historyItem: 123})
}));

I don't think I get why you need to provide the undoitem to the undo/redo functions. Is it because you want the label? Or because you want to get to the undo function from the redo function? Or some entirely different thing?

@Dalzhim
Copy link

Dalzhim commented Dec 3, 2019

@johanneswilm Thank you for the explanation. I now see how control can be assumed by user code. I had the belief that the browser's storage for UndoItem elements had to be used with the actual elements, but the indirection in your example makes it clear that I was wrong.

Nevertheless, I have new concerns I would like to raise:

  1. The § 6. Processing Model leads me to believe that I can click outside of the <input type="text"> (so that it is blurred), but inside the <form> that has an undo scope associated and I should be able to use the native undo/redo controls (i.e. : macOS's menu bar). The proposal doesn't specify whether that is actually true or not. I would expect it to be true. But then I would also expect the beforeinput events to be dispatched to different fields, depending on where the editing events occured, even if none of them are currently focused. Not receiving those events would prevent taking full control of the stacks in the way you described in your previous reply.
  2. I also fear it is going to be hard to take full control of the undo/redo stacks for one user-defined advanced <input> element if the undo scope is set on the <form> element as it forces the user to take care of the undo/redo behavior of the other regular <input> elements. In other words, clearing the undoManager's stacks also clears the UndoItem added by the user agent for other fields, which is too drastic, but there is a need to prevent the user agent from adding items for the advanced field itself.

@johanneswilm
Copy link
Contributor Author

@Dalzhim I think the current proposal works well if EITHER you have a JS-controlled richtext editor for that shares a (simple) undo stack with various browser controlled <input type="text"> and <textarea> elements (for example a small richtext editor within a form) OR if you have a JS-controlled richtext editor element that has a more complex undo stack with a variable number of items (for example a large editor element that dominates the page and that is set to work in collaborative editing with several remote users).

I think you are right that it does not work well for a combination of the two - for a JS-controlled richtext editor that is in collaborative editing mode that shares an undo stack with a browser-controlled <input type="text" id="document-title">-element. In that case you are forced to take over controlling the undo stack for the input-element as well.

@Dalzhim
Copy link

Dalzhim commented Dec 3, 2019

I think a few additions to the proposal could make it possible to combine the two approaches:

  1. Add an extra member to the UndoItem interface that holds a reference to the DOM element that is the target so that user code can easily discriminate items generated for fields it doesn't mean to override versus fields that it is managing in an advanced manner
  2. Add user-configurable callbacks to UndoManager so that user code can be notified when the user agent wants to add new UndoItem to the history and integrate those elements within its own storage so that they can be undone or redone

@johanneswilm
Copy link
Contributor Author

johanneswilm commented Dec 3, 2019

As for 2 I don't think one needs to worry about those that come from entirely browser controlled elements. The browser will simply take care of those by itself and the JS editor doesn't even have to know they happen. Or why would you want to add those steps to your own undo stack as well?

But your comment makes me think that there will likely be cases where the browser tries to auto-add items to the undo stack for the JS controlled element and in the scenario where one actually wants to let the browser maintain the order of undo steps (the no tricks version where it's part of a form together with input fields, etc.), that will likely in most cases lead to annoying issues of the browser having added items to the undo stack that shouldn't be there because the editor also adds it to its own undo stack.

Take for example text input - each character will likely add an item to the undo stack with the merge flag set while the same word is being typed and then without it being set for the next word. The problem is that JS editor and browser may disagree about what constitutes a word, etc. .

So I think that your proposal of having a listener there to then be able to stop specific items from entering the history or, alternatively, a way to simply stop the browser from trying to auto-add any items to the undo stack for a given contenteditable element would likely be helpful.

@Dalzhim
Copy link

Dalzhim commented Dec 3, 2019

You are right. Even though you explained very well how one doesn't need to steal control of the stacks from the browser by inserting proxy UndoItem instances, I instinctively described the problem with my previous mental model.

But just like you said, having the ability to observe and prevent the insertion of UndoItem instances by the user agent reconciles both approaches.

@mitar
Copy link

mitar commented Apr 22, 2024

I have read #150 and w3c/input-events#36 and I am curious about the state of this proposal? It looks to me like the work here has stalled?

@johanneswilm
Copy link
Contributor Author

@mitar Yes, as far as I am aware, nothing has happened on this front since 2019.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants