Fix converting caption shortcode with link #12315

ellatrix · 2018-11-26T14:05:52Z

Description

Avoids a duplicate image when converting from a classic block.

Fixes #12310.

Testing Instructions

Create a classic post
Insert an image.
Add a caption, AND link to the media file
Publish the classic post
Edit the post in the block editor
Convert the classic block to blocks

- In master we see:

In the branch we see:

How has this been tested?

See #12310. E2e tests have been added.

Screenshots

Types of changes

Bug fix

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

jorgefilipecosta

Hi @iseulde this generally worked correctly in my tests. I think there is only one attribute missing from this conversion. We are not setting the linkDestination attribute.
So for example, when I convert the following data:

[caption id="attachment_6" align="alignnone" width="300"]<a href="http://www.example.com"><img class="wp-image-6 size-medium" src="http://enchanting-caterpillar.w5.poopy.life/wp-content/uploads/2018/11/Nov-15-2018-12-08-48-300x240.gif" alt="" width="300" height="240" /></a> aaaaa[/caption]

The link to field goes from "Custom URL" in the classic editor to "None" but if I change "Link to" to custom URL to correct custom link appears.

packages/block-library/src/image/index.js

alexsanford · 2018-12-09T18:01:37Z

I think we're missing a few more attributes as well:

width
height
linkDestination (mentioned above)
linkTarget

width and height may technically be out of scope for this PR, but we should probably get the link attributes working at least.

gwwar · 2018-12-09T18:49:36Z

@alexsanford Did you mean custom width and height? I verified the missing link to the media file, but I choose a large size which appeared to transfer over on block conversion.

alexsanford · 2018-12-09T19:31:16Z

@gwwar Yeah, the custom width and height attributes on the img tag. I'm using the "Custom Size" option when editing the Media in the classic editor. Then, in Gutenberg, when I convert to blocks and then look at the width and height (either in the block sidebar or by looking at the HTML), the custom size does not seem to carry over.

gwwar · 2018-12-09T19:33:47Z

Note that captions can contain arbitrary html

ellatrix · 2018-12-09T20:01:47Z

What is linkDestination?

gwwar · 2018-12-09T20:04:25Z

@iseulde Looks like you beat me :) taking this for another spin.

gwwar · 2018-12-09T20:06:45Z

What is linkDestination?

Preserving these options:

ellatrix · 2018-12-09T20:09:18Z

How do we source it from the example here:

#12315 (review)

gwwar · 2018-12-09T20:16:33Z

How do we source it from the example here:

If there's a link, the first child is an a tag wrapping an image. The href in that tag should be the one we want. Eg:

gwwar · 2018-12-09T20:19:29Z

Manually tests well with plain captions, and captions with links and styling 👍 . The missing link settings is maybe a separate issue. I'd be okay with a follow up PR if this isn't the right spot to modify that.

ellatrix · 2018-12-09T20:33:21Z

I'd be okay with a follow up PR if this isn't the right spot to modify that.

We'd need to get the media and attachment link from the ID, then compare it. I agree that this can be a separate PR.

gwwar

Thanks @iseulde, let's go with this then! Do you mind adding another issue for the follow up bug, if we don't have one already?

jorgefilipecosta

Thank you for iterating on this, In my tests, these changes did not cause any regressions, and make the current behavior better 👍
There are still some improvements that can be done, but they are a good fit for another follow-up PR.

jorgefilipecosta · 2018-12-20T10:38:53Z

I removed comment as my test environment for the case I referred was wrong I think this totally addresses #12310.

* Fix converting caption shortcode with link * Parse HTML to DOM instead of using RegExp * Source anchor info only from first element

@codebykat

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

@codebykat

* 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

@codebykat

* 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

@codebykat

* 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

Fix converting caption shortcode with link

acbf3bc

ellatrix added the [Feature] Paste label Nov 26, 2018

ellatrix added this to the 4.6 milestone Nov 26, 2018

ellatrix requested a review from a team November 26, 2018 14:05

jorgefilipecosta reviewed Nov 26, 2018

View reviewed changes

mtias modified the milestones: 4.6, 4.7 Nov 28, 2018

mtias added the Backwards Compatibility Issues or PRs that impact backwards compatability label Nov 28, 2018

mtias added the [Type] Bug An existing feature does not function as intended label Dec 6, 2018

mcsf reviewed Dec 9, 2018

View reviewed changes

packages/block-library/src/image/index.js Outdated Show resolved Hide resolved

mcsf modified the milestones: 4.7, 4.8 Dec 9, 2018

ellatrix added 2 commits December 9, 2018 13:45

Parse HTML to DOM instead of using RegExp

1453339

Source anchor info only from first element

70aea2f

gwwar approved these changes Dec 9, 2018

View reviewed changes

jorgefilipecosta approved these changes Dec 20, 2018

View reviewed changes

youknowriad merged commit 6cc58e8 into master Dec 20, 2018

youknowriad deleted the fix/covert-caption-shortcode-with-link branch December 20, 2018 10:51

youknowriad pushed a commit that referenced this pull request Jan 3, 2019

Fix converting caption shortcode with link (#12315)

d2454ed

* Fix converting caption shortcode with link * Parse HTML to DOM instead of using RegExp * Source anchor info only from first element

dmsnell mentioned this pull request Mar 10, 2019

Fix: Pasting captions without an image fails #14365

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix converting caption shortcode with link #12315

Fix converting caption shortcode with link #12315

ellatrix commented Nov 26, 2018 •

edited by jorgefilipecosta

Loading

jorgefilipecosta left a comment

alexsanford commented Dec 9, 2018

gwwar commented Dec 9, 2018

alexsanford commented Dec 9, 2018

gwwar commented Dec 9, 2018

ellatrix commented Dec 9, 2018

gwwar commented Dec 9, 2018

gwwar commented Dec 9, 2018

ellatrix commented Dec 9, 2018

gwwar commented Dec 9, 2018

gwwar commented Dec 9, 2018 •

edited

Loading

ellatrix commented Dec 9, 2018

gwwar left a comment

jorgefilipecosta left a comment

jorgefilipecosta commented Dec 20, 2018

Fix converting caption shortcode with link #12315

Fix converting caption shortcode with link #12315

Conversation

ellatrix commented Nov 26, 2018 • edited by jorgefilipecosta Loading

Description

Testing Instructions

How has this been tested?

Screenshots

Types of changes

Checklist:

jorgefilipecosta left a comment

Choose a reason for hiding this comment

alexsanford commented Dec 9, 2018

gwwar commented Dec 9, 2018

alexsanford commented Dec 9, 2018

gwwar commented Dec 9, 2018

ellatrix commented Dec 9, 2018

gwwar commented Dec 9, 2018

gwwar commented Dec 9, 2018

ellatrix commented Dec 9, 2018

gwwar commented Dec 9, 2018

gwwar commented Dec 9, 2018 • edited Loading

ellatrix commented Dec 9, 2018

gwwar left a comment

Choose a reason for hiding this comment

jorgefilipecosta left a comment

Choose a reason for hiding this comment

jorgefilipecosta commented Dec 20, 2018

ellatrix commented Nov 26, 2018 •

edited by jorgefilipecosta

Loading

gwwar commented Dec 9, 2018 •

edited

Loading