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

Consider changing or removing wp_get_lazy_load_tags() #5

Closed
azaozz opened this issue Jan 17, 2020 · 7 comments
Closed

Consider changing or removing wp_get_lazy_load_tags() #5

azaozz opened this issue Jan 17, 2020 · 7 comments
Labels
enhancement New feature or request

Comments

@azaozz
Copy link
Contributor

azaozz commented Jan 17, 2020

Follow up from #3.

PR: #7.

Seems that iframes will not support the loading attribute for now. Support will be added in the future.

Generally this would mean refactoring or even dropping the existing wp_get_lazy_load_tags(). It may be "nice to have" a way to globally disable auto-adding of that attribute on the front-end, but not sure that's needed. Ideally there should be a way to change/disable it on per-case (per-image) basis that is also "simple enough" to use to globally disable it in all cases.

As far as I see there are couple of ways to do that:

  • Introduce a filter that will run on adding the attribute to each <img> tag, and passes some context where that tag is. This will also allow plugins to "fine-tune" where the attribute is added and/or make it conditional on the context. Possible downside is that such filter will be quite "noisy". It will run anywhere form 10-15 times up to few hundred times per page load (in case of big galleries, etc.).
  • Keep wp_get_lazy_load_tags() as a "wrapper" function that will only run the 'wp_get_lazy_load_tags' filter (similarly to how it works now but without support for iframes). In this case it can be extended in the future when iframes start to support the "loading" attribute. Ideally it should also pass some context, for example current_filter() or a hard-coded value when it is called "by hand" like from get_avatar(), etc.
@azaozz azaozz added the enhancement New feature or request label Jan 17, 2020
@draganescu
Copy link

I would vote for the second option as it does no harm and makes it easier to update in the future, should more tags support lazy loading.

@JJJ
Copy link

JJJ commented Jan 21, 2020

Second option seems inline with expectations regarding how WordPress interacts with HTML
tags.

By exposing a global function for plugin authors to use inside their own contexts, they do not need to replicate and maintain a core filter in their own plugins.

@felixarntz
Copy link
Member

felixarntz commented Jan 21, 2020

I also think the second option is more applicable because of easier maintenance and future compatibility. If iframes are not supported, all we need to do is remove them from

$supported_tags = array( 'img', 'iframe' );

Any tag that gets supported can be added to that list in the future.

The counter-argument would be that having flexible tags here would make #2 more complicated, which is fair, especially where we are now with only one specific element supporting the attribute (if that is the case - we still don't know yet for sure).

@azaozz
Copy link
Contributor Author

azaozz commented Jan 21, 2020

Thanks for the feedback, everybody :)

Thinking that both points make sense and ideally should be implemented.

The current wp_get_lazy_load_tags() will be for "globally" enabling/disabling adding of the loading attribute. This will be most useful initially, until the HTML attribute becomes "mainstream", and (eventually) when support for iframes is added.

At the moment wp_get_lazy_load_tags() lacks flexibility. For example it's not possible to disable for post_content but enable for avarats or when images are displayed by using wp_get_attachment_image(). Going to make a PR to add some context to it and perhaps make it return boolean instead of having to do in_array() every time its used.

For more fine-grained control thinking that having a filter for each image (and eventually iframe) would be best. That will make it possible for plugins to handle the exceptions and other cases mentioned in #4.

@JJJ
Copy link

JJJ commented Jan 21, 2020

For example it's not possible to disable for post_content but enable for avarats or when images are displayed

That's a good point. 👍

@azaozz
Copy link
Contributor Author

azaozz commented Jan 22, 2020

PR: #7.

@felixarntz
Copy link
Member

Done via #7, iframe support dropped via #8.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants