From: Andy Allan <git@gravitystorm.co.uk>
Date: Wed, 29 Sep 2021 17:47:20 +0000 (+0100)
Subject: Specify avatar dimensions in html tags
X-Git-Tag: live~1973^2
X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/9f61d6c1cf2d796ba8089f299679c2496ecfbbca?ds=sidebyside;hp=--cc

Specify avatar dimensions in html tags

This prevents reflow when the images are loaded by the browser.

ActiveStorage variants are resized lazily when the image is requested,
so we only know the dimensions if the image was already loaded. This
means that there will be one reflow just after a new avatar is first
viewed.
---

9f61d6c1cf2d796ba8089f299679c2496ecfbbca
diff --git a/app/helpers/user_helper.rb b/app/helpers/user_helper.rb
index 69b0f6d6b..eed55cd9d 100644
--- a/app/helpers/user_helper.rb
+++ b/app/helpers/user_helper.rb
@@ -8,9 +8,9 @@ module UserHelper
     if user.image_use_gravatar
       user_gravatar_tag(user, options)
     elsif user.avatar.attached?
-      image_tag user_avatar_variant(user, :resize_to_limit => [100, 100]), options
+      user_avatar_variant_tag(user, { :resize_to_limit => [100, 100] }, options)
     else
-      image_tag "avatar_large.png", options
+      image_tag "avatar_large.png", options.merge(:width => 100, :height => 100)
     end
   end
 
@@ -19,11 +19,11 @@ module UserHelper
     options[:alt] ||= ""
 
     if user.image_use_gravatar
-      user_gravatar_tag(user, options)
+      user_gravatar_tag(user, options.merge(:size => 50))
     elsif user.avatar.attached?
-      image_tag user_avatar_variant(user, :resize_to_limit => [50, 50]), options
+      user_avatar_variant_tag(user, { :resize_to_limit => [50, 50] }, options)
     else
-      image_tag "avatar_small.png", options
+      image_tag "avatar_small.png", options.merge(:width => 50, :height => 50)
     end
   end
 
@@ -32,11 +32,11 @@ module UserHelper
     options[:alt] ||= ""
 
     if user.image_use_gravatar
-      user_gravatar_tag(user, options)
+      user_gravatar_tag(user, options.merge(:size => 50))
     elsif user.avatar.attached?
-      image_tag user_avatar_variant(user, :resize_to_limit => [50, 50]), options
+      user_avatar_variant_tag(user, { :resize_to_limit => [50, 50] }, options)
     else
-      image_tag "avatar_small.png", options
+      image_tag "avatar_small.png", options.merge(:width => 50, :height => 50)
     end
   end
 
@@ -69,6 +69,22 @@ module UserHelper
   private
 
   # Local avatar support
+  def user_avatar_variant_tag(user, variant_options, options)
+    if user.avatar.variable?
+      variant = user.avatar.variant(variant_options)
+      # https://stackoverflow.com/questions/61893089/get-metadata-of-active-storage-variant/67228171
+      if variant.processed?
+        metadata = variant.processed.send(:record).image.blob.metadata
+        if metadata["width"]
+          options[:width] = metadata["width"]
+          options[:height] = metadata["height"]
+        end
+      end
+      image_tag variant, options
+    else
+      image_tag user.avatar, options
+    end
+  end
 
   def user_avatar_variant(user, options)
     if user.avatar.variable?
@@ -90,7 +106,7 @@ module UserHelper
 
   def user_gravatar_tag(user, options = {})
     url = user_gravatar_url(user, options)
-    options.delete(:size)
+    options[:height] = options[:width] = options.delete(:size) || 100
     image_tag url, options
   end
 end
diff --git a/test/helpers/user_helper_test.rb b/test/helpers/user_helper_test.rb
index 758baa1cb..8cef4d2dc 100644
--- a/test/helpers/user_helper_test.rb
+++ b/test/helpers/user_helper_test.rb
@@ -12,7 +12,6 @@ class UserHelperTest < ActionView::TestCase
 
     image = user_image(user, :class => "foo")
     assert_match %r{^<img class="foo" .* src="/images/avatar_large.png" />$}, image
-
     image = user_image(gravatar_user)
     assert_match %r{^<img class="user_image" .* src="http://www.gravatar.com/avatar/.*" />$}, image
 
@@ -66,6 +65,50 @@ class UserHelperTest < ActionView::TestCase
     assert_match %r{^http://www.gravatar.com/avatar/}, url
   end
 
+  def test_user_image_sizes_default_image
+    user = create(:user)
+
+    image = user_image(user)
+    assert_match %r{^<img .* width="100" height="100" .* />$}, image
+
+    thumbnail = user_thumbnail(user)
+    assert_match %r{^<img .* width="50" height="50" .* />$}, thumbnail
+  end
+
+  def test_user_image_sizes_avatar
+    user = create(:user)
+    user.avatar.attach(:io => File.open("test/gpx/fixtures/a.gif"), :filename => "a.gif")
+
+    # first time access, no width or height is found
+    image = user_image(user)
+    assert_no_match %r{^<img .* width="100" height="100" .* />$}, image
+
+    thumbnail = user_thumbnail(user)
+    assert_no_match %r{^<img .* width="50" height="50" .* />$}, thumbnail
+
+    # Small hacks to simulate what happens when the images have been fetched at least once before
+    variant = user.avatar.variant(:resize_to_limit => [100, 100])
+    variant.processed.send(:record).image.blob.analyze
+    variant = user.avatar.variant(:resize_to_limit => [50, 50])
+    variant.processed.send(:record).image.blob.analyze
+
+    image = user_image(user)
+    assert_match %r{^<img .* width="100" height="100" .* />$}, image
+
+    thumbnail = user_thumbnail(user)
+    assert_match %r{^<img .* width="50" height="50" .* />$}, thumbnail
+  end
+
+  def test_user_image_sizes_gravatar
+    user = create(:user, :image_use_gravatar => true)
+
+    image = user_image(user)
+    assert_match %r{^<img .* width="100" height="100" .* />$}, image
+
+    thumbnail = user_thumbnail(user)
+    assert_match %r{^<img .* width="50" height="50" .* />$}, thumbnail
+  end
+
   def test_openid_logo
     logo = openid_logo
     assert_match %r{^<img .* class="openid_logo" src="/images/openid_small.png" />$}, logo