Make WordPress Core

Opened 16 months ago

Last modified 8 months ago

#59464 new defect (bug)

Images hard-coded in block theme templates lack `width` and `height` attributes

Reported by: spacedmonkey's profile spacedmonkey Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 6.4
Component: Themes Keywords: needs-patch
Focuses: performance Cc:

Description (last modified by flixos90)

All images hard coded into block templates and patterns should have height and width attributes (applies e.g. to the TT4 theme).

It also prevents loading optimization attributes from being added to these images, so effectively this harms both load time performance and leads to layout shifts.

Change History (28)

#1 @spacedmonkey
16 months ago

See #59256 / [56613] for context.

This ticket was mentioned in PR #5323 on WordPress/wordpress-develop by @flixos90.


16 months ago
#2

  • Keywords has-patch added

This is a simple draft PR for https://core.trac.wordpress.org/ticket/59464.

What is currently implemented here is only for testing and does not work as expected.

#3 @flixos90
16 months ago

  • Keywords has-patch removed

After looking into this, unfortunately it seems that this bug cannot be fixed without having the image displayed incorrectly within the block editor. 😞

The only way to add width and height attributes to the hard-coded blocks in the TT4 theme is to add the width and height attributes to the block as well, as well as the is-resized class to the figure tag around the image - otherwise this is invalid core/image block markup.

That is easily possible to accomplish, however apparently the image will then display incorrectly within the block editor only. In the frontend it still looks fine. So in order to fix this in a way that works for both the frontend and the editor, we'll probably need to allow width and height attributes on the image decoupled from enforcing a specific image size.

Last but not least, even after adding the width and height to the block, for some reason the image still doesn't have the loading optimization attributes present. 🤔

See https://github.com/WordPress/wordpress-develop/pull/5323 for testing.

This ticket was mentioned in PR #5324 on WordPress/wordpress-develop by @flixos90.


16 months ago
#4

  • Keywords has-patch added

See https://core.trac.wordpress.org/ticket/59464#comment:3 for context: This PR explores an approach with which block template files with images can receive their image dimensions in a way that keeps the blocks compliant with the block editor markup. It also addresses an additional issue in wp_get_loading_optimization_attributes() that prevents the loading optimization attributes to be granted on the overall block template, which is relevant for the TT4 theme as it displays several images outside of block template parts.

There's a lot to unpack here, so bear with me:

  • To add width and height attributes to images in block template files:
    • Most importantly, this PR enhances the existing wp_img_tag_add_width_and_height_attr filter so that it can be used to return specific width and height attributes for an image. It'll furthermore pass the image src value to the filter, and the filter is run for images that aren't for an attachment as well - since the attachment ID is only necessary to _automatically_ fetch the dimension attributes anyway, which is still the default behavior from the filter's true value.
    • The new filter behavior is used in TT4 to return dimensions for the bundled images specifically. For now, since this is just an early draft, only the main hero image is covered, but this could easily be expanded to all other images.
  • To make loading optimization attributes work on images that are _directly_ within a block template:
    • The template context was so far completely skipped, however TT4 proves that that's not a reasonable approach, since images may also be included directly in a block template, i.e. outside of a template part.
    • The PR changes the logic to only skip images in the template context if they were already modified, which can be more or less reliably be assumed based on the presence of decoding="async", which is added to every image.
    • The PR then also caters for block themes that do not use the header template part, in which case the get_header action never fires.

There are obviously many changes included here, and this is an early draft for consideration. We may decide some of these approaches are reasonable while others are not. When testing though, it can be seen that the TT4 hero image on the home page is now properly receiving all expected attributes.

Trac ticket: https://core.trac.wordpress.org/ticket/59464

This ticket was mentioned in Slack in #core-performance by flixos90. View the logs.


16 months ago

#6 @luminuu
16 months ago

Another TT4 issue @desrosj

@spacedmonkey commented on PR #5324:


16 months ago
#7

This makes unit tests fail, which is a little worry for me.

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


16 months ago

#9 @swissspidy
16 months ago

That is easily possible to accomplish, however apparently the image will then display incorrectly within the block editor only.

I'd love to get thoughts from Gutenberg folks on this so that we can come up with a solution within the Image block that doesn't require each and every theme to hook into wp_img_tag_add_width_and_height_attr. Perhaps @Mamaduka or @gziolo have some thoughts? (Apologies for the random ping)

#10 @Mamaduka
16 months ago

@flixos90, @swissspidy, do you have an example or steps to reproduce for getting this state for an Image block?

#11 @swissspidy
16 months ago

@Mamaduka https://github.com/WordPress/wordpress-develop/pull/5323 can be used for a quick demonstration. Insert the Hero pattern and you'll see the large image. I wrote some notes there.

@swissspidy commented on PR #5323:


16 months ago
#12

So here's what you see when you insert that pattern with this PR:

