@felixarntz have you thought about how we could have more precise heuristics going forwards that can take the semantics and structure of blocks into account to get a sense for what is actually deferrable? For example, an image block or a site logo used in a header template part would be strong indicatives of being above the fold. "The first image of the content" seems instead like a rudimentary measure, that varies a lot depending on preceding layout. With block themes we should have some ahead-of-time awareness of layout which we can use to produce more meaningful instructions.

@mtias Great point, there is definitely room for further refinement as we're moving towards a world of block themes. I've been thinking about detecting the header template part as well.

The analysis I've conducted using the fix here (even though technically I agree that "the first content image" sounds rudimentary) has proven that it already significantly improves performance across the board, so I think it's a useful enhancement on its own - I don't think we should overcomplicate it for now. Once there are more block-based themes available, I agree it would make sense to refine further by using the semantics of blocks. For example, we could also make decisions like "if the first block is a gallery block with three images per row, omit lazy-loading on the first three images".

Also wanted to note that I found that 3,801 themes (out of 4,145) make use of the main element. I was surprised (and heartened) to see that 90%+ of themes use this semantic element. For the AMP plugin, we've used this as an easy way to identify all images that appear in the header, by looking for all that occur before the main. Granted, this wouldn't work the same way in core since the DOM of the page is not available, but eventually in a FSE context this could be implemented similarly through filtering of blocks.

@felixarntz yes, I didn't mean rudimentary in a disparaging way, just that it feels we could be doing more tailored targeting there with block semantics.

Testing with twentynineteen, the header/top image seems to still get the lazy attribute, while for content's first image which you have to scroll to reach does not get it.

verified working correctly in twentytwenty so must be something specific about how 2019 outputs that top image.

@adamsilverstein

Testing with twentynineteen, the header/top image seems to still get the lazy attribute, while for content's first image which you have to scroll to reach does not get it.

That was expected behavior with the proposed fix, since that theme is rendering the featured image outside of the loop. It raises a good point though that maybe we should support that case, since it is fairly common to render the featured image on top of the page when is_singular(). I've implemented this in 03a95a9. Similar to the other changes in this PR, this is not 100% reliable, but it won't have adverse effects, and I'm assuming it will bring even better LCP results than without it.

I will run a separate follow-up A/B test to compare. Until then, happy to hear your feedback @westonruter on this new commit as well.

@adamsilverstein @westonruter @ThierryA I've ran a test between both versions of the fix (see above #1580 (comment)) across the 30 most "popular" themes based on the wordpress.org API. To clarify, the two different versions are:

• "corefix1" counts all the_content and the_post_thumbnail images as long as they're also in_the_loop().
• "corefix2" is based on the latest iteration here, so it supports one extra condition where the_post_thumbnail is also counted for a "singular" context, even if not in_the_loop() (where it e.g. covers the special behavior in Twenty Nineteen).

The difference between the two is barely noticeable:

• For both LCP and image KB loaded, the median difference is 0%.
• Especially for LCP there are no notable wins on either end, although the number of tests where there is an improvement in "corefix2" is slightly higher than the number of tests with an improvement in "corefix1". That isn't statistically sufficient for a decision though.
• In image KB, there is barely any difference, in most individual tests the difference is 0%. It's in fact only 4 themes where any scenario gave a result other than 0%, and two of those are indeed Twenty Nineteen and Twenty Seventeen, which both for a singular post have that special behavior that they display the featured image outside of the loop.

Given that there is no win across the board from also counting featured images in singular context outside the loop and since it is also a special case anyway, I suggest we revert 03a95a9 again and go with the original approach. I don't think adding a special case just for some core themes with a non-standard implementation is justifiable. Thoughts?

Given that there is no win across the board from also counting featured images in singular context outside the loop and since it is also a special case anyway, I suggest we revert 03a95a9 again and go with the original approach. I don't think adding a special case just for some core themes with a non-standard implementation is justifiable. Thoughts?

I agree your code here should be as generalized as possible and you should revert the final change. One thing to consider would be changing twentynineteen itself since we maintain that in core as well and I'm guessing it is still in very wide usage.

@adamsilverstein Reverted!

We can look at changing Twenty Nineteen as well, but it might be tricky. While it should be possible to move that header content into the loop without affecting the actual design, a problem would be child themes. For example a child theme that overrides header.php and nothing else would then most likely see that image twice because they would still use the template based on the old version of the theme.

