silverstripe / silverstripe-asset-admin

Silverstripe assets gallery for asset management
BSD 3-Clause "New" or "Revised" License
20 stars 79 forks source link

CMS field for managing image `loading` state #1214

Closed brynwhyman closed 3 years ago

brynwhyman commented 3 years ago

Overview

With images moving towards being lazy-loaded by default, there will still be a place on a per image basis to opt-out of this setting. This issue focuses on creating a field in the CMS for content editors to decide to 'turn off' lazy-loading for a specific image.

A key example where this might be used is where the content editor knows that the image they are adding to a web page will be in view when the page loads ('above the fold') and so should load automatically.

Background: https://forum.silverstripe.org/t/browser-level-lazy-loading-from-silverstripe-cms/4218

Acceptance Criteria

Notes

PRs

clarkepaul commented 3 years ago

Based on team conversations about various approaches I've assembled 4 main possible directions we could go in. Would be good to get feedback from anyone who has opinions.

brynwhyman commented 3 years ago

Just for a bit more context, I've just had another look through some of the other implementations that we've been referencing. It doesn't appear that Wordpress or Typo3 actually provide a feature within the CMS UI to disable this on a per-image basis. See:

Wordpress's release notes seem to call out the main point where Developers may want to opt-out of lazy loading in in theme development - and expect that lazy loading images 'above the fold' is not expected to cause a notable issue in most cases:

Experiments using Chrome for Android have shown that the impact of such loading=”lazy” images in the initial viewport on the Largest Contentful Paint metric is fairly small, with a regression of <1% at the 75th and 99th percentiles compared to non lazy-loaded images – yet it is a consideration to make where theme developers can apply some fine tuning for even better user experience.

Drupal does has an open issue and WIP PR to add a field:

IMO there's still a non-zero impact on having images in the initial viewport lazy-load so it's worth providing the option to content editors to disable this on a per image basis. But still, it does seem like something that's not expected to be used often, if at all.

[/end rant]

jonom commented 3 years ago

I think the design concepts are great, really well thought out. I like Option 3 the best even if a drop-down isn't best practice.

To be honest though, I think this feature is at odds with the CMS's goal of being just for content editors. Whether or not to lazy-load is a technical (developer) decision and I don't think content authors should be burdened with having to make (or understand) this choice. So I would prefer not to have it in the CMS UI at all.

clarkepaul commented 3 years ago

I couldn't find one implementation of a CMS control at the file level for lazy loading, found lots of system wide CMS controls to turn it on/off though, including for iframes and the like. @brynwhyman good find with the control at the file level, will add to my lot of research.

I did a bit research with 4 non-developers about those 4 mockups:

If we are to have a control I'm leaning towards the dropdown as its already a component, doesn't draw loads of attention, reassures users that "lazy" is the default and not to worry if they are unsure about the control. Potentially could be extended by a developer if they wanted to add to it as more options come along e.g. chrome uses 'auto' but doesn't provide any additional functionality as yet, is supposed to be browser defined option.

@brynwhyman with the stats showing such little difference with 'lazy loading' applied it makes it a little hard to justify having it in the CMS apart from the fact we know clients who would be wanting it, even for peace of mind.

brynwhyman commented 3 years ago

@clarkepaul In case you missed it above, I did see Drupal will be implementing a UI toggle at the image level: Drupal - 'Add UI for 'loading' html attribute to images'.

Another vote from my perspective on the dropdown, again on the basis that it's the most passive option and essentially not calling for an action - which in most cases wouldn't be required.

As above though, I do seeing this being an option that's taking up a decent amount of space in the image edit form, for something that's not often going to be used. I think we should expect content editors to be capable of these decisions, they should have page performance in mind the same way that within their control, they're conscious of creating an accessible page. My beef with it is more: how much value is it actually providing?

We could consider the impact of not including it? Developers have an API to turn the lazy-loading off for theme templates and objects like 'hero banners' which should cover most use cases. If that's missed, then what's the performance impact still? The following blog post from Google states the following:

Fortunately, the impact of marking above-the-fold elements with loading="lazy" is fairly small, with a regression of <1% at the 75th and 99th percentiles compared to eagerly-loaded elements.

I think the thread in this issue so far is in agreement of not proceeding with this work.

emteknetnz commented 3 years ago

I don't think content authors should be burdened with having to make (or understand) this choice

Fortunately, the impact of marking above-the-fold elements with loading="lazy" is fairly small