https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/841956/a968facc-cb94-4f3f-bde1-839841a9f309

I actually see a few issues with this pattern / behavior besides the image dimensions:

  • The image uses wide-alignment, but because it's within a group block you can't actually change the alignment. Very confusing.
  • The image block has no controls for changing the dimensions. They're hidden because the image uses wide-alignment
  • Even if using no alignment and setting dimensions, width & height are actually set via inline style properties rather than HTML attributes

So there doesn't seem to be a way to use wide-aligned images _and_ pass dimensions for use on the frontend just so the aspect ratio is correct.

#13 @Mamaduka
16 months ago

Thanks, @swissspidy. I'll have a look.

@andraganescu, @ajlende, IIRC, you worked recently on image dimension/aspect ratio-related tasks. Any thoughts on the best way to solve the editor side issue?

#14 @spacedmonkey
16 months ago

@flixos90 Wasn't the feedback that all changes to TT4 need to happen on in the TT4 github repo?

joemcgill commented on PR #5324:


16 months ago
#15

Thanks for starting this exploration, @felixarntz. Adding some ability for block themes to handle this via filters is an interesting workaround, but doesn't seem like a good long-term solution to me, given that the experience we want theme authors to have is that they can build their block templates without needing to write a bunch of custom PHP code.

It seems like we've identified a needed enhancement in the workflow for something akin to the Starter Content feature that was added in WP 4.7 for the customizer, so that these theme assets can get converted to attachments in the CMS. Otherwise, I think we would want to be able to instruct theme authors to make sure their placeholder images always have h/w dimensions (or some other way of handling this via aspect ratio, if explicit dimensions isn't possible.).

#16 @flixos90
16 months ago

@spacedmonkey The proposed fix relies only partially on TT4, it's also touching core. It's only a proof of concept anyway though.

@joemcgill I agree with your concerns.

Taking a step back here, I wonder how much value there is in addressing the missing width and height problem for template images in the first place: The issue is about default theme images - which seems significant to address given we commonly use the untouched default theme templates in our benchmarks, but is this realistically even going to be a problem? Most sites probably won't use the default stock images that come with a theme, so they will replace them with media files, and then the problem of missing width and height will be gone.

I think what this indicates to me is that we may want to alter our benchmarking setup somehow, so that we test with sites using proper attachment images in the templates.

Separately though, the PR also covers another problem related to this ticket, which I think is more important: Even when width and height attributes are present, the image optimization attributes are not applied correctly with TT4. See lines https://github.com/WordPress/wordpress-develop/pull/5324/files#diff-4d28b6652f0dab208c19aa45e1cf09f3c967060d8cc77502bfcd473a3216b612R5647 and following for the relevant proposed change (plus further detail in the 2nd bullet point in the PR description). I think we should refocus the effort of this ticket on the latter, which is a bug that even applies to attachment images.

#17 @spacedmonkey
15 months ago

I think the case of a theme like TT4, you would go through and change the images. The images are designed to be placeholders. But if you were using block themes patterns to build a site for a client / friend, you may use block patterns to "hardcode" img tags into patterns. These patterns maybe be heavily designed and are not designed to replace images. Let's get out of the mindset of thinking about themes in themes of how core themes work. I think we should expand our testing to include popular block themes, like frost or ollie.

The issue here is a developer education one. There is no simple way of knowing the height and width of an image. So developers have to give us that.

That said, I do think we should focus on the gutenberg issue. At the moment, even if a developer does the right thing, it will appear broken.

#18 @flixos90
15 months ago

  • Milestone changed from 6.4 to 6.5

Punting this as there is no straightforward solution that would be realistic to get into 6.4 this late in the cycle.

Once we find a way to address it (hopefully for 6.5), we could consider backporting into a 6.4.x release.

#19 @flixos90
15 months ago

  • Description modified (diff)
  • Keywords needs-patch added; has-patch removed
  • Summary changed from Images in 2024 theme do not have modern image features to Images hard-coded in block theme templates lack `width` and `height` attributes

@spacedmonkey @joemcgill I've opened #59577 to focus on the lack of image loading optimization attributes on images within block templates, which is the other bug we found here (i.e. even if we fixed the missing dimensions, that bug would still apply). Therefore, I am refocusing this ticket specifically on the lack of dimension attributes on in-template images.

This ticket was mentioned in Slack in #core-performance by clarkeemily. View the logs.


15 months ago

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


12 months ago

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


12 months ago

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


11 months ago

#24 @swissspidy
11 months ago

  • Milestone changed from 6.5 to 6.6

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


10 months ago

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


9 months ago

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


8 months ago

#28 @mukesh27
8 months ago

  • Milestone changed from 6.6 to Future Release

As discussed in today's performance bug scrub.

Move to Future Release

Note: See TracTickets for help on using tickets.