Block Fields: Cover block issues

[talldan]

This tracks some issues with the cover block related to the 'Block Fields' experiment.

A prerequisite for testing is to enable the 'Block Fields' experiment on the Gutenberg experiment page.

1. backgroundType not set correctly

After #74575, there's a small issue with the cover block. The backgroundType property is not set correctly when adding a video using the media field in the inspector.

I believe this line is the problem:

gutenberg/packages/block-library/src/cover/index.js

Line 88 in b06f167

mediaType: value.backgroundType,

It should read:

backgroundType: value.mediaType, 

It's probably worth rolling this fix into other cover block fixes.

2. Overlay opacity and text color (isDark) are not set correctly when adding new images

Steps to reproduce:

1. Add a cover block
2. From the inspector content tab, upload an image
3. Note that the image uploads isn't visible in the block. In the style tab the block still has 100 opacity

Cause:
I think that the issue is that the block has some internal logic for calculating the dimRatio and isDark -

gutenberg/packages/block-library/src/cover/edit/index.js

Lines 194 to 261 in b06f167

	const onSelectMedia = async ( newMedia ) => {
	const mediaAttributes = attributesFromMedia( newMedia );
	const isImage = [ newMedia?.type, newMedia?.media_type ].includes(
	IMAGE_BACKGROUND_TYPE
	);
	const averageBackgroundColor = await getMediaColor(
	isImage ? newMedia?.url : undefined
	);
	let newOverlayColor = overlayColor.color;
	if ( ! isUserOverlayColor ) {
	newOverlayColor = averageBackgroundColor;
	setOverlayColor( newOverlayColor );
	// Make undo revert the next setAttributes and the previous setOverlayColor.
	__unstableMarkNextChangeAsNotPersistent();
	}
	// Only set a new dimRatio if there was no previous media selected
	// to avoid resetting to 50 if it has been explicitly set to 100.
	// See issue #52835 for context.
	const newDimRatio =
	originalUrl === undefined && dimRatio === 100 ? 50 : dimRatio;
	const newIsDark = compositeIsDark(
	newDimRatio,
	newOverlayColor,
	averageBackgroundColor
	);
	if ( backgroundType === IMAGE_BACKGROUND_TYPE && mediaAttributes?.id ) {
	const { imageDefaultSize } = getSettings();
	// Try to use the previous selected image size if it's available
	// otherwise try the default image size or fallback to full size.
	if (
	sizeSlug &&
	( newMedia?.sizes?.[ sizeSlug ] ||
	newMedia?.media_details?.sizes?.[ sizeSlug ] )
	) {
	mediaAttributes.sizeSlug = sizeSlug;
	mediaAttributes.url =
	newMedia?.sizes?.[ sizeSlug ]?.url ||
	newMedia?.media_details?.sizes?.[ sizeSlug ]?.source_url;
	} else if (
	newMedia?.sizes?.[ imageDefaultSize ] ||
	newMedia?.media_details?.sizes?.[ imageDefaultSize ]
	) {
	mediaAttributes.sizeSlug = imageDefaultSize;
	mediaAttributes.url =
	newMedia?.sizes?.[ imageDefaultSize ]?.url ||
	newMedia?.media_details?.sizes?.[ imageDefaultSize ]
	?.source_url;
	} else {
	mediaAttributes.sizeSlug = DEFAULT_MEDIA_SIZE_SLUG;
	}
	}
	setAttributes( {
	...mediaAttributes,
	focalPoint: undefined,
	useFeaturedImage: undefined,
	dimRatio: newDimRatio,
	isDark: newIsDark,
	isUserOverlayColor: isUserOverlayColor || false,
	} );
	};

This code executes whenever uploading new images from the block, but not if some external code (like the Block Fields) tries to set a new image. A fix might be to move the code into a useEffect.

[harshbhonsle]

Hi, I’d like to help out with this issue. Can I take this one on and start working on a fix?

[talldan]

You're welcome to, is there enough information to get started?

[harshbhonsle]

Yes, there’s enough information here to get started.

I’ll fix the property mapping in packages/block-library/src/cover/index.js so that backgroundType is correctly set from mediaType.

For the second issue, I’ll move the overlay/dimRatio recalculation logic into a useEffect in packages/block-library/src/cover/edit/index.js, so it runs whenever the image URL or backgroundType changes, not just on direct uploads.

I’ll open a PR with these changes once I’ve tested them locally. Would you prefer I keep both fixes in one commit, or split them into separate commits for clarity? Does this approach sound good to you?

[ockham]

👋 Heads-up that I'm planning to extract the color computation logic (isDark, overlay color, etc) from event handlers and move them into useEffect, to ensure that they're run regardless of what event prompts them. I've started working on this as part of #74610 (mostly in 1839720), but I'll most likely move it to a separate PR.

Since the color computation logic is kind of finicky, I'd like to prepare for this by adding e2e coverage for a number of bugfixes to the Cover block that aren't currently guarded against regression. That work is ongoing in #75483.

I hope to land the e2e tests PR soon. Maybe the prospective useEffect PR will be able to fix the bugs described in this issue (in which case we should also add e2e test coverage to guard against regressions).

[harshbhonsle]

Hi @ockham,
I noticed you’re working on the Cover block issue (#74639). Since you’re already handling the color computation logic and planning to move it into a useEffect, let me know if I can help you out with something!

[ockham]

Just noting here that this would undo a previous change where code was deliberately moved out of useEffect and into event handlers, see #53253 (comment) 🤔

cc/ @Mamaduka

[Mamaduka]

@ockham, I think some recent PRs re-introduced the similar useEffect. It's run conditionally when a block uses a feature image, which would be a similar case for bindings - an image from an external source.

Calculating the overlay from the image is probably the most error-prone part of this block. Here are some recent bug reports: #74706 and #74976.