Improve Largest Contentful Paint in Umami front page

I love the focus on performance.

TBH, we should try not to use background-image anymore because we don't support IE.

A better way to do it would be to output the image as normal (via an <img> tag), absolute position it against the container, and then use object-fit: cover (see https://developer.mozilla.org/en-US/docs/Web/CSS/object-fit).

This would have a lot of benefits:

• The image would be immediately discoverable by the browser's preload scanner. Currently for the browser to output the image, it needs to 1) download the CSS file, 2) parse the CSS file, 3) reconcile that with the DOM and realize it needs an image, and 4) download and display it.
• We could easily use responsive images here. Right now the image is 3000px wide. Obviously that's not ideal for mobile.

While we're at it, we could also update the image styles to use webp for the image format. We also need to make sure that when loading the image (like normal) the image is not lazy-loaded.

Here's a patch showing the method.

Notes:

• This isn't implementing responsive images
• Even though lazy-loading is disabled, the image is still lazy-loaded because of 🐛 Setting media field to loading="eager" doesn’t work when using the media_thumbnail field formatter Fixed .

Oops... the absolutely positioned image wasn't properly anchored when not logged in. New patch fixes this as well as removes some unneeded CSS rules.

Could we add a new media view mode instead of using the thumbnail formatter? I think we should add 📌 Direct formatter for media items Closed: duplicate to core and it could use that eventually, but there's disagreement on whether the thumbnail formatter should be used for anything other than admin-facing thumbnails.

While we're at it, we could also update the image styles to use webp for the image format.

We should do that in 📌 Add webp image conversion to core's install profile's image style Fixed rather than here, but yes :)

Could we add a new media view mode instead of using the thumbnail formatter?

Sure thing. Note that this doesn't work out of the box because the media template inserts a wrapping element that gets position: relative; when authenticated. I'll have to remove that wrapping element by overriding the template.

there's disagreement on whether the thumbnail formatter should be used for anything other than admin-facing thumbnails

I know this is an aside, but this blows my mind a bit. The thumbnail formatter is the primary formatter that I use, because it's dramatically easier to set up, less config, less markup, yada, yada.

I'll likely update the patch in a day or two!

Images can be converted to WebP it allows to cut few tens kilobytes, see #3275557-10: Add webp image conversion to core's install profile's image style →

I've been using this as a test case for a an APM dashboard for core, and just got to a point where it's able to show the issue, see ✨ Add OpenTelemetry Application Performance Monitoring to core performance tests Fixed .

Thanks for bumping this to the top @catch (its a fun thing on a rainy Florida afternoon 😀)

New patch attached. This does several things:

• Implements responsive images on for the hero (and creates necessary derivative image styles)
• Converts all hero image image styles to WebP (we should create a followup for all other image styles)
• Displays the image directly from the DOM (instead of using background image)
• Removes lazy-loading of the hero image

Forgot to remove the UUID keys from the config. New patch attached.

I didn't create any interdiffs in #13 (sorry) because lots of changes and necessitates a full review.

Fixing an issue where image wasn't positioned against container when anonymous, and also cleaning up some newly unneeded CSS rules.

Updating issue summary

I really like the general direction of this, but I think we've messed up the CSS a bit (testing on 10.1, as I think it's okay for Umami to bring in new changes in point releases).

Small screen - looks fine

Medium Screen - image becomes a big square taking over the full screen, not the "Read article" link in bottom left, as it's covering the cards that come after it

Large Screen - see how the image is covering the cards that follow after this component.

I'll try fix this up now, but then I think I'll propose that we remove the new 7:3 responsive image style and create one called "Hero", so we can have a portrait image for small screens, some other ratio for tablets, and 7:3 for large screens which I think will be more in tune with the designs.

Here's my suggestion

1. Rewrote the template to use BEM classes
2. Added container class for the summary area
3. Rewrote the CSS to use GRID so things stay withing the DOM flow

As I said in the previous message, I'd prefer us to _not_ use 7:3 aspect ratio for all the image styles in this component and would rather if we had different dimensions/ratios for different view ports.

New patch after running the CSS linter. Interdiff is against #15.

After applying the patch in #20 and running though installation, I get this error:

Medium Screen - image becomes a big square taking over the full screen, not the "Read article" link in bottom left, as it's covering the cards that come after it

