#50367 closed defect (bug) (fixed)
Avoid layout shifting due to images not having dimension attributes
Reported by: | flixos90 | Owned by: | flixos90 |
---|---|---|---|
Milestone: | 5.5 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Media | Keywords: | needs-testing has-patch has-unit-tests has-dev-note |
Focuses: | Cc: |
Description
Images added to post content via the block editor typically do not provide width
and height
attributes which increases layout shifting from images being loaded, harming user experience.
This becomes most obvious on slow network connections, but can be observed even on fast networks. In the split second where the image gets downloaded by the browser, the content below the image shifts if width
and height
are not present on the img
tag.
This problem also has minor implications on lazy-loading (#44427), as it is explicitly recommended to provide dimensions: https://html.spec.whatwg.org/multipage/embedded-content.html#attr-img-loading
WordPress has the wp_image_add_srcset_and_sizes function in place to on-the-fly add srcset
and sizes
attributes to images which already works for images without dimension attributes, by looking at the relevant image metadata. This should be expanded to also add the actual width
and height
attributes in those cases.
The loading
attribute should then only be added to img
tags that have dimensions available.
Attachments (2)
Change History (31)
This ticket was mentioned in PR #334 on WordPress/wordpress-develop by felixarntz.
4 years ago
#1
- Keywords has-patch added; needs-patch removed
#2
@
4 years ago
Related: https://github.com/WordPress/gutenberg/issues/6177
However, these two issues don't block or exclude each other. wp_image_add_srcset_and_sizes
already relies on the image's dimensions if the img
tag doesn't provide them, so it should be safe to add them in that case.
Adding the attributes in Gutenberg would improve specifically for previewing in the editor as well as catering better for the case where a width
and height
that do not match the actual image dimensions should be used.
#5
follow-up:
↓ 6
@
4 years ago
Nice work @flixos90! This worked well in my testing.
- I created a test post with a single medium image and left aligned it next to a block of text.
- Tested loading the post using a throttled connection to simulate a slow load experience.
Result:
Before the patch the text shows a square block, then jumps around once the image loads to make room for it. After the patch the image width/height is included in the img src tag (when I view the html), so the browser shows a blank space while the image is loading, which the text wraps around. Once the image loads, no jumping occurs - the text is already in the right spot. Attaching some screencast gifs of my tests.
This feels like a great improvement: one question though - why are these images missing the width/height in the first place when inserted with the block editor? I'm guessing this is discussed on the issue you linked, I haven't read the entire thing :)
For now though, many users are sure to have created content that includes image tags lacking width/height attributes. This change will likely provide measurable improvements in reducing cumulative layout shift for these sites. Let's get this in before 5.5 beta so it can get some wider testing.
#6
in reply to:
↑ 5
@
4 years ago
Replying to adamsilverstein:
This feels like a great improvement: one question though - why are these images missing the width/height in the first place when inserted with the block editor? I'm guessing this is discussed on the issue you linked, I haven't read the entire thing :)
Yes, it's indeed mostly due to a regression that the block editor introduced; at the moment, width
and height
attributes are only added if the user manually resizes the image (i.e. if they differ from the actual image dimensions). This use-case is covered here as the image dimensions wouldn't be added then.
For now though, many users are sure to have created content that includes image tags lacking width/height attributes. This change will likely provide measurable improvements in reducing cumulative layout shift for these sites. Let's get this in before 5.5 beta so it can get some wider testing.
That's a good point, it's another reason this fix in itself is useful even once the issue gets addressed in Gutenberg (which we should totally do as well).
#7
@
4 years ago
It also works well in my testing. I tried throwing every possible image into the content, from floats to inlines to full-bleeds, including both image blocks and classic block.
To help test I also created a test plugin to slow down the images in the content: https://gist.github.com/westonruter/340dfb31c09e7f82240161529e5ec93c
Adding the width/height makes a huge difference with this plugin. Without the width/height insertion, there is a bunch of content shifting. But when the width/height are in place, the layout is stable.
This ticket was mentioned in Slack in #core-editor by flixos90. View the logs.
4 years ago
This ticket was mentioned in Slack in #core by flixos90. View the logs.
4 years ago
#10
@
4 years ago
As discussed previously, adding width and height attributes to images should have no adverse effect on any styling. For images that have their dimensions modified by CSS (e.g. setting width: 100%
or max-width: 100%
), it is best practice to also specify the respective other dimension as auto
(e.g. height: auto
), in order for the image not to be skewed. Having image dimension attributes present in such a case does not supersede the CSS in any way, images will show up as intended - having the attributes present simply ensures that the browser knows the aspect ratio of the image right away.
An additional consideration is that image dimensions have historically been present on pretty much all content images. The absence of these attributes has only become more common since Gutenberg. If a theme has styling issues because of added dimension attributes (e.g. due to omitting a height: auto
, see above), this would be a long-term problem already relevant to all WordPress content created before Gutenberg. As such, there is no regression in bringing the attributes back here.
I've tested this with the comprehensive theme unit test data, against Twenty Nineteen. There is no visual disparity between trunk
and this patch, image dimensions present only have the positive effect of reserving the space as necessary to reduce layout shift:
#12
follow-up:
↓ 14
@
4 years ago
- Keywords needs-unit-tests needs-patch needs-testing added; has-patch has-unit-tests removed
- Resolution fixed deleted
- Status changed from closed to reopened
There seems to be a bug with this patch when an img tag has either width
or height
set (but not both).
To test:
- Create new post with an image, 1200x900px.
- Switch to Code/Text view and insert a
width="100"
in the img tag. - Save and preview.
On the front-end the img tag has width="100"
but also height="900"
(coming from the image file dimensions) which is the wrong value.
#13
@
4 years ago
Looking further, there doesn't seem to be a way for a plugin or a theme to disable auto-adding of width and height attributes to img tags. Perhaps at least a true/false
filter would be good? Or maybe filter the image_hwstring()
value.
#14
in reply to:
↑ 12
;
follow-up:
↓ 18
@
4 years ago
Replying to azaozz:
There seems to be a bug with this patch when an img tag has either
width
orheight
set (but not both).
I was considering this when working on the patch, however didn't bother adding it as part of this, since this bit was already broken before. For the existing srcset
and sizes
logic, the code would always rely on the actual file dimensions if only one of the two was present - in other words, providing just one of the two attributes is not supported by core at the moment.
I'm definitely for fixing this - it's not really caused by this ticket, but we can follow up on it here since it's definitely related functionality. I think what we should do here is change the condition if ( ! $width || ! $height ) {
to if ( ! $width && ! $height ) {
; and if only one of the two is present, return the original tag because there is no way for us to make an educated guess about srcset
and sizes
either, at least not with the current functionality.
Looking further, there doesn't seem to be a way for a plugin or a theme to disable auto-adding of width and height attributes to img tags. Perhaps at least a
true/false
filter would be good? Or maybe filter theimage_hwstring()
value.
I'd question the need for this, what would be the use-case for that? In what circumstance would not having width and height attributes be a good thing? Having them shouldn't cause any issues except for themes with bad CSS (e.g. missing height:auto
when it modifies image width), but I don't think we should cater for that, since this is clearly not recommended, and those themes would have been broken for a long time.
This ticket was mentioned in PR #358 on WordPress/wordpress-develop by felixarntz.
4 years ago
#15
- Keywords has-patch has-unit-tests added; needs-unit-tests needs-patch removed
Trac ticket: https://core.trac.wordpress.org/ticket/50367
#17
@
4 years ago
- Keywords needs-dev-note added
Related: https://github.com/WordPress/gutenberg/issues/6652
Nice to see this being addressed, but I'm curious about the decision to couple this with the srcset
/sizes
functionality since I would assume you would want this behavior even if someone had disabled core's responsive image handling.
This definitely should get a dev note.
#18
in reply to:
↑ 14
;
follow-up:
↓ 19
@
4 years ago
Replying to flixos90:
since this bit was already broken before. For the existing
srcset
andsizes
logic, the code would always rely on the actual file dimensions if only one of the two was present...
It isn't broken for srcset
and sizes
, this is the right behavior for them :) If both or one is missing, base the sizes attribute on the image file dimensions.
However that's no good for adding the width
and height
attributes. The logic needs to be extended and this case separated from the srcset and sizes use of image file dimensions.
Looking further, there doesn't seem to be a way for a plugin or a theme to disable auto-adding of width and height attributes to img tags.
I'd question the need for this, what would be the use-case for that?
At least for consistency if nothing else? This still may be a bit buggy when the post_content is used in feeds, emails, etc. Auto-adding HTML attributes is fine, but plugins should have a say in it too imho :)
#19
in reply to:
↑ 18
@
4 years ago
Replying to azaozz:
It isn't broken for
srcset
andsizes
, this is the right behavior for them :) If both or one is missing, base the sizes attribute on the image file dimensions.
Ah right, sorry I had something wrong in my thoughts there :D Yeah in this case, makes certainly sense to address this here.
I'd question the need for this, what would be the use-case for that?
At least for consistency if nothing else? This still may be a bit buggy when the post_content is used in feeds, emails, etc. Auto-adding HTML attributes is fine, but plugins should have a say in it too imho :)
We could make it a separate function as @joemcgill suggested, this way plugins could unhook it. The important thing here is we need to have it apply before wp_image_add_srcset_and_sizes
.
@joemcgill @youknowriad Agreed regarding a dev-note, I was planning to incorporate it in the one for #44427, since these changes are closely related. We could expand the scope for the dev note a bit covering not only lazy-loading, but generally modifying image tags on the fly via wp_filter_content_tags()
.
#20
@
4 years ago
@joemcgill @azaozz I've updated the PR to introduce a new function wp_img_tag_add_width_and_height_attr()
which on a high level behaves similarly like wp_img_tag_add_srcset_and_sizes_attr()
and wp_img_tag_add_loading_attr()
. The part that is now common for wp_img_tag_add_width_and_height_attr()
and wp_img_tag_add_srcset_and_sizes_attr()
I've moved into a new utility function wp_image_src_get_dimensions()
- in actual usage and the way the functions are run subsequently in most cases that function will only be called once. If width
and height
attributes are not present, they will be added by wp_img_tag_add_srcset_and_sizes_attr()
, this way the functionality doesn't need to run again as part of wp_img_tag_add_srcset_and_sizes_attr()
.
4 years ago
#21
Looks good. Just need to sync how wp_img_tag_add_loading_attr()
and the new wp_img_tag_add_width_and_height_attr()
deal with src
when in single quotes.
#23
@
4 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Based on https://github.com/WordPress/wordpress-develop/pull/358#pullrequestreview-438654395, we removed support for single quotes - however, due to scenarios like https://core.trac.wordpress.org/ticket/44427#comment:100, we should still only modify img
tags that use double quotes.
#25
@
4 years ago
We're starting to see a lot of warnings and errors that are possibly related to this change on wordpress.org.
Warning: Invalid argument supplied for foreach() in /wp-includes/media.php on line 1520
Warning: Illegal string offset 'file' in /wp-includes/media.php on line 1513
Warning: Illegal string offset 'sizes' in /wp-includes/media.php on line 1520
This is happening pretty much anywhere we have an image. Might need some sanity checks around this for cases where the image is not an attachment in the media library.
#26
@
4 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Specific example: https://developer.wordpress.org/themes/functionality/widgets/
The image on this page (showing a text widget) doesn't come from here, but from an upload to make.wordpress.org, likely where the code was originally. The code for that block is this:
<!-- wp:image {"align":"center","id":2135,"className":"wp-image-2135"} --> <div class="wp-block-image wp-image-2135"><figure class="aligncenter"><img src="https://make.wordpress.org/docs/files/2013/06/widget_visual.png" alt="Sample Editable Widget" class="wp-image-2135"/><figcaption>A widget form in the admin area.</figcaption></figure></div> <!-- /wp:image -->
The code in wp_filter_content_tags() assumes that the "wp-image-2135" is the attachment ID, which it is, on another site entirely.
That gets passed as the attachment_id to wp_img_tag_add_width_and_height_attr(), which calls wp_get_attachment_metadata() with it, which will come back with no data because there's nothing for it to get (or worse case, come back with the data for a different attachment), and then that gets passed without a sanity check into wp_image_src_get_dimensions(). Thus, warnings generated.
The code that gets the attachment number should at least verify that that is the right attachment, somehow, before taking actions using it.
#27
@
4 years ago
The code that gets the attachment number should at least verify that that is the right attachment
Right. This is already done for srcset and sizes, need to also do it for adding width and height. On it.
#28
@
4 years ago
- Resolution set to fixed
- Status changed from reopened to closed
@Otto42 @flixos90 made #50543 for this as it's a separate issue. Please have a look if you can :)
#29
@
4 years ago
- Keywords has-dev-note added; needs-dev-note removed
Dev note published: https://make.wordpress.org/core/2020/07/14/lazy-loading-images-in-5-5/
Trac ticket: https://core.trac.wordpress.org/ticket/50367