UX: generic onebox treats all square images as avatars and renders them smaller

app/assets/javascripts/pretty-text/oneboxer.js.es6

@@ -3,6 +3,38 @@ const loadingQueue = [];
 const localCache = {};
 const failedCache = {};
 
+function resolveSize(img) {
+  $(img).addClass('size-resolved');
+
+  if (img.width > 0 && img.width === img.height) {
+    $(img).addClass('onebox-avatar');
+  }
+}
+
+// Detect square images and apply smaller onebox-avatar class
+function applySquareGenericOnebox($elem, normalizedUrl) {
+  if (!$elem.hasClass('whitelistedgeneric')) {
+    return;
+  }
+
+  let $img = $elem.find('.onebox-body img.thumbnail');
+  let img = $img[0];
+
+  // already resolved... skip
+  if ($img.length !== 1 || $img.hasClass('size-resolved')) {
+    return;
+  }
+
+  if (img.complete) {
+    resolveSize(img, $elem, normalizedUrl);
+  } else {
+    $img.on('load.onebox', () => {
+      resolveSize(img, $elem, normalizedUrl);
+      $img.off('load.onebox');
+    });
+  }
+}
+
 function loadNext(ajax) {
   if (loadingQueue.length === 0) {
     timeout = null;
@@ -19,8 +51,11 @@ function loadNext(ajax) {
     data: { url, refresh, user_id: userId },
     cache: true
   }).then(html => {
-    localCache[normalize(url)] = html;
-    $elem.replaceWith(html);
+    let $html = $(html);
+
+    localCache[normalize(url)] = $html;
+    $elem.replaceWith($html);
+    applySquareGenericOnebox($html, normalize(url));
   }, result => {
     if (result && result.jqXHR && result.jqXHR.status === 429) {
       timeoutMs = 2000;
@@ -53,7 +88,7 @@ export function load(e, refresh, ajax, userId, synchronous) {
   if (!refresh) {
     // If we have it in our cache, return it.
     const cached = localCache[normalize(url)];
-    if (cached) return cached;
+    if (cached) return cached.prop('outerHTML');
 
     // If the request failed, don't do anything
     const failed = failedCache[normalize(url)];
@@ -81,5 +116,6 @@ function normalize(url) {
 }
 
 export function lookupCache(url) {
-  return localCache[normalize(url)];
+  const cached = localCache[normalize(url)];
+  return cached && cached.prop('outerHTML');
 }

lib/cooked_post_processor.rb

@@ -7,7 +7,7 @@ require_dependency 'pretty_text'
 class CookedPostProcessor
   include ActionView::Helpers::NumberHelper
 
-  attr_reader :cooking_options
+  attr_reader :cooking_options, :doc
 
   def initialize(post, opts = {})
     @dirty = false
@@ -180,7 +180,6 @@ class CookedPostProcessor
 
     # FastImage fails when there's no scheme
     absolute_url = SiteSetting.scheme + ":" + absolute_url if absolute_url.start_with?("//")
-
     return unless is_valid_image_url?(absolute_url)
 
     # we can *always* crawl our own images
@@ -331,6 +330,24 @@ class CookedPostProcessor
 
       parent_class = img.parent && img.parent["class"]
       if parent_class&.include?("onebox-body") && (width = img["width"].to_i) > 0 && (height = img["height"].to_i) > 0
+
+        # special instruction for width == height, assume we are dealing with an avatar
+        if (img["width"].to_i == img["height"].to_i)
+          found = false
+          parent = img
+          while parent = parent.parent
+            if parent["class"].include? "whitelistedgeneric"
+              found = true
+              break
+            end
+          end
+
+          if found
+            img["class"] = img["class"].to_s + " onebox-avatar"
+            next
+          end
+        end
+
         img.delete('width')
         img.delete('height')
         new_parent = img.add_next_sibling("<div class='aspect-image' style='--aspect-ratio:#{width}/#{height};'/>")

lib/final_destination.rb

@@ -40,9 +40,7 @@ class FinalDestination
     @opts[:max_redirects] ||= 5
     @opts[:lookup_ip] ||= lambda do |host|
       begin
-        IPSocket::getaddress(host)
-      rescue SocketError
-        nil
+        FinalDestination.lookup_ip(host)
       end
     end
     @ignored = [Discourse.base_url_no_prefix] + (@opts[:ignore_redirects] || [])
@@ -272,7 +270,13 @@ class FinalDestination
   end
 
   def self.lookup_ip(host)
+    # TODO clean this up in the test suite, cause it is a mess
+    # if Rails.env == "test"
+    #   STDERR.puts "WARNING FinalDestination.lookup_ip was called with host: #{host}, this is network call that should be mocked"
+    # end
     IPSocket::getaddress(host)
+  rescue SocketError
+    nil
   end
 
 end

spec/components/cooked_post_processor_spec.rb

@@ -431,13 +431,44 @@ describe CookedPostProcessor do
         .returns("<div>GANGNAM STYLE</div>")
       cpp.post_process_oneboxes
     end
-
-    it "is dirty" do
+    it "inserts the onebox without wrapping p" do
       expect(cpp).to be_dirty
+      expect(cpp.html).to match_html "<div>GANGNAM STYLE</div>"
+    end
   end
 
-    it "inserts the onebox without wrapping p" do
-      expect(cpp.html).to match_html "<div>GANGNAM STYLE</div>"
+  context ".post_process_oneboxes with square image" do
+
+    it "generates a onebox-avatar class" do
+      SiteSetting.crawl_images = true
+
+      url = 'https://square-image.com/onebox'
+
+      body = <<~HTML
+      <html>
+      <head>
+      <meta property='og:title' content="Page awesome">
+      <meta property='og:image' content="https://image.com/avatar.png">
+      <meta property='og:description' content="Page awesome desc">
+      </head>
+      </html>
+      HTML
+
+      stub_request(:head, url).to_return(status: 200)
+      stub_request(:get , url).to_return(status: 200, body: body)
+      FinalDestination.stubs(:lookup_ip).returns('1.2.3.4')
+
+      # not an ideal stub but shipping the whole image to fast image can add
+      # a lot of cost to this test
+      FastImage.stubs(:size).returns([200, 200])
+
+      post = Fabricate.build(:post, raw: url)
+      cpp = CookedPostProcessor.new(post, invalidate_oneboxes: true)
+
+      cpp.post_process_oneboxes
+
+      expect(cpp.doc.to_s).not_to include('aspect-image')
+      expect(cpp.doc.to_s).to include('onebox-avatar')
     end
 
   end