Negotiate max width/height of oEmbed assets more intelligently

I needed responsive YouTube embeds and they weren't working out of the box. I was able to get them working by unsetting the width/height attributes on the oembed wrapper iframe (the iframe that contains the youtube iframe) and adding some additional CSS to .media-oembed-content.

If this is the right approach I could work on a patch, but I'm definitely not thinking of any situations where the width/height on the parent oembed wrapper iframe could need set width/height attributes. Are there? Should we add a "responsive" checkbox to the field display config that removes width/height when checked? Am I out of scope for this issue/should I create a new one?

Thanks all.

Closed #3310520: oEmbed videos should use returned height and width values as a duplicate. Asked @imclean there to move his patch to this issue. Already going ahead and crediting them here! And @cilefen too, because thanks to him we discovered it was a duplicate :)

Also bumping to Major because this fits under Interfere with normal site visitors' use of the site (for example, content in the wrong language, or validation errors for regular form submissions), even if there is a workaround. — see https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-...

More triaging:

1. Very closely related: #3223596: Rename oEmbed formatter settings to default_width and default_height. IMHO we should close that in favor of this?
2. Related: #3088168: Media thumbnail dimensions are wrong for YouTube videos
3. Related: #3061134: oEmbed videos are not fully responsive

The only oEmbed type which doesn't include width and height parameters is Link. We should use the returned values where possible. The YouTube problem mentioned in the IS will need to be reviewed as well to see if anything has changed in 4 years.

This will have at least 1 fail. I can't seem to get enough information from running the test locally, it just says "FATAL Drupal\Tests\media\Functional\FieldFormatter\OEmbedFormatterTest: test runner returned a non-zero error code (2)".

TypeError: PHPUnit\Framework\Assert::assertStringContainsString(): Argument #2 ($haystack) must be of type string, null given, called in /var/www/html/core/modules/media/tests/src/Functional/FieldFormatter/OEmbedFormatterTest.php on line 211

This is probably due to this patch always using ->getWidth() etc. on Rich, Photo and Video types. This is valid so I'll have to look into it.

1) Drupal\Tests\media\FunctionalJavascript\MediaSourceOEmbedVideoTest::testMediaOEmbedVideoSource
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'200'
+'480'

This works fine locally (CLI). I can't see where the 480 comes from if it's requesting the video from Vimeo for the test.

I can't run JS tests yet and I'm having trouble getting a fully functional local test environment. I'm still not sure why run-tests.sh doesn't produce more information. The testbot doesn't really say anything either, probably because the assertions don't have a message.

It might be good to add some extra information to the tests, such as which selector it's using and the value it's trying to compare.

Test URL and return value.

https://vimeo.com/api/oembed.json?url=https://vimeo.com/7073899&maxwidth...

{
   "type":"video",
   "version":"1.0",
   "provider_name":"Vimeo",
   "provider_url":"https://vimeo.com/",
   "title":"Drupal Rap Video - Schipulcon09",
   "author_name":"Tendenci - The Open Source AMS",
   "author_url":"https://vimeo.com/schipul",
   "is_plus":"0",
   "account_type":"basic",
   "html":"<iframe src=\"https://player.vimeo.com/video/7073899?h=ee3654bb73&amp;app_id=122963\" width=\"100\" height=\"67\" frameborder=\"0\" allow=\"autoplay; fullscreen; picture-in-picture\" allowfullscreen title=\"Drupal Rap Video - Schipulcon09\"></iframe>",
   "width":100,
   "height":67,
   "duration":91,
   "description":"Special thanks to Tendenci, formerly Schipul for sponsoring this video with training, equipment and time. The open source way. All creative however was self directed by the individuals - A. Hughes (www.schipul.com/ahughes) featuring QCait (www.schipul.com/qcait) - Hands On Drupal\n\nDrupal is a free software package that allows an individual or a community of users to easily publish, manage and organize a wide variety of content on a website.\n\nNeed a little Drupal help or just want to geek out with us?  Visit our www.schipul.com/drupal for more info - we'd love to connect!\n\nGo here for Drupal Common Terms and Suggested Modules : http://schipul.com/en/helpfiles/v/229",
   "thumbnail_url":"https://i.vimeocdn.com/video/29203255-2fa4c623aa778925f094352206b986b2d7cd0a9ad60a185ebf40050eef5ef4ef-d_100x75",
   "thumbnail_width":100,
   "thumbnail_height":67,
   "thumbnail_url_with_play_button":"https://i.vimeocdn.com/filter/overlay?src0=https%3A%2F%2Fi.vimeocdn.com%2Fvideo%2F29203255-2fa4c623aa778925f094352206b986b2d7cd0a9ad60a185ebf40050eef5ef4ef-d_100x75&src1=http%3A%2F%2Ff.vimeocdn.com%2Fp%2Fimages%2Fcrawler_play.png",
   "upload_date":"2009-10-14 19:27:19",
   "video_id":7073899,
   "uri":"/videos/7073899"
}