Seems like this feature would add complexity and a worse UX for very little benefit to justify it

+1 for ditch it

clarkepaul commented 3 years ago

Just seeing if I understand the time difference, looking at this article using downloadDuration there seems to be about a 1s difference between Lazy and Eager, so for 5 images wouldn't there potentially be a significant difference? maybe not x5 time difference but potentially a second or more again, and that's on top of the rest of the page loading? I think we'd need to make a call based on the difference of several images rather than one.

FYI going by the users I showed it to, they were excited to see a "new" thing, with the thought of potential benefits, even if I didn't mention what the time differences equated to. I think there is some value in providing users some control, even if a minute result, not to mention the engagement value of providing toolsets to both devs and users. I'm on the fence, I don't think the UI is badly impacted with the addition, its more the development effort vs other things we could be doing really.

Surprised to see a difference in Eager and Auto times differ so much.

UPDATE: Just seen that article provides dynamic stats so what I was going off changes each time. Will take another look at better stats tomorrow

maxime-rainville commented 3 years ago

Sequence for loading images

Here's my general understanding of how browsers will lazy load images.

  1. Download HTML.
  2. Download CSS and eager loaded images.
  3. Render page layout.
  4. Figure out which images are above the fold.
  5. Fetch lazy loaded images that are above the fold.

My expectations is that when the images are fetched on step 4, they are fetched in parallel: it downloads all 5 images at once ... as opposed to 1 at a time. So if you 5 lazy loaded images to fetch on render, you are not going to multiply the time you have to wait by five.

The delay is due to the fact that the image fetching is starting later in the rendering process.

CMS UI consideration

We have a wide variety of CMS users ... I suspect it would be safe to lazy load everything, but that there will be some users who want to control this behaviour. Maybe we make it configurable (devs can decide to show/hide the field for their users) and/or we move it the field to some "Advanced options" tab.

Effort to implement the solution

Don't really have any preference on the options used to display the field. But it's worth pointing out that some of those options will be more difficult than others. That's does not mean we don't pick the more difficult options, but we should make an effort to validate that they provide additional value to justify the extra effort. Some of those options might also provide value beyond the scope of this card.

Placement

Getting the the field displayed next to the dimension fields is actually not trivial. It's not outrageously more difficult ... maybe 1-2 extra story points compare to having the field displayed on its own.

Actual fields

If we pick a field that is already available, that considerably reduces the amount and makes this work mostly trivial. This would apply to:

The RadioButton (Option 1a) doesn't currently have an equivalent field in our UI library. However, bootstrap/reactstrap have matching styles for it, so it wouldn't be too difficult to implement and would provide developers with an additional field they can use.

The ToogleButton (Option 1d) doesn't have any equivalent field in our UI or in bootstrap. There's probably some one out there that has implemented something similar, but probably not to the same standard as bootstrap/reactstrap.

Help icon

The (?) icon next to field or field label doesn't currently have an equivalent in our UI. It's probably not super difficult to implement, but we would have to provide some guidance one when to use the help icon and when to use the hint icon.

The hint icon hasn't been implemented on the dropdown field as-far as I know, but we have some pre-existing logic on the textfield we could work of. It's worth pointing out that this would visually only make sense on the drop down field

Maybe the (?) icon solution is equivalent to the hint icon for fields where the hint icon is not possible.

clarkepaul commented 3 years ago

Maybe we make it configurable (devs can decide to show/hide the field for their users) and/or we move it the field to some "Advanced options" tab.

Like the idea of allowing the dev to decide but if we default to it being off we might as well not do it, it's not really ever going to get turned on. Not a fan of a field on a tab by itself.

IMHO I think we can assume that the approach we'd use is the dropdown if we are to continue with this feature.

As for the help icon and popover, the popover component already has a button to trigger the field. We just need to remove the text and add the CSS classes which add the icon. it wouldn't matter if the icon was outside of the label as long as the aria relation is recognised (probably the hardest part). It could also be added as the first field and then negatively/absolutely positioned so can't imagine this is a big deal.

maxime-rainville commented 3 years ago

Like the idea of allowing the dev to decide but if we default to it being off we might as well not do it, it's not really ever going to get turned on. Not a fan of a field on a tab by itself.

Agree with all of that. I guess the advanced tab would only make sense if we had a bunch of other advanced feature that the average users might not care about.

brynwhyman commented 3 years ago

