From 32ac33a7b78b0019bdfaac5cc9832383f896ee20 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Mon, 22 Feb 2021 11:15:46 +0800 Subject: [PATCH] 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 | 5 +- packages/blocks/CHANGELOG.md | 4 + packages/blocks/README.md | 4 +- packages/blocks/src/api/factory.js | 38 ++++- packages/blocks/src/api/index.js | 1 + packages/blocks/src/api/test/factory.js | 5 +- .../specs/widgets/adding-widgets.test.js | 154 +++++++++++++++++- packages/edit-widgets/src/store/resolvers.js | 7 - 8 files changed, 197 insertions(+), 21 deletions(-) diff --git a/packages/block-editor/src/store/actions.js b/packages/block-editor/src/store/actions.js index cf69468a598a2..510a61c2b9cf5 100644 --- a/packages/block-editor/src/store/actions.js +++ b/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, diff --git a/packages/blocks/CHANGELOG.md b/packages/blocks/CHANGELOG.md index 82dffa4eeae62..6f3863f4d68d3 100644 --- a/packages/blocks/CHANGELOG.md +++ b/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 diff --git a/packages/blocks/README.md b/packages/blocks/README.md index 2f3e282cab2ca..ca094c6f148bf 100644 --- a/packages/blocks/README.md +++ b/packages/blocks/README.md @@ -183,8 +183,8 @@ In the random image block above, we've given the `alt` attribute of the image 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_ diff --git a/packages/blocks/src/api/factory.js b/packages/blocks/src/api/factory.js index ad0e9ca4f39a4..bb892d151f887 100644 --- a/packages/blocks/src/api/factory.js +++ b/packages/blocks/src/api/factory.js @@ -88,8 +88,8 @@ 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. @@ -97,7 +97,11 @@ export function createBlocksFromInnerBlocksTemplate( * * @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 ) ), diff --git a/packages/blocks/src/api/index.js b/packages/blocks/src/api/index.js index cedca94134077..50285a7640814 100644 --- a/packages/blocks/src/api/index.js +++ b/packages/blocks/src/api/index.js @@ -8,6 +8,7 @@ export { createBlock, createBlocksFromInnerBlocksTemplate, cloneBlock, + __experimentalCloneSanitizedBlock, getPossibleBlockTransformations, switchToBlockType, getBlockTransforms, diff --git a/packages/blocks/src/api/test/factory.js b/packages/blocks/src/api/test/factory.js index b503664b9e4da..a840f65135cc6 100644 --- a/packages/blocks/src/api/test/factory.js +++ b/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', } ); diff --git a/packages/e2e-tests/specs/widgets/adding-widgets.test.js b/packages/e2e-tests/specs/widgets/adding-widgets.test.js index 27f1c046dfc51..b392ac322d03c 100644 --- a/packages/e2e-tests/specs/widgets/adding-widgets.test.js +++ b/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": "
+

First Paragraph

+
", + } + ` ); + 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": "

First Paragraph

@@ -382,6 +403,108 @@ describe( 'Widgets screen', () => {

First Paragraph

", } + ` ); + 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": "

Search Title

+ + +
+
", + } ` ); } ); } ); @@ -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. diff --git a/packages/edit-widgets/src/store/resolvers.js b/packages/edit-widgets/src/store/resolvers.js index 33c032f00b55e..bdb93d6ec0947 100644 --- a/packages/edit-widgets/src/store/resolvers.js +++ b/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, - }; }