Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 16 months ago

#53232 closed feature request (fixed)

Add async decoding attribute as default to the HTML image tags generated by WordPress

Reported by: isaumya's profile isaumya Owned by: adamsilverstein's profile adamsilverstein
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch has-unit-tests needs-dev-note early
Focuses: performance Cc:

Description

Hi,
WordPress has taken advantage of many amazing features to the image tags like srcset, sizes and recently loading attributes. Thanks to the loading="lazy" attribute all images generated by WordPress system have that attribute in it by default. This is insanely helpful to make page load faster and more responsive.

Recently I have been playing around with another special attribute to the img tag which is basically decoding="async". After implementing async decoding to the images in a page, the page load became, even more, faster and images are decoded asynchronously by the browser, loading the contents almost instantly and also reducing page render time. This is a huge performance booster to any webpages which has a lot of images (so basically most sites).

This decoding attribute also has great browser support according to MDN docs.

While trying to implement the feature on a WordPress site, with the help of the following code snippet I was able to make it work across all the images on the website except for the images added via Gutenberg.

<?php
add_filter( 'wp_get_attachment_image_attributes', function($attr) {
  $attr['decoding'] = 'async';

  return $attr;
} );

As Gutenberg doesn't support either image_send_to_editor filter or wp_get_attachment_image_attributes filter. Adding it to Gutenberg generated images are more complicated and includes overwriting many core blocks like images, gallery etc.

But I was wondering if the WP Core team consider adding this decoding="async" attribute to the images like loading="lazy" as it will help a lot of people, making their webpage render faster by browsers.

Attachments (13)

53232.1.diff (23.6 KB) - added by westonruter 3 years ago.
53232.diff (13.8 KB) - added by adamsilverstein 3 years ago.
53232.2.diff (14.4 KB) - added by adamsilverstein 3 years ago.
53232.3.diff (15.7 KB) - added by adamsilverstein 3 years ago.
53232.4.diff (15.5 KB) - added by adamsilverstein 3 years ago.
53232.5.diff (16.0 KB) - added by peterwilsoncc 3 years ago.
53232.6.diff (16.3 KB) - added by adamsilverstein 3 years ago.
53232.7.diff (15.9 KB) - added by adamsilverstein 3 years ago.
53232.8.diff (16.8 KB) - added by adamsilverstein 3 years ago.
53232.9.diff (19.0 KB) - added by adamsilverstein 3 years ago.
53232.10.diff (24.9 KB) - added by costdev 3 years ago.
Patch refresh.
53232.11.diff (24.1 KB) - added by adamsilverstein 3 years ago.
53232.12.diff (24.9 KB) - added by peterwilsoncc 3 years ago.

Download all attachments as: .zip

Change History (90)

#1 @westonruter
4 years ago

Thanks for filing this. I was actually asking if anyone has raised this yet and I was going to file it, but then you beat me to it.

The use of decoding=async is deemed a best practice to according to Malte Ubl:

Adding decoding="async" to the img gives the browser permission to decode the image off the main thread avoiding user impact of the CPU-time used to decode the image. This should have no discernible downside except that it cannot always be the default for legacy reasons.

I'm not clear on the specifics of the legacy reasons. I think it may have to do with if you try to load an image with JavaScript and then try to access its pixels. With async decoding, you then have to use the decode method to get a promise for when it is decoded.

It's also a best practice according to Addy Osmani.

In AMP all images also get decoding=async for the performance benefit.

In Next.js, the Image component uses decoding=async.

I don't see any downsides to adding decoding=async to all images, even those that we don't currently add loading=lazy. Basically, anywhere that a srcset can be supplied we can also inject decoding=async.

#2 @isaumya
4 years ago

You are absolutely right @westonruter. Haha! It's funny, I also asked in the Slack core channel if anyone has a trac ticket open about decoding="async" and no one replied to me. So, I thought, let's open one, and if there is already an existing ticket about this, at most it will get closed/merged with that ticket. I even tweeted about it.