@markconroy - looking through your comment, I think you tested the wrong patch (I think you tested #15). The patch in #16 should resolve those.

I tested #16 again, and verified it worked (on my local 🙃). Can you check again?

Is responsive_image module installed by demo_umami install profile?

+++ b/core/profiles/demo_umami/config/install/responsive_image.styles.hero.yml
@@ -0,0 +1,35 @@
+    multiplier: 1x

where are the 2x and 3x multipliers?

@mherchel Darn, looks like you are right, not sure how I missed #16

In any case, I think I prefer the HTML and the CSS of my #20. How about we take your config and my template and CSS?

where are the 2x and 3x multipliers?

Not sure they're needed. When adding those, they tend to overcomplicate things IMO. I think the regular 1x image looks fine on my iPad FWIW.

In any case, I think I prefer the HTML and the CSS of my #20. How about we take your config and my template and CSS?

Sounds good. I'll work on this later today or tonight and throw in another patch.

Not sure they're needed. When adding those, they tend to overcomplicate things IMO. I think the regular 1x image looks fine on my iPad FWIW.

This is supposed to also be a training opportunity too. Are we suggesting that adding 2x,3x image styles aren't needed in general and site owners should focus their efforts on other things instead to improve perceived performance?

@heddn for the current issue, let's not expand the scope of it too much. It might be a good idea, however, to open a new issue to have 2x and 3x multipliers added for Umami images.

New patch incorporating #16 and my updates from #20 (with some amendments).

fwiw 2x/3x images as a follow-up seems like a good plan, that's more of a design question than about loading performance (except that it allows the 1x resolution to be lower I guess).

#28 is 99.99999% there!

New patch fixes a couple things:

• Image was taking full viewport when anonymous (same mistake I did in #15)
• The template was using tabs instead of spaces (I'm surprised CI didn't catch this)
• Some CSS comments where put on a new line (probably because of editor config).

Up for another review 😀

Minor indentation fix.

Mostly questions here. This is looking really great.

1. +++ b/core/profiles/demo_umami/config/install/responsive_image.styles.hero.yml
   @@ -0,0 +1,35 @@
   +fallback_image_style: scale_crop_7_3_large
   

   Almost never will the fallback be used with the state of modern browsers. The cases where it will are for older mobile devices and desktops using older browers, i.e. etc.

   I wonder if we should be rendering a 1400px wide image in those cases. In my mind, these older devices go hand in glove with smaller displays. Is the small or medium a better fallback?

   I'm never really sure what to do in this case, so your guys input would be valuable here.

2. +++ b/core/profiles/demo_umami/themes/umami/templates/content/media--scale-crop-7-3-large.html.twig
   @@ -0,0 +1,21 @@
   +{% if content %}
   

   Some comments on why this special template is needed could help someone as it isn't readily apparent why we need it.

3. +++ b/core/profiles/demo_umami/themes/umami/umami.breakpoints.yml
   @@ -19,3 +19,19 @@ umami.wide:
   +  weight: 3
   ...
   +  weight: 4
   ...
   +  weight: 5
   ...
   +  weight: 6
   

   Probably a bit of a nit, but should these weights be 0,1,2,3?

4. +++ b/core/profiles/demo_umami/themes/umami/umami.breakpoints.yml
   @@ -19,3 +19,19 @@ umami.wide:
   +  mediaQuery: 'all and (min-width: 1400px) and (max-width: 3000px)'
   

   Do we need a max width? It feels like if someone had a super large display, they would fall back to the tiny breakpoint. Which doesn't seem right.

Almost never will the fallback be used with the state of modern browsers. The cases where it will are for older mobile devices and desktops using older browers, i.e. etc.

Agree. Right now the field is required though.

I wonder if we should be rendering a 1400px wide image in those cases. In my mind, these older devices go hand in glove with smaller displays. Is the small or medium a better fallback?

That makes sense to me.

Some comments on why this special template is needed could help someone as it isn't readily apparent why we need it.

Comment is there. Right now it's

+{#
+/**
+ * @file
+ * Theme override to display a hero media item for the Umami theme.
+ *
+ * Wrapping elements are intentionally removed because the attributes can add
+ * `position: relative`, which interferes with absolutely positioned image.
+ *
+ * Available variables:
+ * - name: Name of the media.
+ * - content: Media content.
+ *
+ * @see template_preprocess_media()
+ *
+ * @ingroup themeable
+ */
+#}

Probably a bit of a nit, but should these weights be 0,1,2,3?

I followed the current pattern, but I'll take another look.

Do we need a max width? It feels like if someone had a super large display, they would fall back to the tiny breakpoint. Which doesn't seem right.

You are 1000% correct. Good catch!

New patch attached!

I did a manual test of with/without the patch. Results will vary, but I found with the patch we dropped to 1.9seconds for LCP and without it was 3.5seconds.

I'm good with RTBC and we can improve other parts of the page in follow-ups.

we dropped to 1.9seconds for LCP and without it was 3.5seconds

🙌 🙌 🙌 That's like 50% decrease!

There was some points from lighthouse about sizing and format of the images. We can do more to improve the performance. But I suggest we land as-is rather then let perfect be the enemy of good enough.

Trying to use this as a test case for the performance monitoring dashboard and it's seeing a change, but opposite to the one lighthouse and common sense would indicate. This is probably to do with the specifics of the test (browser and Drupal caches etc.) rather than a problem with the patch, so shouldn't hold up commit and I can always -p1 -R the patch for more testing once it's in.

Will update here if I manage to figure out why it's doing it, but otherwise just enjoy the pretty picture and ignore what it actually says.

Note that the initial load of the image takes longer because the image style needs to generate and convert (and then it has to do it for all of the viewport sizes because of responsive images). Make sure you warm the caches and at all screen sizes.

In the meantime, I'm writing an article about this, and will be doing some testing.

Here are my results from throwing up two temp dev sites up on Pantheon, and testing them with WPT

Baseline: LCP 2.443S

With Patch: LCP 1.379S

One thing I'm noticing here is that the background image (without the patch) is loading the full image without image styles. It's highly optimized and has a smaller filesize than the same-sized WebP version:

• The 3000px JPG is highly optimized, and it's coming in at 226KB.
• The 3000px WebP image style is coming in at 301KB.

Either way, we're making progress.

I altered the test so that there's a front page request before a cache clear, then a request to different page, then the front page again - that gives us 'lukewarm' caches with the image derivatives on disk, but without asset aggregates, the page cache etc.

Still a visible change and in the right direction this time :)

Test diff is here: https://git.drupalcode.org/project/drupal/-/merge_requests/4226/diffs?co...

You can see from the graph that the largest contentful pain merges into the first contentful paint with the patch applied - I double checked they're the same size, which means that in the patched version the first contentful paint is also the largest contentful paint.

The webp vs. jpeg issue is unfortunate, but seems quite specific to having a highly-optimized jpeg so I think we should live with it. Unless I guess we wanted to configure the original image for that viewport size? But then even with Umami can we guarantee every image is similarly optimized?

2x and 3x we should definitely do in a follow-up and not here.

For me this is ready, but I am not really qualified to review the CSS. There is less going on it it than there is in HEAD though which is a good sign.

I am qualified to review the CSS 😁

Everything here looks fine. The nits below are completely optional.

.banner-block__image img {
    position: absolute;
    top: 0;
    left: 0;
    width: 100%;
    height: 100%;
    object-fit: cover;
}

"top" and "left" could be replaced with a single "inset:0;". This reduces the CSS by a negligible amount and adheres to the best practices of logical properties

.banner-block__content {
    position: relative; /* Establish stacking context to appear above absolutely positioned background image. */
    max-width: 50%;
    margin: 0;
    padding: 1.777em;
    color: #fff;
    border: 1px solid #464646;
    background: rgba(0, 0, 0, 0.42);
}

This is a super pedantic nit, but these rules don't actually create a new stacking context, they just position the content properly (additional info)

I'm good with committing as is. The inset point is valid, but tbh there are many places where we're doing top and left, and I think it's okay.

This is a super pedantic nit, but these rules don't actually create a new stacking context, they just position the content properly (additional info)

Note sure you're right here. According to https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_positioned_layout/U...,

A stacking context is formed, anywhere in the document, by any element in the following scenarios:

• Element with a position value absolute or relative and z-index value other than auto.

Either way, we can discuss this is Slack.

Let's leave the top and left CSS as is. If we use inset, then it's in effect saying "top: 0; right: 0; bottom: 0; left: 0" and in that case we should also remove the width and height, since inset will have taken care of it.

Let's not hold this issue up any more, looks like enough maintainers and committers are happy with it as is. We can create a follow up to rewrite our CSS in more modern, logical property format.

OK that's the sort of CSS review I was hoping for :) Let's open a follow-up for the more concise properties, but agreed that's fine in a follow-up, this is already improving a lot of things in one go.

Tagging for 10.2.0 highlights, it's not a 'feature' but it's an improvement to a feature (the Umami demo).

Fixing tag.

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