Image Block is replaced as a Youtube block after dragging multiple blocks outside their parent

[jeflopodev]

Description

I have created blank theme with the Create Block Theme Plugin.
I have created a post and I added the following blocks structure:

• Group
  • Group
    • Heading
    • Paragraph
    • Image
    • Youtube (I have created this block by pasting the video URL in the post editor)

Then I have selected the Heading, Paragraph, Image and Youtube blocks and dragged them outside their parent.

The image block got replaced by another instance of a youtube block, with the same video. Doesn't matter if I drag it one level upper or at root level.

Seems to happen only when an Image block is sibling of the youtube block. I can't replicate this behavior with the paragraph or heading blocks.

After dragging the blocks, and trying to press Undo I need to spam it multiple times till I get to the state before dragging the blocks. Doing single clicks to it doesn't work.

I also tried with the TT4 theme. And the exact same happens.

Step-by-step reproduction instructions

1. Create a blank theme using Create Block Theme (or use the TT4 theme)
2. Create a new post
3. Create a Group Block within another Group Block that contains at least one image block and a Youtube block (siblings).
4. Create a multiple selection that comprehends at least the image and youtube blocks.
5. Drag those blocks outside of it's parent.
6. Check if the image block got converted to a Youtube Block
7. Check if single clicks to the undo button doesn't work
8. Check if you have to spam click the undo button for it to work again

Screenshots, screen recording, code snippet

gg.mp4

Environment info

• WP 5.3.0
• Gutenberg 18.3.0
• Blank theme created with the Create Block Theme Plugin
• Chrome v124.0.6367.209
• Desktop Windows 11 23H2

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

[Mamaduka]

I can also reproduce this on the Gutenberg trunk. I can reproduce the issue with WP 6.5 and no Gutenberg plugin.

The order in which Image and Embed blocks are placed also doesn't matter.

What's happening

• After the dragging image block receives Embed block URL as new url attributes. I'm not sure yet why.
• The YT link can be rendered as an image.
• Block calls onImageError and converts it into an embed block.

[jsnajdr]

I can reproduce this. When both an Image and Embed block are selected and dragged to a new location, the React UI for both will be unmounted and re-mounted at the new location.

The Embed block has this effect with a setAttributes call that updates the Embed block's attributes with data from the embed preview:

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

Lines 169 to 187 in 608b890

	useEffect( () => {
	if ( preview && ! isEditingURL ) {
	// When obtaining an incoming preview,
	// we set the attributes derived from the preview data.
	const mergedAttributes = getMergedAttributes();
	setAttributes( mergedAttributes );
	if ( onReplace ) {
	const upgradedBlock = createUpgradedEmbedBlock(
	props,
	mergedAttributes
	);
	if ( upgradedBlock ) {
	onReplace( upgradedBlock );
	}
	}
	}
	}, [ preview, isEditingURL ] );

But this setAttributes function doesn't just update the Embed block, it updates all selected blocks in a multi-selection:

gutenberg/packages/block-editor/src/components/block-list/block.js

