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

External images: Save to media library #54185

Closed
scruffian opened this issue Sep 5, 2023 · 25 comments
Closed

External images: Save to media library #54185

scruffian opened this issue Sep 5, 2023 · 25 comments
Labels
[Feature] Media Anything that impacts the experience of managing media Needs Design Feedback Needs general design feedback. Needs Design Needs design efforts. [Type] Feature New feature to highlight in changelogs. [Type] Performance Related to performance efforts

Comments

@scruffian
Copy link
Contributor

scruffian commented Sep 5, 2023

What problem does this address?

When a site use external images, we can't provide the same performance benefits that we can for images hosted by the site. This includes:

  • A srcset with multiple sources for the browser to choose from, to ensure that bytes aren't wasted downloading excessively large variants.
  • A loading tag with the lazy value for images which we expect to be below the fold or otherwise not the centre of attention, so that the browser can prioritise downloading important resources first.
  • A width and height, so that together with the CSS, the browser can calculate and reserve the space for the image ahead of time, in order to avoid layout shifts.

What is your proposed solution?

I don't think there is any way to achieve this using external images in a performant way. Instead we should prompt users to make these external images a part of their library when they try to save their content, something like this:

Screenshot 2023-09-05 at 12 50 27

I'm not sure if comic sans is the right design choice here XD

props to @sgomes for the description above!

@scruffian scruffian added [Feature] Patterns A collection of blocks that can be synced (previously reusable blocks) or unsynced [Type] Performance Related to performance efforts [Type] Feature New feature to highlight in changelogs. Needs Design Feedback Needs general design feedback. Needs Design Needs design efforts. labels Sep 5, 2023
@aaronrobertshaw aaronrobertshaw added [Feature] Media Anything that impacts the experience of managing media and removed [Feature] Patterns A collection of blocks that can be synced (previously reusable blocks) or unsynced labels Sep 6, 2023
@Ren2049
Copy link

Ren2049 commented Sep 6, 2023

A feature like this or similar to this would be very benefitial for prototyping within the editor. Popular graphics programs like Figma and Canva allow their graphics to be embedded on sites and updated in the graphics software in real time.

For designers it would be a nice feature to be able to iterate on graphics and then commit them at one point to the media library.

Could this be done for all image/graphics related blocks (e.g. the upcoming background feature for group blocks) and featured images?

I think it would increase the appeal of using WP as the primary design tool or place where you validate the design in a real context.

Right now designers have to rebuild pages within their graphic design tool to see the graphics in context, in the future this wouldn't be neccessary if the graphics could be iterated on in real time, and not having to export and re-import final graphics from a design tool into WP manually would also be a big workflow win imo.

@sgomes
Copy link
Contributor

sgomes commented Sep 6, 2023

Thank you for filing this bug report, @scruffian! This looks like an excellent approach 👍

One prospective enhancement we may want to consider is having some mechanism of determining whether an image has already been added to the library, to allow for reuse instead of adding multiple copies. This is particularly relevant if the image is being added via a reusable item like a block pattern, which could potentially get used frequently on new posts.

Also of note is that this mechanism should work well for theme images too, which have the same limitations.

@bph bph changed the title External images: Save to media libray External images: Save to media library Sep 6, 2023
@annezazu
Copy link
Contributor

annezazu commented Sep 6, 2023

Tagging in @WordPress/performance for awareness!

@swissspidy
Copy link
Member

Gutenberg already allows uploading external images to the media library. Then you would automatically get srcset too. But lazy loading works also for external images, so that‘s irrelevant here. Width & height could be detected automatically by Gutenberg, not that hard to do. In fact, I do have a plugin that already does it.

@sgomes
Copy link
Contributor

sgomes commented Sep 7, 2023

But lazy loading works also for external images, so that‘s irrelevant here.

I don't think that's the case; at least not by default. From wp_img_tag_add_loading_optimization_attrs:

// Images should have source and dimension attributes for the loading optimization attributes to be added.
if ( ! str_contains( $image, ' src="' ) || ! str_contains( $image, ' width="' ) || ! str_contains( $image, ' height="' ) ) {
	return $image;
}

The loading attribute should be added automatically when the width and height attributes are present, but the latter attributes only seem to be included in images added via the classic block, and not the image block.

@sgomes
Copy link
Contributor

sgomes commented Sep 7, 2023

Gutenberg already allows uploading external images to the media library. Then you would automatically get srcset too. But lazy loading works also for external images, so that‘s irrelevant here.

Could you elaborate on what you mean here, @swissspidy? Even if we can get external images to reliably lazy load (by always adding width and height attributes to external images), I'm not seeing the connection to srcset.

In my mind, it seems that whether or not an image is lazily loaded, we'll want to fetch the most adequately sized version rather than the potentially massively sized original, so we'll want a srcset in either case.

