Set theme logo fetchpriority to high

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.

I'm looking into doing this on my client site's hero image (which is a media image rendered with a custom view mode).

Adding a note that this is more difficult than it should be. I'd love a setting (maybe grouped with the new Lazy load setting) where I could toggle this on and off.

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.

CR opened.

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. ✨ Add OpenTelemetry Application Performance Monitoring to core performance tests Fixed 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 📌 Holistic logo image insertion and dimensions in themes Active .

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!

Automatically closed - issue fixed for 2 weeks with no activity.