I'm not clear on the specifics of the legacy reasons. I think it may have to do with if you try to load an image with JavaScript and then try to access its pixels. With async decoding, you then have to use the decode method to get a promise for when it is decoded.

I think you are right. But this still should be the default, as there is no easy way to add it to Gutenberg generated images as I stated above.

I'm not clear on the specifics of the legacy reasons. I think it may have to do with if you try to load an image with JavaScript and then try to access its pixels. With async decoding, you then have to use the decode method to get a promise for when it is decoded.

I think it would be beneficial even if there is a single image and no srcset and a single image url. Don't you think?

#3 follow-up: @ayeshrajans
4 years ago

This will be a nice one, thank you for opening the ticket.

I took a quick look at how lazy loading is currently implemented, and I think we should follow suit:

  • Add wp_img_tag_add_decoding_attr filter akin wp_img_tag_add_loading_attr to determine offer attribute value.
  • Add wp_img_tag_add_decoding_attr function akin wp_img_tag_add_loading_attr to make the actual change.
  • Update wp_filter_content_tags to call wp_img_tag_add_decoding_attr.
  • Make sure kses allow-lists the decoding attribute.
  • Add test coverage.

I would suggest to only add the decoding attribute only if loading=lazy is also present. This might(?) reduce sudden content reflows.

#4 @isaumya
4 years ago

Hi @ayeshrajans,
Thanks for the +1. I Googled about those filter you mentioned but couldn't find any documentation about it. Do you have any snippet to currently add it in the Gutenberg generated images? As the code block I provided above won't work for any Gutenberg images and there is no image_send_to_editor for Gutenberg, so I couldn't add it for Gutenberg generated pages.

If you have a working code snippet that will help a lot of people to add it right now to their WP sites until this gets added to the core code.

#5 in reply to: ↑ 3 ; follow-up: @westonruter
4 years ago

Replying to ayeshrajans:

Make sure kses allow-lists the decoding attribute.

This wouldn't be necessary since the decoding attribute is added on the frontend and is not stored in the post content. The loading attribute is not stored in post content either. Nevertheless, I see loading was explicitly added to Kses so might as well add it.

I would suggest to only add the decoding attribute only if loading=lazy is also present. This might(?) reduce sudden content reflows.

I don't believe this is necessary. I believe the layout shifting issue is only relevant to lazy-loading.

I took a quick look at how lazy loading is currently implemented, and I think we should follow suit:

Otherwise, your proposal for how to implement seems good to me!

Replying to isaumya:

If you have a working code snippet that will help a lot of people to add it right now to their WP sites until this gets added to the core code.

This will do the trick for all images in the post content:

<?php
add_filter(
        'the_content',
        static function ( $content ) {
                return str_replace( '<img ', '<img decoding=async ', $content );
        }
);

#6 in reply to: ↑ 5 @isaumya
4 years ago

Thanks a ton, @westonruter for the snippet. Actually, I was trying to avoid using the_content filter and instead was looking for a way to directly hook into the images rather than parsing the whole content. As when using the_content if the content is very large, PHP has to do really a lot of work executing and parsing it. I just thought using the_content was not an optimized code.

#7 @westonruter
4 years ago

Well, using the_content is how the loading attribute is added. It's how core adds srcset and sizes as well. The performance impact should be negligible. True it will be faster when when/if the replacement is done inside of wp_filter_content_tags() since there is less content to search through, but in the meantime a simple the_content filter will get you decoding=async now.

#8 @westonruter
4 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 5.8

Another explanation on the benefits of decoding=async from Matt Fantinel:

decoding="async" tells your browser it can try to parallelize loading your image. When your page is loading, it tries to decode both text and images at the same time. On lower-end devices though, decoding heavy images can take a while, and this might block the rendering of the rest of the content. With this option, the browser will try to proceed rendering the rest of the content and render the image later. This can be a great improvement to perceived performance.

