Onebox images with bad aspect when oEmbed specifies dimentions for rich HTML embeds

[jrivettcsa (Jeff Rivett)]

I’ve tried this with page links from a couple of different Wordpress sites, and the effect is the same:

Both sites definitely have valid OG tags. Image sources are square. We’re not specifying image dimensions in the OG tags.

The example above works fine in the Facebook sharing debugger and iFramely. Click the link below to see it on iFramely.

Generate responsive embed codes - Iframely

I reviewed all the onebox and image settings but I don’t see anything that seems relevant.

I went through the onebox troubleshooting and configuration guides here, but nothing helped.

I figure it has to be something in our Discourse settings, or in Discourse itself.

[jrivettcsa (Jeff Rivett)]

Let’s see what that link looks like on this Discourse instance:

Contacting Developers of Other Apps and Information for Developers – Hookmark

When I pasted that in, the onebox in the preview briefly showed the image with the correct aspect. A second or two later it got squashed.

So whatever is going on isn’t limited to our Discourse instance. I guess we might need to specify the image dimensions in the OG tags, but I didn’t think that was required.

[jrivettcsa (Jeff Rivett)]

So… any progress on this? It’s still happening. If there’s some kind of workaround, let me know. If we’re doing something incorrectly, let me know.

[jrivettcsa (Jeff Rivett)]

Status report: this still happens. When I paste a link into a new topic, initially the associated image looks fine in the preview. But as soon as I hit Enter or spacebar, the image gets squashed. I checked the onebox troubleshooting steps again, and once again reviewed all overridden settings, with no luck. This is bananas.

[merefield (Robert)]

That CSS could be considered suboptimal or might be some kind of compromise. I’m on the hoof but perhaps it’s currently enforcing 16:9. Not great for square logos

I wonder if height could be set to auto? (Although that might present challenges for portrait images)

[jrivettcsa (Jeff Rivett)]

Which CSS are you referring to? I mean, you may be right, but I’m trying to determine whether it’s something I can fix on our Discourse instance, or if it’s something in the Discourse source.

[merefield (Robert)]

Back at desk:

That’s presumably a coded style so you’d need to dig into the code to find out how it’s set.

When 600 / 600 your logo is square …

Here you go:

lib/cooked_processor_mixin.rb

[jrivettcsa (Jeff Rivett)]

Interesting. There’s some code in there that makes assumptions about an image if it’s square. Maybe I’ll try a non-square image.

[jrivettcsa (Jeff Rivett)]

Non-square image used with all else being the same: no aspect issues. I’ll keep experimenting.

[merefield (Robert)]

@jrivettcsa

looks fine in the preview

It would:

Uses the original dimensions and aspect.

This is stripped during the cooking process.

[sam (Sam Saffron)]

The dimensions are coming from oembed:

https://hookproductivity.com/wp-json/oembed/1.0/embed?url=https%3A%2F%2Fhookproductivity.com%2Fhelp%2Fintegration%2Fother-app-developers%2F

Which has:

I think the bug here is that if the type is “rich”, and we are not grabbing the entire presentation from oembed html payload, we should skip adding dimensions, cause we are not interested in this data.

This fixes it:

diff --git a/lib/onebox/engine/standard_embed.rb b/lib/onebox/engine/standard_embed.rb
index e3175d6247..fc8c300d81 100644
--- a/lib/onebox/engine/standard_embed.rb
+++ b/lib/onebox/engine/standard_embed.rb
@@ -159,8 +159,9 @@ module Onebox
         @json_ld ||= Onebox::JsonLd.new(html_doc)
       end
 
-      def set_from_normalizer_data(normalizer)
+      def set_from_normalizer_data(normalizer, skip_dimensions: false)
         normalizer.data.each do |k, _|
+          next if skip_dimensions && k.in?(%i[width height])
           v = normalizer.public_send(k)
           @raw[k] ||= v unless v.nil?
         end
@@ -179,7 +180,8 @@ module Onebox
 
       def set_oembed_data_on_raw
         oembed = get_oembed
-        set_from_normalizer_data(oembed)
+        skip_dimensions = oembed.data[:type] == "rich"
+        set_from_normalizer_data(oembed, skip_dimensions:)
       end
 
       def set_json_ld_data_on_raw

However I am not sure what other side effects this would have, flagging the member-experience team who will have a look at this over the next month.

I am reluctant to simply add my patch cause there are lots of layers here and complexity, someone needs to ensure we are able to add the patch in a very safe and tested way.

---

This is pri-medium cause the impact of this bug is quite wide given wp-json surfaces this.

[jrivettcsa (Jeff Rivett)]

Nice! That oembed data does seem to be the smoking gun. Interestingly, the Wordpress (software) documentation only talks about oembed in terms of Wordpress sites embedding content from other sites, not the reverse. However, the wordpress.com (service) documentation says “Every post, page, attachment, and VideoPress video hosted on WordPress.com supports the oEmbed format through our public oEmbed API.” So it’s probably safe to assume that the Wordpress software does this by default.

I reviewed all the Wordpress settings, and was unable to find anything related to oembed, so it looks like any assumptions made by Wordpress are built into the code.

The Wordpress oembed API also explains why there’s no reference to the 600/338 dimensions anywhere in the page source, which I previously found baffling.

I agree that any possible fix for this in Discourse may have many (possibly unwanted) knock-on effects, and needs to be tested before being released.

Also, I’m not even sure that this is a Discourse problem. The bogus ‘338’ dimension is coming from Wordpress. It seems to me that there should be a way in Wordpress to override any oembed-related defaults like this. I plan to look for a Wordpress plugin that allows more control over oembed.

Thanks!

[jrivettcsa (Jeff Rivett)]

On the other hand, since these links look fine in iFramely and elsewhere, maybe this really is something that should be changed in Discourse.

[davidb]

This one was missed previously but it should now be fixed with this change:

https://github.com/discourse/discourse/commit/fb2a2c02f106f29ee7837bf7c5bd94361ee9c330