Add filtering for each tag after adding the loading attribute

[azaozz]

Based on #7.

• Add some checks before running the regex in wp_add_lazy_load_attributes().
• Filter each tag after adding the attribute.

See: #4.

[azaozz]

This brings some of the changes from #9, but "scopes" them only for adding of the "loading" attribute to img tags and eventually to iframes.

[felixarntz]

Can you give your thinking on why you chose 25 as priority specifically?

Looping in @spacedmonkey because he originally set it to 8.

[azaozz]

25 is somewhat arbitrary for "after all other default filters".

Not sure why it was set to 8, but that means we were not adding the attribute to images added with shortcodes, like to the (old style) galleries, etc. Running it "early" means it misses few cases where images may be added, don't see why these images should be skipped.

[spacedmonkey]

8 was set because this filter should be applied before blocks are rendered.

[azaozz]

...should be applied before blocks are rendered

What's the reason to exclude dynamic and rendered blocks? Did you run into some troubles with any of them?

Generally thinking we want the attribute on all images in all cases. Then, if there are cases where lazy-loading doesn't work well, find a way to skip adding it if possible.

[spacedmonkey]

Not applying these filters early is the desired behaviour here. Adding this attribute to rendered blocks, shortcodes and other filtered content as we have no idea what effect that will have on third party code. I think they would be breakages and backlash if we opt everyone in. Take for example a tracking pixel loaded in via a shortcode or block. If this is then lazy loaded, then it would be a breakage.

We should make sure all core functionality like galleries shortcodes supports lazy loading attribute. We should also have a way for third party code to opt in.

[azaozz]

...we have no idea what effect that will have on third party code.

This is the main purpose of this plugin, to test such cases. Frankly I don't see much difference between HTML content added by the users directly (may be pasted, etc.) and HTML content added by plugins. Excluding images that may be added by the user using a plugin that adds a shortcode or a specific block is not a good idea imho, at least for now. If any breakage is detected during testing, lets look at the specific cases.

(Having said that, we are running late... Looks like this feature will miss WP 5.4 if this plugin is not released asap, as there will not be sufficient time to test).

Take for example a tracking pixel loaded in via a shortcode or block.

This case is specifically discussed in the HTML specs. Also, as the browser downloads the first 2KB of each image, tracking pixels will work regardless of the attribute or its value (these images are usually smaller than 2KB anyway).

[felixarntz]

I don't like the idea of introducing another filter just for this functionality. What would this be useful for? I think we should try to find a solution that only relies on wp_lazy_loading_enabled.

[azaozz]

This filter allows more granular control and follows the same "architecture logic" as when adding the srcset and sizes attributes. For example plugins that currently add skip-lazy to img tags, can use this filter to either stop adding of loading="lazy" or replace it with loading="eager" etc.

This will allow much better interoperability and "fine tuning" for edge cases.

[felixarntz]

The filter adds quite a bit of overhead for a very specific case. There already is a filter to manage the addition of a loading attribute by core, having two is quite confusing.

I see what you're saying that this gives control on a per img basis, but I think it'd be much cleaner to handle this internally, which we already do, by checking whether there already is a loading attribute. This is a simple solution that plugin developers can leverage:

• They could simply add a loading="eager" so ensure that core skips it. Not only does this get rid of the need for a filter, it also encourages the use of standard attributes to indicate the loading method over arbitrary data attributes or class names.
• Even if for whatever reason a plugin wants to or has to keep using skip-lazy classes or similar, it could just automatically add loading="eager" if that class is present.

[azaozz]

The filter adds quite a bit of overhead for a very specific case.

Yes, there is some overhead but it's almost negligible when running the filter without any callbacks. However if a plugin needs to do something on a per-image basis, it would most likely need to add another preg_replace_callback() or similar.

I think it'd be much cleaner to handle this internally, which we already do, by checking whether there already is a loading attribute.

Yep, that's the "ideal" case.

They could simply add a loading="eager" so ensure that core skips it.
...
it could just automatically add loading="eager" if that class is present.

Right, but how are the plugins going to make these changes? As far as I see there are two possibilities in general:

