Make WordPress Core

Opened 7 weeks ago

Last modified 6 weeks ago

#59641 new defect (bug)

manually setting fetchpriority on image should prevent core from adding fetchpriority attribute

Reported by: adamsilverstein's profile adamsilverstein Owned by:
Milestone: 6.5 Priority: normal
Severity: normal Version: 6.3
Component: Media Keywords: needs-patch
Focuses: performance Cc:

Description

When working on testing the automated fetchpriorty attribute core now adds for images, I discovered this bug:

If users manually set fetchpriority="high" on an image either programmatically or using a plugin, core will not change that setting, however it unexpectedly sets fetchpriority="high" on another image.

Steps to reproduce

  1. install this plugin to add a manual fetchpriority dropdown: https://github.com/adamsilverstein/wp-fetchpriority-control
  2. create a post with several large images
  3. select the first image and apply fetchpriority="high" (under advanced)
  4. publish the post and view its source

Expected results:
Only the first image should have the fetchpriority="high" attribute, setting high fetchpriority on more than one image reduces the effect of adding the attribute.

Actual result:
Core applies fetchpriority="high" to the second image so the first two images contain the attribute.

Change History (8)

#1 @adamsilverstein
7 weeks ago

  • Milestone changed from Awaiting Review to 6.5

#2 @flixos90
7 weeks ago

@adamsilverstein How does the plugin add fetchpriority="high"? Is it dynamic (via PHP logic), or is it hard-coded in the saved block content in the database? Would be great if you could provide additional context on that.

#3 follow-up: @adamsilverstein
7 weeks ago

@adamsilverstein How does the plugin add fetchpriority="high"? Is it dynamic (via PHP logic), or is it hard-coded in the saved block content in the database? Would be great if you could provide additional context on that.

Good question!

It is saved as a block attribute, then applied to the markup dynamically using the render_block filter - see https://github.com/adamsilverstein/wp-fetchpriority-control/blob/dd0281b319794d7fe44317cb588a6c135f970200/wp-fetchpriority-control.php#L34-L54

Do you think the timing of this filtering is part of the issue?

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


7 weeks ago

#5 in reply to: ↑ 3 @flixos90
7 weeks ago

Replying to adamsilverstein:

It is saved as a block attribute, then applied to the markup dynamically using the render_block filter - see https://github.com/adamsilverstein/wp-fetchpriority-control/blob/dd0281b319794d7fe44317cb588a6c135f970200/wp-fetchpriority-control.php#L34-L54

Do you think the timing of this filtering is part of the issue?

Hmm, the render_block filter should be fired before the relevant logic. I wonder whether the issue is more about the image ordering. Here's why: WordPress core only goes through all the images sequentially. So by the time it encounters the image that your plugin has added fetchpriority="high" to, it may have already added the attribute to one of the images before.

When you encounter the bug, is the image that WordPress adds fetchpriority="high" to before the image that your plugin adds fetchpriority="high" too? If so, that would explain the problem.

#6 @adamsilverstein
7 weeks ago

Hmm, the render_block filter should be fired before the relevant logic.

Yes, I'm pretty sure core "saw" the attribute because before applied it manually, core added fetchpriority="high" to the first image. When I manually added fetchpriority="high" to the first image, core added it to the second image.

When you encounter the bug, is the image that WordPress adds fetchpriority="high" to before the image that your plugin adds fetchpriority="high" too? If so, that would explain the problem.

In my test, I'm pretty sure I set fetchpriority="high" on the first large image and core still added it to the second image.

Regardless - I feel we should probably avoid adding fetchpriority="high" to any image if the content already contains an image with fetchpriority="high". What do you think? We could avoid this with a content check early, before adding it.

Here is a test post where you can see the behavior: https://wp-sample-pages.instawp.xyz/with-manually-set-high-and-low/

#7 @flixos90
7 weeks ago

@adamsilverstein We can see if we can find a solution to that problem, but at the moment, due to the sequential processing of images, we can't update an image that was already given fetchpriority="high" when later another image already had the attribute present from a plugin. We would need to somehow parse the entire content in advance, which I'm not sure is feasible unless we apply output buffering - especially since the problem could go beyond images in the post content, these images could appear anywhere.

Based on where we are now, I'm curious whether you could apply the new 6.4 filters in your plugin to ensure the fetchpriority="high" attribute is only on the one image that it was added to. See https://make.wordpress.org/core/2023/10/18/image-loading-optimization-enhancements-in-6-4/#:~:text=You%20could%20use%20the%20wp_get_loading_optimization_attributes%20filter%20to%20ensure%20a%20specific%20image%20within%20post%20content%20receives%20the%20fetchpriority%3D%22high%22%20attribute%20while%20other%20ones%20do%20not%3A for a related reference example.

#8 @adamsilverstein
6 weeks ago

Good suggestion about adding filters in the plugin to prevent the duplication. I’ll give that a try and also keep thinking about how we could do better automatically.

Note: See TracTickets for help on using tickets.