Twitter returns height and width but they're always null.

YouTube has minimum dimensions of 200 x 200.

I can't remember why I changed it 200, I was probably still working out what was testing what. I also couldn't see where the size of 240 was arrived at.

Updated the IS with what I think is the real problem here.

The formatter settings.

Video without the patch. The video is slightly stretched in this case with the whitespace caused by the iFrame being set to 1000 x 1000.

Video after applying the patch. The iFrame is set to the size the returned by Vimeo.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Tried testing this with 1000X1000 and the image appears zoomed in to me with the patch.

This happens if you set the max_width too high for the region the video is in. I'm not sure what the media module should (or can) do here.

That said, it appears to be a thumbnail issue. I can replicate #35 but when I play the video it fits within the iFrame.

The thumbnail is a fixed width. This was handled in another issue - #3088168: Media thumbnail dimensions are wrong for YouTube videos.

So this seems to be just Vimeo videos and not affecting YouTube ones. Using a Vimeo video I was able to replicate the same as #31.

Moving to NW for an issue summary to specifically call out what is the proposed solution. Might be worth noting this doesn't affect YouTube, even though the original post mentions this issue was primarily with YouTube videos.

I'm not sure what else I can add to this. The patch does what's described in the updated IS. Google has problems, there are a few issues around that. In this case, the thumbnail is the problem.

Plain embed:

https://www.youtube.com/oembed?url=https://www.youtube.com/watch?v=dPiLx...

With maxwidth and maxheight set:

https://www.youtube.com/oembed?url=https://www.youtube.com/watch?v=dPiLx...

The thumbnail size doesn't change, but that also doesn't fully explain the problem here.

Plain Google embed without Drupal has the thumbnail problem you describe in #35. Review the following at various browser widths:

https://www.youtube.com/embed/dPiLxdBOnTs

The Oembed spec states thumbnail dimensions must respect maxwidth and maxheight parameters, but clearly this isn't happening with YouTube.

(As an aside, Vimeo seems to handle responsive thumbnails well.)

What do you think we could do here to get around the problem with YouTube's thumbnails? I'd be happy to look into it, but I don't really have any suggestions, other than a theme or JS hack.

No don’t need the thumbnail addressed here but I see the expected behavior in the IS but not what clear what was done to address it.

But if you think that’s not needed can remove the tag and put back into review also

+++ b/core/modules/media/src/Plugin/Field/FieldFormatter/OEmbedFormatter.php
@@ -224,8 +224,8 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
-            'width' => $max_width ?: $resource->getWidth(),
-            'height' => $max_height ?: $resource->getHeight(),
+            'width' => $resource->getWidth() ?: $max_width,
+            'height' => $resource->getHeight() ?: $max_height,

This is the key change in this issue and it affects all video providers.

$max_width and $max_height are the values set in the field formatter. These are sent to the oembed provider as the parameters maxwidth and maxheight.

$resource->getWidth() and $resource->getHeight() are the values returned by the video provider, which take into account the values sent with the request.

Before the patch the dimensions of the video were set to exactly the values of $max_width and $max_height.

After the patch, the dimensions are set to the returned values $resource->getWidth() and $resource->getHeight().

It'd be nice to get a few more reviews here.

I've tested patch #27 (can't use patch #34 because it does not apply on Drupal v10.0.x, but code in #34 looks pretty similar). The patch has errors when any dimension field is empty. Error log shows access denied, most likley due missing query parameter values.

Field formatter setting width="1920", height="" causes an error:
Path: /media/oembed?url=https%3A//www.youtube.com/watch.....&max_width=1920&max_height&hash=.....

Field formatter setting width="1920", height="1080" works:
Path: /media/oembed?url=https%3A//www.youtube.com/watch.....&max_width=1920&max_height=1080&hash=.....

UPDATE: This problem was not introduced by this patch, it also happens on an unpatched v10.0.4

#41:

UPDATE: This problem was not introduced by this patch, it also happens on an unpatched v10.0.4

Retested following #31 and got the whitespace and with the patch I did not.

Also may somewhat be related to #3350299: Allow oEmbed video iFrames to expand to available space

Could this be related? #3332456: [random test failure] MediaTest::testLinkManualDecorator()

Random ckeditor error.

Know spokje has been working on the random failures.

Appears to be unrelated failure.

The patch and fix look good, but I think this needs a release note - this may change the size of embedded video on existing sites when they upgrade?

Hiding patches for clarity

Updated issue summary with release note (not great at those)

Moved patch into MR.

Tests are all green. Since this was previously RTBC going to restore status.

@nod_ has asked a question that needs answering on the MR.

Tried the suggestion but the caused the tests to fail https://git.drupalcode.org/issue/drupal-2966656/-/jobs/1143627

So reverted back.

Let's add the comment @nod_ is suggesting

Added comment.

Thanks

Committed 5559dba and pushed to 11.x. Thanks!

For anyone reading the comments via email (and not seeing the merge request updates), the change was also merged to the 10.3.x branch.