Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Pasting captions without an image fails #14365

Merged
merged 2 commits into from
Mar 18, 2019

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented Mar 10, 2019

Fixes #13527
See original work in #12315
See reduced input that produces the problem, shortened from the original.

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...

[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 <-

Testing

Confirm that the new unit tests pass:

npm run test-unit packages/block-library/src/image
npm run test test/integration/blocks-raw-handling.spec.js

Also test by pasting in the input text which triggers the bug. In master it
should fail as described with the error in the console but in this
branch it should paste as expected without error and an image block
should appear.

Finally verify that no regressions occur when converting a classic block
which contains images with the [caption] shortcode. This was the goal
of #12315 when it was merged.

ReallyLongPostPaste mov

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
@dmsnell dmsnell force-pushed the fix/13527-captions-missing-image-on-paste branch from 5a6ac0c to 3518cdd Compare March 11, 2019 00:25
@dmsnell dmsnell removed the [Status] In Progress Tracking issues with work in progress label Mar 11, 2019
@gziolo gziolo added this to the 5.3 (Gutenberg) milestone Mar 11, 2019
@gziolo gziolo added the Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code label Mar 11, 2019
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome teamwork Lannister! Thanks for fixing it with the unit test included. I'd love to see more PRs like this in the future :)

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too soon. There is a failing unit test:

FAIL test/integration/blocks-raw-handling.spec.js (12.199s)
  ● rawHandler › should convert a caption shortcode with link
    expect(value).toMatchSnapshot()
    Received value does not match stored snapshot "rawHandler should convert a caption shortcode with link 1".
    - Snapshot
    + Received
      "<!-- wp:image {"id":754,"align":"none"} -->
    - <figure class="wp-block-image alignnone"><a href="http://build.wordpress-develop.test/wp-content/uploads/2011/07/100_5478.jpg"><img src="http://build.wordpress-develop.test/wp-content/uploads/2011/07/100_5478.jpg?w=604" alt="Bell on Wharf" class="wp-image-754"/></a><figcaption>Bell on wharf in San Francisco</figcaption></figure>
    + <figure class="wp-block-image alignnone"><a href="http://build.wordpress-develop.test/wp-content/uploads/2011/07/100_5478.jpg"><img src="http://build.wordpress-develop.test/wp-content/uploads/2011/07/100_5478.jpg?w=604" alt="Bell on Wharf" class="wp-image-754"/></a><figcaption><a href="http://build.wordpress-develop.test/wp-content/uploads/2011/07/100_5478.jpg"></a> Bell on wharf in San Francisco</figcaption></figure>
      <!-- /wp:image -->"
      271 | 	it( 'should convert a caption shortcode with link', () => {
      272 | 		const HTML = readFile( path.join( __dirname, 'fixtures/shortcode-caption-with-link.html' ) );
    > 273 | 		expect( serialize( rawHandler( { HTML } ) ) ).toMatchSnapshot();
          | 		                                              ^
      274 | 	} );
      275 | 
      276 | 	it( 'should convert a caption shortcode with caption', () => {
      at Object.toMatchSnapshot (test/integration/blocks-raw-handling.spec.js:273:49)

It looks like image should be removed together wit all wrapping html tags like anchor in this case.

@gwwar
Copy link
Contributor

gwwar commented Mar 15, 2019

@gziolo this is ready for another look. I also added another unit test to account for the extra case.

@gziolo
Copy link
Member

gziolo commented Mar 18, 2019

Thanks for addressing my comment. This change fixes this newly introduced side effect of improved handling 👍

@gziolo gziolo merged commit cafb041 into master Mar 18, 2019
@gziolo gziolo deleted the fix/13527-captions-missing-image-on-paste branch March 18, 2019 06:44
youknowriad pushed a commit that referenced this pull request Mar 20, 2019
* 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
youknowriad pushed a commit that referenced this pull request Mar 20, 2019
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Image Affects the Image Block [Feature] Paste Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JS Error when pasting a post (shortcode related)
3 participants