Skip to content
This repository has been archived by the owner on Dec 23, 2020. It is now read-only.

Add filtering for each tag after adding the loading attribute #10

Merged
merged 3 commits into from
Jan 28, 2020
Merged

Add filtering for each tag after adding the loading attribute #10

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b5b3d26
Update wp_add_lazy_load_attributes()
azaozz Jan 23, 2020
395e126
Merge branch 'master' into add/filter-for-each-tag
felixarntz Jan 24, 2020
a23a342
Replace the "wp_add_lazy_loading_to_img" filter with the more targete…
azaozz Jan 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 27 additions & 7 deletions wp-lazy-loading.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,8 @@
function _wp_lazy_loading_initialize_filters() {
// The following filters would be merged into core.
foreach ( array( 'the_content', 'the_excerpt', 'comment_text', 'widget_text_content' ) as $filter ) {
// Before parsing blocks and shortcodes.
// TODO: Comments do not support images. Revisit.
// TODO: This should not exclude images from dynamic blocks and shortcodes. Look at fixing the filter priority.
add_filter( $filter, 'wp_add_lazy_load_attributes', 8 );
// After parsing blocks and shortcodes.
add_filter( $filter, 'wp_add_lazy_load_attributes', 25 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@azaozz azaozz Jan 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...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).

}

// The following filters are only needed while this is a feature plugin.
Expand Down Expand Up @@ -141,14 +139,36 @@ function wp_add_lazy_load_attributes( $content, $context = null ) {

return preg_replace_callback(
'/<img\s[^>]+>/',
function( array $matches ) {
function( array $matches ) use( $content, $context ) {
if ( ! preg_match( '/\sloading\s*=/', $matches[0] ) ) {
return str_replace( '<img', '<img loading="lazy"', $matches[0] );
$tag_html = $matches[0];

/**
* Filters the `loading` attribute value. Default `lazy`.
*
* Returning `false` or an empty string will not add the attribute.
* Returning `true` will add the default value.
*
* @since (TBD)
*
* @param string $default The filtered value, defaults to `lazy`.
* @param string $tag_html The tag's HTML.
* @param string $content The HTML containing the image tag.
* @param string $context Optional. Additional context. Defaults to `current_filter()`.
*/
$value = apply_filters( 'wp_set_image_loading_attr', 'lazy', $tag_html, $content, $context );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@azaozz azaozz Jan 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@felixarntz felixarntz Jan 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


if ( $value ) {
if ( ! in_array( $value, array( 'lazy', 'eager' ), true ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@azaozz azaozz Jan 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$value = 'lazy';
}

return str_replace( '<img', '<img loading="' . $value . '"', $tag_html );
}
}

return $matches[0];
},
$content
);
}