Set theme logo fetchpriority to high

This seems like something that should be configurable. Theme logos are likely not a universally standard LCP element. It will likely vary from site to site.

If we prioritize a non-critical image, it could actually be a performance regression in some cases.

I think we need some concrete before/after metrics here at least on the core themes.

I think the issue may benefit from taking a step back and defining the goal. Is the goal to improve the LCP web vital or something else?

I just took https://aplpestcontrol.com/ (arbitrary site, Olivero based) as an example and a paragraph (<p>) is the LCP element on mobile, and a hero image is the LCP element on desktop.

When we think of an image as "important", I think there needs to be some quantifiable measure of that.

I'm not saying that adding fetchpriority to the branding image is necessarily a bad idea, but just that the decision to do so should be data driven.

Trying to address #5, seems like there's broadly three considerations:

1. Is the branding image likely to be the largest image or text block on some sites or screenports? Landscape on a phone seems like the most likely case, or say on a page that shows a list of titles with no images or large blocks of text. If so, then this will help LCP in a probably tiny-but-non-zero percentage of requests. Something like https://www.drupal.org/project/issues/user it might (but without the highlighted block).

2. If the branding image isn't the largest element, is there still a benefit to it having high fetch priority? We know it's very likely to be in the viewport, so seems like a good chance it could reduce overall paint time, but not sure how to measure that.

3. If the branding image isn't the largest element, and the largest element is an image, and it also has fetchPriority=high set, is there a drawback (i.e. do we need fetchpriority=high on only one image), from discussions in the previous thread it seems like a couple of images is probably fine but not much more than that.

FWIW, fetchpriority=high shouldn't slow down the LCP of text elements under most conditions (doesn't hurt to get field data to be comfortable though).

At least in Chromium (only engine that supports it for now), fetchpriority=high on images changes the image priority to be "high" which is the same priority as parser-blocking scripts but resources of a given priority are still requested in the order they are discovered.

Usually images will get a "low" priority which is the same as async/defer scripts so any async scripts early in the document would cause images to be delayed and any parser-blocking scripts late in the document would also jump ahead of images. fetchpriority=high jumps those images ahead of any async scripts and ahead of any blocking scripts that are later in the HTML than the image.

It also moves the important images out of the 2-step loading that Chrome does where "low" priority resources are held back until the main parser has reached the body (all blocking resources have loaded and executed). The hold-back can cause an additional RTT of delay where if it is tagged as "high" it could already be in-flight (but still behind the parser-blocking scripts before it).

Text-based elements for LCP mostly just depend on the parser being able to make it to the body of the document and any stylesheets to have been loaded, all of which would be requested before an image that is in the body of the document even with fetchpriority=high. Fonts are a bit of an edge case depending on if they are preloaded or not and the best way to help a text block LCP is to make sure the fonts it needs (if using font-display: block or defaults) are preloaded so the browser doesn't have to wait for layout to discover that they are needed.

As far as putting fetchpriority=high on multiple images goes, it doesn't necessarily hurt but the images will be prioritized in the order they are discovered so if a theme is using a layout where the main image is later than other images that are tagged, it could be loaded after those other images but if those other images are also important for the user experience it probably makes sense to tag them. Strictly-speaking, to optimize for the LCP metric itself it would be best to tag just the LCP image but optimizing for the user experience in general could involve tagging a few other images (logo, maybe some nav images, etc).

The thing I'd strongly recommend against is preloading the LCP image early in the head AND tagging the preload with fetchpriority=high. In that case, the image would get loaded before all of the parser-blocking scripts and it would slow things down.

any parser-blocking scripts late in the document would also jump ahead of images.

Drupal core doesn't currently add defer to any core JavaScript, and puts as much as possible at the end of the page, so this is the base behaviour unless you're altering things. This should mean that adding fetchPriority=high to the branding image would be worth doing.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Posted a comment to the wrong issue. Please ignore!

Is phpstan supposed to fail on twig files? That's the only item that failed the needs-review-queue-bot's tests in #10.

Wonder if this could get a simple change record also.

Tested this out making sure fetchpriority appeared

On the olivero theme
Updated the Appearing setting to use the theme logo
Verified it appeared and had fetchpriority

Claro theme
Set as default frontend theme
Placed a block Sitebranding block
Verified it appeared and had fetchpriority

Umami
Did an install
Verified logo had fetchpriority

Stark
Installed as default theme
Verified Site branding block is placed
Verified logo had fetchpriority

LGTM

Here's before/after.

Steps to reproduce:

