FEATURE: Show a blurry preview when lazy loading images

FEATURE: Show a blurry preview when lazy loading images

This generates a 10x10 PNG thumbnail for each lightboxed image. If Image Lazy Loading is enabled (IntersectionObserver API) then we’ll load the low res version when offscreen. As the image scrolls in we’ll swap it for the high res version.

We use a WeakMap to track the old image attributes. It’s much less memory than storing them as data-* attributes and swapping them back and forth all the time.

diff --git a/app/assets/javascripts/discourse/lib/lazy-load-images.js.es6 b/app/assets/javascripts/discourse/lib/lazy-load-images.js.es6
index 4f2b772..8835b0c 100644
--- a/app/assets/javascripts/discourse/lib/lazy-load-images.js.es6
+++ b/app/assets/javascripts/discourse/lib/lazy-load-images.js.es6
@@ -2,24 +2,39 @@ const OBSERVER_OPTIONS = {
   rootMargin: "50%" // load images slightly before they're visible
 };
 
+const imageSources = new WeakMap();
+
+const LOADING_DATA =
+  "data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw==";
+
 // We hide an image by replacing it with a transparent gif
 function hide(image) {
   image.classList.add("d-lazyload");
   image.classList.add("d-lazyload-hidden");
-  image.setAttribute("data-src", image.getAttribute("src"));
+
+  imageSources.set(image, {
+    src: image.getAttribute("src"),
+    srcSet: image.getAttribute("srcset")
+  });
+  image.removeAttribute("srcset");
+
   image.setAttribute(
     "src",
-    "data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw=="
+    image.getAttribute("data-small-upload") || LOADING_DATA
   );
+  image.removeAttribute("data-small-upload");
 }
 
-// Restore an image from the `data-src` attribute
+// Restore an image when onscreen
 function show(image) {
-  let dataSrc = image.getAttribute("data-src");
-  if (dataSrc) {
-    image.setAttribute("src", dataSrc);
-    image.classList.remove("d-lazyload-hidden");
+  let sources = imageSources.get(image);
+  if (sources) {
+    image.setAttribute("src", sources.src);
+    if (sources.srcSet) {
+      image.setAttribute("srcset", sources.srcSet);
+    }
   }
+  image.classList.remove("d-lazyload-hidden");
 }
 
 export function setupLazyLoading(api) {
diff --git a/app/assets/stylesheets/common/base/lightbox.scss b/app/assets/stylesheets/common/base/lightbox.scss
index 7fc53af..9ddc6e9 100644
--- a/app/assets/stylesheets/common/base/lightbox.scss
+++ b/app/assets/stylesheets/common/base/lightbox.scss
@@ -1,6 +1,7 @@
 .lightbox-wrapper .lightbox {
   position: relative;
   display: inline-block;
+  overflow: hidden;
   background: $primary-low;
   &:hover .meta {
     opacity: 0.9;
@@ -9,7 +10,6 @@
 }
 
 .d-lazyload-hidden {
-  opacity: 0;
   box-sizing: border-box;
 }
 
diff --git a/app/models/optimized_image.rb b/app/models/optimized_image.rb
index c9cdbd9..a80103c 100644
--- a/app/models/optimized_image.rb
+++ b/app/models/optimized_image.rb
@@ -68,7 +68,7 @@ class OptimizedImage < ActiveRecord::Base
         Rails.logger.error("Could not find file in the store located at url: #{upload.url}")
       else
         # create a temp file with the same extension as the original
-        extension = ".#{upload.extension}"
+        extension = ".#{opts[:format] || upload.extension}"
 
         if extension.length == 1
           return nil
@@ -96,6 +96,7 @@ class OptimizedImage < ActiveRecord::Base
             url: "",
             filesize: File.size(temp_path)
           )
+
           # store the optimized image and update its url
           File.open(temp_path) do |file|
             url = Discourse.store.store_optimized_image(file, thumbnail)
@@ -173,7 +174,7 @@ class OptimizedImage < ActiveRecord::Base
   IM_DECODERS ||= /\A(jpe?g|png|tiff?|bmp|ico|gif)\z/i
 
   def self.prepend_decoder!(path, ext_path = nil, opts = nil)
-    extension = File.extname((opts && opts[:filename]) || ext_path || path)[1..-1]
+    extension = File.extname((opts && opts[:filename]) || path || ext_path)[1..-1]
     raise Discourse::InvalidAccess if !extension || !extension.match?(IM_DECODERS)
     "#{extension}:#{path}"
   end
@@ -189,10 +190,14 @@ class OptimizedImage < ActiveRecord::Base
     from = prepend_decoder!(from, to, opts)
     to = prepend_decoder!(to, to, opts)
 
+    instructions = ['convert', "#{from}[0]"]
+
+    if opts[:colors]
+      instructions << "-colors" << opts[:colors].to_s
+    end
+
     # NOTE: ORDER is important!
-    %W{
-      convert
-      #{from}[0]
+    instructions.concat(%W{
       -auto-orient
       -gravity center
       -background transparent
@@ -204,7 +209,7 @@ class OptimizedImage < ActiveRecord::Base
       -quality 98
       -profile #{File.join(Rails.root, 'vendor', 'data', 'RT_sRGB.icm')}
       #{to}
-    }
+    })
   end
 
   def self.resize_instructions_animated(from, to, dimensions, opts = {})
@@ -212,7 +217,7 @@ class OptimizedImage < ActiveRecord::Base
 
     %W{
       gifsicle
-      --colors=256
+      --colors=#{opts[:colors] || 256}
       --resize-fit #{dimensions}
       --optimize=3
       --output #{to}
@@ -248,7 +253,7 @@ class OptimizedImage < ActiveRecord::Base
     %W{
       gifsicle
       --crop 0,0+#{dimensions}
-      --colors=256
+      --colors=#{opts[:colors] || 256}
       --optimize=3
       --output #{to}
       #{from}
diff --git a/app/models/upload.rb b/app/models/upload.rb
index 3f1f279..a14b71b 100644
--- a/app/models/upload.rb
+++ b/app/models/upload.rb
@@ -65,7 +65,8 @@ class Upload < ActiveRecord::Base
     opts = opts.merge(raise_on_error: true)
     begin
       OptimizedImage.create_for(self, width, height, opts)
-    rescue
+    rescue => ex
+      Rails.logger.info ex if Rails.env.development?
       opts = opts.merge(raise_on_error: false)
       if fix_image_extension
         OptimizedImage.create_for(self, width, height, opts)
diff --git a/lib/cooked_post_processor.rb b/lib/cooked_post_processor.rb
index 7458622..eefac2f 100644
--- a/lib/cooked_post_processor.rb
+++ b/lib/cooked_post_processor.rb
@@ -10,6 +10,8 @@ class CookedPostProcessor
 
   INLINE_ONEBOX_LOADING_CSS_CLASS = "inline-onebox-loading"
   INLINE_ONEBOX_CSS_CLASS = "inline-onebox"
+  LOADING_SIZE = 10
+  LOADING_COLORS = 32
 
   attr_reader :cooking_options, :doc
 
@@ -27,6 +29,8 @@ class CookedPostProcessor
     @doc = Nokogiri::HTML::fragment(post.cook(post.raw, @cooking_options))
     @has_oneboxes = post.post_analyzer.found_oneboxes?
     @size_cache = {}
+
+    @disable_loading_image = !!opts[:disable_loading_image]
   end
 
   def post_process(bypass_bump = false)
@@ -332,11 +336,19 @@ class CookedPostProcessor
           upload.create_thumbnail!(resized_w, resized_h, crop: crop)
         end
       end
+
+      unless @disable_loading_image
+        upload.create_thumbnail!(LOADING_SIZE, LOADING_SIZE, format: 'png', colors: LOADING_COLORS)
+      end
     end
 
     add_lightbox!(img, original_width, original_height, upload, cropped: crop)
   end
 
+  def loading_image(upload)
+    upload.thumbnail(LOADING_SIZE, LOADING_SIZE)
+  end
+
   def is_a_hyperlink?(img)
     parent = img.parent
     while parent
@@ -398,6 +410,10 @@ class CookedPostProcessor
       else
         img["src"] = upload.url
       end
+
+      if small_upload = loading_image(upload)
+        img["data-small-upload"] = small_upload.url
+      end
     end
 
     # then, some overlay informations
diff --git a/spec/components/cooked_post_processor_spec.rb b/spec/components/cooked_post_processor_spec.rb
index 0c5d05c..21d360d 100644
--- a/spec/components/cooked_post_processor_spec.rb
+++ b/spec/components/cooked_post_processor_spec.rb
@@ -15,7 +15,7 @@ describe CookedPostProcessor do
       RAW
     end
 
-    let(:cpp) { CookedPostProcessor.new(post) }
+    let(:cpp) { CookedPostProcessor.new(post, disable_loading_image: true) }
     let(:post_process) { sequence("post_process") }
 
     it "post process in sequence" do
@@ -288,10 +288,22 @@ describe CookedPostProcessor do
           filesize: 800
         )
 
+        # Fake a loading image
+        OptimizedImage.create!(
+          url: 'http://a.b.c/10x10.png',
+          width: CookedPostProcessor::LOADING_SIZE,
+          height: CookedPostProcessor::LOADING_SIZE,
+          upload_id: upload.id,
+          sha1: SecureRandom.hex,
+          extension: '.png',
+          filesize: 123
+        )
+
         cpp = CookedPostProcessor.new(post)
 
         cpp.add_to_size_cache(upload.url, 2000, 1500)
         cpp.post_process_images

[... diff too long, it was truncated ...]

GitHub sha: 662cfc41

1 Like

I personally think we should avoid be_present because it isn’t specific enough. If there is a bug and the loading_image returns something other than an instance of OptimizedImage, the test will still pass.

I feel like the test case is insufficient. It doesn’t seem like we’re testing that the cooked post processor will automatically generate an optimized image of the right size and color. Based on the assertion that has been added, we’re only testing the logic of Upload#loading_image.

I have mixed feelings about this.

I understand that yes, if I returned a foreign object the test would pass. However, in practice, how often does that particular thing happen? This is anecdotal, but I can’t think of a single time I’ve seen that mistake.

One huge benefit of a duck typed language is that we don’t need to declare the type of a thing everywhere. I think we should resist the temptation to use tests to enforce types and instead use them to catch the kinds of bugs that are most likely to happen.

In this case, it’s significantly more likely that a programmer would prevent the optimized image from being returned than that they would return a different object type altogether.

2 Likes

I agree, but I looked for examples of tests that generated OptimizedImages, and these were the best examples I found.

Testing images has traditionally been very difficult and you’ll see another commit of mine that has a comment to this effect. We have either used too many mocks so nothing is being tested, or we are testing binary contents that vary based on minor changes to ImageMagick.

The true solution here would be a serious refactor of the mocks to something more integrated, but that is a serious piece of work and outside of the scope of this feature.

It’s tempting to see a lack of tests in a place like this and go on a many hour long adventure improving it, but for now I opted to mimic the tests we already had rather than have the scope of this feature expand.

5 Likes

I’ve seen it plenty of times in the past and most recently had to fix one.

We’re not really enforcing types here and the assertion isn’t about types IMO. Based on the code that we’ve added, the assertion is to expect a specific instance of OptimizedImage with the right attributes to be returned. By using be_present, we’re also decreasing the readability of the test case because what exactly are we expecting isn’t clear.

I personally do not think our tests should leave room for any "what if"s. Right now, there is a possibility that the test might incorrectly pass should the return value of the method be incorrect. The trade off here to eliminate that from happening is low as well.

diff --git a/spec/components/cooked_post_processor_spec.rb b/spec/components/cooked_post_processor_spec.rb
index 21d360d0b6..e282006b77 100644
--- a/spec/components/cooked_post_processor_spec.rb
+++ b/spec/components/cooked_post_processor_spec.rb
@@ -289,7 +289,7 @@ describe CookedPostProcessor do
         )
 
         # Fake a loading image
-        OptimizedImage.create!(
+        optimized_image = OptimizedImage.create!(
           url: 'http://a.b.c/10x10.png',
           width: CookedPostProcessor::LOADING_SIZE,
           height: CookedPostProcessor::LOADING_SIZE,
@@ -303,7 +303,7 @@ describe CookedPostProcessor do
 
         cpp.add_to_size_cache(upload.url, 2000, 1500)
         cpp.post_process_images
-        expect(cpp.loading_image(upload)).to be_present
+        expect(cpp.loading_image(upload)).to eq(optimized_image)
 
         # 1.5x is skipped cause we have a missing thumb
         expect(cpp.html).to include('srcset="http://a.b.c/666x500.jpg, http://a.b.c/1998x1500.jpg 3x"')

Got it :slight_smile: Now that you explained it the tradeoff here makes sense.

2 Likes

This is a philosophical thing and perhaps a bigger debate than the feedback in this PR.

I think the core question is here is how much coverage do you need? Some people argue that you should have 100% coverage, where every single branch of your code is followed. In extreme cases like that, you end up with a test suite that matches your codebase 1:1 (or even higher!)

Over time, this leads to a much slower test suite, where the majority of tests never fail during refactoring or development. What is the point of a test that never, ever fails?

Personally I think we’re better off doing something like the following:

  1. Accept that some tests are better than no tests
  2. Try to guess the thing that is most likely to break in the future, test that first
  3. If something regresses, add a test for the broken functionality. Our test suite is now better!
2 Likes