Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMS field for managing image loading state #1214

Closed
13 tasks done
brynwhyman opened this issue Jun 22, 2021 · 26 comments
Closed
13 tasks done

CMS field for managing image loading state #1214

brynwhyman opened this issue Jun 22, 2021 · 26 comments

Comments

@brynwhyman
Copy link

brynwhyman commented Jun 22, 2021

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

  • There is a CMS field in the image edit form that the user can use to turn lazy-loading on or off for the specified image
  • If the developer has globally opted out of the feature, the CMS field is not shown
  • There is enough information presented to the user in the context of the field that they understand what the feature does
  • If the attribute is broken for the image (e.g. the user manually fudged the values through the WYSIWYG HTML edit field) default to the 'false' state
  • 4.9.0 changelog has been updated to reflect language used on the component
  • Behat test js added to support the field
  • Option 3 (Dropdown) is implemented
  • The field is at the bottom of the form (validate with Paul)
  • The "tooltip on label" is implemented globally for all fields and replaces the bulb icon (validate with Paul)

Notes

  • This should require behat tests

PRs

@clarkepaul
Copy link
Contributor

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
Copy link
Author

brynwhyman commented Jun 24, 2021

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
Copy link
Contributor

jonom commented Jun 24, 2021

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
Copy link
Contributor

clarkepaul commented Jun 25, 2021

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:

  • Most liked approaches were 2 & 3, fairly equally.
  • The dropdown looked the part, seamless with UI and provided 'default' text.
  • The button toggle was liked because it clearly provided both options in view, but potentially stood out a bit too much (have since changed blue to grey)
  • Everyone preferred using the proper naming conventions for the controls (lazy, eager) and didn't think it was too complex of a concept, or weird to expect content authors to understand web terms.
  • Good fit to have the control being in the vicinity of file size and position (appearance stuff).

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
Copy link
Author

@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
Copy link
Member

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
Copy link
Contributor

clarkepaul commented Jun 28, 2021

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
Copy link
Contributor

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:

  • Dropdown field
  • Checkbox field
  • radio button list field

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
Copy link
Contributor

clarkepaul commented Jun 28, 2021

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

clarkepaul commented Jul 6, 2021

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
Copy link
Member

emteknetnz commented Jul 16, 2021

@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 emteknetnz assigned clarkepaul and unassigned clarkepaul Jul 16, 2021
@emteknetnz
Copy link
Member

emteknetnz commented Jul 19, 2021

@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?

@emteknetnz emteknetnz assigned clarkepaul and unassigned emteknetnz Jul 19, 2021
@clarkepaul
Copy link
Contributor

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

  • It was already my expectation that the 'Lazy' title would get cropped at smaller viewport sizes, this is only in extreme cases so happy with how thats turned out.
  • I'm okay with the line break of the pop-over text being removed, unfortunate limitation.
  • Love the fact the icon can popover is reusable on other fields :)
  • Regarding the space left when Lazy functionality is removed is both a good thing and bad UX wise (fields are ideal size but space makes a visual break which can affect cognitive flow of eyes). I think I'd prefer to have them fall back to 100% of the width, mainly as that is what people will be accustomed to and fields would follow same visual format—not a biggy either way but if you can get it to consume the space if Lazy is removed that is a bonus.

@michalkleiner
Copy link
Contributor

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
Copy link
Contributor

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

@brynwhyman
Copy link
Author

brynwhyman commented Jul 19, 2021

@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:

  • Lazy loading will speed up the page performance (not may)
  • I thought it was worth adding 'slight delay' as we should try to not put people off this feature by just saying 'delaying'
  • Eager loading won't 'increase page times', it will keep things as they are now - so I removed that bit
  • Removed the hyphens from 'lazy-loading' and 'eager-loading' to be more inline with the dropdown options.

@michalkleiner
Copy link
Contributor

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
Copy link
Contributor

clarkepaul commented Jul 19, 2021

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
Copy link
Contributor

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
Copy link
Member

@clarkepaul
At squishy width

image

Are you happy to sign this one off?

@clarkepaul
Copy link
Contributor

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 brynwhyman assigned emteknetnz and unassigned clarkepaul Jul 20, 2021
@brynwhyman
Copy link
Author

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

Yeah, looks good.

@emteknetnz
Copy link
Member

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

rsz_60f8a3ec97cad

All good to go from a design point of view?

@maxime-rainville
Copy link
Contributor

All PRs have been merged!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants