Bogus undo and unsaved changes state

[Mamaduka]

Problem

When you open the Navigation Editor or reload the page after saving (CMD + R), the page is displayed if you have unsaved changes. It also shows the active "Undo" button, which does nothing.

Screencast

CleanShot.2021-09-14.at.13.11.37.mp4

[Mamaduka]

I think this "side effect" causes the issue. If I comment out the setAttribute, there's no more unsaved state.

gutenberg/packages/block-library/src/navigation-link/edit.js

Lines 366 to 373 in 639a603

	useEffect( () => {
	// This side-effect should not create an undo level as those should
	// only be created via user interactions. Mark this change as
	// not persistent to avoid undo level creation.
	// See https://github.com/WordPress/gutenberg/issues/34564.
	__unstableMarkNextChangeAsNotPersistent();
	setAttributes( { isTopLevelLink } );
	}, [ isTopLevelLink ] );

Is it possible for __unstableMarkNextChangeAsNotPersistent not to work in context of Navigation Screen?

/cc @talldan.

[talldan]

You're right, it doesn't seem to be working in the navigation editor. I'm not sure why that would be though. Looking into it.

An option might just be to get rid of isTopLevelLink. I'm not sure why it should be needed, I think it should be possible to determine using a CSS selector whether something is a top level link. Using an attribute for this isn't great.

[Mamaduka]

I'm not sure about isTopLevelLink. Based on #31149, we need that attribute to have different color options for submenus.

Side note: If we end up keeping isTopLevelLink, we should wrap setAttributes in if condition and only set it for actual top links.

[talldan]

So there are a couple of differences between the post editor and the nav editor in terms of this issue:

1. Block is loaded differently

In the post editor isTopLevelLink is saved as an attribute so when loading a post this value is restored. setAttributes( { isTopLevelLink } ) has no effect because the attribute already has the correct value. Contrast this to the navigation editor, blocks are converted to menu items when saving, isTopLevelLink isn't saved anywhere. When loading the block always has an undefined value, so now setAttributes( { isTopLevelLink } ) actually does trigger a change. It could be possible to set a correct value for isTopLevelLink in our menuItemToBlock code, perhaps true when the parent is 0, or false otherwise.

2. Nav editor behaves slightly differently to the post editor in adding things to the undo stack.

The reducer that adds things to the undo stack works slightly differently in the post editor. There, changes to blocks are considered 'transient' edits, and the edit is flattened onto the previous item:

gutenberg/packages/core-data/src/reducer.js

Lines 448 to 464 in 79be738

	// Transient edits don't create an undo level, but are
	// reachable in the next meaningful edit to which they
	// are merged. They are defined in the entity's config.
	if (
	! isCreateUndoLevel &&
	! Object.keys( action.edits ).some(
	( key ) => ! action.transientEdits[ key ]
	)
	) {
	nextState = [ ...state ];
	nextState.flattenedUndo = {
	...state.flattenedUndo,
	...action.edits,
	};
	nextState.offset = state.offset;
	return nextState;
	}

This is defined in the entities definition, like this one for widgets:

gutenberg/packages/core-data/src/entities.js

Lines 71 to 79 in 79be738

	{
	name: 'widget',
	kind: 'root',
	baseURL: '/wp/v2/widgets',
	baseURLParams: { context: 'edit' },
	plural: 'widgets',
	transientEdits: { blocks: true },
	label: __( 'Widgets' ),
	},

I don't think the nav editor has a definition for its 'stub' navigation post entity, so maybe it needs one and to define block edits as transient.

[Mamaduka]

Thanks for the detailed explanation.

The Widget editor also uses an in-memory post entity but does have the same issue.

[talldan]

Just testing the widget editor, it does indeed have the same issue. Preventing the nav block from serialising isTopLevelLink (by deleting the attribute declaration) makes it become apparent.

I've put together a fix in #34839, but it introduces another issue. (edit: now fixed)