FIX: Perform crop using user-specified image sizes (discourse#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
Arxnel · Mar 26, 2020 · 7952cbb · 7952cbb
1 parent ba1a085
commit 7952cbb
Show file tree

Hide file tree

Showing 4 changed files with 63 additions and 42 deletions.
diff --git 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
@@ -308,29 +308,34 @@ def is_valid_image_url?(url)
   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 @@ def html
 
   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
@@ -416,12 +416,17 @@ def s3_setup
       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 @@ def s3_setup
       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 @@ def s3_setup
         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 @@ def s3_setup
           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 @@ def s3_setup
           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 @@ def s3_setup
             end
 
             it 'should not add lightbox' do
+              FastImage.expects(:size).returns([1750, 2000])
+
               SiteSetting.crawl_images = true
               cpp.post_process
 
@@ -537,8 +546,8 @@ def s3_setup
 
           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 @@ def s3_setup
       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 @@ def s3_setup
 
         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 @@ def s3_setup
       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 @@ def s3_setup
 
         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 @@ def s3_setup
       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 @@ def s3_setup
 
         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)
-
-          FileStore::BaseStore.any_instance.expects(:get_depth_for).returns(0)
         end
 
         it "generates overlay information" do
@@ -670,6 +674,8 @@ def s3_setup
       end
 
       context "with title and alt" do
+        fab!(:upload) { Fabricate(:image_upload, width: 1750, height: 2000) }
+
         fab!(:post) do
           Fabricate(:post, raw: <<~HTML)
           <img src="#{upload.url}" title="WAT" alt="RED">
@@ -681,9 +687,6 @@ def s3_setup
         before do
           SiteSetting.max_image_height = 2000
           SiteSetting.create_thumbnails = true
-          FastImage.expects(:size).returns([1750, 2000])
-          OptimizedImage.expects(:resize).returns(true)
-          FileStore::BaseStore.any_instance.expects(:get_depth_for).returns(0)
         end
 
         it "generates overlay information using image title and ignores alt" do
@@ -701,6 +704,8 @@ def s3_setup
       end
 
       context "with title only" do
+        fab!(:upload) { Fabricate(:image_upload, width: 1750, height: 2000) }
+
         fab!(:post) do
           Fabricate(:post, raw: <<~HTML)
           <img src="#{upload.url}" title="WAT">
@@ -712,9 +717,6 @@ def s3_setup
         before do
           SiteSetting.max_image_height = 2000
           SiteSetting.create_thumbnails = true
-          FastImage.expects(:size).returns([1750, 2000])
-          OptimizedImage.expects(:resize).returns(true)
-          FileStore::BaseStore.any_instance.expects(:get_depth_for).returns(0)
         end
 
         it "generates overlay information using image title" do
@@ -732,6 +734,8 @@ def s3_setup
       end
 
       context "with alt only" do
+        fab!(:upload) { Fabricate(:image_upload, width: 1750, height: 2000) }
+
         fab!(:post) do
           Fabricate(:post, raw: <<~HTML)
           <img src="#{upload.url}" alt="RED">
@@ -743,9 +747,6 @@ def s3_setup
         before do
           SiteSetting.max_image_height = 2000
           SiteSetting.create_thumbnails = true
-          FastImage.expects(:size).returns([1750, 2000])
-          OptimizedImage.expects(:resize).returns(true)
-          FileStore::BaseStore.any_instance.expects(:get_depth_for).returns(0)
         end
 
         it "generates overlay information using image alt" do

diff --git a/spec/fabricators/upload_fabricator.rb b/spec/fabricators/upload_fabricator.rb
@@ -21,6 +21,20 @@
   extension "png"
 end
 
+Fabricator(:image_upload, from: :upload) do
+  after_create do |upload|
+    file = Tempfile.new(['fabricated', '.png'])
+    `convert -size #{upload.width}x#{upload.height} xc:white "#{file.path}"`
+
+    upload.url = Discourse.store.store_upload(file, upload)
+    upload.sha1 = Upload.generate_digest(file.path)
+
+    WebMock
+      .stub_request(:get, "http://#{Discourse.current_hostname}#{upload.url}")
+      .to_return(status: 200, body: File.new(file.path))
+  end
+end
+
 Fabricator(:video_upload, from: :upload) do
   original_filename "video.mp4"
   width nil