Resolve "Lazy load images on the Frontend"

What does this MR do?

It introduces Lazy Image Loading to the Frontend. Currently its about avatars and content images of notes.

See updated Frontend Performance Documentation :

To improve the time to first render we are using lazy loading for images. This works by setting the actual image source on the data-source attribute. After the HTML is rendered and JS is loaded , automatically JS will move the url from data-source to src, if the image is in the current viewport.

• Prepare images in html for lazy loading by renaming the src attribute to data-source
• If you are using the Ruby helper image_tag you can simply add the option lazy: true

If you are adding asynchronously content which contains lazy images then you need to call the function gl.lazyLoader.searchLazyImages() which will search for lazy images and load them if needed.

Are there points in the code the reviewer needs to double check?

Backend : The chaining of the image_tag, String replacement to get src to data-src Frontend : Better setup possible ? UX : Ok with Empty Avatar state ? Maybe have special styling for inline content images

Why was this MR needed?

To make GitLab load faster especially if there are many images + avatars

Screenshots

Does this MR meet the acceptance criteria?

• Changelog entry added, if necessary
• Documentation created/updated
• Tests
  • Added for this feature/bug
  • All builds are passing
• Conform by the merge request performance guides
• Conform by the style guides
• Branch has no merge conflicts with master (if it does - rebase it please)
• Squashed related commits together

What are the relevant issue numbers?

Closes #34361 (closed)

assigned to @timzallmann

added 1 commit

• 4affd192 - Image Replacements for Notes

Compare with previous version

added 1 commit

• a1fe46c6 - Created Lazy Loader

Compare with previous version

unmarked as a Work In Progress

changed the description

added frontend ~18308 labels

@timzallmann. Nice job! Very exciting to get this in. Huge performance benefits.

added 601 commits

• 2c02e5f8...7f381461 - 584 commits from branch master
• d6e9700a - Image Replacements for Notes
• 9f54959b - Created Lazy Loader
• 77997aea - - JS Lazy Loading
• d44f067c - Updated Frontend Documentation
• 9d735b7c - Added Changelog Entry
• f1b577fe - Adds Lazy Loading to Blobs + Commit Views
• def4d2fa - Wiki Pre-Rendering img src replace
• 0322b6ee - Content Observer for checking if we had a reflow
• ff219647 - - Fix for mutable observer if no content-body is available
• 2ab41bc4 - FIxed Tests
• 03ee4e9b - Lazy Loading is no default for image_tag
• efb4b48b - Fixed Version Check to be non lazy
• 1311bc76 - Even more Test Fixes
• 072cb639 - Fix for removed Var in Test line
• 69db73a8 - Fix for Gollum Filter with lazy loaded images
• df3106ea - Gollum Tags Spec also needed to be fixed
• e27634a0 - Found wrong path in matching URL

Compare with previous version

@timzallmann why don't you get a backend review first then pass it to me? @rspeicher are you available to review?

assigned to @rspeicher

mentioned in merge request gitlab-com/www-gitlab-com!6652 (merged)

@timzallmann Couple notes. I agree with Douwe that defining our own image_tag and using prepend to override the ActionView version is the cleaner and more modern-Ruby way of doing what we want to here.

assigned to @timzallmann

I think we should also add a karma test for the lazy_loader.js and make sure it has full coverage before merging this in. Considering how this has quite a big change, I think we should merge it into %9.5 rather than rushing it in before the window closes so that we can make sure that this works flawlessly and that the code is

Im not entirely sure how this is working, but Im not a huge fan of the empty placeholders that pop in images later. We could just add some slight animation but I was also wondering if we've looked into doing something like medium or facebook do:

https://jmperezperez.com/medium-image-progressive-loading-placeholder/

https://jmperezperez.com/webp-placeholder-images/

https://code.facebook.com/posts/991252547593574/the-technology-behind-preview-photos/

@ClemMakesApps Ok i am doing anyhow the last try with the tests, one of the reasons is that i just found , there is already lazy loading for images at one point for avatars in dd, and that contradicted now with the global lazy loading. Lets see what comes out of that test and maybe i get it ready tomorrow so that if there is a chance it can be picked next week , as important it could be.

@tauriedavis Yeah that would be great for content images, but at the moment we don't have any image resizing. So even for a 12x12 pixel big avatar we are loading the 200x200 version. Also the size of content images is never tracked nor are they resized for viewing size (there is an issue for that) and maybe then we could do in the future something like the blurry medium way. The avatars have a background and a size anyhow so this doesn't look strange. Only the content images look like popping up if you scroll fast. We need to find a balance between visual esthetics and performance, this is one of the 3 biggest performance slow downs currently (as a lot of images are loaded before anything happens on the screen) this could mean a performance improvement of 30-40% in some areas especially issues + MR's with a lot of images like design issues :-)

Yeah, I definitely agree with improving performance and finding a balance. No argument there. Is it reasonable to open an issue/s that would move us in the direction of the medium way if it is not possible now? How difficult would be for gitlab to implement something like that?

@tauriedavis Could take some time , i started an issue on the avatar topic : #34364 (moved) (there we also will need to reconsider our avatar sizes as i found like 12, 14, 16, ... and for the automatic resizing its best to have 1:1 the exact size.

