-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SEO][WP] Add a lazy-loading option for image fields #3659
Comments
My first thought is that it'd be nice to have an option to turn this on/off (since it effectively changes how things have been working up to now, and some people don't necessarily like change), but then explaining that that option will only affect certain browsers/versions, blah blah blah could be tricky. So next thought is to include a polyfill for all other browsers, so then on/off option works for everyone. But then this sounds more like a contrib candidate than a core feature... But that's just my 2c. |
I don't think that anyone will complain about this, since it sounds more like an improvement rather than an undesired change. Do we have any use case where someone has reported that this has broken things for them? ...also, what are our Drupal brethren doing about this? Having said that, I wouldn't mind a global option for it. Under "Performance" seems proper. |
Don't know if it will actually break things, but https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/jxiJvQc-gVg and https://docs.google.com/document/d/1e8ZbVyUwgIkQMvJma3kKUDg8UUkLRRdANStqKuOIvHg mention some risks. If we implement it site-wide, the default should be More thoughts: In HTML |
Well it is just an attribute, so it should be easy enough to turn on/off by adding/removing the attribute when any image is rendered. But I do think -- if not otherwise specified -- all images should lazy-load.
I could see this being a good candidate for contrib. My guess is that more than 80% of people would rather have have their sites load quickly than have images people can't see load first. However, it should be simple enough to integrate the option with all existing image fields so people can disable that on a per-image basis. |
I think that lazy-loading by default, with a global option under "Performance" should be core; per-image field instance can be contrib. |
I've skimmed through some of the linked information about the new attribute, and I'm still not sure if there will be unwanted side effects at the beginning, when |
That could work too. Safest also. If at a later point we are more confident that |
Tracking pixels, hidden icons in drop down menus that may cause the menu to redraw multiple times as lazyload images come in, sprite sheets, collapsed form fields... there are a number of cases where the web developer may want them to load even though they're hidden. When adding a new feature, the default setting should always be the previous behavior. Otherwise, Backdrop will earn a reputation for breaking sites on minor-number updates. |
If we use @klonos's recommendation of having a single site-wide on/off checkbox for this, then it would be easy to set to off for existing sites, and on for new ones. Also, adding the [SEO] tag because making pages load faster will improve the search rankings for all Backdrop sites. I'm going to implement this on 3 of my sites and see if there is a markable change in google's rankings. |
FTR, the freshly-released WordPress v5.5 supports that feature OOTB:
|
I would like us to test if these are actual problems, or assumptions.
Yes! Let's please do this, and let's add an on/off switch under |
PR up for review: backdrop/backdrop#3256 Most of the "system" images, like the admin bar icons and the hero block image, are being set in CSS (using $output = theme('image', array('attributes' => array('loading' => 'eager'))); No update hook added to the PR, so existing sites should be left intact, and will need to opt-in for the feature. For now, I've added the checkbox that controls this under Configuration -> Development -> Performance: Let me know if you think that it deserves its own dedicated fieldset, or it should live in a different section in the admin UI. |
...my initial thinking was that hero blocks are usually placed at the top of the page, so they are likely to be within viewport at all times soon as the page loads, so lazy-loading does not make sense there. I then thought of @stpaultim and #4095, and realized that there are use cases where multiple hero blocks may be placed on a single page. Since hero images are large, lazy-loading those that are further down the viewport will have legit performance/loading gain, so I have actually enabled hero block images to also take advantage of this (if enabled). |
The PR still needs work. Many tests are currently failing. |
Tests fixed. |
Thanks @jenlampton 👍 ...I've closed my PR in favor of yours.
Also on text formats as a filter. If not as part of this PR, then we should file a follow-up. Furthermore, we should also make sure that this works with Views if the view format is set to show individual fields instead of "content". |
...the main functionality (adding the |
Yes, please, can you create another issue?
Done, and I added tests for lazy/eager loading! |
@jenlampton many thanks for updating the PR and for adding tests. One thing still isn't addressed: hook_field_formatter_settings_summary() There's no summary text yet, as proposed in my previous comment: "Loading: xxx" |
Done 😉 |
Looks good now - tested and reviewed the code. |
I merged backdrop/backdrop#3273 into 1.x for 1.17.0. Thanks @jenlampton, @klonos, @indigoxela, @BWPanda, @stpaultim, @herbdool, and @olafgrabienski! |
Description of the need
Native
<img>
lazy-loading is coming to the web! https://bit.ly/loading-attribute …<img loading=lazy>
defers offscreen images until the user scrolls near them. Shipping in Chrome ~75 https://bit.ly/loading-i2sThis was one of the recommendations Google PageSpeed gave me for one of my Backdrop sites.
We should support this in Backdrop core!
References:
Advocate: @klonos
PR that adds a global setting under 'Performance':
backdrop/backdrop#3256PR that adds a setting for image fields: backdrop/backdrop#3273
The text was updated successfully, but these errors were encountered: