FIX: Perform crop using user-specified image sizes (#9224)

FIX: Perform crop using user-specified image sizes (#9224)

  • FIX: Perform crop using user-specified image sizes

It used to resize the images to max width and height first and then perform the crop operation. This is wrong because it ignored the user specified image sizes from the Markdown.

  • DEV: Use real images in test
diff --git a/app/assets/stylesheets/common/d-editor.scss b/app/assets/stylesheets/common/d-editor.scss
index c4a7979..401a4cc 100644
--- a/app/assets/stylesheets/common/d-editor.scss
+++ b/app/assets/stylesheets/common/d-editor.scss
@@ -187,6 +187,10 @@
   &.site-icon {
     padding-bottom: 0;
   }
+  &.resizable {
+    object-fit: cover;
+    object-position: top;
+  }
 }
 
 .d-editor-preview .image-wrapper {
diff --git a/lib/cooked_post_processor.rb b/lib/cooked_post_processor.rb
index bd56c12..2ec33c0 100644
--- a/lib/cooked_post_processor.rb
+++ b/lib/cooked_post_processor.rb
@@ -308,29 +308,34 @@ class CookedPostProcessor
   end
 
   def convert_to_link!(img)
+    w, h = img["width"].to_i, img["height"].to_i
+    user_width, user_height = (w > 0 && h > 0 && [w, h]) ||
+                              get_size_from_attributes(img) ||
+                              get_size_from_image_sizes(img["src"], @opts[:image_sizes])
+
+    limit_size!(img)
+
     src = img["src"]
     return if src.blank? || is_a_hyperlink?(img) || is_svg?(img)
 
-    width, height = img["width"].to_i, img["height"].to_i
-    # TODO: store original dimentions in db
     original_width, original_height = (get_size(src) || [0, 0]).map(&:to_i)
-
-    # can't reach the image...
     if original_width == 0 || original_height == 0
       Rails.logger.info "Can't reach '#{src}' to get its dimension."
       return
     end
 
-    return if original_width <= width && original_height <= height
     return if original_width <= SiteSetting.max_image_width && original_height <= SiteSetting.max_image_height
 
-    crop   = SiteSetting.min_ratio_to_crop > 0
-    crop &&= original_width.to_f / original_height.to_f < SiteSetting.min_ratio_to_crop
+    user_width, user_height = [original_width, original_height] if user_width.to_i <= 0 && user_height.to_i <= 0
+    width, height = user_width, user_height
+
+    crop = SiteSetting.min_ratio_to_crop > 0 && width.to_f / height.to_f < SiteSetting.min_ratio_to_crop
 
     if crop
-      width, height = ImageSizer.crop(original_width, original_height)
-      img["width"] = width
-      img["height"] = height
+      width, height = ImageSizer.crop(width, height)
+      img["width"], img["height"] = width, height
+    else
+      width, height = ImageSizer.resize(width, height)
     end
 
     # if the upload already exists and is attached to a different post,
@@ -700,10 +705,7 @@ class CookedPostProcessor
 
   def post_process_images
     extract_images.each do |img|
-      unless add_image_placeholder!(img)
-        limit_size!(img)
-        convert_to_link!(img)
-      end
+      convert_to_link!(img) unless add_image_placeholder!(img)
     end
   end
 
diff --git a/spec/components/cooked_post_processor_spec.rb b/spec/components/cooked_post_processor_spec.rb
index 799e3a3..d986651 100644
--- a/spec/components/cooked_post_processor_spec.rb
+++ b/spec/components/cooked_post_processor_spec.rb
@@ -416,12 +416,17 @@ describe CookedPostProcessor do
       end
 
       context "with unsized images" do
+        fab!(:upload) { Fabricate(:image_upload, width: 123, height: 456) }
+
+        fab!(:post) do
+          Fabricate(:post, raw: <<~HTML)
+          <img src="#{upload.url}">
+          HTML
+        end
 
-        fab!(:post) { Fabricate(:post_with_unsized_images) }
         let(:cpp) { CookedPostProcessor.new(post) }
 
         it "adds the width and height to images that don't have them" do
-          FastImage.expects(:size).returns([123, 456])
           cpp.post_process
           expect(cpp.html).to match(/width="123" height="456"/)
           expect(cpp).to be_dirty
@@ -430,6 +435,8 @@ describe CookedPostProcessor do
       end
 
       context "with large images" do
+        fab!(:upload) { Fabricate(:image_upload, width: 1750, height: 2000) }
+
         fab!(:post) do
           Fabricate(:post, raw: <<~HTML)
           <img src="#{upload.url}">
@@ -441,13 +448,9 @@ describe CookedPostProcessor do
         before do
           SiteSetting.max_image_height = 2000
           SiteSetting.create_thumbnails = true
-          FastImage.expects(:size).returns([1750, 2000])
         end
 
         it "generates overlay information" do
-          OptimizedImage.expects(:resize).returns(true)
-          FileStore::BaseStore.any_instance.expects(:get_depth_for).returns(0)
-
           cpp.post_process
 
           expect(cpp.html).to match_html <<~HTML
@@ -468,6 +471,8 @@ describe CookedPostProcessor do
           end
 
           it 'should not add lightbox' do
+            FastImage.expects(:size).returns([1750, 2000])
+
             cpp.post_process
 
             expect(cpp.html).to match_html <<~HTML
@@ -482,6 +487,8 @@ describe CookedPostProcessor do
           end
 
           it 'should not add lightbox' do
+            FastImage.expects(:size).returns([1750, 2000])
+
             cpp.post_process
 
             expect(cpp.html).to match_html <<~HTML
@@ -495,6 +502,8 @@ describe CookedPostProcessor do
             end
 
             it 'should not add lightbox' do
+              FastImage.expects(:size).returns([1750, 2000])
+
               SiteSetting.crawl_images = true
               cpp.post_process
 
@@ -537,8 +546,8 @@ describe CookedPostProcessor do
 
           context "when the upload is attached to the correct post" do
             before do
+              FastImage.expects(:size).returns([1750, 2000])
               OptimizedImage.expects(:resize).returns(true)
-              FileStore::BaseStore.any_instance.expects(:get_depth_for).returns(0)
               Discourse.store.class.any_instance.expects(:has_been_uploaded?).at_least_once.returns(true)
               upload.update(secure: true, access_control_post: post)
             end
@@ -568,6 +577,8 @@ describe CookedPostProcessor do
       end
 
       context "with tall images" do
+        fab!(:upload) { Fabricate(:image_upload, width: 860, height: 2000) }
+
         fab!(:post) do
           Fabricate(:post, raw: <<~HTML)
           <img src="#{upload.url}">
@@ -578,10 +589,6 @@ describe CookedPostProcessor do
 
         before do
           SiteSetting.create_thumbnails = true
-          FastImage.expects(:size).returns([860, 2000])
-          OptimizedImage.expects(:resize).never
-          OptimizedImage.expects(:crop).returns(true)
-          FileStore::BaseStore.any_instance.expects(:get_depth_for).returns(0)
         end
 
         it "crops the image" do
@@ -594,6 +601,8 @@ describe CookedPostProcessor do
       end
 
       context "with iPhone X screenshots" do
+        fab!(:upload) { Fabricate(:image_upload, width: 1125, height: 2436) }
+
         fab!(:post) do
           Fabricate(:post, raw: <<~HTML)
           <img src="#{upload.url}">
@@ -604,10 +613,6 @@ describe CookedPostProcessor do
 
         before do
           SiteSetting.create_thumbnails = true
-          FastImage.expects(:size).returns([1125, 2436])
-          OptimizedImage.expects(:resize).returns(true)
-          OptimizedImage.expects(:crop).never
-          FileStore::BaseStore.any_instance.expects(:get_depth_for).returns(0)
         end
 
         it "crops the image" do
@@ -625,6 +630,8 @@ describe CookedPostProcessor do
       end
 
       context "with large images when using subfolders" do
+        fab!(:upload) { Fabricate(:image_upload, width: 1750, height: 2000) }
+
         fab!(:post) do
           Fabricate(:post, raw: <<~HTML)
           <img src="/subfolder#{upload.url}">
@@ -635,13 +642,10 @@ describe CookedPostProcessor do
 
         before do
           set_subfolder "/subfolder"
+          stub_request(:get, "http://#{Discourse.current_hostname}/subfolder#{upload.url}").to_return(status: 200, body: File.new(Discourse.store.path_for(upload)))
 
           SiteSetting.max_image_height = 2000
           SiteSetting.create_thumbnails = true
-          FastImage.expects(:size).returns([1750, 2000])
-          OptimizedImage.expects(:resize).returns(true)
-

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

GitHub sha: 7952cbb9

This commit appears in #9224 which was approved by davidtaylorhq. It was merged by nbianca.