From cafb041d6d8f0d592bde0bba1083ea4c32480728 Mon Sep 17 00:00:00 2001 From: Dennis Snell Date: Sun, 17 Mar 2019 23:44:41 -0700 Subject: [PATCH] Fix: Pasting captions without an image fails (#14365) * Fix: Pasting captions without an image fails Fixes #13527 See original work in #12315 When pasting content which includes the `[caption]` shortcode we assume that the content is well-formed (that there is not only an `` in there but also in the first position). In this patch we fix the problem by removing the old code, which removed the first `Element` node, and replaced it with code that removes the first `IMG` node _if one is found_. We're leaving other image nodes in place in case the caption contains image nodes and we're not requiring that the `IMG` be the first child of the caption shortcode in case people are wrapping the image in other valid HTML like this... ```html [caption][/caption] ``` See the new unit tests for a more complete specification of the intended behaviors. PR reviewed, debugged, and created by: -> LANNISTER MOB <- - @codebykat - @dmsnell - @gwwar - @kwight - @mmtr - @obenland - @rodrigoi - @vindl * Update stripFirstImage behavior to also remove matching topmost parent node --- packages/block-library/src/image/index.js | 28 +++++++++++----- .../block-library/src/image/test/index.js | 32 +++++++++++++++++++ 2 files changed, 52 insertions(+), 8 deletions(-) create mode 100644 packages/block-library/src/image/test/index.js diff --git a/packages/block-library/src/image/index.js b/packages/block-library/src/image/index.js index 92b9af7ec9a42c..ace6b55f31636b 100644 --- a/packages/block-library/src/image/index.js +++ b/packages/block-library/src/image/index.js @@ -123,6 +123,25 @@ function getFirstAnchorAttributeFormHTML( html, attributeName ) { } } +export function stripFirstImage( attributes, { shortcode } ) { + const { body } = document.implementation.createHTMLDocument( '' ); + + body.innerHTML = shortcode.content; + + let nodeToRemove = body.querySelector( 'img' ); + + // if an image has parents, find the topmost node to remove + while ( nodeToRemove && nodeToRemove.parentNode && nodeToRemove.parentNode !== body ) { + nodeToRemove = nodeToRemove.parentNode; + } + + if ( nodeToRemove ) { + nodeToRemove.parentNode.removeChild( nodeToRemove ); + } + + return body.innerHTML.trim(); +} + export const settings = { title: __( 'Image' ), @@ -196,14 +215,7 @@ export const settings = { selector: 'img', }, caption: { - shortcode: ( attributes, { shortcode } ) => { - const { body } = document.implementation.createHTMLDocument( '' ); - - body.innerHTML = shortcode.content; - body.removeChild( body.firstElementChild ); - - return body.innerHTML.trim(); - }, + shortcode: stripFirstImage, }, href: { shortcode: ( attributes, { shortcode } ) => { diff --git a/packages/block-library/src/image/test/index.js b/packages/block-library/src/image/test/index.js new file mode 100644 index 00000000000000..5b21dcefdf6ebd --- /dev/null +++ b/packages/block-library/src/image/test/index.js @@ -0,0 +1,32 @@ +/** + * Internal dependencies + */ +import { stripFirstImage } from '../'; + +describe( 'stripFirstImage', () => { + test( 'should do nothing if no image is present', () => { + expect( stripFirstImage( {}, { shortcode: { content: '' } } ) ).toEqual( '' ); + expect( stripFirstImage( {}, { shortcode: { content: 'Tucson' } } ) ).toEqual( 'Tucson' ); + expect( stripFirstImage( {}, { shortcode: { content: 'Tucson' } } ) ).toEqual( 'Tucson' ); + } ); + + test( 'should strip out image when leading as expected', () => { + expect( stripFirstImage( {}, { shortcode: { content: '' } } ) ).toEqual( '' ); + expect( stripFirstImage( {}, { shortcode: { content: 'Image!' } } ) ).toEqual( 'Image!' ); + expect( stripFirstImage( {}, { shortcode: { content: 'Image!' } } ) ).toEqual( 'Image!' ); + } ); + + test( 'should strip out image when not in leading position as expected', () => { + expect( stripFirstImage( {}, { shortcode: { content: 'Before' } } ) ).toEqual( 'Before' ); + expect( stripFirstImage( {}, { shortcode: { content: 'BeforeImage!' } } ) ).toEqual( 'BeforeImage!' ); + expect( stripFirstImage( {}, { shortcode: { content: 'BeforeImage!' } } ) ).toEqual( 'BeforeImage!' ); + } ); + + test( 'should strip out only the first of many images', () => { + expect( stripFirstImage( {}, { shortcode: { content: '' } } ) ).toEqual( '' ); + } ); + + test( 'should strip out the first image and its wrapping parents', () => { + expect( stripFirstImage( {}, { shortcode: { content: '

' } } ) ).toEqual( '

' ); + } ); +} );