I'm going to milestone this for 5.8 even though we're nearing the May 25th date for the feature freeze, although the patch should be small. This will probably get punted to 5.9.

#9 @westonruter
4 years ago

I'm checking with some colleagues who focus on web performance and it may actually be preferable to not add decoding=async to images in the first viewport. So it may indeed be that decoding=async should only be added if loading=lazy is also being added.

#10 @westonruter
4 years ago

OK, I checked with Malte Ubl and he says:

Another highly relevant nuance:
decoding=async does ~nothing on a reasonably sized image, but it is impactful on the occasional 20 Megapixel outlier. That page will still load slow but the UI thread won't be impacted.

If you know that you're appropriately sizing images, then it doesn't really matter what value the decoding attribute has. But if you don't know (e.g. you are WordPress and you unfortunately don't have image optimization build in), then async decoding helps prevent outlier bad performance while having effectively no impact on the average page.

Therefore, this does seem relevant to add to all images in WordPress since it is relatively common for oversized images to be served, where image smaller sizes may fail to be generated for the srcset. Granted, the chance of a 20MB image being added to a post is greatly reduced since WordPress 5.3 improved handling of big images. Nevertheless, I'm not sure the big image handling is always working. I just tried adding a 90MB image post and it was added to the post without reduced size or other image sizes being created. There probably wasn't enough memory available to do any manipulation of the image at all. So this shows that decoding=async is a useful protection to have enabled by default in WordPress.

#11 @isaumya
4 years ago

Thanks a ton, @westonruter for all the in-depth analysis on this matter. It seems more impactful than I initially thought, for most WordPress sites especially the sites using page builders. Improper image size is super common in WP page builder world.

#12 @westonruter
4 years ago

  • Milestone changed from 5.8 to Future Release

#13 @isaumya
4 years ago

Hey @westonruter, so 5.9 or maybe later to that?

#14 @westonruter
4 years ago

Yeah, 5.9. The feature freeze for 5.8 is tomorrow, so it seems too late to include for this release.

#15 @westonruter
4 years ago

Aside: HTTP Archive just added tracking for usage of the decoding attribute on images: https://github.com/HTTPArchive/legacy.httparchive.org/pull/206

#16 @westonruter
3 years ago

  • Milestone changed from Future Release to 5.9

This makes sense to do in 5.9 alongside #53675.

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


3 years ago

#18 @antpb
3 years ago

If #53675 is landed for 5.9 and review is needed on this ticket please feel free to reach out to me and I can prioritize with the media team! Thanks!

#19 @isaumya
3 years ago

Hey @westonruter,
One minor thing to add here. As this ticket is being considered with the combination with the not lazy loading first image ticket that's why saying.

I think for the first image that is not being lazy-loaded we should also have importance="high" as from Chrome 95 it is fully supported in the native browsers. Also if someone is using < Chrome 95 it will not cause any issues.

What do you think?

#20 @westonruter
3 years ago

@isaumya That's a good question. First of all, Priority Hints aren't yet standardized (see draft spec and Chrome Platform status) so I think it's a bit premature to add the the importance attribute to non-lazy images; it's only supported by Chrome and not any other browsers.

But secondly, I think adding importance should probably be the domain of themes (or FSE blocks) which can know for sure whether an image is in the first viewport and it is an important image. It's one thing to omit the loading=lazy attribute to let the browser decide whether to lazy-load, but it's another thing to add the importance=high attribute to tell the browser to PLEASE DOWNLOAD THIS NOW when we actually aren't 100% certain based on our heuristics for whether it will be in the first viewport.

#21 @isaumya
3 years ago

Hi @westonruter,
Yes, I agree with you on that. I thought for the above the fold images you guys are gonna add loading="eager" instead of ommiting it. Sorry my misunderstanding.

Generally, when I build custom themes for custom development I always add the loading="eager" importace="high" for the first above the fold image that is going to general the LCP value.

