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

Add support for skip-lazy #4

Closed
wants to merge 2 commits into from
Closed

Conversation

futtta
Copy link

@futtta futtta commented Jan 17, 2020

Ensure that loading="lazy" is not set if skip-lazy class or data-skip-lazy attribute are set.

Add missing test scenario back and remove duplicate line.
@azaozz
Copy link
Contributor

azaozz commented Jan 18, 2020

Frankly not sure if this belongs in core. I understand there is a push to use skip-lazy (class name) and data-skip-lazy across multiple plugins but what about plugins that don't use these? Also, if the class name/data attribute is hard-coded, what about all the content that was published and doesn't have either of these?

There are also certain differences between the js method (enforcing late loading of images) and the tag attribute which is just a hint for the browser that the image is suitable for lazy-loading. Generally the browser is "better equipped" to make a good decision how far below the fold to start loading images, can look at network/download speed, etc. In that terms thinking that there are benefits of having the attribute even on images with skip-lazy. Of course, this needs testing. That's the main reason to release this in a plugin first :)

Thinking that the best solution would be to have a filter when adding the attribute to each <img> tag. Then plugins (and eventually core) will have a granular control and be able to fine-tune this. Such filter will also be useful to perhaps dynamically remove skip-lazy or other classes/attributes, etc.

Also, see the first point in #5, and the discussion on #2.

@remyperona
Copy link

Instead of not adding the loading attribute, the approach when the skip-lazy class or attribute is present could be to add the loading="eager" attribute and value. This let the browser handle how to manage the image, while also following the interoperability initiative.

@azaozz
Copy link
Contributor

azaozz commented Jan 19, 2020

the approach when the skip-lazy class or attribute is present could be to add the loading="eager" attribute and value.

Yeah, ideally plugins that add js based lazy-loading would "standardize" on using the loading attribute instead of using a class name or a data attribute for the same thing.

The addition of the loading attribute to the HTML specs means that the plugins that do lazy-loading "the old way" will need to be refactored into working like polifills. Ideally they should switch to using the loading attribute, there's no point in using a class name or a data attribute to "replace" the standard HTML feature.

Also, the above point about the browsers being better equipped to determine when to start loading images means that some images that currently need skip-lazy will work well when using the native loading="lazy".

In that terms thinking that WordPress core should not use/depend on things that may have been added by plugins. Instead, it should facilitate/make it easy for plugins to reuse or "fix" their previous methods.

Generally this means that when a plugin has determined that a particular image is not suitable for lazy loading, it should add loading="eager" instead of adding skip-lazy or data-skip-lazy. Image tags that have the loading attribute will not be changed while auto-adding it in core.

In cases where skip-lazy was hard-coded in user content, the plugin would use the (proposed) new filter to prevent adding of loading="lazy" and perhaps remove skip-lazy and add loading="eager" (currently loading="eager" is the default, can be omitted).

Then on the front-end lazy-loading plugins should be looking for the HTML attribute and immediately load images that don't have loading="lazy". I.e. work (as a polifill) "together" with the browser, not "against" it.

This means that pretty much all lazy-loading plugins will need updating to work well in WP 5.4. The sooner that starts, the better. The plan is to get this feature plugin to a usable state and release it as fast as possible, then try to popularize it so it can be well tested before merging to core. Would be ideal if the authors of lazy-loading plugin participate in both the testing and the development :)

@felixarntz
Copy link
Member

felixarntz commented Jan 21, 2020

Yeah, ideally plugins that add js based lazy-loading would "standardize" on using the loading attribute instead of using a class name or a data attribute for the same thing.

Agreed. I think now that there is a dedicated attribute for lazy-loading, for a core implementation only that attribute should be relevant to "skip" images. Core's implementation should be that loading="lazy" will only be added to an img tag if it doesn't already have a loading attribute (which e.g. a plugin could filter in before as loading="eager").

Classes or data attributes are non-standardized workarounds for which the loading attribute now provides an actual standard. If a plugin would like core to skip img tags that have specific non-standard attribute values, it could still do that manually. The generally recommended way going forward would be to rely on loading though.

@futtta
Copy link
Author

futtta commented Jan 21, 2020

I'm afraid that in spite of "browsers know best" and "plugins would 'standardize' on using the loading attribute" in reality things will break for some users when this ships in core for many a reason. This probably will not be a problem for plugins that also implement lazy-load (I tested and all works at least for my plugin without any change needed) but rather for JS-heavy plugins that handle images somehow, thinking of sliders & gallery plugins for example (I've seen some weird stuff already).

If the solution for things breaking once this ships in core is "wait until plugin X updates" or "use a filter", then I'm afraid we'll frustrate many end-users, so the question is how can we help those.

