Media options are not automatically opened when inserting a media block from the slash inserter

[fluiddot]

Description

When adding a media block from the inserter menu, the media options are automatically opened, however, this behavior is not being replicated when adding the blocks from the slash inserter.

The blocks that support this feature are:

• Image block
• Audio block
• Video block
• Gallery block
• File block

NOTE: The embed block uses a similar mechanism to request the embed link, probably the same workaround to enable this feature could be applied to this block.

Step-by-step reproduction instructions

1. Select a paragraph block.
2. Type / to display the slash inserter.
3. Select a media block (i.e.: Image block)
4. Observe that the media options are not automatically opened.

Expected behaviour

Media options should be automatically opened when inserting a media block from both the inserter menu and slash inserter.

Actual behaviour

Media options are not automatically opened when inserter a media block from the slash inserter.

Screenshots or screen recording (optional)

open-media-options-slash-inserter.mp4

WordPress information

• WordPress version: N/A
• Gutenberg version: N/A
• Are all plugins except Gutenberg deactivated? N/A
• Are you using a default theme (e.g. Twenty Twenty-One)? N/A

Device information

• Device: iPhone 12 Pro Max (Simulator)
• Operating system: iOS 15.0
• WordPress app version: develop

[mchowning]

Dug into this a bit today, and this is what I have figured out to this point. The way we know to pop-up the media options menu is by checking if the gallery/image/etc. ("media") block was the lastBlockInserted. For example, the image block has:

wasBlockJustInserted: wasBlockJustInserted(
	clientId,
	'inserter_menu'
)

which calls wasBlockJustInserted:

export function wasBlockJustInserted( state, clientId, source ) {
	const { lastBlockInserted } = state;
	return (
		lastBlockInserted.clientId === clientId &&
		lastBlockInserted.source === source
	);
}

This is using lastBlockInserted:

export function lastBlockInserted( state = {}, action ) {
	switch ( action.type ) {
		case 'INSERT_BLOCKS':
			if ( ! action.blocks.length ) {
				return state;
			}

			const clientId = action.blocks[ 0 ].clientId;
			const source = action.meta?.source;

			return { clientId, source };
		case 'RESET_BLOCKS':
			return {};
	}
	return state;
}

The thing to note here is that lastBlockInserted only gets updated on INSERT_BLOCKS. It does not get updated on REPLACE_BLOCKS which is what is called when we use the slash inserter (or when we transform a block, which also fails to automatically open the media options).

Simply adding a case: 'REPLACE_BLOCKS': inside lastBlockInserted that also saves the "lastBlockInserted" when blocks are replaced helps, but does not fully address the problem. When the media blocks call wasLastBlockInserted they specify that the source was 'inserter_menu' (for example), but 'REPLACE_BLOCKS' actions do not have a source like 'INSERT_BLOCKS', so wasLastBlockInserted still returns false on account of the source not being 'inserter_menu'.

If I both persist the lastBlockInserted on 'REPLACE_BLOCK' and also make it so that wasBlockJustInserted no longer cares about the source, then I do get the media options automatically opening from the slash inserter like we want.

That's as far as I've gotten so far. To be clear, I'm not at all confident ^that^ is an actual fix because (1) updating the lastBlockInserted any time a block is replaced might have side effects and I haven't done much testing of this yet and, and (2) the source check was probably added for a reason, and I don't have a sense for what the reason was (original PR that added that).

cc: @guarani - you were one of the reviewers on the original PR, I doubt you'll remember anything from that because it's been awhile, but wanted to ping you just in case any of this sounds familiar.

[guarani]

Hi @mchowning, I'm curious what's the reason the slash inserter uses REPLACE_BLOCKS instead of INSERT_BLOCKS. I'm sure it's done for a reason, but do you have any insight into that? Intuitively, it seems like INSERT_BLOCKS would be used for both the block inserter and the slash inserter.

[mchowning]

I'm curious what's the reason the slash inserter uses REPLACE_BLOCKS instead of INSERT_BLOCKS. I'm sure it's done for a reason, but do you have any insight into that? Intuitively, it seems like INSERT_BLOCKS would be used for both the block inserter and the slash inserter.

