Make WordPress Core

Opened 10 months ago

Last modified 8 weeks ago

#59638 new defect (bug)

Images: repeating a single image causes `fetchpriority` to be repeated

Reported by: adamsilverstein's profile adamsilverstein Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Media Keywords: dev-feedback
Focuses: performance Cc:

Description (last modified by adamsilverstein)

I noticed a small bug in how core auto-applies the fetchpriority="high" attribute. This feature aims to add this attribute only to the LCP image, which is frequently the first large image in a layout.

The bug is this: if a user inserts a single image several times, every instance of the image will have the fetchpriority="high" attribute.

Steps to reproduce

  1. create a new post
  2. insert a large image at the top of the post
  3. Insert the image several more times, so the post now contains several copies of the image.
  4. save and view the post source code

Expected result:
The first large image contains the fetchpriority="high" attribute.

Actual results:
Each copy of the image contains the fetchpriority="high" attribute.

Notes

I am posting this here because it feels like a bug, even though I'm not sure we should fix it:

  • The image URL for each item is the same so there may be zero impact to repeating the attribute (the same URL is prioritized). Unless there is a case where the actually fetched image from the srcset could be different for two copies of the same image?
  • There could be a slight negative performance impact when fixing this since the repetition is the result of an optimization where we only consider each unique image once.

Change History (16)

#1 @felipeelia
10 months ago

I found this one so interesting that took a minute to investigate why this was happening. Here are my findings:

This happens due to the logic applied in wp_filter_content_tags(). First, we run a regex to get all <img>s and <iframe>s, then all the images blocks. Then we go over all the <img> but on
this line we avoid changing the same image twice. As we've applied the attribute in the first one, it'll be applied to all the others.

#2 @adamsilverstein
10 months ago

this line we avoid changing the same image twice. As we've applied the attribute in the first one, it'll be applied to all the others.

Exactly! thanks for linking to the code that does this.

#3 @adamsilverstein
10 months ago

  • Description modified (diff)

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


9 months ago

#5 @afercia
9 months ago

Unless there is a case where the actually fetched image from the srcset could be different for two copies of the same image?

Question: what would happen in the following scenario?

  • A page layout is made of a wide area at the top of the page, typically a header.
  • The page content below lives in a narrower column.
  • The image is added a first time within the wide header.
  • The same image is then added multiple times in the narrower column of the content (or a sidebar, for example).

Would the actually image fetched by the browser be different in this case?

This ticket was mentioned in Slack in #core by jorbin. View the logs.


9 months ago

#7 @jorbin
9 months ago

  • Version trunk deleted

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


8 months ago

#9 @joemcgill
8 months ago

  • Milestone changed from Awaiting Review to 6.5

I'm moving this into the 6.5 milestone for further investigation. This may seem unexpected, but was in fact an intentional decision as I understand it, so we may end up closing this as a wontfix. For a use case like @afercia outlines above, I believe browsers will use the larger image from the srcset that it already fetched for the subsequent img tags rather than fetching a smaller file, which is the same behavior that browsers will use if it already has a larger that needed file from the source list in cache, but worth confirming.

It would also be worth seeing what the real performance impact of removing the optimization would be, since this is likely an edge case optimization anyway.

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


7 months ago

This ticket was mentioned in Slack in #core-media by joedolson. View the logs.


5 months ago

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


5 months ago

#13 @swissspidy
5 months ago

  • Milestone changed from 6.5 to 6.6

Punting to 6.6 since there is no activity, patch, nor an owner. Feel free to move back to 6.5 if there's a patch ready.

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


8 weeks ago

#15 @adamsilverstein
8 weeks ago

  • Milestone changed from 6.6 to Future Release

Moving this to Future Release as we continue investigating, this likely has a very small impact overall so can be considered low priority.

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


8 weeks ago

Note: See TracTickets for help on using tickets.