Or maybe we can actually "fake" the real loop within the header.php file, that should be more realistic.

Given that there is no win across the board from also counting featured images in singular context outside the loop and since it is also a special case anyway, I suggest we revert 03a95a9 again and go with the original approach. I don't think adding a special case just for some core themes with a non-standard implementation is justifiable. Thoughts?

Sounds totally reasonable to me, +1!

I'm actually having some "second thoughts" on this. As far as I understand this is a browser problem. The browsers should be able to detect when an image is going to be in the viewport and load it eagerly. That's in the specs.

It is somewhat unfortunate that this doesn't currently work particularly well as seen by @felixarntz's post, but I'm not 100% sure WordPress should be trying to partially remedy the situation. No matter what we do it will always be a partial, hit-and-miss solution as there's no chance to guess the viewport from the server. The browsers are much better equipped for that and should be fixing it.

Another thing is that WordPress already slows things down pretty considerably by implementing srcset and sizes and loading attributes on-the-fly at the front-end. Making many billions of page loads on many millions of sites even a bit slower is a hefty price to pay for a somewhat partial improvement.

Thinking it would be better to see if the browsers fix this and in case they don't/won't perhaps try to add some code on the server. My suggestion would be to come back to this in 6 months to a year and asses it again.

@azaozz

I'm actually having some "second thoughts" on this. As far as I understand this is a browser problem. The browsers should be able to detect when an image is going to be in the viewport and load it eagerly. That's in the specs.

The problem is that the browser is only able to detect that when the layout has been computed. This is why there is a slight delay on lazy-loaded images at the top - the browser still today loads the top images as soon as it knows they're at the top, but that is after the layout has been computed. Waiting for this layout computation is what causes the LCP regression I outlined in the post. However, we shouldn't just disable lazy-loading either, since then all images would loaded right away, which is obviously bad.

No matter what we do it will always be a partial, hit-and-miss solution as there's no chance to guess the viewport from the server. The browsers are much better equipped for that and should be fixing it.

See above, the browsers aren't particularly better equipped unfortunately due to the layout computation being required. I used to believe the same thing that you're saying, but it is unfortunately not the case. I agree a partial fix isn't perfect, but it is still a partial fix, and the analysis has shown that it brings a notable performance improvement in most cases. The performance improvement things brings on the client side is vastly more relevant than a tiny bit of extra logic being run on the server (which is not particularly expensive).

Thinking it would be better to see if the browsers fix this and in case they don't/won't perhaps try to add some code on the server. My suggestion would be to come back to this in 6 months to a year and asses it again.

I will keep pushing and we can see if at some point the browser can improve some heuristics for this, but the browsers are limited in fixing this problem per my explanation above. The enhancement here is entirely non-disruptive, there is no API change, so it could even be removed again at some point, should a better solution arise. This isn't going to be the case anytime in the not-so-distant future though. We should proactively improve performance of WordPress, not hope that somebody else will do it for us.

Right, browsers are inherently limited in that they cannot foresee the future. It's impossible for them to know whether to start loading a lazily-loaded image prior to layout since they won't know if it is going to be in the viewport. WordPress can help the browser out by making educated guesses as to which images are going to be in the first viewport so that the browser doesn't have to wait for layout to occur. This is achieved by omitting the loading=lazy attribute from the images likely in the first viewport.

Seconding; in an ideal world, the browser would handle this. But it can't, until it's too late for it to be beneficial.

Note that this is also going to start creating warnings for everybody who's checking their site in Lighthouse: https://developer.chrome.com/blog/new-in-devtools-95/#lighthouse

Note also that we're also expecting priority hints to be a thing soon (see https://www.chromestatus.com/feature/5273474901737472#:~:text=Priority%20Hints%20provide%20developers%20a,preload%20status%20of%20a%20resource.); that firmly solidifies the direction of travel as "tell the browser how it should handle resources", not, "hope that this is fixed by magic".

I am in favor of prioritizing the end user experience vs the ideal implementation details (without ignoring its cost indeed).

And the analysis has shown that it brings a notable performance improvement in most cases. The performance improvement things brings on the client side is vastly more relevant than a tiny bit of extra logic being run on the server (which is not particularly expensive).

There is a clear improvement in most cases.

