Use single regex for loading/srcset/sizes, and only modify attachment images #20

felixarntz · 2020-02-04T21:38:18Z

This PR addresses #2, and various other related concerns:

It introduces a new function wp_filter_content_attachment_images() which essentially is a copy of core's wp_make_content_images_responsive(), modulo the addition of an optional $context parameter and the second foreach loop (for each attachment image). This is the key change in this PR, which addresses Optimize performance of regex for adding loading attribute #2.
- The call to the core function wp_image_add_srcset_and_sizes() remains in wp_filter_content_attachment_images(). It is now only called if the $context (the current filter) is the_content, because that is the only filter this logic currently applies to, and it shouldn't be modified here because it's unrelated (even though maybe worth considering in the future). The check for an existing srcset attribute was moved here because it shouldn't determine the img tags to generally iterate through.
- wp_filter_content_attachment_images() is hooked into the regular content filters like the previous wp_add_lazy_load_attributes() was.
- wp_make_content_images_responsive() is unhooked from the_content. Once this is merged to core, the hook should be removed, and the function could be deprecated.
A valid concern pointed out by @westonruter in a comment on the announcement post is that replacing random images that are not WP attachments may unexpectedly add loading attributes to images that shouldn't be modified, such as tracking pixels. Since WordPress core's existing logic already gets the attachment IDs and even image meta anyway, there's no performance concern here.
A new function wp_image_add_loading() now takes care of modifying a single image tag. Its name, signature and behavior is inspired by the existing wp_image_add_srcset_and_sizes(). It now also receives the $image_meta and $attachment_id already fetched by core anyway. There are now also passed to the wp_set_image_loading filter.
In order to maintain the current behavior, the filter priority was set to the default of 10, from the previous 25. This is still after do_blocks() (which runs on 9) so should cover enough, while at the same time ensuring there's no unexpected breakage with srcset / sizes. Related: Fix priority #12

…nt images.

felixarntz · 2020-02-07T09:43:20Z

@joemcgill Could you maybe have a look at this?

azaozz · 2020-02-07T20:44:33Z

wp-lazy-loading.php

 	}

 	// The following filters are only needed while this is a feature plugin.
 	add_filter( 'wp_get_attachment_image_attributes', '_wp_lazy_loading_add_attribute_to_attachment_image' );
 	add_filter( 'get_avatar', '_wp_lazy_loading_add_attribute_to_avatar' );
+
+	// The following relevant filter from core should be removed when merged.
+	remove_filter( 'the_content', 'wp_make_content_images_responsive' );


wp_make_content_images_responsive is used in a lot of plugins, some with > 1m installs. Removing or changing how it works is not a good idea.

It wouldn't be changed, it just would no longer be used by core. Also I'm curious how it is being used by plugins?

Mostly to run content through it: https://wpdirectory.net/search/01E0PTA8MSD8ZHCC7V4QDG9EV3 but some may be removing it, which will fail if it is not used in core.

Also deprecating a "popular" function is generally not a good idea. I know, sometimes there's no way around it, but not convinced this case qualifies :) (There are about 200 plugins that use the function. If we rename and deprecate it, they will have to "catch up" or will fill millions of error logs with "deprecated" entries.)

For the purposes of plugin development, the function needs to be replaced.

This discussion can be moved to an issue for wider input from the media team closer to any merge proposal.

@peterwilsoncc How do you mean "the function needs to be replaced", can you clarify? Not sure I understand.

@felixarntz I mean if you'd like to use a single regex while developing this plugin, you'll need to remove the core filter and replace it with a duplicate in this plugin (as the team has done in this PR).

This allows the naming/deprecation discussion to happen outside of this PR.

azaozz · 2020-02-07T20:57:48Z

wp-lazy-loading.php

+		if ( $add_loading && false === strpos( $filtered_image, ' loading=' ) ) {
+			$filtered_image = wp_image_add_loading( $filtered_image, $image_meta, $attachment_id, $content, $context );
+		}


Doing that here will bypass all images that don't have a wp-image-#### class. This means all images that were not added directly in the post editor, and images that were added in the editor by pasting the URL will be excluded.

As images are not supported in comments and excerpts by default, that will make it pointless to filter there at all. Chances are that even if the users have added images there (perhaps by using a plugin), the images will not have the required class name.

Frankly don't think this limitation makes sense. We are still looking only in user-added content. Excluding some images because they are not "local" or depending on the way they were added doesn't seem good?

I agree with that sentiment, but there's also no harm in being conservative at the start and only covering attachment images. We can easily make this work also with other images too (pass attachment ID 0), but that doesn't mean we need to right away.

I don't have a strong opinion on this, but 3P images, e.g. tracking pixels, may have different implications. We assume there's no issue, but we don't know for sure. Covering 3P images here could easily become a follow-up (separate PR or even separate core release if needed).