Lines 270 to 280 in 608b890

	setAttributes( newAttributes ) {
	const { getMultiSelectedBlockClientIds } =
	registry.select( blockEditorStore );
	const multiSelectedBlockClientIds =
	getMultiSelectedBlockClientIds();
	const { clientId } = ownProps;
	const clientIds = multiSelectedBlockClientIds.length
	? multiSelectedBlockClientIds
	: [ clientId ];
	updateBlockAttributes( clientIds, newAttributes );

Because both the Embed and Image blocks are selected, the Image block's url attribute is set to a YouTube URL. This URL fails to load as an image, and that triggers an automatic transform of the Image to YouTube Embed:

gutenberg/packages/block-library/src/image/image.js

Lines 248 to 256 in 608b890

	function onImageError() {
	// Check if there's an embed block that handles this URL, e.g., instagram URL.
	// See: https://github.com/WordPress/gutenberg/pull/11472
	const embedBlock = createUpgradedEmbedBlock( { attributes: { url } } );
	if ( undefined !== embedBlock ) {
	onReplace( embedBlock );
	}
	}

@youknowriad @ellatrix This behavior where setAttributes updates all selected block has been there for years, and yet it seems horribly wrong? The documentation for the block Edit function says quite clearly that I can do this:

function Edit( { attributes, setAttributes } ) {
  <button onClick={ () => setAttributes( { foo: 'bar' } ) }>set</button>
}

and the intent is quite obviously to set the attributes of exactly one block. And yet it's setting foo to bar on all multi-selected blocks.

[Mamaduka]

Thanks for finding the attribute update cause, @jsnajdr! I should've guessed it was a multi-selected block attribute update.

Jarda, do you plan to work on a fix? I have an idea for an Embed block "hotfix" and happy to push it.

P.S. This proves again that "on-mount effects" should be avoided in blocks or used very carefully.

[jsnajdr]

This proves again that "on-mount effects" should be avoided in blocks or used very carefully.

I think that the Embed block doesn't do anything wrong. It simply wants to update its own attributes, that's legit. The problem is that the setAttributes function updates all selected blocks. There's certainly also some good reason why that was added. We need to discuss that and figure out what is the right fix.

[ellatrix]

Should we add some early returns in that setAttributes action? It should only update a multi-selection if the blocks are of the same type. I believe there are checks on render, but not in that action.

[Mamaduka]

Should we add some early returns in that setAttributes action? It should only update a multi-selection if the blocks are of the same type. I believe there are checks on render, but not in that action.

That will only partially solve the issue. If you drag multiple embed blocks with different URLs, they'll have the same URL after the dragging.

Edit: The original logic was introduced in #22470.

[jsnajdr]

It should only update a multi-selection if the blocks are of the same type.

This is still wrong, because if I select and drag two adjacent YouTube Embed blocks, they will update each other's URLs.

The question is more like: why is setAttributes looking at the multi-selection at all? Why isn't it updating just the one clientId that it is bound to? What's the use case?

[jsnajdr]

What's the use case?

I see it now: the use case is that you select two paragraph blocks, and then in the sidebar edit the text color:

This will update the attributes of both blocks.

But it looks like a bad idea to use setAttributes for that. The controls should be able to explicitly choose if they want to update just one block, or multiple ones.

[Mamaduka]

I don't think we can just remove current behavior, as it would be considered a breaking change.

FYI, updating values for multiple blocks of the same type also has issue #51609.

[Mamaduka]

@ellatrix, @jsnajdr, what do you think about creating a separate issue to continue discussions regarding solving potential bugs with current setAttributes behavior?

The Embed block's "on-mount effect" bug can be solved separately.

[jeflopodev]

Oh I'm very happy to see everyone's response in my bug report. Unfortunately I yet lack the knowledge to contribute beyond filing a report. I would be completely lost creating pull requests, committing changes etc. I guess no further actions are required from me, is that correct ?

[Mamaduka]

Thanks for reporting the bug, @jeflopodev!

That's right; you don't need to do anything else.

P.S. You might get a notification to link your GitHub account with WP.org when the pull request is merged so that you get contribution credit.

[cbirdsong]

But it looks like a bad idea to use setAttributes for that. The controls should be able to explicitly choose if they want to update just one block, or multiple ones.

This may be the source of the bug in #41260?

[jsnajdr]

This may be the source of the bug in #41260?

@cbirdsong Yes, this bug has the same underlying cause. The Layout control is not really aware that it's editing layout for multiple blocks. It receives the attributes of one block, the first selected one. It reads .layout, which is:

{
  type: 'flex',
  justifyContent: 'right',
  flexWrap: 'wrap',
}

When you disable the "Allow to wrap" toggle, it changes the flexWrap field and calls:

setAttributes( {
  layout: {
    type: 'flex',
    justifyContent: 'right',
    flexWrap: 'nowrap',
  }
} )

Here setAttributes will update the attributes of all selected blocks, and that's how the justifyContent: 'right' of the first selected block is applied to all others.

I don't think we can just remove current behavior, as it would be considered a breaking change.

@Mamaduka This will be tough, because the current API behavior is seriously broken and there are many cases that are handled incorrectly. We can't keep it that way forever.

If setAttributes is fixed to always modify just one block, and there is a new API introduced for block controls to modify multiple blocks at once, how will that break existing blocks? Instead of modifying all selected blocks, they will modify only the first. They will need to be patched if they want to modify multiple blocks. That's not awful. When balanced against the value of having a well-behaved stable API, it might turn out to be acceptable.

[Mamaduka]

@jsnajdr, that's a good point. I doubt many are aware of the current setAttributes behavior anyway. A dev note with a migration path should help mitigate any breakage.

What do you think of my proposal to create a separate issue for the general setAttributes fix?

P.S. The updateBlockAttributes has an uniqueByBlock flag/argument, which I believe was an attempt to fix this behavior. Currently used only by the Gallery block. See: #29099.

[ellatrix]

Sure, we can create a new API to modify multiple blocks. This was a tradeoff we made when adding the capability to modify multiple blocks. Unfortunately all controls are nested within block edit functions. If we do something like Block API: try separate controls registration, it will also fix this problem, because we can just get all the controls for a block type.

[jsnajdr]

what do you think about creating a separate issue to continue discussions regarding solving potential bugs with current setAttributes behavior?

Maybe #41260 could be the "canonical" bug for this? It is more directly related to the setAttributes behavior.

The effects in the Embed block can be tidied up: currently setAttributes is called even when no attributes are going to change -- it should be conditional. That alone will fix this bug.

The updateBlockAttributes has an uniqueByBlock flag/argument, which I believe was an attempt to fix this behavior.

It seems to me only weakly related. It's used by the Gallery block to loop through the child blocks, Images, and set attributes on them. It's a shortcut for registry.batch + loop + updateBlockAttributes (single).

[stronenv]

While it might be known already, sharing that this affects all embed blocks (Spotify, YouTube, etc.) and images wherever you drag them. Also, dragging them into a group or block with group-like behavior will have the same bug.

[chrisbellboy]

Hey everyone 👋

I've just created a PR to address the embed block's side of the problem, as it shouldn't be calling setAttributes() when remounted if no attributes have actually been changed.

Just to add another perspective on how to improve setAttributes(), I’m not sure introducing a new API is strictly necessary as the intention from a block calling it is already clear. Adding another layer of abstraction might complicate things unnecessarily, without any real benefits. Could we not explore tweaking setAttributes() itself to handle all cases more efficiently? It's an opportunity to further improve the UX too.

Here's a mini breakdown of the main (only?) use cases for setAttributes():

Single block selected:

• [No bugs] User wants to update one of its attributes - Working as expected, although entire properties are getting replaced, even if just one single nested property (e.g. layout.flexWrap) changed.

Multiple blocks selected of same block type e.g. Multiple Row blocks

• [No bugs] User updates a root level string attribute (e.g. fontSize) - works as expected.
• [Bug] User updates a single nested property (e.g. layout.flexWrap) - not working as expected, as the entire root property (layout) is taken from one block and applied to all selected blocks.
• [Bug] If one block inadvertently calls setAttributes() (e.g. as seen in this embed issue), all the attributes from one of those blocks is applied to all other selected blocks, causing unintended changes.

Multiple blocks selected of different block types e.g. Paragraph block and a Heading block

• [Bugs] If any block calls setAttributes(), an attempt is made to set its attributes to all other selected blocks, even if they’re of different types. E.g. as seen in this embed issue again.
• [Not currently possible] Users cannot change shared attributes (e.g. textColor) across different block types.

In an ideal world setAttributes() would be tweaked to handle:

1. Only update the exact property being changed. For object attributes, ensure only the specific nested property is updated without changing sibling properties in the object. That would fix Layout panel options cannot be edited across multiple blocks without data loss #41260 and all related issues.
2. Check if any attributes have actually changed before doing updates to prevent redundant changes.
3. When multiple blocks of different types are selected, show any shared controls, e.g. typography/color settings, in the block settings panel. This would improve the UX and allow users to easily update things across multiple block types simultaneously, similar to how it works for blocks of the same type.

I may be missing more use cases, but if I'm not, that would fix all the bugs and not impact any other existing code, or require any blocks to change their behaviour.

Just some thoughts 👍