The enhancement here is entirely non-disruptive, there is no API change, so it could even be removed again at some point, should a better solution arise.

While the implementation is not perfect, its cost is very low and non disruptive.

To me it is a clear cut, the pros outgrows the cons.

@azaozz If we make these variables static, there still has to be a way to reset them. There is no way to make WP's function-based code 100% bulletproof to other developers - if they want to misuse/break it they can already in a million ways.

Your latest commit broke the tests because now the filter isn't re-triggered for new tests. So again, I don't disagree with the approach of making it static, but it doesn't work just like that. There has to be a way to "reset" the filter. I suggest we put it into a function like wp_omit_loading_attr_threshold( $force = false ), with the $force parameter allowing to enforce re-running it, should the value change, which is necessary for tests to work.

If we make these variables static, there still has to be a way to reset them.

Hmm, yes but, why? We have to change the functionality just for the sake of testing? Shouldn't be the other way round -- make this work the best way possible then add/fix/tweak the tests to work with the code.

Your latest commit broke the tests because...

Yeah, was looking at how to "suggest" changes, i.e. make a variant of the existing patch. However that seems impossible on GH. The only way would be to make another PR which is not good in this case. But you're right, should have added a comment explaining it.

Basically I'm thinking that both changes should be made inside wp_get_loading_attr_default(). Then it won't need yet another function like wp_increase_content_media_count() which doesn't do anything in production and even could be misused, eventually, because it can set or reset that count instead of using the filter. The only thing stopping this now are the tests that need to be made to work with the changed code.

I'll look at changing/fixing the tests later today and propose another change (well, commit another change as per the above). Then we can see/decide :)

@azaozz

Hmm, yes but, why? We have to change the functionality just for the sake of testing? Shouldn't be the other way round -- make this work the best way possible then add/fix/tweak the tests to work with the code.

I overall agree with that statement. However, code still has to be written in a testable way if we want to add tests for it - and having state which cannot be controlled in tests prevents that (at a minimum we need to allow reset between tests). We simply cannot "lock" the code so that nobody can override it, as otherwise we also lock it for tests. What we can do is define solid patterns to make it less likely to happen "by accident", and to your point the approach with static variables and dedicated functions makes the access more intentional than just having a global.

Basically I'm thinking that both changes should be made inside wp_get_loading_attr_default(). Then it won't need yet another function like wp_increase_content_media_count() which doesn't do anything in production and even could be misused, eventually, because it can set or reset that count instead of using the filter. The only thing stopping this now are the tests that need to be made to work with the changed code.

See above, I don't think there's a way to make this entirely "locked" static code testable without reducing our test coverage. The reason most WP core tests work out of the box without any of this extra "magic" is that core uses globals, and those can always easily be overwritten, and they automatically get reset between tests. Anywhere where we don't use globals, a special mechanism is needed.

I'll look at changing/fixing the tests later today and propose another change (well, commit another change as per the above). Then we can see/decide :)

Curious what you come up with. I'll for now update the PR based on your latest commit to make this static mutable again to "unlock" it.

With all that said, the discussion we're having here talks about general code and test patterns in WP core, so I'm not sure this issue is the proper place to have it. Core uses globals throughout, so from that perspective I'd even be in favor of sticking with the approach. If we change this here, it doesn't mean anything, the rest of the codebase will still follow the old pattern, and almost no other core developers will be aware of the new pattern unless it is explicitly called out e.g. in coding standards documentation - it could even be that some folks don't agree with it, another reason not to silo this wide-reaching topic discussion in a very specific PR like this.

Another option for making memoization testable is to move the code into a class and making the memoization value a property. If the class is used as a singleton, the static is not needed; else, it is. That's a design choice. Tests can access the property via reflection or by instantiating a new instance for each test.

Advantages to this approach are:

• the avoidance of added test related code into the production code
• ability to break up larger functions into smaller tasks with private/protected methods

code still has to be written in a testable way...

Right, the question here is what are we testing. As the wp_omit_loading_attr_threshold filter has to run only once per page load (see above), we should probably be testing that (if it makes sense), not running it multiple times which is not supposed to happen.

We simply cannot "lock" the code so that nobody can override it

Hm, we are not "locking" the code, think of $content_media_count as a private property of a class. Functionality is pretty similar.

