Make WordPress Core

Opened 7 months ago

Last modified 3 months 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.6 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 (15)

#1 @adamsilverstein
7 months ago

  • Milestone changed from Awaiting Review to 6.5

#2 @flixos90
7 months 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 months 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 months ago

#5 in reply to: ↑ 3 @flixos90
7 months 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 months 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 months 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
7 months 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.

#9 @flixos90
5 months ago

@adamsilverstein Checking in here. Two things:

#10 @adamsilverstein
5 months ago

In https://wp-sample-pages.instawp.xyz/with-manually-set-high-and-low/, which of the images is the one you manually added fetchpriority="high" for?

The first, core adds it to the second.

Regarding your last comment, have you been able to see about using the new 6.4 filters?

Not yet, thanks for the reminder.

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


5 months ago

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


3 months ago

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


3 months ago

#14 @joemcgill
3 months ago

@adamsilverstein have you been able to verify whether the filters provide your plugin with the necessary approach to addressing this issue? Not seeing movement on this for a while, I'd suggest we move out of the 6.5 milestone unless you're planning to take a look at this soon.

#15 @swissspidy
3 months ago

  • Milestone changed from 6.5 to 6.6

I'd suggest we move out of the 6.5 milestone

Agreed. Feel free to move back once there is a patch & owner.

Note: See TracTickets for help on using tickets.