But thanks a lot for your reply. I am hoping (kind of sure) that the importance attribute will be standardized very soon. I think Firefox will do it first and hopefully, Safary will either not do it or will do it after 5-10 years or so. As the other chromium-based browsers will have it (I think) as they are chromium based.

Last edited 3 years ago by isaumya (previous) (diff)

#22 @westonruter
3 years ago

@isaumya It's actually not a best practice to add loading=eager to the first image, and to instead opt to just omit the loading attribute altogether. The reason for this is that in Chrome, for example, omission of the attribute implies that the browser should decide whether to lazy-load (e.g. based on the network connection). However, if you add loading=eager then it will download it unconditionally. See caution in Browser-level lazy-loading for CMSs:

If an element should not be lazy-loaded, require or encourage skipping the loading attribute entirely. While using loading="eager" is a supported alternative, this would tell the browser explicitly to always load the image right away, which would prevent potential benefits if browsers implemented further mechanisms and heuristics to automatically decide which elements to lazy-load.

#23 @isaumya
3 years ago

Yes @westonruter. But in many cases when you know for sure that whether the user is gonna visit the site on a mobile device or desktop that image is going to be the first image that they will see and cause the LCP. Doing this improves the LCP a lot as otherwise, the browser might download some other images below and then download the top image increasing the LCP. But yes I do agree with you it should not be used without knowing what exactly it is going to do and its consequences.

@westonruter
3 years ago

#24 @westonruter
3 years ago

  • Keywords has-patch added; needs-patch removed

I've drafted a patch.

I based it off of the pull request for #53675, so I haven't opened a PR yet since it is not yet committed.

Commit on GitHub: https://github.com/westonruter/wordpress-develop/commit/61d904c1f4e48363d4d2622cc47b8e09c611a75c

Note that most of the test changes are to make testing work on more environments, specifically where the wp-content directory is not in the standard location.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


3 years ago

#26 @hellofromTonya
3 years ago

With [52065] now in core, @westonruter do you have time to finish the PR today? (i.e. 5.9 feature freeze is today)

This ticket was mentioned in PR #1853 on WordPress/wordpress-develop by westonruter.


3 years ago
#27

  • Keywords has-unit-tests added

#28 follow-up: @westonruter
3 years ago

  • Milestone changed from 5.9 to Future Release

Punting this as we'll need to have more discussion than about this and do testing for what the defaults are, namely whether decoding=async should be on images in the first viewport where we now omit loading=lazy. We'll need to do more testing to see the impact.

#29 in reply to: ↑ 28 ; follow-up: @michaelbourne
3 years ago

Replying to westonruter:

We'll need to do more testing to see the impact.

What kind of testing did you have in mind? Happy to help here as I greatly want to see this added to core. I also saw no downsides in my tests with adding it to all images, but willing to deep dive it if needed.

#30 @isaumya
3 years ago

Hi @michaelbourne,
Yes, I also use the decoding="async" attribute to all image tags and have not faced any problems. Honestly, I also couldn't wait to see this being part of the core. But don't know when that will happen. I thought it's gonna be added to v5.9 but it seems it is not.

#31 in reply to: ↑ 29 @westonruter
3 years ago

Replying to michaelbourne:

What kind of testing did you have in mind? Happy to help here as I greatly want to see this added to core. I also saw no downsides in my tests with adding it to all images, but willing to deep dive it if needed.

Probably showing how specifically decoding=async improves performance in results from WebPageTest, showing before/after.

#32 @sergiomdgomes
3 years ago

I tried to find some performance research on this option but unfortunately came up empty-handed.

Is anyone aware of any research that indicates the degree of benefit we can expect from this, the situations it benefits the most, and whether there are any situations where adding decoding=async to an image could potentially cause it to render later, and thus have a negative impact on metrics like LCP?

Performance options tend to come with tradeoffs, instead of being asymptotically better, so it would be beneficial to understand what the tradeoffs are a bit better before potentially adding it to every image.

#33 @mihai2u
3 years ago

