Embed block: Fix inline preview cut-off when editing URL (#35326)

* Use empty classname when handling incoming preview * Force sandbox render when type prop changes * Add ignore previous classname option to merged attributes * Move get merged attributes function to util class
WordPress · Apr 19, 2022 · 49bf72b · 49bf72b
1 parent 4d503c4
commit 49bf72b
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 40 deletions.
diff --git a/packages/block-library/src/embed/edit.js b/packages/block-library/src/embed/edit.js
@@ -5,8 +5,8 @@ import {
 	createUpgradedEmbedBlock,
 	getClassNames,
 	fallback,
-	getAttributesFromPreview,
 	getEmbedInfoByProvider,
+	getMergedAttributesWithPreview,
 } from './util';
 import EmbedControls from './embed-controls';
 import { embedContentIcon } from './icons';
@@ -99,21 +99,19 @@ const EmbedEdit = ( props ) => {
 	);
 
 	/**
-	 * @return {Object} Attributes derived from the preview, merged with the current attributes.
+	 * Returns the attributes derived from the preview, merged with the current attributes.
+	 *
+	 * @param {boolean} ignorePreviousClassName Determines if the previous className attribute should be ignored when merging.
+	 * @return {Object} Merged attributes.
 	 */
-	const getMergedAttributes = () => {
-		const { allowResponsive, className } = attributes;
-		return {
-			...attributes,
-			...getAttributesFromPreview(
-				preview,
-				title,
-				className,
-				responsive,
-				allowResponsive
-			),
-		};
-	};
+	const getMergedAttributes = ( ignorePreviousClassName = false ) =>
+		getMergedAttributesWithPreview(
+			attributes,
+			preview,
+			title,
+			responsive,
+			ignorePreviousClassName
+		);
 
 	const toggleResponsive = () => {
 		const { allowResponsive, className } = attributes;
@@ -145,15 +143,12 @@ const EmbedEdit = ( props ) => {
 	// Handle incoming preview.
 	useEffect( () => {
 		if ( preview && ! isEditingURL ) {
-			// Even though we set attributes that get derived from the preview,
-			// we don't access them directly because for the initial render,
-			// the `setAttributes` call will not have taken effect. If we're
-			// rendering responsive content, setting the responsive classes
-			// after the preview has been rendered can result in unwanted
-			// clipping or scrollbars. The `getAttributesFromPreview` function
-			// that `getMergedAttributes` uses is memoized so that we're not
-			// calculating them on every render.
-			setAttributes( getMergedAttributes() );
+			// When obtaining an incoming preview, we set the attributes derived from
+			// the preview data. In this case when getting the merged attributes,
+			// we ignore the previous classname because it might not match the expected
+			// classes by the new preview.
+			setAttributes( getMergedAttributes( true ) );
+
 			if ( onReplace ) {
 				const upgradedBlock = createUpgradedEmbedBlock(
 					props,

diff --git a/packages/block-library/src/embed/edit.native.js b/packages/block-library/src/embed/edit.native.js
@@ -5,8 +5,8 @@ import {
 	createUpgradedEmbedBlock,
 	getClassNames,
 	fallback,
-	getAttributesFromPreview,
 	getEmbedInfoByProvider,
+	getMergedAttributesWithPreview,
 } from './util';
 import EmbedControls from './embed-controls';
 import { embedContentIcon } from './icons';
@@ -130,19 +130,14 @@ const EmbedEdit = ( props ) => {
 	 * @param {boolean} ignorePreviousClassName Determines if the previous className attribute should be ignored when merging.
 	 * @return {Object} Merged attributes.
 	 */
-	const getMergedAttributes = ( ignorePreviousClassName = false ) => {
-		const { allowResponsive, className } = attributes;
-		return {
-			...attributes,
-			...getAttributesFromPreview(
-				preview,
-				title,
-				ignorePreviousClassName ? undefined : className,
-				responsive,
-				allowResponsive
-			),
-		};
-	};
+	const getMergedAttributes = ( ignorePreviousClassName = false ) =>
+		getMergedAttributesWithPreview(
+			attributes,
+			preview,
+			title,
+			responsive,
+			ignorePreviousClassName
+		);
 
 	const toggleResponsive = () => {
 		const { allowResponsive, className } = attributes;

diff --git a/packages/block-library/src/embed/util.js b/packages/block-library/src/embed/util.js
@@ -292,3 +292,33 @@ export const getAttributesFromPreview = memoize(
 		return attributes;
 	}
 );
+
+/**
+ * Returns the attributes derived from the preview, merged with the current attributes.
+ *
+ * @param {Object}  currentAttributes       The current attributes of the block.
+ * @param {Object}  preview                 The preview data.
+ * @param {string}  title                   The block's title, e.g. Twitter.
+ * @param {boolean} isResponsive            Boolean indicating if the block supports responsive content.
+ * @param {boolean} ignorePreviousClassName Determines if the previous className attribute should be ignored when merging.
+ * @return {Object} Merged attributes.
+ */
+export const getMergedAttributesWithPreview = (
+	currentAttributes,
+	preview,
+	title,
+	isResponsive,
+	ignorePreviousClassName = false
+) => {
+	const { allowResponsive, className } = currentAttributes;
+	return {
+		...currentAttributes,
+		...getAttributesFromPreview(
+			preview,
+			title,
+			ignorePreviousClassName ? undefined : className,
+			isResponsive,
+			allowResponsive
+		),
+	};
+};
diff --git a/packages/components/src/sandbox/index.js b/packages/components/src/sandbox/index.js
@@ -227,11 +227,11 @@ export default function Sandbox( {
 
 	useEffect( () => {
 		trySandbox();
-	}, [ title, type, styles, scripts ] );
+	}, [ title, styles, scripts ] );
 
 	useEffect( () => {
 		trySandbox( true );
-	}, [ html ] );
+	}, [ html, type ] );
 
 	return (
 		<iframe