Fix legacy widgets not previewing and widgets saving issue (#29111)

* Fix editing blocks in widgets screen by introducing duplicateBlock in useBlockSync

* Update e2e test

* Add test for displaying legacy widgets

* Fix test

* Rename to __experimentalCloneSanitizedBlock

* Update changelog

packages/block-editor/src/store/actions.js

@@ -8,6 +8,7 @@ import { castArray, findKey, first, last, some } from 'lodash';
  */
 import {
 	cloneBlock,
+	__experimentalCloneSanitizedBlock,
 	createBlock,
 	doBlocksMatchTemplate,
 	getBlockType,
@@ -1223,7 +1224,9 @@ export function* duplicateBlocks( clientIds, updateSelection = true ) {
 		last( castArray( clientIds ) ),
 		rootClientId
 	);
-	const clonedBlocks = blocks.map( ( block ) => cloneBlock( block ) );
+	const clonedBlocks = blocks.map( ( block ) =>
+		__experimentalCloneSanitizedBlock( block )
+	);
 	yield insertBlocks(
 		clonedBlocks,
 		lastSelectedIndex + 1,

packages/blocks/CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+### Breaking Change
+
+-   Reverted `cloneBlock` back to its original logic that doesn't sanitize block's attributes. [#28379](https://github.com/WordPress/gutenberg/pull/29111)
+
 ## 7.0.0 (2021-02-01)
 
 ### Breaking Change

packages/blocks/README.md

@@ -183,8 +183,8 @@ In the random image block above, we've given the `alt` attribute of the image a
 
 <a name="cloneBlock" href="#cloneBlock">#</a> **cloneBlock**
 
-Given a block object, returns a copy of the block object, optionally merging
-new attributes and/or replacing its inner blocks.
+Given a block object, returns a copy of the block object,
+optionally merging new attributes and/or replacing its inner blocks.
 
 _Parameters_
 

packages/blocks/src/api/factory.js

@@ -88,16 +88,20 @@ export function createBlocksFromInnerBlocksTemplate(
 }
 
 /**
- * Given a block object, returns a copy of the block object, optionally merging
- * new attributes and/or replacing its inner blocks.
+ * Given a block object, returns a copy of the block object while sanitizing its attributes,
+ * optionally merging new attributes and/or replacing its inner blocks.
  *
  * @param {Object} block              Block instance.
  * @param {Object} mergeAttributes    Block attributes.
  * @param {?Array} newInnerBlocks     Nested blocks.
  *
  * @return {Object} A cloned block.
  */
-export function cloneBlock( block, mergeAttributes = {}, newInnerBlocks ) {
+export function __experimentalCloneSanitizedBlock(
+	block,
+	mergeAttributes = {},
+	newInnerBlocks
+) {
 	const clientId = uuid();
 
 	const sanitizedAttributes = sanitizeBlockAttributes( block.name, {
@@ -109,6 +113,34 @@ export function cloneBlock( block, mergeAttributes = {}, newInnerBlocks ) {
 		...block,
 		clientId,
 		attributes: sanitizedAttributes,
+		innerBlocks:
+			newInnerBlocks ||
+			block.innerBlocks.map( ( innerBlock ) =>
+				__experimentalCloneSanitizedBlock( innerBlock )
+			),
+	};
+}
+
+/**
+ * Given a block object, returns a copy of the block object,
+ * optionally merging new attributes and/or replacing its inner blocks.
+ *
+ * @param {Object} block              Block instance.
+ * @param {Object} mergeAttributes    Block attributes.
+ * @param {?Array} newInnerBlocks     Nested blocks.
+ *
+ * @return {Object} A cloned block.
+ */
+export function cloneBlock( block, mergeAttributes = {}, newInnerBlocks ) {
+	const clientId = uuid();
+
+	return {
+		...block,
+		clientId,
+		attributes: {
+			...block.attributes,
+			...mergeAttributes,
+		},
 		innerBlocks:
 			newInnerBlocks ||
 			block.innerBlocks.map( ( innerBlock ) => cloneBlock( innerBlock ) ),

packages/blocks/src/api/index.js

@@ -8,6 +8,7 @@ export {
 	createBlock,
 	createBlocksFromInnerBlocksTemplate,
 	cloneBlock,
+	__experimentalCloneSanitizedBlock,
 	getPossibleBlockTransformations,
 	switchToBlockType,
 	getBlockTransforms,

packages/blocks/src/api/test/factory.js

@@ -11,6 +11,7 @@ import {
 	createBlock,
 	createBlocksFromInnerBlocksTemplate,
 	cloneBlock,
+	__experimentalCloneSanitizedBlock,
 	getPossibleBlockTransformations,
 	switchToBlockType,
 	getBlockTransforms,
@@ -424,7 +425,9 @@ describe( 'block factory', () => {
 				block.innerBlocks[ 1 ].attributes
 			);
 		} );
+	} );
 
+	describe( '__experimentalCloneSanitizedBlock', () => {
 		it( 'should sanitize attributes not defined in the block type', () => {
 			registerBlockType( 'core/test-block', {
 				...defaultBlockSettings,
@@ -439,7 +442,7 @@ describe( 'block factory', () => {
 				notDefined: 'not-defined',
 			} );
 
-			const clonedBlock = cloneBlock( block, {
+			const clonedBlock = __experimentalCloneSanitizedBlock( block, {
 				notDefined2: 'not-defined-2',
 			} );
 

packages/e2e-tests/specs/widgets/adding-widgets.test.js

@@ -306,22 +306,43 @@ describe( 'Widgets screen', () => {
 	} );
 
 	it( 'Should duplicate the widgets', async () => {
-		const firstWidgetArea = await page.$(
+		let firstWidgetArea = await page.$(
 			'[aria-label="Block: Widget Area"][role="group"]'
 		);
 
 		const addParagraphBlock = await getParagraphBlockInGlobalInserter();
 		await addParagraphBlock.click();
 
-		const firstParagraphBlock = await firstWidgetArea.$(
+		let firstParagraphBlock = await firstWidgetArea.$(
 			'[data-block][data-type="core/paragraph"][aria-label^="Empty block"]'
 		);
 		await page.keyboard.type( 'First Paragraph' );
 
+		await saveWidgets();
+		await page.reload();
+		// Wait for the widget areas to load.
+		firstWidgetArea = await page.waitForSelector(
+			'[aria-label="Block: Widget Area"][role="group"]'
+		);
+
+		const initialSerializedWidgetAreas = await getSerializedWidgetAreas();
+		expect( initialSerializedWidgetAreas ).toMatchInlineSnapshot( `
+		Object {
+		  "sidebar-1": "<div class=\\"widget widget_block widget_text\\"><div class=\\"widget-content\\">
+		<p>First Paragraph</p>
+		</div></div>",
+		}
+	` );
+		const initialWidgets = await getWidgetAreaWidgets();
+		expect( initialWidgets[ 'sidebar-1' ].length ).toBe( 1 );
+
+		firstParagraphBlock = await firstWidgetArea.$(
+			'[data-block][data-type="core/paragraph"]'
+		);
+		await firstParagraphBlock.focus();
+
 		// Trigger the toolbar to appear.
-		await page.keyboard.down( 'Shift' );
-		await page.keyboard.press( 'Tab' );
-		await page.keyboard.up( 'Shift' );
+		await pressKeyWithModifier( 'shift', 'Tab' );
 
 		const blockToolbar = await page.waitForSelector(
 			'[role="toolbar"][aria-label="Block tools"]'
@@ -372,8 +393,8 @@ describe( 'Widgets screen', () => {
 		).toBe( true );
 
 		await saveWidgets();
-		const serializedWidgetAreas = await getSerializedWidgetAreas();
-		expect( serializedWidgetAreas ).toMatchInlineSnapshot( `
+		const editedSerializedWidgetAreas = await getSerializedWidgetAreas();
+		expect( editedSerializedWidgetAreas ).toMatchInlineSnapshot( `
 		Object {
 		  "sidebar-1": "<div class=\\"widget widget_block\\"><div class=\\"widget-content\\">
 		<p>First Paragraph</p>
@@ -382,6 +403,108 @@ describe( 'Widgets screen', () => {
 		<p>First Paragraph</p>
 		</div></div>",
 		}
+	` );
+		const editedWidgets = await getWidgetAreaWidgets();
+		expect( editedWidgets[ 'sidebar-1' ].length ).toBe( 2 );
+		expect( editedWidgets[ 'sidebar-1' ][ 0 ] ).toBe(
+			initialWidgets[ 'sidebar-1' ][ 0 ]
+		);
+	} );
+
+	it( 'Should display legacy widgets', async () => {
+		await visitAdminPage( 'widgets.php' );
+
+		const [ searchWidget ] = await page.$x(
+			'//*[@id="widget-list"]//h3[text()="Search"]'
+		);
+		await searchWidget.click();
+
+		const [ addWidgetButton ] = await page.$x(
+			'//button[text()="Add Widget"]'
+		);
+		await addWidgetButton.click();
+
+		// Wait for the changes to be saved.
+		// TODO: Might have better ways to do this.
+		await page.waitForFunction( () => {
+			const addedSearchWidget = document.querySelector(
+				'#widgets-right .widget'
+			);
+			const spinner = addedSearchWidget.querySelector( '.spinner' );
+
+			return (
+				addedSearchWidget.classList.contains( 'open' ) &&
+				! spinner.classList.contains( 'is-active' )
+			);
+		} );
+		// FIXME: For some reasons, waiting for the spinner to disappear is not enough.
+		// eslint-disable-next-line no-restricted-syntax
+		await page.waitForTimeout( 500 );
+
+		await visitAdminPage( 'themes.php', 'page=gutenberg-widgets' );
+		// Wait for the widget areas to load.
+		await page.waitForSelector(
+			'[aria-label="Block: Widget Area"][role="group"]'
+		);
+
+		const legacyWidget = await page.waitForSelector(
+			'[data-block][data-type="core/legacy-widget"]'
+		);
+
+		const legacyWidgetName = await legacyWidget.$( 'h3' );
+		expect(
+			await legacyWidgetName.evaluate( ( node ) => node.textContent )
+		).toBe( 'Search' );
+
+		await legacyWidget.focus();
+
+		// Trigger the toolbar to appear.
+		await pressKeyWithModifier( 'shift', 'Tab' );
+
+		let [ previewButton ] = await page.$x(
+			'//button[*[contains(text(), "Preview")]]'
+		);
+		await previewButton.click();
+
+		const iframe = await legacyWidget.$( 'iframe' );
+		const frame = await iframe.contentFrame();
+
+		// Expect to have search input.
+		await frame.waitForSelector( 'input[type="search"]' );
+
+		const [ editButton ] = await page.$x(
+			'//button[*[contains(text(), "Edit")]]'
+		);
+		await editButton.click();
+
+		// TODO: Should query this with role and label.
+		const titleInput = await legacyWidget.$( 'input' );
+		await titleInput.type( 'Search Title' );
+
+		// Trigger the toolbar to appear.
+		await pressKeyWithModifier( 'shift', 'Tab' );
+
+		[ previewButton ] = await page.$x(
+			'//button[*[contains(text(), "Preview")]]'
+		);
+		await previewButton.click();
+
+		// Expect to have search title.
+		await frame.waitForXPath( '//h2[text()="Search Title"]' );
+
+		await saveWidgets();
+		const serializedWidgetAreas = await getSerializedWidgetAreas();
+		expect( serializedWidgetAreas ).toMatchInlineSnapshot( `
+		Object {
+		  "sidebar-1": "<div class=\\"widget widget_search\\"><div class=\\"widget-content\\"><h2 class=\\"widget-title subheading heading-size-3\\">Search Title</h2><form role=\\"search\\"  method=\\"get\\" class=\\"search-form\\" action=\\"http://localhost:8889/\\">
+			<label for=\\"search-form-1\\">
+				<span class=\\"screen-reader-text\\">Search for:</span>
+				<input type=\\"search\\" id=\\"search-form-1\\" class=\\"search-field\\" placeholder=\\"Search &hellip;\\" value=\\"\\" name=\\"s\\" />
+			</label>
+			<input type=\\"submit\\" class=\\"search-submit\\" value=\\"Search\\" />
+		</form>
+		</div></div>",
+		}
 	` );
 	} );
 } );
@@ -415,6 +538,23 @@ async function getSerializedWidgetAreas() {
 	return serializedWidgetAreas;
 }
 
+async function getWidgetAreaWidgets() {
+	const widgets = await page.evaluate( () => {
+		const widgetAreas = wp.data
+			.select( 'core/edit-widgets' )
+			.getWidgetAreas();
+		const sidebars = {};
+
+		for ( const widgetArea of widgetAreas ) {
+			sidebars[ widgetArea.id ] = widgetArea.widgets;
+		}
+
+		return sidebars;
+	} );
+
+	return widgets;
+}
+
 /**
  * TODO: Deleting widgets in the new widgets screen seems to be unreliable.
  * We visit the old widgets screen to delete them.

packages/edit-widgets/src/store/resolvers.js

@@ -75,12 +75,10 @@ export function* getWidgets() {
 		query
 	);
 
-	const widgetIdToClientId = {};
 	const groupedBySidebar = {};
 
 	for ( const widget of widgets ) {
 		const block = transformWidgetToBlock( widget );
-		widgetIdToClientId[ widget.id ] = block.clientId;
 		groupedBySidebar[ widget.sidebar ] =
 			groupedBySidebar[ widget.sidebar ] || [];
 		groupedBySidebar[ widget.sidebar ].push( block );
@@ -95,9 +93,4 @@ export function* getWidgets() {
 			);
 		}
 	}
-
-	yield {
-		type: 'SET_WIDGET_TO_CLIENT_ID_MAPPING',
-		mapping: widgetIdToClientId,
-	};
 }