From 7952cbb9a2598d6389bdb77e23c2ce64665a1d4f Mon Sep 17 00:00:00 2001 From: Bianca Nenciu Date: Thu, 26 Mar 2020 16:40:00 +0200 Subject: [PATCH] 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 --- app/assets/stylesheets/common/d-editor.scss | 4 ++ lib/cooked_post_processor.rb | 30 +++++----- spec/components/cooked_post_processor_spec.rb | 57 ++++++++++--------- spec/fabricators/upload_fabricator.rb | 14 +++++ 4 files changed, 63 insertions(+), 42 deletions(-) diff --git a/app/assets/stylesheets/common/d-editor.scss b/app/assets/stylesheets/common/d-editor.scss index c4a7979588b..401a4cc4f60 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 bd56c12ca04..2ec33c0d2c7 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 799e3a3cd3a..d9866515626 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) + + 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) @@ -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) @@ -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) @@ -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) @@ -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) - - FileStore::BaseStore.any_instance.expects(:get_depth_for).returns(0) end it "generates overlay information" do @@ -670,6 +674,8 @@ describe CookedPostProcessor do end context "with title and alt" do + fab!(:upload) { Fabricate(:image_upload, width: 1750, height: 2000) } + fab!(:post) do Fabricate(:post, raw: <<~HTML) RED @@ -681,9 +687,6 @@ describe CookedPostProcessor do 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 @@ describe CookedPostProcessor do end context "with title only" do + fab!(:upload) { Fabricate(:image_upload, width: 1750, height: 2000) } + fab!(:post) do Fabricate(:post, raw: <<~HTML) @@ -712,9 +717,6 @@ describe CookedPostProcessor do 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 @@ describe CookedPostProcessor do end context "with alt only" do + fab!(:upload) { Fabricate(:image_upload, width: 1750, height: 2000) } + fab!(:post) do Fabricate(:post, raw: <<~HTML) RED @@ -743,9 +747,6 @@ describe CookedPostProcessor do 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 index 01d05d9da55..72bcb6c7232 100644 --- a/spec/fabricators/upload_fabricator.rb +++ b/spec/fabricators/upload_fabricator.rb @@ -21,6 +21,20 @@ Fabricator(:upload) do 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