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

“Automatically use the first image in post as the Featured Image” setting doesn't work for Atomic sites #86093

Open
simison opened this issue Jan 8, 2024 · 13 comments
Labels
[Pri] Low Address when resources are available. Triaged To be used when issues have been triaged. [Type] Bug

Comments

@simison
Copy link
Member

simison commented Jan 8, 2024

Repoted at p1704606505767399-slack-C02NQ4HMJKV:

For whatever reason they had ended up choosing the Classic Rowling theme and they were on the Creator plan and had made the aite Atomic. This resulted in the option to “Automatically use the first image in post as the Featured Image” to not work. They manually added 16 Featured Images out of nearly 200 posts and then contacted support to see if there was a better way. Unfortunately, this appears to be a bug on Atomic sites, the feature does work on Simple sites.

Screenshot 2024-01-08 at 10 52 02
@simison
Copy link
Member Author

simison commented Jan 8, 2024

@TimBroddin is this something Apex could look into, as you folks developed the feature originally?

Might just need adding the option to the list of options to sync in Jetpack?

@github-actions github-actions bot added the [Pri] TBD Review and assign an appropriate [Pri] label as soon as possible. label Jan 8, 2024
@TimBroddin
Copy link
Contributor

Did this not work:

This resulted in the option to “Automatically use the first image in post as the Featured Image” to not work.

Or did the toggle not work?

We'll look into it.

@simison
Copy link
Member Author

simison commented Jan 8, 2024

I haven't yet tested personally. Thanks!

@ivan-ottinger
Copy link
Contributor

I have investigated the issue and here's what I found:

  • the issue isn't related to the "Enable Featured image on your new post emails" setting
  • the cause seems to lie in the way the site content import works, not the particular theme, nor site being WoA or Simple

When you look at the user's site and the source of posts where the first Image wasn't picked up as the post's Featured image, you can notice that the image ID is missing from the block markup:

<!-- wp:image {"sizeSlug":"large","linkDestination":"none"} --><figure class="wp-block-image size-large"><img src="here-was-the-image-url" alt="" /></figure><!-- /wp:image -->

For comparison, Image blocks normally include that ID. Example:

<!-- wp:image {"id":36,"className":"size-full"} -->
<figure class="wp-block-image size-full"><img src="here-was-the-image-url" alt="" class="wp-image-36"/></figure>
<!-- /wp:image -->

This issue can be reproduced easily with any site / theme:

  1. Create a new post.

  2. Add a new image using the Image block.

  3. Go to the post source and remove the image ID from the block markup.

  4. When you go back to the visual editor, you will even get a broken block error message:
    Markup on 2024-01-09 at 10:14:32

  5. Clicking the "Attempt Block Recovery" button won't help (it only removes class from the block markup) and you will see that the image isn't being picked up as Featured image (even in the Posts list):

Markup on 2024-01-09 at 10:00:18

I haven't looked into how the site import works yet - maybe the folks that handle imports could take a quick look - as they are more familiar with that part of the codebase.

@ivan-ottinger
Copy link
Contributor

I am now checking whether the image ID is actually being stripped out on import or when the site goes Atomic. As that could be where the issue is introduced as well. 🤔

@ivan-ottinger
Copy link
Contributor

ivan-ottinger commented Jan 9, 2024

I can confirm that when we import content from Substack, the imported posts don't include the full Image markup.

Here's how imported post with one Image block looks like:

<!-- wp:image {"sizeSlug":"large","linkDestination":"none"} --><figure class="wp-block-image size-large"><img src="here-was-the-image-url" alt="" /></figure><!-- /wp:image -->

Instead, it should look like:

<!-- wp:image {"id":2152,"sizeSlug":"large","linkDestination":"none"} --><figure class="wp-block-image size-large"><img src="here-was-the-image-url" alt="" class="wp-image-2152" /></figure><!-- /wp:image -->

In other words, what is missing is the image ID and also the class. If both are present, the image will be picked up as a Featured image correctly.


When testing the import from Medium, the imported content looks as follows:

<figure><img data-width="3672" data-height="3096" src="https://cdn-images-1.medium.com/max/800/1*mmPxZftrwj4bmIaDR_zBNQ.png"></figure>

The Image block isn't used at all, neither the image itself is imported to the site. As a result, the image isn't picked up as a Featured image either. It actually is picked up on the front-end, but not in Calypso. I haven't dug deeper, but probably the data-width="3672" and data-height="3096" attributes help there.


What we may want to do is to either A) adjust how the importers work - or - B) change the way the image is picked up from the post content.

When looking at why the image is not loaded on the Posts page in Calypso for instance, the reason is that the candidateImage in client/lib/post-normalizer/rule-pick-canonical-image.js won't get set properly:

		const candidateImage = find( post.content_images, isCandidateForCanonicalImage );

