"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 (Improve performance of the gallery block, by removing unnecessary requests. #6325, Fix undo trap in Gallery block when transforming from shortcode #42001)
• Embeds: Conversion from raw (Impossible to undo Convert to Blocks when post_content contains a core oEmbed link #7323)
• Image: Adding via upload (Image Block: Undo includes pulsing state as an undo step #3553)
• File: Adding via drag-and-drop (File block retains href after uploading an unsupported file type causing block to encounter an error after page refresh #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: Improve performance of the gallery block, by removing unnecessary requests. #6325

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 #8066 (comment).

[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.

[Mamaduka]

I've encountered another issue related to keeping file blobs in attributes while working on #32157.

Conditions

• Block handles temporary image upload on the mount
• Doesn't provide block example
• User registers custom block styles

Currently affected blocks

• File
• Audio

When a block has no example, the "Styles" preview will use attributes. Then the block preview will trigger another upload based on the file blob attribute.

A simple solution will be to provide examples for both blocks. Another one will be to store temporary URLs in a state like #30114.

[Mamaduka]

I went ahead and created PR for audio block #32333.

File block doesn't have the "Insert from URL" option, so not sure how the example preview should work here.

[mcsf]

I've encountered another issue related to keeping file blobs in attributes while working on #32157. […]

Thanks for spotting that one.

[Mamaduka]

@mcsf Any preference on how I should fix this for the File block? Add an example or use the state for temporary URLs?

[mcsf]

Referencing #36807 for tracking purposes.

[Mamaduka]

@mcsf, we fixed a few others recently. I'm going to add them for tracking purposes - #30203, #34565, #35767.

[mcsf]

Thanks, keeping that trail is always useful. :)

[kevin940726]

I had some time to dig into the problem of the gallery block and the "undo trap" issue in general. This “undo trap” issue can happen when these conditions meet:

1. The block calls setAttributes or any other updating methods that push to the undo stack (create an undo level).
2. The method is NOT called as an “event”, coined by the useEvent RFC.

As a general rule of thumb, an action should only be able to push to the undo stack if the action is performed by user interactions. However, there are existing patterns in the gutenberg repository (I believe in other third-party blocks too) that don’t follow this practice. The most common mistake is the misuse of the useEffect hook.

The details of when to and not to use the useEffect hook are outlined in the new official React documentation. We can take real code in the Gallery block as an example here:

gutenberg/packages/block-library/src/gallery/edit.js

Lines 304 to 314 in 7f7c24d

	useEffect( () => {
	// linkTo attribute must be saved so blocks don't break when changing
	// image_default_link_type in options.php
	if ( ! linkTo ) {
	setAttributes( {
	linkTo:
	window?.wp?.media?.view?.settings?.defaultProps?.link ||
	LINK_DESTINATION_NONE,
	} );
	}
	}, [ linkTo ] );

Upon first creating the Gallery block, the useEffect here will run to update itself and set the linkTo attribute via setAttributes. This is an incorrect use of the useEffect hook to update states (attributes) based on props or states (attributes). More importantly, the setAttributes here doesn’t run on user interactions, but runs automatically on change of the attributes.

The solutions

There are some options for this issue:

1. __unstableMarkNextChangeAsNotPersistent

A common pattern in our codebase, is to use a lesser-known API __unstableMarkNextChangeAsNotPersistent. Calling it before setAttributes will mark the action as not persistent, which means it won’t push to the undo stack. It’s exactly how it’s done in #26377. This solution works but relies on an unstable API (which has been unstable for 2 years already) that’s hard and explicit for third-party authors to learn and use in day-to-day life.

__unstableMarkNextChangeAsNotPersistent();
setAttributes( { ... } );

2. __unstableMarkPersistent

I propose a refined API to wrap the underlying actions inside the callback so that it's easier to reason about.

__unstableMarkPersistent( () => {
  setAttributes( { ... } );
} );

The implementation could roughly look like:

function __unstableMarkPersistent( callback ) {
  __unstableMarkNextChangeAsNotPersistent();
  callback();
}

So that we can reuse the old API but with a nicer-looking signature. Note that the implementation still only supports one action at a time, but I think it's possible to refactor it so that it can mark all synchronous actions in the callback as persistent (while disallowing asynchronous callback with a linter plugin or runtime warning). A bonus benefit is that the __unstableMarkNotPersistent API will look exactly the same with just some tweaking in the underlying implementation.

3. Refactor away from useEffect

The more correct way to fix this, IMHO, is to get rid of the misused useEffect and calculate the linkTo attribute inline.

const linkTo = attributes.linkTo || window?.wp?.media?.view?.settings?.defaultProps?.link || LINK_DESTINATION_NONE;

This requires a larger refactor though and could potentially introduce breaking changes if not treated carefully. But I think it's the right way and can prepare us for a more resilient codebase.

How can we prevent this?

Since this is a hard problem and none of the solutions so far is trivial to fix by third-party authors, I've been thinking of a general solution to this problem so that we can prevent it from happening from the beginning. There are a few options I think we can try.

1. Lint plugin to warn when updating blocks inside useEffect

As mentioned above, updating attributes should be performed by user interactions. There's little to no reason that we want to set them on change of some props or states. We would want to also provide a link to the alternatives so that they can follow.

2. Runtime warning

Since pushing to the undo stack should always come from user interactions, maybe we can listen to user events to automatically mark the updating methods as persistent or not. I've tried listening to events that can be triggered by users, similar to how trusted events work.

We can listen to all the possible events globally and set a variable like isFromUserGesture to true. Then we just have to make sure the updating is called within the same synchronous cycle.

// Pseudo code below.
let isFromUserGesture = false;

window.addEventListener( 'click', () => {
  isFromUserGesture = true;
}, true ); // Capturing phase runs first.

// Or an arbitrary timeout.
setImmediate( () => {
  isFromUserGesture = false
} );

But that quickly faces a problem when deleting a block. For some reason, deleting a block is asynchronous. Probably because the action is a thunk and the implementation of thunk depends on asynchronous calling? @adamziel might know better 😅.

Deleting blocks isn't the only action though, I believe there are tons of existing actions that don't follow this rule, so we'll have to go through them one by one.

---

This issue is fairly complicated and I don't think I fully understand it even after days of studying. That's why this is more like a brain dump than a deep dive 😅.

There's no easy solution either, I'd be happy to know if there are better solutions. For now, I believe the best solution is to follow the best practices of useEffect and try to rethink our approaches.

[mcsf]

I appreciate the detailed account, @kevin940726. As I see it, we could:

• Try adding a linting rule for setAttributes calls inside useEffect if not preceded by __unstableMarkNextChangeAsNotPersistent.
• Stabilise markNextChangeAsNotPersistent . I have a slight preference for keeping the current approach instead of what you proposed with a callback, because IMO it lowers the "magic" factor and makes it clearer that there is nothing special about markNextChangeAsNotPersistent.

As asked by @annezazu, I don't know what else we can do with this issue. The time may be coming when we finally close it. Thoughts? :)

[youknowriad]

As asked by @annezazu, I don't know what else we can do with this issue. The time may be coming when we finally close it. Thoughts?

I think so :)