Skip to content

Commit

Permalink
Fix: Pasting captions without an image fails (#14365)
Browse files Browse the repository at this point in the history
* 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 `<img />` 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]<a href="some.link"><img src="some.image"></a>[/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
  • Loading branch information
dmsnell authored and gziolo committed Mar 18, 2019
1 parent 54a5f42 commit cafb041
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 8 deletions.
28 changes: 20 additions & 8 deletions packages/block-library/src/image/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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' ),

Expand Down Expand Up @@ -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 } ) => {
Expand Down
32 changes: 32 additions & 0 deletions packages/block-library/src/image/test/index.js
Original file line number Diff line number Diff line change
@@ -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: '<em>Tucson</em>' } } ) ).toEqual( '<em>Tucson</em>' );
} );

test( 'should strip out image when leading as expected', () => {
expect( stripFirstImage( {}, { shortcode: { content: '<img>' } } ) ).toEqual( '' );
expect( stripFirstImage( {}, { shortcode: { content: '<img>Image!' } } ) ).toEqual( 'Image!' );
expect( stripFirstImage( {}, { shortcode: { content: '<img src="image.png">Image!' } } ) ).toEqual( 'Image!' );
} );

test( 'should strip out image when not in leading position as expected', () => {
expect( stripFirstImage( {}, { shortcode: { content: 'Before<img>' } } ) ).toEqual( 'Before' );
expect( stripFirstImage( {}, { shortcode: { content: 'Before<img>Image!' } } ) ).toEqual( 'BeforeImage!' );
expect( stripFirstImage( {}, { shortcode: { content: 'Before<img src="image.png">Image!' } } ) ).toEqual( 'BeforeImage!' );
} );

test( 'should strip out only the first of many images', () => {
expect( stripFirstImage( {}, { shortcode: { content: '<img><img>' } } ) ).toEqual( '<img>' );
} );

test( 'should strip out the first image and its wrapping parents', () => {
expect( stripFirstImage( {}, { shortcode: { content: '<p><a><img></a></p><p><img></p>' } } ) ).toEqual( '<p><img></p>' );
} );
} );

0 comments on commit cafb041

Please sign in to comment.