no harm in being conservative

Right, but this is mostly an "artificial" limitation: "You get lazy loading of images but only if they are uploaded to WP and are inserted properly by using the editor. Other images will not be lazy-loaded because...." (we said so?) :)

But yes, I agree this is not a huge issue as the majority of images in post_content will have the attachment ID class (as it has been used for srcset and sizes for a while). Skipping images that don't have it will skip all "images from URLs" perhaps including images from CDNs, which would be pretty disappointing for the feature.

wp-lazy-loading.php

azaozz · 2020-02-07T21:17:03Z

wp-lazy-loading.php

 */
-function wp_add_lazy_load_attributes( $content, $context = null ) {
+function wp_filter_content_attachment_images( $content, $context = null ) {


If we are going to deprecate wp_make_content_images_responsive() (this is a replacement function for it), thinking that limiting it to only images that have a specific class name would be wrong.

Perhaps something like https://github.com/WordPress/wp-lazy-loading/pull/9/files#diff-229cc76004131ce077176ab5955e8707R236 but limited only to img tags would be better? Otherwise the new function still has a limitation that doesn't seem to make sense.

We could easily change the regex to also cover images without that class if we decide to. In that case, we would only try to get the attachment if the class is present, and otherwise assume 0.

Yeah, looked a bit at how to do this and didn't like any of the alternatives. As it is currently, we will do _prime_post_caches() (expensive) even when not adding srcset and sizes. Can probably make that and wp_get_attachment_metadata() conditional on $add_srcset_sizes. But then cannot pass $image_meta to wp_image_add_loading() and passing $attachment_id will be "undesirable" as plugins would probably use it to get the meta anyway.

Still thinking moving wp_image_add_loading() to the first loop makes more sense. Alternatively we should only run this on the_content as it is currently for srcset and sizes.

azaozz · 2020-02-07T21:31:09Z

wp-lazy-loading.php

+ * @param string $context       Additional context to pass to the filters.
+ * @return string Converted 'img' element with 'loading' attribute added.
+ */
+function wp_image_add_loading( $image, $image_meta, $attachment_id, $content, $context ) {


Looking at the patch as a whole, not sure it makes sense for this to be a separate function. Also the name doesn't sound that well, perhaps. If it is to stay, perhaps something like
wp_image_add_loading_attribute( $image_tag, ... )
to make it clear it is only for <img> HTML tags, etc.

I think it makes sense for this to be separate, just like the existing function for srcset and sizes is separate. Regarding the name, I followed the convention set in that existing function too.

it makes sense for this to be separate, just like the existing function for srcset and sizes

Yeah, you're right.

Regarding the name, I followed the convention set in that existing function too.

Wouldn't call it a convention, it's rather a self-documenting thing: describe what the function does in 4-5 words or less. The add srcset (and) sizes sounds pretty specific, add loading is quite general though. Perhaps add loading attr? At least it suggests that there's an HTML attribute involved.

Well, srcset and sizes are attributes just like loading and the term "attribute" is excluded there. But I don't have a strong opinion on this.

azaozz · 2020-02-07T22:27:16Z

It would be nice to reuse the existing regex that filters post_content to add srcset and sizes attributes to image tags.

However looking at how to do it brings several problems:

Filter priority. The existing regex is run at priority 10, before shortcodes are parsed. This will skip adding of the loading attribute to some images in post_content seemingly for no good reason.
The wp_make_content_images_responsive filter and function are used in many plugins. They can be deprecated, but the replacement should probably be more abstracted, i.e. allow more possible uses in the future, not be limited to only one specific addition.

replacing random images that are not WP attachments may unexpectedly add loading attributes to images that shouldn't be modified, such as tracking pixels (where "replacing" means adding the loading attribute).

Why the limitation? As far as I understand "tracking pixels" work fine when they have loading=lazy. Any evidence they do not? Any other evidence/edge cases where all "non-local" images should be excluded?

Since WordPress core's existing logic already gets the attachment IDs and even image meta anyway, there's no performance concern here.

Right. The attachment_ID is required when adding srcset and sizes, but is not required when adding loading. Again, why the limitation?

My understanding is that all images in user added content should be loaded lazily. Excluding some for seemingly no good reasons doesn't look good? If there are specific edge cases that are currently not handled well in the browsers lets find them.

azaozz · 2020-02-10T06:55:55Z

Another thing that this change doesn't take into account is the eventual need to add loading to iframes. For now it sounds like it's coming, sooner or later.

Some (small) optimizations and name tweaks. Return early if there's nothing to do, etc.

felixarntz · 2020-02-17T19:46:28Z

@azaozz

The wp_make_content_images_responsive filter and function are used in many plugins. They can be deprecated, but the replacement should probably be more abstracted, i.e. allow more possible uses in the future, not be limited to only one specific addition.

Can you point out what kind of abstraction you envision here? Should we maybe turn it into wp_filter_content_elements() to make the intent more clear so that this may in the future include other elements like iframes too?

Another thing that this change doesn't take into account is the eventual need to add loading to iframes. For now it sounds like it's coming, sooner or later.

I think we can modify the regex to become (img|iframe) in the future and make other changes as necessary, so I don't think there's a blocker here.

Your latest changes look good to me. What do you think about merging this and then following up with another PR on expanding loading to also non-attachment images? The current state gives a good foundation that can be expanded on, I don't like getting hung up on massive PRs that outgrow their original scope :)

azaozz · 2020-03-08T22:55:29Z

Looking again, there have been quite a few developments nicely outlined in https://make.wordpress.org/core/2020/02/25/lazy-loading-update/.

In that terms seems the "only modify attachment images" part should go. By the time this gets merged, all major browsers will support the new HTML living standard. It doesn't seem good to artificially limit this functionality to only images where we know the attachment ID.

I think we can modify the regex to become (img|iframe) in the future and make other changes as necessary

Yep, changing functions and vars names to make them non-image-specific as in f02acc2 would be enough for now (imho).

Still a bit concerned about having to remove add_filter( 'the_content', 'wp_make_content_images_responsive' ); from wp-includes/default-filters.php, but don't see a way around it.

azaozz · 2020-03-09T00:22:04Z

Added this PR as an enhanced/alternate patch (didn't want to overwrite this PR). Can be merged/backported here if needed.

felixarntz · 2020-03-17T00:39:57Z

Closing this in favor of #23

Use a single regex for loading/srcset/sizes, and only modify attachme…

a418120

…nt images.

felixarntz requested a review from azaozz February 4, 2020 21:38

felixarntz added 4 commits February 4, 2020 22:42

Add missing filter parameters.

66105ac

Fix too early return.

4977fbd

Fix tests.

0c1e7d9

Move checks for modifying attributes out of loops for performance.

8b545a4

azaozz reviewed Feb 7, 2020

View reviewed changes

azaozz added 2 commits February 9, 2020 23:26

Update wp-lazy-loading.php

ab4ffda

Some (small) optimizations and name tweaks. Return early if there's nothing to do, etc.

Few more name changes

051b5f1

Rename wp_filter_content_images to wp_filter_content_tags.

f02acc2

azaozz mentioned this pull request Mar 8, 2020

Enhancement: reuse the current content regex #23

Merged

felixarntz closed this Mar 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use single regex for loading/srcset/sizes, and only modify attachment images #20

Use single regex for loading/srcset/sizes, and only modify attachment images #20

felixarntz commented Feb 4, 2020 •

edited

Loading

felixarntz commented Feb 7, 2020

azaozz Feb 7, 2020

felixarntz Feb 8, 2020

azaozz Feb 10, 2020

peterwilsoncc Feb 23, 2020

felixarntz Feb 25, 2020

peterwilsoncc Feb 25, 2020

azaozz Feb 7, 2020

felixarntz Feb 8, 2020

azaozz Feb 10, 2020

azaozz Feb 7, 2020 •

edited

Loading

felixarntz Feb 8, 2020

azaozz Feb 10, 2020

azaozz Feb 7, 2020

felixarntz Feb 8, 2020

azaozz Feb 10, 2020 •

edited

Loading

felixarntz Feb 17, 2020

azaozz commented Feb 7, 2020

azaozz commented Feb 10, 2020

felixarntz commented Feb 17, 2020 •

edited

Loading

azaozz commented Mar 8, 2020 •

edited

Loading

azaozz commented Mar 9, 2020

felixarntz commented Mar 17, 2020

Use single regex for loading/srcset/sizes, and only modify attachment images #20

Use single regex for loading/srcset/sizes, and only modify attachment images #20

Conversation

felixarntz commented Feb 4, 2020 • edited Loading

felixarntz commented Feb 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azaozz Feb 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azaozz Feb 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azaozz commented Feb 7, 2020

azaozz commented Feb 10, 2020

felixarntz commented Feb 17, 2020 • edited Loading

azaozz commented Mar 8, 2020 • edited Loading

azaozz commented Mar 9, 2020

felixarntz commented Mar 17, 2020

felixarntz commented Feb 4, 2020 •

edited

Loading

azaozz Feb 7, 2020 •

edited

Loading

azaozz Feb 10, 2020 •

edited

Loading

felixarntz commented Feb 17, 2020 •

edited

Loading

azaozz commented Mar 8, 2020 •

edited

Loading