Skip to content

/gutenberg

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

Reviewers

@gziolo gziolo

@youknowriad youknowriad

@noisysocks noisysocks

@talldan talldan

@Soean Soean

@ajitbohra ajitbohra

@notnownikki notnownikki

@ellatrix ellatrix

@jorgefilipecosta jorgefilipecosta

Assignees
No one assigned
Labels
Good First Review [Block] Image [Component] Raw Handling
Projects
None yet
Milestone
  5.3 (Gutenberg)
3 participants
@dmsnell @gwwar @gziolo
@dmsnell
Copy link
Contributor

commented Mar 10, 2019 •
edited by gwwar

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

@dmsnell dmsnell requested a review from ellatrix Mar 10, 2019

@dmsnell dmsnell requested review from ajitbohra, gziolo, jorgefilipecosta, noisysocks, notnownikki, Soean, talldan and youknowriad as code owners Mar 10, 2019

@dmsnell dmsnell added [Status] In Progress [Component] Raw Handling [Block] Image labels Mar 10, 2019

@dmsnell
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
Loading status checks…
3518cdd

@dmsnell dmsnell force-pushed the fix/13527-captions-missing-image-on-paste branch from 5a6ac0c to 3518cdd Mar 11, 2019

@dmsnell dmsnell removed the [Status] In Progress label Mar 11, 2019

@youknowriad youknowriad referenced this pull request Mar 11, 2019

Closed

Avoid JS error when pasing a caption without image #13557

@gziolo gziolo added this to the 5.3 (Gutenberg) milestone Mar 11, 2019

@gziolo gziolo added the Good First Review label Mar 11, 2019

@gziolo

gziolo approved these changes Mar 11, 2019
View changes

Copy link
Member

left a comment

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

gziolo requested changes Mar 11, 2019
View changes

Copy link
Member

left a comment

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
Update stripFirstImage behavior to also remove matching topmost paren… 
…t node
Verified
This commit was signed with a verified signature.
@gwwar gwwar Kerry Liu
GPG key ID: 8D75EC748751D990 Learn about signing commits
Loading status checks…
eb79d92
@gwwar

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Mar 15, 2019

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

gziolo approved these changes Mar 18, 2019
View changes

@gziolo

This comment has been minimized.

Sign in to view
Copy link
Member

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 Mar 18, 2019

This was referenced Mar 18, 2019

Merged
Merged
Closed

@jorgefilipecosta jorgefilipecosta referenced this pull request Mar 20, 2019

Closed

Update WordPress trunk to Gutenberg 5.3 #14532

youknowriad added a commit that referenced this pull request Mar 20, 2019

@dmsnell @youknowriad
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 `<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
c69c6b3

youknowriad added a commit that referenced this pull request Mar 20, 2019

@dmsnell @youknowriad
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 `<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
993f8c4

mkevins added a commit to mkevins/gutenberg that referenced this pull request Mar 26, 2019

@dmsnell @mkevins
Fix: Pasting captions without an image fails (WordPress#14365) 
* Fix: Pasting captions without an image fails

Fixes WordPress#13527
See original work in WordPress#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
bba120a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.