The current code as of 1c218d7 does not achieve these objectives:

• The wp_omit_loading_attr_threshold filter can be run more than once by using the just introduced wp_omit_loading_attr_threshold( $force = false ) function.
• The wp_increase_content_media_count( $amount = 1 ) can be used to arbitrarily reset the $content_media_count which should not be re-settable/changeable at all. For example: think of a theme or plugin doing something like wp_increase_content_media_count( 1000 ), i.e. disabling the whole thing in the wrong way.

With all that said, the discussion we're having here talks about general code and test patterns in WP core...

Hmm, not sure these are "new" things. It's best if tests cover the intended usage, that has been the case for a long time. The current/old tests do not follow the intended usage exactly any more, so they will need adjusting.

The need to change the tests arises from making some changes to the code, namely the wp_omit_loading_attr_threshold filter has to run only once per page load, and the $content_media_count variable has to be "private" or non-changeable (and it's pretty bad idea to introduce a PHP global at this point).

...move the code into a class and making the memoization value a property.

Yes, this would have worked if that code was in a class/wrapper. However changing this is a major refactoring considering back-compat, etc. It may happen one day, but not for 5.9. This patch is a relatively small correction/enhancement to how WordPress adds the loading HTML attribute for images and iframes.

@azaozz

we should probably be testing that (if it makes sense), not running it multiple times which is not supposed to happen.

Agreed, we should be testing that as well, this requires adding one more test. Update: I've added one in b0addfa.

Hm, we are not "locking" the code, think of $content_media_count as a private property of a class. Functionality is pretty similar.

The current code as of 1c218d7 does not achieve these objectives:

• The wp_omit_loading_attr_threshold filter can be run more than once by using the just introduced wp_omit_loading_attr_threshold( $force = false ) function.
• The wp_increase_content_media_count( $amount = 1 ) can be used to arbitrarily reset the $content_media_count which should not be re-settable/changeable at all. For example: think of a theme or plugin doing something like wp_increase_content_media_count( 1000 ), i.e. disabling the whole thing in the wrong way.

See my previous comment. What you are proposing here makes this code untestable, except if the WP core test suite was starting a new PHP process for every single test which... would make the tests take hours, if not days. I understand this is similar to a private property of a class - to @hellofromtonya's point, that approach would be an appropriate solution, and this is how most sophisticated PHP projects do it. Still, I agree this is too much of a refactoring which this PR is not the place for.

The limitation in the approach you're proposing is that the point of only running once per page-load only applies to that, a page-load. However, PHPUnit tests aren't like a page load, they run tons of things that would normally happen through tons of page loads in a single run. If a theme or plugin does wp_increase_content_media_count( 1000 ), it's clearly wrong but this is not a problem related to this PR - you can already break core like $GLOBALS['post_types'] = 'not-an-array' or in a million other ways.

Hmm, not sure these are "new" things. It's best if tests cover the intended usage, that has been the case for a long time. The current/old tests do not follow the intended usage exactly any more, so they will need adjusting.

This statement confirms to me that this is not the right place to make this change. We are speaking about a change in coding paradigm that requires refactoring a ton of existing tests and also the related production code - basically "remove globals and manage state elsewhere in a testable way", which is surely not something we can or should arbitrarily decide in an unrelated PR. I think we should revert this back to using globals - we could then still change the code so that the filter runs once per page-load, while still keeping it testable.

What you are proposing here makes this code untestable

Hmmm, not exactly? Lets look at the tests. The problem with the test for the wp_omit_loading_attr_threshold is that it is being initialized more than once with different values. However I don't see what is being tested by initializing it multiple times. Yeah, I understand that using static in functions may not be a very well used method, but it seems simplest and fastest. Feel free to implement that in a different way.

Same for the $content_media_count variable. The current/old tests were written to test a global variable. However it was established that it should not be a global variable and the code was changed. So the tests will have to be changed to account for that. A good way would be if the tests do not access that variable at all, as that's how the code is supposed to work now.

This statement confirms to me that this is not the right place to make this change. We are speaking about a change in coding paradigm that requires refactoring a ton of existing tests...

Uh, sorry but I don't seem to understand exactly what you want to say. What's happening with this patch is: you asked for a review, I did a review, this review established that some changes are needed, the changes were implemented. What's left is to remove the old tests that were written with the old code in mind, and to add new tests that cover the new code. What am I missing?

@azaozz

Hmmm, not exactly? Lets look at the tests. The problem with the test for the wp_omit_loading_attr_threshold is that it is being initialized more than once with different values. However I don't see what is being tested by initializing it multiple times.

The filter being applied as expected is being tested. If we can only call it once, we are unable to test that. We could remove that test, but then the production code function could literally be function() { return 1; } (i.e. without applying the filter), and the remaining tests wouldn't catch that.

Uh, sorry but I don't seem to understand exactly what you want to say. What's happening with this patch is: you asked for a review, I did a review, this review established that some changes are needed, the changes were implemented. What's left is to remove the old tests that were written with the old code in mind, and to add new tests that cover the new code. What am I missing?

The tests were already updated to fit the new code, they still test that the filter is applied correctly, and they still pass.

The filter being applied as expected is being tested. If we can only call it once, we are unable to test that.

Ummm, you mean for a filter to be tested is has to run multiple times? Why? We know the default value and we know the filtered value. Running the filter is supposed to change the default to the filtered value. What's the point in running the same test again?

The tests were already updated to fit the new code..

If I'm not mistaken that update should have included a change to exclude use of the (now supposedly "private") $content_media_count variable. Again, may be missing something here too...

Ummm, you mean for a filter to be tested is has to run multiple times? Why? We know the default value and we know the filtered value. Running the filter is supposed to change the default to the filtered value. What's the point in running the same test again?

Technically, if we only run the filter once (e.g. with returning 3), it could also be that the production function has no filter at all and it just returns 3 hard-coded. This is arguably a bit of nit-picking, but there is another more significant problem: Even if we only ran the filter once in that tests, we would still need to run it again in one of the other tests. If we in any test use the filter to set the number to 3, it will be 3 throughout all tests, even the ones where we want the default value of the filter to be used. For the content media count, this is even more problematic: There is more than one test that needs to use this functionality, but for any test that is not the very first one being run, it will never start at a count of 0 (since the other test already increased the count). This is why running the filter as well as the content media count variable have to be reset between tests.

There is surely some way to make the tests all pass without a reset, but it would make them hard to understand as well unstable (e.g. what if they run in a different order), since it would pollute the overall state which is a big no-go in tests. Global state needs to be reset between tests so that we mirror production behavior, i.e. ensuring the filter runs once per test which is the test "equivalent" of per page load.

Today, after every core test, all globals are reset, all filters and actions are removed, etc. We have to ensure here as well that the state modified via one test doesn't affect another, that's why a simple static without a way to reset won't do it.

There is surely some way to make the tests all pass without a reset, but it would make them hard to understand...

Right, testing "static caching" is a bit different and somewhat harder. Think of it as testing PHP constants: they need to be tested if they are set, and are set to the proper values, but no other tests are needed as they can never be changed later. So generally they require less testing that variables, etc.

Today, after every core test, all globals are reset, all filters and actions are removed, etc.

Yes, and the tests here do exactly the same: there are no globals, and the filter is removed. However there is a constant that is set by WordPress when using this functionality. It cannot be reset which is the proper behavior as that's exactly what happens in production.

In any case, you're an experienced developer and know this part of the code very well. We worked really well together when implementing the main functionality. This is a relatively small (by code size) enhancement. Lets add it as-is if you think it's fine.

@azaozz It's a good point to bring up the comparison with constants, particularly for the filter which shouldn't change its value on page load. The thing with those though is that they're typically defined early on at runtime (e.g. in wp-config.php, if a constant is defined within a "random" function that needs to be tested, it would make adding test coverage for it very challenging, if not impossible as well.

That said, it briefly made me think about whether we should actually have a constant here instead of a filter. That probably wouldn't be flexible enough though since the filter may be used in different ways e.g. based on which template is currently being loaded, which we wouldn't be able to do with a constant.

Thanks for trusting my judgement here. We're indeed nit-picking about a rather minor part of the functionality, so we can probably move this forward.

That said, it briefly made me think about whether we should actually have a constant here instead of a filter. That probably wouldn't be flexible enough though..

Right, thinking a filter is a lot better there, as it is now.

We're indeed nit-picking about a rather minor part of the functionality, so we can probably move this forward.

Exactly right 👍

Committed in https://core.trac.wordpress.org/changeset/52065.