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 #3307409: Setting media field to loading="eager" doesn’t work when using the media_thumbnail field formatter.

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?

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

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?

+++ 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).

#28 is 99.99999% there!

New patch fixes a couple things:

• Image was taking full viewport when anonymous (same mistake I did in #15). Re-added position: relative; to the container.
• The template was using tabs instead of spaces (I'm surprised CI didn't catch this)
• Some CSS comments were 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.

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 (at mobile widths, 4G) 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 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.

Fixing tag.