An option to disable native lazy-loading is not an option (I'm all for smart defaults), but at least being able to end-users to add a class name (skip-lazy or other) to an image for it not to be natively lazy-loaded would most certainly help.

Summary: it's about end-users, not plugin developers ;-)

@felixarntz
Copy link
Member

@futtta Could you clarify what type of breakage your concern is specifically?

Since the loading attribute is not a JS-based solution, it shouldn't conflict with existing JS-based solutions, it would just be another "random" attribute on an img tag. I think worst case an image would get "double lazy-loaded", with the browser trying to lazy-load it, but because it e.g. doesn't have a src it would still actually only be lazy-loaded by the JS solution like before.

@futtta
Copy link
Author

futtta commented Jan 21, 2020

One example from the top of my mind; I've seen plugins that use JS to get the dimensions of an image to act on that (I think for layout purposes). If an image is lazyloaded (natively or JS-wise), getting dimensions fails and the image display is broken.

As I wrote; there's some crazy stuff out there ;-)

@JJJ
Copy link

JJJ commented Jan 21, 2020

Lazy loading of images is the kind of enhancement that end users should not need to think very much about.

Just like the way themes come with decent SEO, there will be people who want to take that to the next level, and that should be allowed.

Adding the ability to manage a new img attribute will require exposing a UI specifically for it beyond just a class, and that seems beyond the scope of this plugin and feature to me, though it certainly should be allowable for a plugin to create its own UI and support this.

(Anchors already have/had support for the target attribute, and that has come up numerous times for removal from folks who forget how useful it is, simply because it is a bespoke anomaly of a UI.)

@futtta
Copy link
Author

futtta commented Jan 21, 2020

Having UI to manage the attribute instead of using an exclusion class-name is great, but given lazy-loading is planned for inclusion in WordPress 5.4 (release March 31th) and as this feature-plugin is meant as a path to iterate towards core inclusion, shouldn't this be considered here, earlier rather then later?

Or will this ship default on without any possibilities for average users to alter the behavior in case of issues?

@JJJ
Copy link

JJJ commented Jan 21, 2020

shouldn't this be considered here, earlier rather then later?

Probably, and we are considering it together. I just happen to feel like a UI isn't necessary. 😄

Or will this ship default on without any possibilities for average users to alter the behavior in case of issues?

We get to decide this together, probably here, and possibly in Slack chats (though I'd prefer no decisions be made there for synchronicity reasons.)

@azaozz
Copy link
Contributor

azaozz commented Jan 23, 2020

I'm on board with @JJJ and @felixarntz here :)

Lazy loading of images is the kind of enhancement that end users should not need to think very much about.

Exactly! It should "just work". The users shouldn't care about having to "micro-manage" it. Looking at the discussions on the specs: whatwg/html#2806 and whatwg/html#3752, all of the possible (edge) cases seem well covered. In addition we're making this WP plugin so it can be tested "in the field" with existing content.

One example from the top of my mind; I've seen plugins that use JS to get the dimensions of an image to act on that...

I believe this is handled by the browser downloading the first 2KB of the image file (when the image will be lazy-loaded). That way the browser can get the (intrinsic) size and reserve appropriate space for the image. Seems that the same ensures the image dimensions will be available in js. Worth a test, but I see it discussed in the specs.

#10 implements a wp_add_lazy_loading_to_img filter that will run after adding the loading attribute to all img tags. There plugins will be able to manage it well.

@futtta
Copy link
Author

futtta commented Jan 23, 2020

My 2c: assuming no issues will arise and hence deciding end-users need no functionality to stop the loading attribute from being added is a big gamble. It might work out (yay), but I for one would not want to have to support people for whom it does not. Let's hope the feature plugin gains enough traction before this goes into core default on with no straightforward way for end-users to disable it :-)

@JJJ
Copy link

JJJ commented Jan 23, 2020

@futtta fwiw, I do appreciate your alternative point of view and perspective. You may be correct, and there may be feedback from users who wish to disable this.

I can imagine scenarios where plugins or themes may want to opt in-or-out of it, like with what happens already with add_theme_support() or add_post_type_support().

I can imagine wanting to skip it on a per image basis.

I can imagine a piece of meta-data connected to something in the media library to prevent an image from ever lazy loading.

The nuance with a feature like this, and with WordPress core, is deciding how small of a footprint will make the biggest impact from release to release; a smart default with flexible overrides.

One or all of the above ideas may end up in core, and you’d be justified in saying “hey @JJJ I told you so” when they do. 😁

@azaozz
Copy link
Contributor

azaozz commented Jan 23, 2020

Thinking this should be closed for now in favor of specific (edge) cases that may require considering in WP core.

One such case could be the presence/absence of width and height attributes in the img tag. Keep seeing

[Intervention] An element was lazyloaded with loading=lazy, but had no dimensions specified. Specifying dimensions improves performance. See https://crbug.com/954323

in the browser console while testing.

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

Successfully merging this pull request may close these issues.

5 participants