wvuweb / cleanslate-cms

A place to file issues and view releases for CleanSlate CMS. http://cleanslatecms.wvu.edu
6 stars 0 forks source link

Addressing CLS: Force Height to CleanSlate Generated Img Tags #267

Open dmolsen opened 2 years ago

dmolsen commented 2 years ago

Background

Google is pushing UX/web performance as a signal for search results. The initiative is called Web Vitals. There are three key measurements. The one that will be helped by addressing this issue is Cumulative Layout Shift. The basic premise is that we want to avoid the layout of a page shifting as elements load.

A good example is reading an article. As you scroll reading the article and an image or ad loads shifting the layout around and you have to scroll or find what you were just reading. In our case, responsive images generally don't take up their height in the page layout because the browser doesn't know how tall the image actually is until it's loaded and scaled to what its responsive width is.

Example of layout where image loads:

With Images Screenshot

Example of layout where image doesn't load:

Without Images Screenshot

Implementation Suggestions

I'm not sure there's a great solution here. Either data-asset-version could be used to populate img height andimg width or a CSS solution based on aspect ratio needs to be baked in.

I'm not sure how reliable data-asset-version is or how the AssetDecorator could be leveraged. I'm assuming that's where that solution would come in.

nreckart commented 2 years ago

data-asset-version is set when a user inserts an image into the editor via the "Insert Media" Mercury modal. It defaults to the width of the region the image is being inserted into and it maintains the aspect ratio, setting the height accordingly. The user can manually edit the width and height values and disable aspect ratio adherence, if they want. It is used by the system to automagically scale the actual image file on the fly, via Lambda these days.

So...I'd say it's fairly reliable, but it's only available on images inserted via the "Insert Media" Mercury modal interface. You could manually add/edit it within the HTML editor, but I highly doubt many people do that.

Adding height and width attributes to any <img> elements that have data-asset-version present, should be fairly simple. There is already a magical process (HTMLInjector) that dynamically processes all those images for scaling via Lambda. Could be modified to insert the proper height and width attributes at the same time.

nreckart commented 2 years ago

Added the width and height attributes to <img> tags that are inserted via the editor. May not help all that much since people tend to use images elsewhere, such as for thumbnails, and don't include the data-asset-version attribute. They also tend to use 1200x800 images for thumbnails.

It's available now on volutus if you want to take a look.

nreckart commented 2 years ago

Think this is good to go for production?

dmolsen commented 2 years ago

Nope. I still need to talk to Adam about the Bootstrap thing. I need to write up the issue and... I've been writing a bunch of other crap ;)

dmolsen commented 2 years ago

Fun times. As @nreckart noted, the solution he built isn't going to work for all the things but it's a start. The current solution on Volutus appears to mess up image dimensions. See example below:

Screen Shot 2022-01-14 at 10 58 51 AM

And what it should look like:

Screen Shot 2022-01-14 at 11 01 16 AM

This may be related to this StackOverflow ask and answer. I'm not going to dig into it too much. That's because I think I found another way to address this.

The other way to address CLS and images is to set the aspect ratio via CSS. Usually that would be to use something like aspect-ratio: attr(width) / attr (height) to use the image attributes. But that's fucking up on Bootstrap (and seems pointless since the attributes are already there?).

Anywho, if we instead stick the width and height attributes directly into the CSS like aspect-ratio: attr(600) / attr(400) the space is held to address CLS and no bugs with Bootstrap.

To replicate the solution:

  1. Disable images
  2. Load https://online.volutus.wvu.edu/admission
  3. Inspect the first image
  4. Add an inline style on the image style='aspect-ratio: 725/403'
  5. Remove the inline height and width attributes

The space that's held should be less (no stretch) but it's held. You can do the same with the image loaded and the image gets "fixed."

So my solution would be to drop the inline width and height and go to inline aspect-ratio

Thoughts, @adamglenn or @adamjohnson?

adamjohnson commented 2 years ago

A few things:

Automatically adding an inline style to all images with data-asset-version image attributes feels a bit hacky to me.

I was hoping the CSS attr() property had adequate browser support; alas, support is non-existent at this time. If support for that was better, we could avoid the inline style.

What do you think about making this feature opt in? Include a checkbox in Manage > Settings: "Add an inline CSS aspect-ratio to image tags in Mercury to help with Google Core Web Vitals". This could be enabled by default on all newly created sites.

I say opt-in because, at some point in the future if attr() support comes, it would be up to the theme developer to address this. We all know that a temporary fix can become permanent as life whirs on.

Also, the CSS aspect-ratio property is relatively new and not supported before Safari 15. As of right now, Safari 14 represents 25% of all Safari traffic to WVU sites according to Google Analytics. If we went forward with this, we'd want to be sure to test on Safari 14 / IE / some other old browser.

My .02 cents.

dmolsen commented 2 years ago

@adamjohnson -

I don't think I was quite clear enough in my response. Explicit height and width on images screws up responsive image layouts in Bootstrap 4. That's the first example above. Nathan had implemented that. Doing the "hack" of inlining the aspect ratio avoids the Bootstrap bug but holds the space for CLS issues.

I personally don't see this as a temporary fix. I don't understand why baking in aspect-ratio would be a problem when it's literally the dimensions of the photo that's been placed in there. It's not changing anything. It just holds space if no image is loaded. It's exactly the same as baking in the width and height on the image tag itself.

If the aspect-ratio "hack" is a dead-end, I'm totally open to having Nathan rip out the height/width fix. It can never go live with Bootstrap 4. It also only handles a subset of images that have been added to the site. The "save space" issue for images needs a long-term solution that I can communicate though. This is probably a Design System issue at that point though (e.g. place a featurette image and it has to have this aspect ratio).

adamjohnson commented 2 years ago

Explicit height and width on images screws up responsive image layouts in Bootstrap 4. That's the first example above. Nathan had implemented that. Doing the "hack" of inlining the aspect ratio avoids the Bootstrap bug but holds the space for CLS issues.

Yes, I would consider adding width, height, and inline aspect-ratio style attributes all part of that "opt in" checkbox. My apologies, I should have been clearer about that.

Thinking more about Safari 14.x: If someone visits a WVU site with this enabled, images are going to look stretched/bad due to the width and height attributes on the <img> tag & lack of aspect-ratio support. What are people's thoughts on this? Technically, we state that we only support the latest versions of browsers.

Thanks for your feedback. I'm open to whatever we want to implement.

dmolsen commented 2 years ago

Yes, I would consider adding width, height, and inline aspect-ratio style attributes all part of that "opt in" checkbox. My apologies, I should have been clearer about that.

Philosophical issue then... if Google recommends this for SEO (fire up Lighthouse) then is this something we should really opt everyone out of and only let people opt into? It shouldn't be a breaking change since it's literally the file they already uploaded.

If someone visits a WVU site with this enabled, images are going to look stretched/bad due to the width and height attributes on the <img> tag

My "fixed" proposal is only doing the aspect-ratio hack and leave off the width and height. Having width and height defined is what is screwing things with Bootstrap 4. So a final img tag would look like <img src='hey-world.jpg' alt='Hey World' style='aspect-ratio: 600/400'>

There's other shit in the image tag but that's the basics.

Next step is off-list and a discussion with @adamglenn. Not sure if Designer Meeting is the right place but seems like it. I don't think any of us feel that strongly to die on any particular hill.

adamjohnson commented 2 years ago
  1. Adding the aspect-ratio inline style and no width and height attributes sounds like a great idea.
  2. I'd be fine checking the "opt in" checkbox by default when rolling this out (people get auto-opted in).
    • Might want to do that for the Sitemap feature while we're at it.