I have experience with this, and will take on the task to provide before/after benefits and potential negative circustances (there's just one scenario I am aware about, but I haven't measured its impact, as I didn't encounter it on a site, just as a hypothesis).

#34 @mihai2u
3 years ago

Hi,

I've spent time on this, and this was my thought process doing the research to confirm the viability of this addition and potential edge-cases.

Scenario with the biggest benefit, huge images

Tested these here:
https://files.urldocs.com/share/test-async-on/test-async-on.html
https://files.urldocs.com/share/test-async-off/test-async-off.html

Before running this test, I've ran this without a decoding attribute, and in tests I've discovered the default state of auto was triggering the decoding of the image on its own thread automatically due to the heuristics in Chrome.
This is something that came in newer versions of the browser, as it was not the case a half a year ago.
That is why for the tests to have a confirmation that we're testing sync vs async decoding, for the 'off' tests, I've included the attribute of decoding="sync", otherwise the tests (webpagetest, lighthouse, gpsi were yielding the same result in both scenarios).

the webpagetest results, running this 9 times for first view only:
https://webpagetest.org/result/220224_BiDc0D_D7J/
https://webpagetest.org/result/220224_BiDc0D_D7J/

There is no difference (or negative impact) on the LCP with the presence of async or sync decoding. The only measurable difference is in the Total Blocking Time increasing when decoding is setup to sync due to the long running task decoding the image on the main thread.

Adding decoding sync did not block the rendering of the text underneath it (according to the specs this is what I expected to see, but was not the case), so this isn’t a benefit after all with current browsers

Decoding async still shows the progressing loading of a webp / jpg, as it comes through the network, with no perceivable difference using one decoding setting over the other.

Conclusion: No negative impact on LCP. Positive impact on interactivity. Progressive loading is working and consistent with both decoding options.

Animated Images

Spec: “Animated images will become completely available only after all their frames are loaded. Thus, even though an implementation could decode the first frame before that point, the above steps will not do so, instead waiting until all frames are available.”
( https://html.spec.whatwg.org/multipage/embedded-content.html#dom-img-decoding )

Gif with decoding sync: https://files.urldocs.com/share/test-async-off-gif/test-async-off-gif.html
Gif with decoding async: https://files.urldocs.com/share/test-async-on-gif/test-async-on-gif.html

I've used global network throttling to test this one on most popular browsers on OSX.
https://www.npmjs.com/package/@sitespeed.io/throttle

As per the test, can confirm that with or without decoding async, the first frame of the gif is displayed just as quick, and as frames keep being downloaded, they're rendered sooner, before completing the download of all of the frames, which is an unexpected (positive though) difference vs. the spec on Safari, Firefox & Chrome.

Conclusion: async decoding on Gifs does not produce negative consequences.

Large images without width/height

Spec: Decoding sync indicates a preference to decode this image synchronously for atomic presentation with other content.

Because of this, I was afraid adding decoding async on large images that don't have width/height attributes defined would have a negative impact with layout shifts introduced.

Tested these here:
https://files.urldocs.com/share/test-async-on-webp-no-size/test-async-on-webp-no-size.html
https://files.urldocs.com/share/test-async-off-webp-no-size/test-async-off-webp-no-size.html

In either scenarios, with CPU & network throttling enabled, the text below the images was rendered in the correct spot in either scenarios.

Conclusion: No layout shift introduced by using decoding async on above the fold images.

Vector graphics

Spec: If decoding does not need to be performed for this image (for example because it is a vector graphic), resolve promise with undefined.

Tested this here:
https://files.urldocs.com/share/test-async-on-svg/test-async-on-svg.html
https://files.urldocs.com/share/test-async-off-svg/test-async-off-svg.html

The results here were surprising, as I was expecting there would be no difference, but I've noticed varying results with total blocking time increasing due to decoding times (and othertimes yielding 10ms)
https://pagespeed.web.dev/report?url=https%3A%2F%2Ffiles.urldocs.com%2Fshare%2Ftest-async-off-svg%2Ftest-async-off-svg.html
https://pagespeed.web.dev/report?url=https%3A%2F%2Ffiles.urldocs.com%2Fshare%2Ftest-async-on-svg%2Ftest-async-on-svg.html

Conclusions: There is no difference when including async decoding on SVG graphics, while based on the spec, it might be better to avoid adding a decoding async attribute on these type of image tags.

Many images, not lazy loaded

Included many images using async decoding vs. sync decoding to have a test scenario with a normal scenario where there's more images present on a page.

https://files.urldocs.com/share/test-async-on-many/test-async-on-many.html
https://files.urldocs.com/share/test-async-off-many/test-async-off-many.html

Past experience

We, at XWP, have defaulted to adding the async decoding for images on all projects for more than a year now, and this was based on the research on the positive impact on interactivity we've confirmed.

Next steps

TL;DR: Explored several edge-cases in order to find potential downsides from including this attribute. Used webpagetest, GPSI & local lighthouse tests for all tests to confirm conclusions. Used performance profiling to go deeper and confirm the data/conclusions when the results were too consistent in the lab tests.

A big thumbs-up for this to be the default option for all images, not just the ones in the_content.

mehigh commented on PR #1853:


3 years ago
#35

Have done the tests and covered the potential caveats I could think about, and this is passing with flying colors, as the lab+local tests have contradicted some of the conclusions I've drawn after reading the specifications.

Full result of the research posted on Trac :
https://core.trac.wordpress.org/ticket/53232#comment:34

TL;DR:
Explored several edge-cases in order to find potential downsides from including this attribute. Used webpagetest, GPSI & local lighthouse tests for all tests to confirm conclusions. Used performance profiling to go deeper and confirm the data/conclusions when the results were too consistent in the lab tests.
A big thumbs-up for this to be the default option for all images, not just the ones in the_content.

#36 @westonruter
3 years ago

@mihai2u Amazing and thorough research! As I commented on the performance repo:

I suppose the next question would be: should there be a way for site owners to selectively turn off decoding=async as they can for loading=lazy? In your research you found no downsides, then perhaps not?

#37 @mihai2u
3 years ago

Correct, there's no need for site owners to selectivey turn it off according to the findings.

#38 follow-up: @adamsilverstein
3 years ago

@westonruter - is 53232.1.diff ready? I noticed a bunch of small changes in the test files, can we leave those off to make the patch smaller or are those matching existing test paths?

#39 @adamsilverstein
3 years ago

  • Milestone changed from Future Release to 6.0
  • Owner set to adamsilverstein
  • Status changed from new to assigned

#41 @adamsilverstein
3 years ago

53232.diff is a refresh against trunk with some unrelated path changes in test files removed. Pushed to https://github.com/WordPress/wordpress-develop/pull/2409 as well for test runs.

#42 @adamsilverstein
3 years ago

53232.2.diff fixes failing avatar test

#43 @isaumya
3 years ago

I am so happy to see that this is finally coming to life. Thank you everyone for your support and suggestions. Can't wait to experience this in v6.0 and finally removing the custom functions to removing the decoding async attribute from the img tags.

#44 in reply to: ↑ 38 @westonruter
3 years ago

Replying to adamsilverstein:

I noticed a bunch of small changes in the test files, can we leave those off to make the patch smaller or are those matching existing test paths?

Those changes can be left out, yes. I mentioned them in my above comment.

#46 @isaumya
3 years ago

Hi @adamsilverstein,
I looked at your PR in GitHub and just have one question. So, when these changes are applied will it also work on images added by Gutenberg blocks like the image block, gallery block, cover block, etc.
The reason I am asking this is that I saw that the changes have been made in media.php under wp-includes and within tests. But did not see any changes in the Gutenberg files.

#47 @adamsilverstein
3 years ago

So, when these changes are applied will it also work on images added by Gutenberg blocks like the image block, gallery block, cover block, etc.

@isaumya yes - these changes are applied via a content filter so this should work for almost every image in the content, regardless of how you added it (Gutenberg, classic editor, etc). The only exception would be sites that disable "the_content" filtering.

#48 @adamsilverstein
3 years ago

53232.3.diff addresses feedback raised on the PR

#49 @adamsilverstein
3 years ago

  • Keywords commit added

#50 @adamsilverstein
3 years ago

@westonruter this looks good to me, marked for commit, nice work!

This ticket was mentioned in Slack in #core-media by adamsilverstein. View the logs.


3 years ago

#52 @peterwilsoncc
3 years ago

  • Keywords needs-refresh added; commit removed

@adamsilverstein The latest PR is in draft and there are a few error_log() calls in 53232.3.diff so I've removed the commit keyword.

Will you have bandwidth to refresh the patches or would it be best if the ticket were bumped from the 6.0 milestone?

#53 @adamsilverstein
3 years ago

@peterwilsoncc thanks, i will update the patch/pr.

#54 @adamsilverstein
3 years ago

@peterwilsoncc updated patch, ready for review.

The latest PR is in draft

I generally open draft PRs to run tests and garner feedback but since we don't merge in GitHub I always keep them as draft. The draft status is not an indication of readiness!

#55 @peterwilsoncc
3 years ago

53232.5.diff is 53232.4.diff with a few minor modifications:

  • fixes the alignment of a few docblock @param tags
  • expands the description of wp_img_tag_add_decoding_async_attr()
    • generically indicates it adds the decoding attribute rather than the value
    • expands the explanation to indicate what the attribute does, accepted values and how developers can modify it.

I've also noticed that how this works is quite different to adding the default lazy tag, lazy includes:

  • `wp_lazy_loading_enabled()
  • wp_get_loading_attr_default()
  • wp_omit_loading_attr_threshold()

This results in some differences in how the decoding attribute is added by default to images, particularly avatars. As these features are very similar in intent, it seems logical to use a similar APIs.

I realise there was some discussion above as to whether developers ought to disable it but we need to account for environments in which they need to. For example headless environments converting WP images tags to their smarter defaults.

In wp_get_attachment_image() the attribute ought to be added as a default argument to allow developers to override the value by passing an argument. The new argument should be added to the docblock too.

Sorry I didn't post all this yesterday, but the error_log()s in the previous patch were early in the source and an immediate blocker.

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


3 years ago

#57 @adamsilverstein
3 years ago

Thanks for the feedback and update patch @peterwilsoncc - will review & test!

#58 @adamsilverstein
3 years ago

53232.7.diff is a refresh of 53232.5.diff against trunk

#59 @adamsilverstein
3 years ago

53232.8.diff adds the decoding attribute to the new test_wp_filter_content_tags_filter_with_identical_image_tags_custom_attributes test

@peterwilsoncc this should be good to go now, can I go ahead and commit once tests pass?

#60 @costdev
3 years ago

  • Milestone changed from 6.0 to 6.1

With 6.0 Beta 1 released on Tuesday, we are now past Feature Freeze for the 6.0 cycle. Moving this ticket to the 6.1 milestone.

#61 @adamsilverstein
3 years ago

53232.9.diff addresses feedback on the PR.

@costdev
3 years ago

Patch refresh.

#62 @costdev
3 years ago

  • Keywords needs-refresh removed

53232.10.diff refreshes the previous patch with the following updates:

  • Fix failing tests.
  • Add dedicated tests for wp_img_tag_add_decoding_attr().
  • Change false === strpos() to ! str_contains(), which was polyfilled in WordPress 5.9.
  • Fix some documentation alignment issues.
  • Adjust the documented type of $value for the wp_img_tag_add_decoding_attr filter.
    • This was previously string|bool, but is documented to be string|falsey. Reasonable falsey values for a callback for this filter would be '', false or null. The type for $value has been adjusted to string|false|null.
  • @peterwilsoncc has applied these changes to PR 2409.

@adamsilverstein can you take a look at the above changes and let me know your thoughts?

#63 @costdev
3 years ago

  • Keywords needs-dev-note added

#64 @adamsilverstein
3 years ago

This looks great @costdev thanks for the improvements! I pushed one tiny doc block improvement (commit).


#65 @costdev
3 years ago

@adamsilverstein The PR looks good to me, but the updated patch 53232.5.2.diff doesn't have all of the changes in the PR. Should this have been a refresh of 53232.10.diff instead?

I think PR 2490 is ready for commit consideration, and the Dev Note can be written.

#66 @adamsilverstein
3 years ago

hmmm, should have been all the changes from the PR, fixing

#67 @adamsilverstein
3 years ago

@costdev I corrected the patch - uploaded the correct file this time :) see 53232.11.diff

#68 @spacedmonkey
3 years ago

  • Keywords needs-refresh early added

This correct patch looks good to go to me. I have added early keyword. But it does need a refresh as version numbers need to be changed.

#69 @peterwilsoncc
3 years ago

53232.12.diff :

  • refreshed against trunk
  • version numbers changed to 6.1.0 in docblocks

#70 @peterwilsoncc
3 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 53480:

Media: Add decoding="async" to image attributes.

Dynamically add decoding="async" to image tags on the front end of a site to instruct browsers to download them in parallel.

Modifies wp_get_attachment_image(), get_avatar() to include the attribute by default. Modifies wp_filter_content_tags() to add the attribute during the front-end render of the site.

Introduces wp_img_tag_add_decoding_attr() to take an image tag and modify it to include the attribute. Introduces the filter wp_img_tag_add_decoding_attr used to define the default value for the attribute.

Props adamsilverstein, ayeshrajans, costdev, flixos90, hellofromtonya, isaumya, michaelbourne, mihai2u, mitogh, sergiomdgomes, spacedmonkey, westonruter, peterwilsoncc.
Fixes #53232.

#71 @SergeyBiryukov
3 years ago

In 53481:

Media: Some documentation and test improvements for wp_img_tag_add_decoding_attr():

  • Add a @since note for the decoding attribute in wp_get_attachment_image().
  • Adjust wp_img_tag_add_decoding_attr() DocBlocks per the documentation standards.
  • Wrap some long sprintf() calls in unit tests for better readability. In at least one case, the_content was unnecessarily passed to sprintf() as an extra (unused) parameter.

Follow-up to [53480].

See #53232.

This ticket was mentioned in PR #2943 on WordPress/wordpress-develop by peterwilsoncc.


3 years ago
#72

  • Keywords needs-refresh removed

#73 @peterwilsoncc
3 years ago

Reopening this as I missed some tests from Adam's patch while committing [53480], I've created a PR to ensure they still pass.

#74 @peterwilsoncc
3 years ago

In 53652:

Media: Add tests for wp_img_tag_add_decoding_attr().

Add new tests skipped in original commit. Follow up to [53480].

Fixes #53232.

This ticket was mentioned in Slack in #core-media by adamsilverstein. View the logs.


2 years ago

#77 @flixos90
16 months ago

In 56690:

Media: Rely on wp_get_loading_optimization_attributes() to add decoding="async" to images.

The wp_get_loading_optimization_attributes() function was introduced in 6.3, as a single centralized place to control loading optimization attributes for various tags, most importantly images.

This changeset consolidates the decoding="async" optimization, which was added in 6.1, to occur solely as part of wp_get_loading_optimization_attributes(), removing duplicate code and allowing centralized filtering based on [56651].

As part of the change, the wp_img_tag_add_decoding_attr() function has been deprecated. The filter of the same name continues to be maintained for backward compatibility, as before covering only images that are part of a content blob such as post content (the_content).

Props pereirinha, mukesh27, joemcgill, flixos90.
Fixes #58892.
See #53232.

Note: See TracTickets for help on using tickets.