1. install Umami
2. Log out (or go incognito)
3. Make sure caches are warm
4. rm -rf sites/default/files/css and sites/default/files/js - this is to make sure aggregation generation has to happen, although I'm not sure that made any difference in this case.

Repeat steps 3 and 4 with an without the patch, with an extra cache clear before #3 to make sure the markup changes.

Before

After

So the big change is that the logo gets requested before the JavaScript aggregate but they're requested almost simultaneously in this request because there aren't that many things to get in total.

My theory is that if there were a lot of JavaScript files - for example aggregation off, or just lots of aggregates, then this might be a more noticeable change because the image can be loaded before those files. Looks like the magic number might be six?

If we used image styles for site logos, this could have a bigger impact still when the derivatives don't exist on disk, because it would start requesting earlier, allowing image style generation to get started faster too. Not sure how many files chromium requests at once before they go into a queue.

Notably because the image is first in the HTML, it's currently getting requested before any other images. If we add fetchpriority=high to the hero image, that might change, but I think the big gain here is requesting images before JavaScript more than requesting images before each other (because they're otherwise requested by order of appearance in the HTML markup anyway).

Have any performance tests been performed against this change with core themes?

Taking one step back -- has "performance" even been defined here? Web vitals? Something else? Does hypothetically delaying javascript increase the likelihood for real-world CLS regressions?

I'm not trying to naysay efforts, but only that there should be real data to base performance changes from versus theoreticals (re: #17). We have blackfire for php...but what do we have for the front-end?

@luke.leber hitting web pages with dev tools and/or lighthouse and interpreting the results is about as close to blackfire/xhprof as we have. It's true that page loading performance is a lot less objective - there's more network differences, viewport sizes etc. that will impact things in real life as well, so it's always going to be a bit theoretical compared to profiling PHP. Even profiling PHP is doesn't necessarily give reliable results when trying to assess the real-life impact of things like optimizing away function calls, because it adds a lot of overhead to calling functions compared to other things. #3352459: Add OpenTelemetry Application Performance Monitoring to core performance tests is trying to improve things so we can see trends over time (both front and back end, but starting with front end).

I found a better example page - switched the admin theme to umami and hit node/add/recipe. Due to ckeditor and other things this has enough JavaScript on it that some queuing happens. Also in that configuration, the umami logo is the largest contentful paint element on desktop.

Including a screenshot of the waterfall too this time since it shows what happens slightly better. Same steps for this patch - apply the patch, clear caches, warm cache, shift-refresh the page.

Before:

After:

As you can see there's enough files this time that the files are requested in two batches, and the logo shifts from the second batch back to the first.

The js file that's still requested before the logo is touch-events.js which is in the header, @luke.leber this partially answers your layout shift question, if js has been put into the header explicitly to avoid layout shift (as has been done for touch-events.js), this change won't impact that. However the bigger thing we can do for layout shift is to add dimensions to the logo image, see #3345259: Theme logo image is rendered without dimensions. Also worth mentioning that depending on what it's doing the js being loaded may not cause layout shift at all.

After realising the logo was the LCP on node/add I checked the user/login page where it shows up with the default configuration. On mobile, the largest contentful paint element is indeed the logo image. On desktop it's somewhat unexpectedly the password field description. In my testing of the user/login page, I'm not actually seeing a difference in LCP, but it would be more likely to make a difference on a slow connection instead of testing on the same machine.

However, logging into a site with a logo on mobile is pretty common so I think that shows this will improve LCP in some circumstances, just not as many as on a hero image. I also think the results above show that it's the right place to put fetchpriority=high as well as a hero image.

The overall effect of this is extremely subtle and unlikely to make a difference for a lot of requests (and my guess is this will be the same for the hero image too), but if it makes a small improvement on some requests and doesn't make things worse it's worth doing.

Sounds good, @catch. FWIW we've found that the best thing for our particular theme is to serve logos as inline SVG's in the DOM response. We went down the path of asset downloads getting stuck in "queueing" like you're seeing with the waterfall already and the only thing we found that cut out the blocking "roundtrip-like-condition" was reducing render blocking http requests (and treating LCP images as importantly as render-blocking calls!).

That cuts out the potential 600ms "3G latency" base cost at the expense of adding ~0.25kb to the page size. That was something that actually moved the needle in an observable fashion in our Lighthouse CI testing suite.

Patch in #1 doesn't apply.

Re-rolled patch at #1 against 11.x. Please review

Reroll seems good so restoring previous status.

Committed/pushed to 11.x, thanks!