Let’s see what that link looks like on this Discourse instance:
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.
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.
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)
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.
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.
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.