It might just be because, technically, the slash inserter is always replacing a paragraph block.

I think it's worth considering whether we want these kinds of actions (i.e., automatically opening the media options) triggered in other instances where a block is "replaced". For example, if we transformed an empty gallery block into an empty image block, would we want th media options menu to automatically open? My initial reaction is yes, but I'm not sure there is an obvious/clearly-right answer either.

[guarani]

It might just be because, technically, the slash inserter is always replacing a paragraph block.

I see, the slash inserter is always invoked from inside a paragraph block.

It seems from what you're saying that the slash inserter is very similar to the block transform operation in that it essentially replaces blocks. By extension, if you made the media options appear when using the slash inserter, that might also make the options appear when using block transforms. Is that correct?

Looking at the current behavior, the media options doesn't appear when transforming (e.g. an Image block to a Gallery block). From a UX perspective, making media options appear when transforming might be beneficial to the user. That said, I have a feeling you might run into #29672 on iOS if you change this and two bottom action sheets are shown one after the other.

I don't remember why the source attribute was added (I don't know if it supports other values than inserter_menu).

[mchowning]

This isn't urgent, so it feels like the best course of action may be to just wait until Joel has a chance to chime in since he has the most context.

[mchowning]

👋 @jd-alexander ! Wondering if you might have any thoughts on this ticket. I know it's been quite a while since you worked on this, so no worries if you don't remember anything about this. 🙂

[jd-alexander]

Hi @mchowning Thanks for the ping. I am just seeing this because I am mainly using Github enterprise for Tumblr work. Your investigation here is spot on. The refactor to include INSERT_BLOCKS was done with @fluiddot so he may have as much context as me in terms of an appropriate fix. Wdyt @fluiddot ?

Simply adding a case: 'REPLACE_BLOCKS': inside lastBlockInserted that also saves the "lastBlockInserted" when blocks are replaced helps, but does not fully address the problem. When the media blocks call wasLastBlockInserted they specify that the source was 'inserter_menu' (for example), but 'REPLACE_BLOCKS' actions do not have a source like 'INSERT_BLOCKS', so wasLastBlockInserted still returns false on account of the source not being 'inserter_menu'.

I am not sure how the slash inserter interfaces with REPLACE_BLOCKS if we could find the source of that we could then include it as a component/flow that should trigger the auto opening. I know that the slash inserter shares it's core logic with the web so that may be a good place to look.

the source check was probably added for a reason, and I don't have a sense for what the reason was

This check was added because there are multiple operations that can trigger INSERT_BLOCKS so we wanted to be sure that it was coming from the Inserter.

If I both persist the lastBlockInserted on 'REPLACE_BLOCK' and also make it so that wasBlockJustInserted no longer cares about the source, then I do get the media options automatically opening from the slash inserter like we want.

Good find.

[fluiddot]

Hi @mchowning Thanks for the ping. I am just seeing this because I am mainly using Github enterprise for Tumblr work. Your #37275 (comment). The refactor to include INSERT_BLOCKS was done with @fluiddot so he may have as much context as me in terms of an appropriate fix. Wdyt @fluiddot ?

Sure, I hope I can provide further context regarding this issue. The idea behind the lastBlockInserted state was to provide a way to detect when a block was recently inserted via a specific source, especially for detecting when a media block was added and automatically opening the media picker options. However, we only wanted to do this when the block was added from the block picker, so, the rest of the insertion flows like the following ones won't have this behavior:

• Copy/paste a media block
• Transform a block into a media block

In case we'd like to include this behavior when using the slash inserter, following a similar pattern using a source parameter might be complex. Unlike the insertBlock action which allows passing optional meta values like source, the replaceBlocks action doesn't.

If I both persist the lastBlockInserted on 'REPLACE_BLOCK' and also make it so that wasBlockJustInserted no longer cares about the source, then I do get the media options automatically opening from the slash inserter like we want.

This would be an alternative but I understand that we'll make this behavior (i.e. automatically opening the media options picker) the default for all cases, right? I can't remember exactly why we didn't consider this option, but we could try going this way if there's no other route. In any case, I'd recommend checking first that this approach doesn't introduce any undesired side-effects.