Given that we can only provide image resizing for images in the media library, having a simple workflow to move them there seems like a good idea, and I'd go as far as doing it by default as the mockup appears to suggest.

@scruffian
Copy link
Contributor Author

Width & height could be detected automatically by Gutenberg, not that hard to do. In fact, I do have a plugin that already does it.

I did start to look into this but I think we'd need to use something like getimagesize on these images, which I think would probably have negative performance implications.

@ellatrix
Copy link
Member

ellatrix commented Sep 7, 2023

We already did this in prepublish, could be reuse/rework that?

@sgomes
Copy link
Contributor

sgomes commented Sep 7, 2023

I did start to look into this but I think we'd need to use something like getimagesize on these images, which I think would probably have negative performance implications.

Do you mean that you were looking into adding the attributes when serving? Assuming that there aren't any concerns with markup compatibility, perhaps the width/height could be added to the external image markup in the editor instead, as is the case with images in the classic block. This would mean that there should be no performance issues when serving.

This would also mean, however, that if the URL were to change to a different image in the future it could lead to sizing issues. I think that's an acceptable tradeoff, though, given that there is no expectation of control or reliability on external images anyway.

@sgomes
Copy link
Contributor

sgomes commented Sep 7, 2023

We already did this in prepublish, could be reuse/rework that?

This looks fantastic! 🙌 Thank you, @ellatrix!

@joemcgill
Copy link
Member

Even if we can get external images to reliably lazy load (by always adding width and height attributes to external images), I'm not seeing the connection to srcset.

Lazy loading requires width and height on the image in order to avoid CLS when the image is loaded. For locally hosted images, those dimensions get added dynamically by calling wp_img_tag_add_width_and_height_attr from wp_filter_content_tags if the image doesn't already have those attributes.

There is no connection to srcset and sizes, other than also needing to have a width attribute available to generate the default sizes attribute. For srcset, we would need for WordPress to be aware of alternate sizes that could be included, which only happens when the file is uploaded to WordPress as an attachment so the intermediate sizes can be generated.

@swissspidy
Copy link
Member

@sgomes I meant you get srcset when uploading the external image to the media library.

Given that we can only provide image resizing for images in the media library, having a simple workflow to move them there

We already do. In the image block toolbar IIRC.

@sgomes
Copy link
Contributor

sgomes commented Sep 7, 2023

@sgomes I meant you get srcset when uploading the external image to the media library.

Oh, I see, sorry for the misunderstanding!

Given that we can only provide image resizing for images in the media library, having a simple workflow to move them there

We already do. In the image block toolbar IIRC.

I am unfamiliar with that feature, and wasn't able to find it in Gutenberg 16.6.0, which appears to have been released yesterday. Is it hidden behind an option that I'm missing?

@felixarntz
Copy link
Member

@sgomes

Assuming that there aren't any concerns with markup compatibility, perhaps the width/height could be added to the external image markup in the editor instead, as is the case with images in the classic block. This would mean that there should be no performance issues when serving.

That sounds right to me. Preferably the editor should add image width/height to images automatically. I'd argue even for media library images that would be useful (I believe it's not the case today unless they are resized by the user), but especially for external images where getting the dimensions while serving would indeed be a no-go from a performance perspective.

This would also mean, however, that if the URL were to change to a different image in the future it could lead to sizing issues. I think that's an acceptable tradeoff, though, given that there is no expectation of control or reliability on external images anyway.

Can you clarify this concern? How would the URL change? If the user changed the image, we would also be able to fetch the dimensions of said new image, or not?

@westonruter
Copy link
Member

but especially for external images where getting the dimensions while serving would indeed be a no-go from a performance perspective.

A complication here is if the author is linking to a URL that returns a random image with differing dimensions: WordPress/performance#49 (comment). Maybe this use case is not common enough to warrant consideration. But to help guard against this, external images should perhaps get object-fit:contain.

@westonruter
Copy link
Member

but especially for external images where getting the dimensions while serving would indeed be a no-go from a performance perspective.

Nevertheless, I wouldn't necessarily say this is a no-go since there's not really anything different between fetching dimensions of a remote image versus handling oEmbeds. The AMP plugin fetches dimensions of dimensionless images and stores them in transients. Granted, this is not without its own issues, and if they could be fetched up-front client-side in the editor it would be best.

@sgomes
Copy link
Contributor

sgomes commented Sep 8, 2023

Can you clarify this concern? How would the URL change? If the user changed the image, we would also be able to fetch the dimensions of said new image, or not?

Sorry, I wasn't very clear 🙂

My concern is similar to the one that @westonruter expressed; if we measure the external image during an initial phase in the editor, and then always serve it with the dimensions we measured at that point in time, then we run the risk of presenting a new image with the old dimensions if the external URL is ever changed to serve a different image (as in the case of random images served on the same URL, like Weston mentioned).