I started also an issue on content images , which would be then the follow up (as soon as we have a base thing for image resizing) #34365 (moved) , please put there the medium thing.

Added! Thanks!

Thanks for those issues @timzallmann and @tauriedavis! This will fall under our OKRs of improving perceived performance

I think we need something in place. @timzallmann you could just have any old CSS class in the images or do a generic CSS animation for all placeholders to begin with. Shouldn't be difficult right? If the CSS animation is applied to all img[data-src].. Then when it fills the data-src is removed.

@jschatz If you don't have something in the src attribute , browsers will render an image is missing box. Currently i would be against putting anything bigger then an empty gif there as we don't have gzip currently on html content. We also need to differentiate between different image types (avatars, content images). Avatars don't feel currently like blopping up , the only content changing/moving around happens on the content images.

My idea was that either we get the uploaded sizes and do those medium type images (which will take some time, see above) , or we at least have a special gif for content images (which is easier but needs gzip first). But having CSS animation right now in place would be another hit for rendering/scrolling performance as we have an "unlimited" number of posts that are rendered currently. So I would rather add this later after we have all improvements in place and its really fast then we can measure FPS impacts etc on animations and deicde for or against it.

mentioned in issue #34900 (moved)

resolved all discussions

@ClemMakesApps + @rspeicher it's finally ready for your final review :-)

assigned to @ClemMakesApps

As of yet, I don't think we are super picky on unit tests versus integrated tests but it seems like we only have integrated tests here. It would be more robust with unit tests imo, so I think we can create a technical debt issue for that if we want to leave this as first iteration @timzallmann

assigned to @timzallmann

@ClemMakesApps As the actual JS functionality is very small with just that function (loadImage) that does something except standard event listeners and dom functionality, i was more focusing on the integration tests. If you have ideas how to unit test the scrolling i am happy to do it on the next iteration.

Discussions done and tests are green , so back to you

assigned to @ClemMakesApps

resolved all discussions

@timzallmann can we create an issue to track unit tests for next iteration?

Everything else LGTM

Handing it off for BE final review

assigned to @rspeicher

@timzallmann Couple more notes.

assigned to @timzallmann

resolved all discussions

Thanks @rspeicher , fixed them and just pushed. As its the end of my day i already assign it to you without waiting for the tests to pass

assigned to @rspeicher

I don't think we can use this without it holding the size of the image. It's going to move the page around a lot. People load really big images. And if it takes a while to load you could potentially scroll past the unloaded image and the page would start jumping around. Did you take this into account?

Passing to @jschatz1 while y'all discuss.

assigned to @jschatz1

added 991 commits

• 650ca38d...4766a77b - 958 commits from branch master
• c41e25ae - Image Replacements for Notes
• f4f4d02c - Created Lazy Loader
• 71396f4b - - JS Lazy Loading
• 2d89b1ba - Updated Frontend Documentation
• 6fea6f47 - Added Changelog Entry
• 206bc7ed - Adds Lazy Loading to Blobs + Commit Views
• 535389ba - Wiki Pre-Rendering img src replace
• d3f6449c - Content Observer for checking if we had a reflow
• 37d52f59 - - Fix for mutable observer if no content-body is available
• bd0282ce - FIxed Tests
• c5561423 - Lazy Loading is no default for image_tag
• 827e01b4 - Fixed Version Check to be non lazy
• 67612c57 - Even more Test Fixes
• 135f9921 - Fix for removed Var in Test line
• 73a30f52 - Fix for Gollum Filter with lazy loaded images
• 3a5b4dec - Gollum Tags Spec also needed to be fixed
• e9c5231c - Found wrong path in matching URL
• 998d0f12 - Fixes based on MR comments
• bcafa73a - Transformed the Markup Replacement from String Replace to a Markup Lazy Load Filter
• 3dbd3fb7 - Moved Lazy Image Tag Helper Functionality to a simple override
• ff99dfba - Test Fixes for new image_tag variant
• 0a7bb12f - Fixes for Tests and based on MR
• 024658a0 - Fixed tests
• 227eec1b - Globalising for Banzai Filters
• ef0b5180 - Writing the Karma Tests for the Lazy Loader
• 5ea16342 - Lazy Img Loading Karma Tests
• 5ff3b59d - Bumped the Markdown to HTML Cache Version
• 4cfcbc2e - Forgot the brackets :-/
• 13ef8094 - Fixed the relative rewrite based on the namespace for images to take data-src attribute
• ba7bad12 - img - needs to check the data-src and the src attribute could be that we have…
• eb59407a - Optimised Spec
• 414af5ec - Restructure avatar rendering
• 24fe2fc6 - Updated based on MR discussion

Compare with previous version

@timzallmann on your latest commit don't forget to remove the scss variables $avatar-*.

This is really good code @timzallmann. Excited to have it in. It's a huge change and slightly dangerous. But the code is good.

mentioned in issue #35476 (moved)

@jschatz1 Back to you, after rewriting it to a class based selection and some optimizations here and there, i think its ready to be merged.

resolved all discussions

mentioned in issue #35501 (moved)

approved this merge request

merged

mentioned in commit f39f54c6

mentioned in issue #37200 (closed)

Closed #35157 (closed)

mentioned in merge request !12898

mentioned in issue #35157 (closed)

mentioned in issue gitlab#10967

mentioned in issue gitlab#11242