Embed block: use preview html for flickr embed previews in editor

[mashikag]

What?

This commit is fixing an issue where flickr embeds are displayed with low quality images in the block editor.

The issue was reported here.

Why?

The issue was that thumbnail image (low resolution image) was used for flickr's embed previews. On top of that the preview was stretched to 100% width of the parent element, which most of the time meant more than width of the preview's thumbnail image. This resulted in flickr embed previews appear "pixelated" in the editor.
Flickr oEmbed previews returned by their oEmbed API are marked as 'photo' types. According to our source code we prefer to use the thumbnail url, over preview's html when embed's type is 'photo'.

How?

I fixed this by introducing new block attribute of 'preferHtml' which, when set to true, forces the use of preview's html even when an embed is of 'photo' type.

Testing Instructions

1. Open Post or Page editor.
2. Copy any flickr share url of your liking, e.g. https://flic.kr/p/oj7o7q
3. Paste it into your block editor.
4. After the fix the preview should not appear to be a "pixelated" image, it should look the same way as when you preview the page with the embed in it.

Screenshots or screencast

Before fix

After fix

[mashikag]

Despite the above initial fix implementation, I wanted to ask people reviewing this whether they can justify the need for getPhotoHtml() function (see code).

I tried to look for other examples than flickr which would return embeds with photo type. I could not find any.

1. My worry is that the thumbnail preview is broken for all photo type embed previews
2. and I just cannot justify to myself ever using the thumbnail url, over preview's html - given both of them are available.

[ntsekouras]

Thanks for the PR @mashikag!

I took a look here and I was wondering if it would be simpler to just change this line of code and give priority to the full image. Would that work?

[mashikag]

I took a look here and I was wondering if it would be simpler to just change this line of code and give priority to the full image. Would that work?

Yes, it does work. However would that not break any other photo type embeds, that are not flickr? I do not know myself if any other supported embeds provide photo type previews?

It would make even more sense to me to always use html provided with preview, even if the preview's type is photo. This could be done by rewriting this line as:

const html =  preview.html || (type === 'photo' && getPhotoHtml( preview )) ;

Or even rename the getPhotoHtml to getHtml or createHtml, and let it handle a case where html is not provided for any embed type, by using its current internal logic, like so:

const html =  preview.html || createHtml( preview );

But it just makes me question what was the purpose for this function when it was written, which embedded service (live example) required this handling? Would you have any insight to this @ntsekouras ?

[ntsekouras]

Yes, it does work. However would that not break any other photo type embeds, that are not flickr? I do not know myself if any other supported embeds provide photo type previews?

I'm not sure how this could break things. It was actually used before in code and if the photo.url doesn't exist, it will fallback to the thumbnail url.

It would make even more sense to me to always use html provided with preview, even if the preview's type is photo.

But it just makes me question what was the purpose for this function when it was written, which embedded service (live example) required this handling? Would you have any insight to this @ntsekouras ?

I'm not really sure about the reasoning of the original coding of this and I'm not sure if your suggested change will affect other embedded previews. That's why I suggested the above change of priority - it was used before and gives us the wanted result.

[mashikag]

I'm not sure how this could break things. It was actually used before in code and if the photo.url doesn't exist, it will fallback to the thumbnail url.

Yes, it was used but only if the photo.thumbnail_url did not exist. 😄 I am just worried that for some photo type embed scenario, for which the function was originally written, using photo.url over photo.thumbnail_url will result in an unexpected user experience, just like using the photo. thumbnail_url does for flickr embed.

On the other hand, I could just leave a note beside the change, linking to the issue, in case it will break something as per above.

[mashikag]

Hey @ntsekouras ! 👋 Could you have a look at the PR again, please? 🙏

[ntsekouras]

Thanks for your work @mashikag! Let's try this.

[mashikag]

Thanks @ntsekouras ! 🙇

BTW just letting you know because I am not sure if it is obvious or not, but I do not have merge rights here.

[ntsekouras]

BTW just letting you know because I am not sure if it is obvious or not, but I do not have merge rights here.

I knew that, yes 😄 . I was waiting for the CI to go 🟢 . Thanks again!