This happens because isCandidateForCanonicalImage in client/lib/post-normalizer/utils/is-candidate-for-canonical-image.js returning false because such image doesn't have any height or width` set:

export function isCandidateForCanonicalImage( image ) {
	if ( ! image ) {
		return false;
	}

	if ( image.width < 100 ) {
		return false;
	}

	if ( image.width * image.height < 100 * 75 ) {
		return false;
	}
	return true;
}

But as soon as the image in the post content has correct markup, the height and width get correct information and that image will make isCandidateForCanonicalImage return true.


I think it might make more sense to fix the importers to make sure the imported content has proper markup instead of trying to make the rest of the system be able to read images from the content that don't have correct markup. What do you folks think?

@simison
Copy link
Member Author

simison commented Jan 9, 2024

change the way the image is picked up from the post content.

Is this lib used in Gutenberg editor, or somewhere else? I didn't quite follow that bit.

I think it might make more sense to fix the importers to make sure the imported content has proper markup instead of trying to make the rest of the system be able to read images from the content that don't have correct markup. What do you folks think?

This would help things also when they don't use Calypso, correct? @vishnugopal there's an issue with image IDs not getting assigned with Substack content importer. (Things work with Medium). Could the team look into that? Could the issue exist in other content importers as well?

I think the lack of ID causes other issues as well, like how the images get optimized by Photon or Jetpack's lightbox feature. (Haven't tested, tho)

But as soon as the image in the post content has correct markup, the height and width get correct information and that image will make isCandidateForCanonicalImage return true.

@ivan-ottinger why would the featured image depend on width/height, would anything break if that limitation is removed?

@ivan-ottinger
Copy link
Contributor

Thanks, Mikael!

Is this lib used in Gutenberg editor, or somewhere else? I didn't quite follow that bit.

I don't think the post-normalizer lib is used in Gutenberg (at least I didn't find the reference anywhere). What I was focusing at was the Posts page in Calypso where I noticed the issue first:

Markup on 2024-01-09 at 16:22:50

If the Image block doesn't have the proper markup, Calypso won't consider it at all. That is because the height and width of the image won't be present in the dom parameter of the detectMedia( post, dom ):

/**
* Adds an ordered list of all of the content_media to the post
* @param {post} post - the post object to add content_media to
* @param {dom} dom - the dom of the post to scan for media
* @returns {PostMetadata} post - the post object mutated to also have content_media
*/
export default function detectMedia( post, dom ) {
const imageSelector = 'img[src]';
const embedSelector = 'iframe';
const media = dom.querySelectorAll( `${ imageSelector }, ${ embedSelector }` );
const contentMedia = map( media, ( element ) => {
const nodeName = element.nodeName.toLowerCase();
if ( nodeName === 'iframe' ) {
return detectEmbed( element );
} else if ( nodeName === 'img' ) {
return detectImage( element );
}
return false;
} );
post.content_media = compact( contentMedia );
post.content_embeds = filter( post.content_media, ( m ) => m.mediaType === 'video' );
post.content_images = filter( post.content_media, ( m ) => m.mediaType === 'image' );
// TODO: figure out a more sane way of combining featured_image + content media
// so that changes to logic don't need to exist in multiple places
if ( post.featured_image ) {
post.featured_image = maxWidthPhotonishURL( post.featured_image, READER_CONTENT_WIDTH );
}
return post;
}

@ivan-ottinger why would the featured image depend on width/height, would anything break if that limitation is removed?

Looking at the related code in Calypso, I believe the width/height check (in isCandidateForCanonicalImage()) is there only to make sure the image is not too small. But if the Image block markup is incomplete, both height and width that enter isCandidateForCanonicalImage() are 0 and hence the logic won't allow that image to be used on the Calypso's Posts page.

I did not dig deeper into the code to see exactly why incomplete Image block markup prevents the "Automatically use first image in post" setting from working properly because correcting the block markup fixes the issue everywhere.

Once the correct markup is used, the image starts displaying on the Post page in Calypso, but also in the front-end on pages where the image is displayed thanks to that "Automatically use first image in post" setting):

Markup on 2024-01-09 at 16:55:28

[...] there's an issue with image IDs not getting assigned with Substack content importer.

Apart from the image ID, there should be the image CSS class as well, e.g. class="wp-image-2152":

<!-- wp:image {"id":2152,"sizeSlug":"large","linkDestination":"none"} --><figure class="wp-block-image size-large"><img src="here-was-the-image-url" alt="" class="wp-image-2152" /></figure><!-- /wp:image -->

When both are present, Calypso can pick up the height / width of the image correctly. Also, the "Automatically use first image in post" setting will work as expected again.

Simple sites are more forgiving than Atomic in a way that even with the incomplete Image block markup, the "Automatically use first image in post" setting will still work as expected. If the image ID is missing on an Atomic site, the image simply won't load (the behaviour will be basically the same as if the "Automatically use first image in post" setting was disabled). This is the case the user encountered.

