Responsive Video Remove Wrap Targeting Incorrect blockName

rpasillas commented 4 years ago

Steps to reproduce the issue

Go to the editor, insert a YouTube embed.
Click on the alignment in the block tool for the embed and set to Full-Width and save post.
View the post on the front-end, YouTube video is not sized correctly.

What I expected

The YouTube embed should be full-width (assuming the theme support is there.)

What happened instead

The YouTube embed gets resized to a smaller size via the responsive-videos.min.js loaded by the responsive video module.

Problem seems to lie here: https://github.com/Automattic/jetpack/blob/master/modules/theme-tools/responsive-videos.php#L156 core-embed should be core/embed

jeherve commented 4 years ago

Related: #11171

scottsweb commented 3 years ago

I tested these steps in the 2020 theme and I am seeing full width videos. Can you confirm what theme you were testing with. Many thanks.

jeherve commented 3 years ago

I tested these steps in the 2020 theme and I am seeing full width videos

I assume you'd need to test with a theme that doesn't declare support for Core's responsive embeds, and instead declare support for Jetpack responsive videos. Altofocus, for example, should be a good example.

scottsweb commented 3 years ago

Altofocus, for example, should be a good example.

The problem with Altofocus is that it does not support full width content in Gutenberg. I will see if I can find another.

scottsweb commented 3 years ago

I have been testing on the zakra theme that declares support for add_theme_support( 'jetpack-responsive-videos' ); but not add_theme_support( 'responsive-embeds' );. The following combinations work on this theme/block:

add_theme_support( 'jetpack-responsive-videos' ); - FAILS add_theme_support( 'jetpack-responsive-videos' ); and add_theme_support( 'responsive-embeds' ); - WORKS add_theme_support( 'responsive-embeds' ); - WORKS

The code linked to in the original bug report (https://github.com/Automattic/jetpack/blob/master/modules/theme-tools/responsive-videos.php#L156) seems to be working as expected, without it, the second test is failing too.

So I wouldn't want to prescribe a wider fix here without knowing more about the original theme, because it could be that if that theme updates to support add_theme_support( 'responsive-embeds' ); all will be well.

jeherve commented 3 years ago

add_theme_support( 'jetpack-responsive-videos' ); and add_theme_support( 'responsive-embeds' ); - WORKS

Right, this is most likely because we stop Jetpack Responsive Videos when a theme declares support for Responsive Embeds. https://github.com/Automattic/jetpack/blob/d37c4eca5842ca81a9f0c368fad1b0b83561acef/modules/theme-tools/responsive-videos.php#L27-L28

it could be that if that theme updates to support add_theme_support( 'responsive-embeds' ); all will be well.

👍

On the long term, now that the block editor has been adopted by many, this may be best for us to deprecate Jetpack's implementation and favor Core's Responsive embeds.

slambert commented 3 years ago

I tested these steps in the 2020 theme and I am seeing full width videos. Can you confirm what theme you were testing with. Many thanks.

I'm seeing this in TwentyNineteen theme. It took a bit of testing to link it to jetpack. I even disabled every setting within jetpack to see if I could isolate it, but that didn't work. If jetpack is active, wide or full width embeds don't work.

slambert commented 3 years ago

add_theme_support( 'responsive-embeds' );

I was hoping to implement a quick fix so added this into our functions.php in the various ways listed above. Could be some user error oversight I made, but I couldn't get it to change the outcome. 🤔

slambert commented 3 years ago

Noticed today that embedded vimeo videos aren't showing up at all with Jetpack enabled. 😕

jeherve commented 3 years ago

embedded vimeo videos aren't showing up at all with Jetpack enabled

Could you tell me more about your setup? Videos appear to work well for me, regardless of how I insert them:

When using the Vimeo embed block
When pasting a Vimeo URL on its own line (thus using the embed block)
When using the vimeo shortcode like so: [vimeo https://vimeo.com/471701386]
When pasting the embed iFrame code.

slambert commented 3 years ago

Sure! I've got multiple sites I've tested this on and disabled all other plugins (see above). Thanks for the prompt because the testing I just did narrowed it down even more.

Here's how I'm seeing it happen:

Disabled all plugins, except Jetpack.
- In Jetpack settings, under Writing > Composting select "Compose using shortcodes to embed media from popular sites"
Post a vimeo URL on it's own line or use the embed block, the video does not appear on the published page - just a space where it would be
Disable jetpack or turn off that setting and the video appears.

Realizing now maybe I should have made "not showing up at all" a separate issue. I made the comment quickly while doing some other work yesterday. Let me know if I should open a separate issue.

jeherve commented 3 years ago

Thank you.

Do you see any kind of JavaScript error on the page where the video should appear? Is there somewhere where I could see a page with such an error, maybe?

Realizing now maybe I should have made "not showing up at all" a separate issue. I made the comment quickly while doing some other work yesterday. Let me know if I should open a separate issue.

If I'm able to reproduce, then yes we'll definitely have to open a new issue to track this and get it fixed.

slambert commented 3 years ago

Try this: https://notes.visitsteve.com/merith-on-solidarity-live

jeherve commented 3 years ago

Could you tell me what vimeo URL you used? That seems to be the issue here: 

slambert commented 3 years ago

https://vimeo.com/483701122/685e159c8d

The privacy settings under "Who can watch?" are set to "People with the private link" - I've used this in WordPress before and it has worked. It also works in WordPress when you use the vimeo embed iframe, like this:

<div style="padding:56.25% 0 0 0;position:relative;"><iframe src="https://player.vimeo.com/video/483701122?color=ec008c&byline=0&portrait=0" style="position:absolute;top:0;left:0;width:100%;height:100%;" frameborder="0" allow="autoplay; fullscreen" allowfullscreen></iframe></div><script src="https://player.vimeo.com/api/player.js"></script>

But that's an iframe.

jeherve commented 3 years ago

Ah yes, this explains things. This issue is logged in #10529.

laras126 commented 3 years ago

For anyone running into this issue needing an immediate fix:

Where xx is a namespace for your theme functions (this can be omitted).

function xx_remove_jetpack_responsive_videos_support() {
    remove_theme_support( 'jetpack-responsive-videos' );
}
// Priority 11 required because Jetpack adds support at the default priority, 10
add_action( 'after_setup_theme', 'xx_remove_jetpack_responsive_videos_support', 11 );

Automattic / jetpack