Gutenberg: "Undo trap": Avoid getting stuck in an editing state

[mcsf]

Undo trap: The situation in which a user is "stuck" when continuously pressing Undo. The typical cause: as Undo is pressed and the content is reverted, some event somewhere causes a new undo level to be created. The result is that the user is unable to undo past a certain point, thereby losing access to previous editing states.

This issue aggregates all bugs stemming from that common problem:

• [ ] Gallery: Conversion from raw (#6325)
• [ ] Embeds: Conversion from raw (#7323)
• [ ] Image: Adding via upload (#3553)
• [ ] File: Adding via drag-and-drop (#8155)

Problem

At the root of these bugs is the fact that blocks need to manage a transition using specific ephemeral state:

• when converting from [gallery ids="x,y,z"], a Gallery block needs to temporarily keep the IDs x, y, z of the images while it waits for API data necessary to the block (the URL and alt attribute for each image); once it receives the API data, it adds it to its attributes [resolved state];
• when adding an Image via file upload, the Image block initially keeps the image blob in order to render a pulsing image while waiting for upload conclusion; then, it updates its attributes to store the newly created URL for the uploaded image [resolved state];

Since the temporary states are encoded in the attributes themselves of the blocks, they automatically constitute an undo level of the editor history. Thus, the action of undoing will bring the blocks back to that temporary state, which itself triggers a transition "back" into the resolved state.

Solutions

There are, as I see it, two approaches:

• Altogether avoid encoding transitional states in a block's attributes. Since this is a problem that is likely to affect third-party blocks as well, ideally the problem wouldn't be fixed _ad hoc_ but rather using a standardized method to express such states. It could be expressed in terms of creating a new block by passing it a "seed" for a future state (e.g. the seed of an image ID to later yield an image URL).
• Go in a very different direction and consider that certain "events" (term used loosely) in the application are made up of a sequence of action dispatches over time. So, for the constitution of a Gallery block converted from shortcode, the actions would be and ; these would fold into a single event, thus a single undo level. This approach is much more ambitious and likely to be overkill and less robust. I explain it at length in this comment: https://github.com/WordPress/gutenberg/pull/6325/#issuecomment-402242054

The first option seems to be worth exploring first. A quick stab at the second one — by hacking withHistory — doesn't inspire a lot of confidence.

[noisysocks]

when adding an Image via file upload, the Image block initially keeps the image blob in order to render a pulsing image while waiting for upload conclusion; then, it updates its attributes to store the newly created URL for the uploaded image [resolved state];

I was looking into this a little. An image can be uploaded by 1) pressing _Upload_ on an image block; 2) drag-and-dropping an image onto an image block; and 3) drag-and-dropping an image straight into the editor. Using setState() instead of setAttributes() to store image blobs in ImageEdit fixes (1) and (2) nicely, but doesn't handle (3). Like you say, we need some way of passing data from createBlock() to the ImageEdit constructor.

[noisysocks]

@talldan discovered another bug to do with this in https://github.com/WordPress/gutenberg/pull/8066#issuecomment-406996686.

[claudiobrandt]

I also came across this undo trap when I pasted a text that included a URL. The URL became its own block, and when trying to undo, the browser kept reaching out to the URL and the paste could not be undone.

[mcsf]

Thanks for the input, @claudiobrandt. That sounds like an Embed issue (#7323), as listed in this issue's description, correct?

[claudiobrandt]

Yes, @mcsf, the same behavior, except that I noticed it while copying/pasting content that included lines with nothing but an URL. These lines were converted to a block after the paste operation, and undoing the conversion was not possible. This behavior doesn't happen all the time and further testing with copy/paste operation should be performed. As you can see in the video I prepared right now, while testing it again, if I copy and paste a list of URLs from a Notepad.exe document, the URLs are not converted and they become a single text block. If I paste the same list from a word processor, where each URL is already presented as a link (I used LibreOffice for testing), the results vary. The first time a paste, the URLs are converted to embeds, and I can undo it. When pasting the same content a second time, I can't undo it anymore. 1'19

[mcsf]

Thanks for the details and video, @claudiobrandt.

The difference in results between Notepad and LibreOffice is expected: although sometimes perhaps surprising, Gutenberg determines whether the pasted content is "plain text" or not, and depending on that it may skip certain types of processing (because it assumes the user doesn't want added formatting).

As for the rest, your report will be taken into account. Internally, pasting and manual conversion of blocks essentially work in the same way. :)

[youknowriad]

@mcsf I'm moving out of 4.2 but if your work is ready, we can reconsider.

[aduth]

Assorted unorganized thoughts:

• If the transient values could be held in state outside editor.blocks (the undoable history) and merged with non-transient attributes, we avoid an issue where the transient values would ever be reachable by Undo, because those history states would never exist.
  
  
  
  • A challenge here is, especially for block transforms (which return a block object, not actions/dispatches), where does this partitioning of transient vs. non-transient values take place?
  
  
  • It shouldn't be understated the advantage of this since, as prior attempts have illustrated, trying to scrub history of transient values is very cumbersome.
  
  
• I find it interesting the idea of a trackable history "narrative" where earlier states are collapsed into subsequent ones where a future action completes the story. I think some challenges here are: (a) what approach do we take to generate unique identifiers for these, (b) how does the block know whether it's in a narrative (and which one) to progress it, and (c) from a user's perspective, are these progressions sensible, since a completion could occur after some other intermediary actions they take (e.g. a gallery is created, but before it finishes, the users adds some more text; Undo-ing could remove the gallery before it'd undo the text changes)
• To the point of a block tracking itself through transient states, it occurred to me an idea that perhaps a block would define some property (e.g. isTransientState) where, given a set of attributes, the function would return a value indicating to the framework that it be considered in a transient state. This avoids the block implementation needing to account for (a) being initialized in a transient state and marking as such only after-the-fact, (b) tracking itself and managing progression over time.
• I started considering this with respect to a separate need to track history as a single timeline _across_ stores (https://github.com/WordPress/gutenberg/pull/13088#discussion_r251565024). For more than just as it impacts this issue, I'm not as confident now with what I'd proposed there with actions to replay on initial state. Ideally the block implementation doesn't need to search through and replace actions which occurred previously, as it's both inefficient and because there could be _many_ ways a block enters a transient state (unless we made assumptions merely about these very specific creation workflows).
  
  
  
  • I mention because it may be something which ought to be solved separate from what's considered here.
  
  

[mcsf]

and (c) from a user's perspective, are these progressions sensible, since a completion could occur after some other intermediary actions they take (e.g. a gallery is created, but before it finishes, the users adds some more text; Undo-ing could remove the gallery before it'd undo the text changes)

In my mind, resolution actions would amend the undo history into the past, e.g.

• a gallery is created, but before it finishes the user adds some text
• if the user undoes at that point, the text is removed and the partial gallery is kept
• if the user doesn't undo and the gallery finishes loading, the history becomes [ "gallery created and complete", "text inserted" ]
• if the user undoes at that point, the text is removed and the finished gallery is kept

Is this achievable and desirable with a cheaper and more predictable model of keeping track of states, such as the "single timeline across stores" you suggested?

• To the point of a block tracking itself through transient states, it occurred to me an idea that perhaps a block would define some property (e.g. isTransientState) where, given a set of attributes, the function would return a value indicating to the framework that it be considered in a transient state.

Would it be all-or-nothing or attribute-based? It sounds like the former, i.e. isTransientState would just return a boolean. It sounds interesting, but how can we tie it back to particular states or attributes (and do we want to)?

For more than just as it impacts this issue, I'm not as confident now with what I'd proposed there with actions to replay on initial state. Ideally the block implementation doesn't need to search through and replace actions which occurred previously, as it's both inefficient and because there could be _many_ ways a block enters a transient state (unless we made assumptions merely about these very specific creation workflows).

Yeah, I think we need to audit a little and codify possible transient states, and whether the same block type should be allowed more than one such state. As for efficiency, there probably needs to be a "window of replay", after which the resolution of a transient state doesn't amend anything (and it presumably would just add another undo step).

[talldan]

A challenge here is, especially for block transforms (which return a block object, not actions/dispatches), where does this partitioning of transient vs. non-transient values take place?

From a look at the code, the handlers in BlockDropZone are a good example:
https://github.com/WordPress/gutenberg/blob/bb6dfea267980c959080fbf9dab8c362159bba47/packages/editor/src/components/block-drop-zone/index.js#L65-L84

They result in an INSERT_BLOCKS action that's handled by the attributes reducer.

Could an option be that every INSERT_BLOCKS action is considered an initial transient state, providing the opportunity for later attribute updates to collapse their undo state?

That would allow something like media blocks to also specify that some of their early attribute updates can be considered part of the same undo action as that created by INSERT_BLOCKS. I have this vague idea that the states would be named to identify that they can be collapsed:

setAttributes( {
    id, url
}, 'INSERT_BLOCKS' );

edit: Thinking on it some more, you could have a situation where a user inserts multiple successive blocks, and those undo states shouldn't be collapsed. Would need a way to link those particular states 🤔

[epiqueras]

Go in a very different direction and consider that certain "events" (term used loosely) in the application are made up of a sequence of action dispatches over time. So, for the constitution of a Gallery block converted from shortcode, the actions would be and ; these would fold into a single event, thus a single undo level. This approach is much more ambitious and likely to be overkill and less robust. I explain it at length in this comment: #6325

The entities refactor allowed me to take an approach like this in #19937 quite trivially.