• The skip-lazy class name or attribute was added to post_content (i.e. it's hard-coded). To handle the new loading attribute the plugin would have to run the content through another regex.
• The skip-lazy is being added by some regex filtering of the user content, perhaps a shortcode. In this case it may be possible to change the callback and also add loading="eager", but perhaps not in all cases.

Having a per-image filtering also matches how srcset and sizes are handled: https://core.trac.wordpress.org/browser/tags/5.3.2/src/wp-includes/media.php#L1297 and https://core.trac.wordpress.org/browser/tags/5.3.2/src/wp-includes/media.php#L1407.

Thinking that a per-image filter will see more use at the beginning, then little by little will be used less and less as browser support is being refined and all plugins that currently do lazy-loading adopt the loading attribute

[felixarntz]

The skip-lazy class name or attribute was added to post_content (i.e. it's hard-coded). To handle the new loading attribute the plugin would have to run the content through another regex.

I think even with this filter a regex may be necessary. For someone using data-skip-lazy they might get away with strpos( $str, ' data-skip-lazy=' ), but with a skip-lazy class you'd already need a regex since there might be other classes.

My main problem with this filter though is that it does not clarify its intent. The name does, but the arguments are just two full img tags, one looks different than the other.

Having a per-image filtering also matches how srcset and sizes are handled: https://core.trac.wordpress.org/browser/tags/5.3.2/src/wp-includes/media.php#L1297 and https://core.trac.wordpress.org/browser/tags/5.3.2/src/wp-includes/media.php#L1407.

Both of these filter specific data, one the sources, the other the sizes, so I don't think these are comparable. Here we're passing through the entire tag, twice. I'd be okay including a more targeted filter like wp_set_image_loading which would only filter the value of the loading attribute that core would add. We could even allow empty, and then not add it. The signature could be:

apply_filters( 'wp_set_image_loading', 'lazy', $tag ) (where $tag would be $matches[0])

What do you think?

[azaozz]

Yeah, I see your point. Filtering the whole img tag is more like #9 which may eventually happen one day but is out of scope here.

I'd be okay including a more targeted filter like wp_set_image_loading which would only filter the value of the loading attribute that core would add. We could even allow empty, and then not add it.

Yes, makes sense. And yes, in this case it will have to support false (or any "falsey" like empty string?) to skip adding the loading attribute. Thinking perhaps it should still pass $content and $context so plugins have all data necessary to make a decision. More context is always good :)

Patch coming up.

[felixarntz]

Overall sounds good to me. I've added follow-up comments in a review of the updated PR below.

[felixarntz]

I think passing contextual data here is good, but I'm a bit wary of passing the content here. Anything this could be used for is probably to run another regex, which isn't something we should encourage. It would be great to have a post ID or potentially attachment ID, but since neither is passed to here, and it may be unreliable or expensive to get these, maybe we should just leave it out for now and only pass $context.

This is something where maybe we can enhance in the future, but I'd prefer to leave out $content for now, since it wouldn't really help much.

[felixarntz]

I wonder whether we should remove the $context from the more generic wp_lazy_loading_enabled filter and only have it here. It would result in a more clarified distinction between the two filters:

• wp_lazy_loading_enabled is to enable/disable lazy-loading globally for specific tags.
• wp_set_image_loading_attr allows modifying the loading attribute for images specifically on an individual level.

[azaozz]

This is something where maybe we can enhance in the future, but I'd prefer to leave out $content for now, since it wouldn't really help much.

Don't think this is a good idea. Having more context in filters/actions makes them more useful. If we remove it, and a plugin has to get the context in order to work, the only option would be to disable core's functionality, copy the code to the plugin and "tweak" it so it works. That brings a lot more problems down the line. Why would we want to "restrict" the context and not support edge cases?

[spacedmonkey]

This filter isn't specially useful, as not all the tags on put through this function..

[azaozz]

I wonder whether we should remove the $context from the more generic wp_lazy_loading_enabled filter...

Same as above. Less context means less useful hooks :)

wp_lazy_loading_enabled is to enable/disable lazy-loading globally for specific tags.

Not exactly. Currently this is "enable/disable lazy-loading globally for img tags in specific context". It is not particularly useful at the moment, would eventually become more useful if/when lazy-loading for iframes is added to the specs. However removing the context makes it even less useful. Thinking that without context there's no point of having it at all at the moment.

wp_set_image_loading_attr allows modifying the loading attribute for images specifically on an individual level.

Right. However there are some inconsistencies:

• This filter runs only in some cases, not in all of the cases where WP is adding the attribute (as @spacedmonkey pointed out above).
• It is designed so plugins can add it to other HTML content where adding the loading attribute makes sense.

Passing more context about why, where, how it runs would cover more cases and generally make it more useful. "Restricting" the context means that plugins won't be able to use it in certain cases.

[felixarntz]

Same as above. Less context means less useful hooks :)

Generally yes, but not really in this case IMO. It would help distinguish what these two filters should be used for. One would be for enabling/disabling the feature for a specific HTML element overall, the other would be for controlling per image. Removing $context from the more generic wp_lazy_loading_enabled filter would clarify its intent and not remove any capability, because exactly that could be done with the more specific wp_set_image_loading_attr filter still.

[azaozz]

Removing $context from the more generic wp_lazy_loading_enabled filter would clarify its intent and not remove any capability, because exactly that could be done with the more specific wp_set_image_loading_attr filter still.

Yes, ideally that should have been the case. Unfortunately wp_set_image_loading_attr is fired only when adding the attribute to HTML content. If we remove the context from wp_lazy_loading_enabled how would you know when it was called from get_avatar or from wp_get_attachment_image?

[felixarntz]

I'd prefer if we didn't check for a whitelist of values here, as it's overly strict and makes maintenance on our end harder. As the spec evolves (e.g. there's still a good chance for auto to be added in a future iteration), we would need to do another WordPress release for something like that.

[azaozz]

Hmm, not sure how potentially adding another value makes maintenance harder? I mean, if the specs are changed and/or we have to change the default value in WP, we can commit it today and do a minor release tomorrow.

This is one of the advantages of adding the loading attribute from a "display filter". Maintenance is easy :)

[felixarntz]

Suggested change

	if ( ! in_array( $value, array( 'lazy', 'eager' ), true ) ) {
	if ( ! is_string( $value ) ) {

[azaozz]

Don't think this is a good idea. If we wanted plugins to be able to add "unrestricted HTML" there, better to filter the whole <img> tag (as it was previously). If we change this to ! is_string( $value ) that string could be an arbitrary amount of HTML.

[felixarntz]

We would need to use esc_attr() and it would be okay. Anyway, this is a minor concern here.

[azaozz]

Yeah, esc_attr() would work but adds some overhead. Ensuring the attribute value is "standard" seems to make most sense but agree this is a minor concern.

[felixarntz]

Let's merge this as is, I think this looks good for the initial feature plugin release. We can iterate later if needed.

[spacedmonkey]

Why was this merged? We were still discussing the propertiy here. https://github.com/WordPress/wp-lazy-loading/pull/10/files#r371407221

[felixarntz]

@spacedmonkey We'd like to release a first version of the plugin today, and this looked good to go for that. Things will continue to be iterated on, we can also continue discussing the hook priority (as I see you already opened #12 for that 👍).