I do think that's an acceptable compromise, though; as I see it, it's part of the inherent risk of relying upon external images.

@sgomes
Copy link
Contributor

sgomes commented Sep 8, 2023

I'd argue even for media library images that would be useful (I believe it's not the case today unless they are resized by the user), but especially for external images where getting the dimensions while serving would indeed be a no-go from a performance perspective.

As of right now, the width/height attributes for media library images are not present in the editor-generated markup (by default), but they are added when serving, without any performance concerns that I'm aware of because they're part of the image meta — but please correct me if I'm wrong; I'm in no way an expert on server-side performance.

If the user resizes the image in the editor, the width/height attributes get directly added to the generated markup with the chosen dimensions, and inline styles are added as well with CSS width, height, and object-fit. Or at least that appears to be the case according to my testing.

@felixarntz
Copy link
Member

As of right now, the width/height attributes for media library images are not present in the editor-generated markup (by default), but they are added when serving, without any performance concerns that I'm aware of because they're part of the image meta — but please correct me if I'm wrong; I'm in no way an expert on server-side performance.

If the user resizes the image in the editor, the width/height attributes get directly added to the generated markup with the chosen dimensions, and inline styles are added as well with CSS width, height, and object-fit. Or at least that appears to be the case according to my testing.

The server performance cost of adding width/height is not considerable, but it has limitations such as when using external images, plus another limitation is when using images in e.g. block templates that are part of the theme (and thus not in the media library either), see WordPress/wordpress-develop#5124 for a related performance bug.

More foundationally, I think it is a problem that the only way to have width/height attributes in the editor-generated markup is to resize the image. This makes for an awkward fix in the above PR for example, where we would have to add all the is-resized etc markup to the block, even though the image is not resized, it's merely having its original width and height included as attributes. So allowing width/height to be present regardless of resizing would be a simpler starting point, and from there we could then continue exploring ways to automatically infer width/height of e.g. external images.

@sgomes
Copy link
Contributor

sgomes commented Sep 11, 2023

The server performance cost of adding width/height is not considerable, but it has limitations such as when using external images, plus another limitation is when using images in e.g. block templates that are part of the theme (and thus not in the media library either), see WordPress/wordpress-develop#5124 for a related performance bug.

Right, sorry I wasn't clear. I meant specifically in the case of media library images; I think the status quo (where the attributes are added when serving) is absolutely fine there, because we get all the browser performance benefits without any server performance impact. So while it also would be fine to make width and height part of the markup for media library images (if e.g. it makes implementation easier, as you point out), performance-wise there's no need to do so.

For external and theme images, however, it would definitely be beneficial to start adding width and height to the markup, while also encouraging/guiding users to upload images to the media library instead, such as through the mockups @scruffian put together, or the existing feature that @ellatrix linked to.

@richtabor
Copy link
Member

Noting that we do have this flow currently, which catches images from within the theme's assets, as well as externally linked image. You can see this with Twenty Twenty Four active, and by adding any pattern from the pattern directory to a page.

But it does allow duplicates.

CleanShot 2023-09-11 at 13 14 25

@scruffian
Copy link
Contributor Author

I wasn't aware of that. I think we can close this then?

@alvarotrigo
Copy link

I wasn't aware of that. I think we can close this then?

Is this working with Google Docs images?

I get this errors:

image

Access to fetch at 'https://lh7-rt.googleusercontent.com/docsz/AD_4nXflLOcl2GYxlC7reoghdrtgUYrMPOQEN2kcMjyGosHxV61SooSJ-Z9d4ud9UBqDseXlnDfYewyb8qIqgnIHSAf-aj3LNTVN4TOwSMxSGYU3q1xte8f-8uZXd2FGtmip2lbZ38ysPf53gFwZUhJ5n6U47pQl?key=9Q0NNQNYjDrTx3qolS5zAQ' from origin 'https://example.com' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource. If an opaque response serves your needs, set the request's mode to 'no-cors' to fetch the resource with CORS disabled.

@solaceten
Copy link

Also experiencing this :

#2515 (comment)

@UpstairsEmpire
Copy link

I love this functionality, but for me, I only see it when I hit "Publish" on a newly created page/post with pasted content. I would expect to also see it when editing an existing page/post and hitting "Save" but I don't see it.

Noting that we do have this flow currently, which catches images from within the theme's assets, as well as externally linked image. You can see this with Twenty Twenty Four active, and by adding any pattern from the pattern directory to a page.

But it does allow duplicates.

CleanShot 2023-09-11 at 13 14 25

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Media Anything that impacts the experience of managing media Needs Design Feedback Needs general design feedback. Needs Design Needs design efforts. [Type] Feature New feature to highlight in changelogs. [Type] Performance Related to performance efforts
Projects
None yet
Development

No branches or pull requests