Auto Sizes for Lazy-loaded Images

[mukeshpanchal27]

Trac ticket: https://core.trac.wordpress.org/ticket/61847

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[github-actions]

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

• The Plugin and Theme Directories cannot be accessed within Playground.
• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props mukesh27, flixos90, joemcgill, westonruter, peterwilsoncc.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[felixarntz]

@mukeshpanchal27 This looks great to me! Only a few minor points of feedback.

[felixarntz]

@mukeshpanchal27 Awesome stuff, looks great!

A few minor nit-pick comments, but no blockers.

[joemcgill]

This looks like it's working as expected. A couple thoughts...

Most dynamic optimizations WP applies can be short circuited through a filter, but there doesn't seem to be an easy way to disable this functionality. I think both places where we are adding auto sizes, we should add a filter like:

$is_auto_sizes_enabled = apply_filters( 'wp_img_auto_sizes_enabled`, true );

I also think this logic could all be simplified a bit if we added auto-sizes in wp_img_tag_add_loading_optimization_attrs() rather than a separate function. In the Enhanced Responsive Images plugin, creating a separate function was required since there wasn't a way to filter the image attributes during wp_filter_content_tags() when the functionality was first written.

[felixarntz]

@joemcgill Being able to disable auto sizes could be a good idea. Though I would lean against baking this functionality into wp_img_tag_add_loading_optimization_attrs(), as it's not a loading optimization attribute like loading, decoding and fetchpriority are, so semantically that'd be confusing. I'd be wary of making that a "catch-all" function just because it's convenient to implement.

[mukeshpanchal27]

@joemcgill Thanks for the feedback. In deec3ba, I introduced a new filter that disables the auto sizes functionality.

I also agree with what @felixarntz mentioned #7253 (comment).

[felixarntz]

@joemcgill @mukeshpanchal27 Given this some further thought, I'll have to revise my take on having a filter.

I'm questioning why we should introduce one to allow disabling auto sizes, for a few reasons:

• Not everything that WordPress does needs to be modifiable. We shouldn't introduce filters for the sake of having filters. We should ask ourselves whether having a filter makes sense for the concrete functionality.
• For lazy-loading, the rationale for having a filter is that lazy-loading is a double-edged sword: It is good for sustainability, and it can be good for performance, but if the wrong image is lazy-loaded, it can affect LCP negatively. In other words, overall having it enabled by default makes sense, but there are valid reasons to disable it based on how well it works for a site and, related to that, how much the site owner cares about sustainability vs LCP performance.
• For the auto keyword on the sizes attribute, I question what such a rationale would be. What are the potential negative side effects of having the keyword present on every lazy-loaded image?
• On another note, we can always introduce filters later if a need for them is established, but we cannot remove a filter once introduced. This IMO is another reason to introduce filters (and actions) carefully, either based on real use-cases or by anticipating a good reason for it to be needed.

@joemcgill Can you provide a rationale on how a filter to disable auto sizes would help?

[mukeshpanchal27]

• For lazy-loading, the rationale for having a filter is that lazy-loading is a double-edged sword: It is good for sustainability, and it can be good for performance, but if the wrong image is lazy-loaded, it can affect LCP negatively. In other words, overall having it enabled by default makes sense, but there are valid reasons to disable it based on how well it works for a site and, related to that, how much the site owner cares about sustainability vs LCP performance.

The auto value is added to the sizes attribute when images are lazy-loaded. However, if the wrong image is lazy-loaded, the auto value is also incorrectly added to the sizes attribute. We have a filter that updates lazy loading, but it can't remove the auto value from the sizes attribute. In this case, I believe we need a filter that removes the auto value from the sizes attribute. As shown in Joe's example WordPress/performance#791 (comment), the sizes calculation is worse when an image has only the auto value but is not lazy-loaded.

[felixarntz]

The auto value is added to the sizes attribute when images are lazy-loaded. However, if the wrong image is lazy-loaded, the auto value is also incorrectly added to the sizes attribute. We have a filter that updates lazy loading, but it can't remove the auto value from the sizes attribute.

Which lazy-loading filter are you referring to? As far as I'm aware, it's not possible for the logic here to add sizes="auto" to an image that isn't lazy-loaded, so disabling lazy-loading will effectively also disable sizes="auto" addition anyway.

[mukeshpanchal27]

Which lazy-loading filter are you referring to?

In quick test i add wp_img_tag_add_loading_attr filter for wp_get_attachment_image() so get auto when the lazy-load attribute is not added.

As far as I'm aware, it's not possible for the logic here to add sizes="auto" to an image that isn't lazy-loaded, so disabling lazy-loading will effectively also disable sizes="auto" addition anyway.

You are right here.

[felixarntz]

Which lazy-loading filter are you referring to?

In quick test i add wp_img_tag_add_loading_attr filter for wp_get_attachment_image() so get auto when the lazy-load attribute is not added.

I don't understand. I think that filter only runs for content images, not wp_get_attachment_image(). Can you clarify what you mean?

[joemcgill]

@joemcgill Can you provide a rationale on how a filter to disable auto sizes would help?

Sure. This functionality introduces an optimization that should work well for a majority of sites—particularly ones that aren't using any plugins or custom code that modifies the way WordPress handles lazy-loading. However, for sites which have a unique setup where this functionality could negatively impacts their site's performance, I think it's good practice to allow sites to easily be able to opt out of these optimizations through the use of a filter, so they can customize how images are served to better fit their specific use case.

In the specific case of auto sizes, if auto is included on an image that is NOT lazy loaded, the current implementation of this feature in Chromium would result in these sizes attributes being calculated at 100vw, rather than the fallback that WordPress provides for browsers that do not yet support auto sizes. Allowing sites that might be impacted by this problem due to third party (or custom) code to opt out of WP's automatic auto sizes implementation would help in those cases.

[felixarntz]

@joemcgill

Sure. This functionality introduces an optimization that should work well for a majority of sites—particularly ones that aren't using any plugins or custom code that modifies the way WordPress handles lazy-loading. However, for sites which have a unique setup where this functionality could negatively impacts their site's performance, I think it's good practice to allow sites to easily be able to opt out of these optimizations through the use of a filter, so they can customize how images are served to better fit their specific use case.

In the specific case of auto sizes, if auto is included on an image that is NOT lazy loaded, the current implementation of this feature in Chromium would result in these sizes attributes being calculated at 100vw, rather than the fallback that WordPress provides for browsers that do not yet support auto sizes. Allowing sites that might be impacted by this problem due to third party (or custom) code to opt out of WP's automatic auto sizes implementation would help in those cases.

To me, your points are only theoretical concerns. The logic here only adds sizes="auto" if the image is lazy-loaded, so I don't see what you're saying as a good reason to add a filter to disable it. This would only make sense to me if there was a margin of error in the functionality like there is with lazy-loading - but I don't think there is.

Even if let's say a plugin removes WordPress's lazy-loading attributes from an image, the code here to add sizes="auto" runs later, so it would not add the attribute in such a case.

[joemcgill]

@felixarntz We've already seen cases in plugins that the Performance Team has written, like Image Prioritizer (related issue), where the loading attribute is modified after all of WP's logic has run, so I don't think this is a theoretical concern. Either way, giving sites a way of opting out of this optimization can help if they instead wanted to implement this in their own way (or not at all).

Given that sites can still work around any issues with this functionality by using filters that run after this is applied, I'm ok with us not adding a filter to disable this functionality initially, if you feel strongly about it. That said, I do generally think it's good practice for us to give easy ways to opt-out of these types of optimization decisions that affect the markup of a site since we can't account for every use case a site might have and this type of filter is not all that difficult to maintain.

[joemcgill]

If we add a filter to opt out of this feature (pending this discussion), I think a better location would be inside the wp_img_tag_add_auto_sizes() function, and have that function return the original sizes value if the filter is set to false.

[felixarntz]

Per #7253 (comment), I'd still prefer not to have one, but this would work for me, if y'all feel strongly about having one. Basically the filter should work like wp_img_tag_add_loading_attr, i.e. also receive the image and context.

[felixarntz]

@joemcgill

We've already seen cases in plugins that the Performance Team has written, like Image Prioritizer (related issue), where the loading attribute is modified after all of WP's logic has run, so I don't think this is a theoretical concern, and giving sites a way of opting out of this optimization if needed could help if they instead wanted to implement this in their own way (or not at all).

That's fair, though the way that this is achieved by that plugin is a (valid) hack via output buffering, so IMO not something core should cater for. I think Core should ensure that anything that is possible via its own APIs (actions and filters) works as expected. You can modify basically anything with things like output buffer, modifying globals, etc., but those aren't encouraged to use, or rather "use at your own risk". So from that perspective I don't find the Image Prioritizer usage relevant for this.

Given that sites could still work around any issues with this functionality by using filters that run after this is applied, I'm ok with us not adding a filter to disable this functionality initially, but I do generally think it's good practice for us to give easy ways to opt-out of these types of optimization decisions that affect the markup of a site since we can't account for every use case a site might have.

Related to your comment in #7253 (review), I would say that if we introduce a filter, it should be specific to each image (e.g. receive the image tag and context like e.g. the wp_img_tag_add_loading_attr filter works), not a general enable/disable filter. I'd be okay with that because that's more specific to the image being modified and would be suitable for individual modifications. It would still allow completely disabling sizes="auto" (by unconditionally returning false), but that would be neither the primary purpose of the filter nor encouraged, so I think that'd be more reasonable.

My primary concern is about having a filter to generally enable/disable sizes="auto" since, per the above, I don't see any reason that it should be universally disabled (again, different from e.g. lazy-loading where they can be adverse effects from Core not getting it right).

[joemcgill]

Totally agree that it would be useful for someone to filter whether this is enabled on based on which specific image the filter was being applied to. However, I think we should go ahead and commit this without a filter for now. We can determine whether a filter is needed and what context will need to be added to that filter after this functionality has been in trunk for a while.

[mukeshpanchal27]

I think we should go ahead and commit this without a filter for now. We can determine whether a filter is needed and what context will need to be added to that filter after this functionality has been in trunk for a while.

I agree. In e4a141a, I have removed the filter. The PR is now ready for review and commit. 🎉

Should we open a separate Trac ticket for this discussion now, or can we wait and open one in the future if any issues arise?

[felixarntz]

@mukeshpanchal27 Looks great! 🎉

Regarding a potential filter, I think we can keep discussing on the Trac ticket if needed.

[westonruter]

One other nit, but pre-approving.

[westonruter]

Should this remove auto if it is present in $attr['sizes'] while $attr['loading'] is not lazy? This is implemented in WordPress/performance#1476.

[peterwilsoncc]

Should this remove auto if it is present in $attr['sizes'] while $attr['loading'] is not lazy? This is implemented in WordPress/performance#1476.

Do any of the implementations respect it in spite of the spec if the CSS is available and the image isn't set for lazy loading? If so then I think it would be good to keep it given the sizes attributes can be inaccurate in many situations and the CSS may be in the HTML header and therefore available.

[mukeshpanchal27]

Should this remove auto if it is present in $attr['sizes'] while $attr['loading'] is not lazy? This is implemented in WordPress/performance#1476.

@westonruter Core only adds the "auto" in sizes when image is lazy-loaded so if user added that through filter then core will not remove "auto".

@peterwilsoncc Even if the CSS is available early in the document (such as in the HTML header), implementations do not dynamically adjust the sizes attribute based on CSS unless lazy loading is enabled.

[felixarntz]

I don't think this is needed as Core doesn't add sizes="auto" in a way where this could be relevant. If a plugin does that, it's doing it wrong.

Now I'm not strongly against adding this, but it's not a requirement for this feature. We could discuss whether this is worth adding in a separate ticket, or wait if it comes up as a real use-case problem somewhere.

[joemcgill]

I think this is a good initial pass at adding this functionality. Let's commit this and iterate on questions like this one with the benefit of more testing.

[joemcgill]

Merged in https://core.trac.wordpress.org/changeset/59008.