Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#50367 closed defect (bug) (fixed)

Avoid layout shifting due to images not having dimension attributes

Reported by: flixos90's profile flixos90 Owned by: flixos90's profile 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)

before-patch.gif (377.8 KB) - added by adamsilverstein 4 years ago.
after-patch.gif (365.7 KB) - added by adamsilverstein 4 years ago.

Download all attachments as: .zip

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

#3 @flixos90
4 years ago

  • Keywords has-unit-tests added

#4 @flixos90
4 years ago

  • Owner set to flixos90
  • Status changed from new to assigned

#5 follow-up: @adamsilverstein
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 @flixos90
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 @westonruter
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 @flixos90
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:

#11 @flixos90
4 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 48170:

Media: Ensure images have dimensions to reduce layout shift and facilitate lazy-loading.

This changeset ensures that attachment images which are inserted without width and height attributes still receive them in the frontend, to reduce cumulative layout shift. Adding the dimensions happens as part of the logic for adding srcset and sizes attributes, which already assume the specific width and height of the respective image.

Images are now only lazy-loaded if they have width and height attributes present. While missing these attributes itself is what causes layout shifts, lazy-loading such images can make this problem more apparent to the user.

Props adamsilverstein, westonruter.
Fixes #50367. See #44427.

#12 follow-up: @azaozz
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.

Last edited 4 years ago by azaozz (previous) (diff)

#13 @azaozz
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.

Last edited 4 years ago by azaozz (previous) (diff)

#14 in reply to: ↑ 12 ; follow-up: @flixos90
4 years ago

Replying to azaozz:

There seems to be a bug with this patch when an img tag has either width or height 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 the image_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

#16 @youknowriad
4 years ago

Do you think deserves a dev note?

#17 @joemcgill
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: @azaozz
4 years ago

Replying to flixos90:

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

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 @flixos90
4 years ago

Replying to azaozz:

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.

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 @flixos90
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().

azaozz commented on PR #358:


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.

#22 @flixos90
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 48237:

Media: Introduce wp_img_tag_add_width_and_height_attr() to add dimension attributes to images.

Following up on [48170], this changeset moves the new logic to add missing img dimension attributes into a separate function that is run first within wp_filter_content_tags(). It also adds a utility function wp_image_src_get_dimensions() with logic reused from wp_image_add_srcset_and_sizes(), and it ensures that width and height attributes only get added if both of the attributes are missing on the original img tag.

This changeset furthermore improves test coverage and separates tests for the different aspects of img tag modification.

Props azaozz.
Fixes #50367. See #44427.

#23 @flixos90
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.

#24 @flixos90
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 48239:

Media: Only add loading attribute to img tags using double quotes.

Props azaozz.
Fixes #50367.

#25 @Otto42
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 @Otto42
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 @azaozz
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 @azaozz
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 @flixos90
4 years ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.