Again, I haven't dug deeper since completing the Image block markup solves the issue. But if needed, I can check how exactly is the incomplete block markup related to the "Automatically use first image in post" setting. I can imagine it could also be something with Photon - as you mention in your previous comment.

Also, I think it is worth mentioning that both image ID and related CSS class are part of the Image block markup in WordPress core (I have checked this on a self-hosted WPORG site without Jetpack or anything WordPress.com related). That's why I believe we should try to get it correctly when importing content.


And I am hoping I didn't miss anything. 🙂 Please let me know if something isn't clear.

@maciejpilarski maciejpilarski added the Triaged To be used when issues have been triaged. label Jan 9, 2024
@maciejpilarski maciejpilarski moved this from Needs Triage to Triaged in Automattic Prioritization: The One Board ™ Jan 9, 2024
@inaikem inaikem closed this as completed Oct 13, 2024
@inaikem inaikem reopened this Oct 13, 2024
@inaikem inaikem moved this from Done 🎉 to In Triage in Automattic Prioritization: The One Board ™ Oct 15, 2024
@inaikem inaikem moved this from In Triage to Triaged in Automattic Prioritization: The One Board ™ Oct 15, 2024
@inaikem
Copy link
Contributor

inaikem commented Oct 15, 2024

@ivan-ottinger, can you please review this issue and let us know the next steps? It feels like this may have been deprioritised in favour of Developers Developers Developers.

@ivan-ottinger
Copy link
Contributor

ivan-ottinger commented Nov 5, 2024

Thank you for the ping, @inaikem! I was AFK so getting to your message just today.


@ivan-ottinger, can you please review this issue and let us know the next steps? It feels like this may have been deprioritised in favour of Developers Developers Developers.

The next step is to make sure the Substack importer (https://wordpress.com/import/) produces correct markup for Image blocks.

Here is a specific example:

Image imported from Substack currently:

<!-- wp:image {"sizeSlug":"large","linkDestination":"none"} --><figure class="wp-block-image size-large"><img src="https://ivanthemetest.files.wordpress.com/2024/11/79762-c9798b17-b46c-4d8a-a7d6-5f98a839f22c_2436x1608.png" alt="" /></figure><!-- /wp:image -->

How it should be imported instead:

<!-- wp:image {"id":140507978,"sizeSlug":"large","linkDestination":"none"} --><figure class="wp-block-image size-large"><img src="https://ivanthemetest.files.wordpress.com/2024/11/79762-c9798b17-b46c-4d8a-a7d6-5f98a839f22c_2436x1608.png" alt="" class="wp-image-140507978" /></figure><!-- /wp:image -->

The only difference between the two is that the correct markup includes "id":140507978 in the enclosing wp:image tag and class="wp-image-140507978" in the figure tag.

140507978 is the ID of the media/image of that specific example.

As soon as the markup is correct, the Automatically use the first image in post as the Featured Image setting will start working.

Does that make sense? Please let me know if you have any questions. Happy to clarify! 🙂

@inaikem
Copy link
Contributor

inaikem commented Nov 6, 2024

Thank you! Do you have a sense of priority for getting this change shipped given the focus on inbound migrations and customer acquisition?

@ivan-ottinger ivan-ottinger added [Pri] Low Address when resources are available. and removed [Pri] TBD Review and assign an appropriate [Pri] label as soon as possible. labels Nov 6, 2024
@ivan-ottinger
Copy link
Contributor

ivan-ottinger commented Nov 6, 2024

Thank you! Do you have a sense of priority for getting this change shipped given the focus on inbound migrations and customer acquisition?

The content with images is imported without any errors and images are loading properly in both frontend and the block editor.

The issue affects those users who:

  • import their content from Substack to an Atomic site and, at the same time
  • rely on the Automatically use the first image in post as the Featured Image setting

However, the setting is available only in Customizer of a small fraction of old non-block-based themes like Hemingway Rewritten or Rowling.

On top of the above, I was not able to reproduce the issue where the setting would not work for images that are missing full markup anymore.

The only visible issue I can see at the moment is that the image thumbnails of posts imported from Substack are missing in Calypso Posts page:

Image

Because of that, I am closing this issue as won't fix / can't repro and will create a separate new low priority one with the goal of fixing the image markup produced by the Substack importer.

@ivan-ottinger ivan-ottinger closed this as not planned Won't fix, can't repro, duplicate, stale Nov 6, 2024
@ivan-ottinger
Copy link
Contributor

ivan-ottinger commented Nov 6, 2024

Actually, turns out the issue is still reproducible, but manifesting on Atomic sites only. I am reopening the issue again and leaving its priority as low.

I have also updated my previous comment.

@ivan-ottinger ivan-ottinger reopened this Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Pri] Low Address when resources are available. Triaged To be used when issues have been triaged. [Type] Bug
Projects
Development

No branches or pull requests

5 participants