It sounds like whether there is a material difference in the performance impact of the site or not (claims by Google literally say it's a less that 1% regression for the majority), this could be a good feature for CMS editors to take more control over how the pages they create behave during the first meaningful paint and the resulting load performance. Keen to get @clarkepaul's decision on this.

Let's go ahead and groom the issue to get a better idea of the effort involved in the meantime. We can work through some of the finer points Maxime has raised.

clarkepaul commented 3 years ago

I'm for having the control in the UI, if development isn't going to be a concern time wise. I think users will appreciate the control and ability to enhance the experience for their users based on their own understandings—even if it is only a fraction of a second. For some sites they might want to set a file as eager even its just below the fold if they anticipate people to scroll its contents quickly. We don't have the understandings of individual page perceived load speeds so I think it's best to leave that to individuals in control so they can taylor to suit, most wont adjust the lazy setting which is all good.

emteknetnz commented 3 years ago

@clarkepaul - how does this look?

The new icon and popover is usable on any form field, not just drop downs

Note: there's no line break between the paragraphs within the popover because line-break/paragraphs require rendering potentially unsafe html within the popover. Since we support translations people may do XSS attacks via transifex. We could potentially only support line breaks within the HTML, but then we'd need to figure out what the allowlist is, document it, etc. It's a fair bit of extra work for a very minor feature.. I've opted to just not support HTML. Let me know if this is all good.

image

Lazy-loading globally disabled:

Dimensions fields don't got go 100% like they do currently, a bit of work to get them to go to 100%, though probably fine as is (possibly better?)

image

emteknetnz commented 3 years ago

@clarkepaul Have updated CSS to get reduce padding and ensure the 3 inputs are 1/3 width each

This is how it looks at its 'squishiest'

image

Are you happy to give design sign off?

clarkepaul commented 3 years ago

@emteknetnz yes you've done a great job here.

michalkleiner commented 3 years ago

Are the field labels also different font sizes, should they be the same? It seems Alignment and Caption are marginally bigger than Width, Height, File loading.

clarkepaul commented 3 years ago

@michalkleiner Yes they should be the same size as other fields, good spotting. cc @emteknetnz

brynwhyman commented 3 years ago

@clarkepaul can I make a suggestion on the tooltip wording? As I understand it, the line break that was provided in the designs is non-trivial to implement. Without it, it's a pretty big tooltip IMO.

What about:

'Lazy loading ~(default setting)~ is used to ~can~ speed up ~the time it takes to view the page~ page performance by slightly delaying media loading. Eager loading disables this feature and should be used ~may increase page load times as file are loaded as soon as possible. Use this option~ if the ~files~ image ~are~ is in view as the page loads (above the fold).'

(Without the edits): Lazy loading is used to speed up page performance by slightly delaying media loading. Eager loading disables this feature and should be used if the image is in view as the page loads (above the fold).

Considerations:

michalkleiner commented 3 years ago

speed up page performance

is pulling my ears quite a bit. How about "increase perceived page performance"?

  • Lazy loading will speed up the page performance (not may)

Not always true, based on the content, so may is safer than will as it doesn't make any promises.

clarkepaul commented 3 years ago

Generally happy with the recommendations @brynwhyman, was something I needed to spend more time on.

Eager loading won't 'increase page times', it will keep things as they are now - so I removed that bit

This was in comparison to Lazy loading, not the current situation. In a few years people wont know what the old way was.

Lazy loading will speed up the page performance (not may)

Not always the case, can't quite remember the scenarios I read about but Lazy can slow it down. Probably if external things/scripts take time to load.

Will take another look tomorrow and give the text some more thought.

clarkepaul commented 3 years ago

How about this...

Lazy loading can increase perceived page performance by slightly delaying media loading. Eager loading will load files as soon as possible and can be used if the image is in view as the page loads (above or near the fold).

emteknetnz commented 3 years ago

@clarkepaul At squishy width

image

Are you happy to sign this one off?

clarkepaul commented 3 years ago

Now that the labels are the right size the '?' icon looks a little small, can you bump it up a px or 2. @brynwhyman what did you think about the popup text I provided, Its a little more wordy again but thought its okay?

brynwhyman commented 3 years ago

what did you think about the popup text I provided, Its a little more wordy again but thought its okay?

Yeah, looks good.

emteknetnz commented 3 years ago

@clarkepaul I've bumped the icon size up 2px.

rsz_60f8a3ec97cad

All good to go from a design point of view?

maxime-rainville commented 3 years ago

All PRs have been merged!!