search

WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.51k stars 4.2k forks source link

Giant space above all Youtube video in Posts #10109

Closed Pikkals closed 6 years ago

Pikkals commented 6 years ago

Describe the bug Giant space above all Youtube video when VIEWING Posts in Gutenberg: Version 3.9.0

To Reproduce Steps to reproduce the behavior:

  1. Go to 'ALL POSTS.'
  2. Click on 'EDIT' A POST YOU KNOW HAS A YOUTUBE VIDEO
  3. Scroll down to 'YOUTUBE VIDEO BLOCK'
  4. You CAN'T SEE the error in the actual Post Edit, but you can see the giant space when VIEWING the post.

Expected behavior There should be reasonable space around the Youtuve video.

Screenshots error - space above youbue video

Desktop (please complete the following information):

Additional context Latest version of Gutenberg: Version 3.9.0

mkaz commented 6 years ago

Thanks for reporting the bug, agreed that does not look right, and I'd imagine frustrating.

I see the whitespace when I visit your site, however I'm not able to reproduce it. I installed the Hueman Free theme, and using the Autooptimize plugin, which I noticed are what you use on your site at: https://www.discerningtheworld.com/

When I create a post with a YouTube embed, I'm not able to get the large space.

To help anyone else when troubleshooting:

I noticed on your embed there is an additional classname wp-embed-aspect-4-3 which when present adds the padding at the top. I can see in the code if the frame width and height are specified that it will add, but I did not see how that would be specified directly in Gutenberg.

Code in: packages/block-library/src/embed/index.js

Can you let me know how you added the embed, and if there is anything you did after adding the embed to help reproduce and solve the issue?

Pikkals commented 6 years ago

Hi mkaz

Yes I saw that "wp-embed-aspect-4-3" in the CSS and because I am not a programmer at all, I had no idea if this should be there or not.

So I will remove it and come back to you.

I added the embed video, by adding a block, then clicking Youtube and only adding the youtube video url nothing else, just the plain url to the video. Everything was working fine till the latest version upgrade.

Pikkals commented 6 years ago

Hi mkaz

Ahhhhh Ha! "wp-embed-aspect-4-3" is indeed the culprit, I removed it and the giant space is gone. But now the big question, where is this additional CSS coming from ? It was not there yesterday. All was hunky dory until I updated to new version of Gutenberg last night.

I see on other posts it's added additional CSS "wp-has-aspect-ratio wp-embed-aspect-16-9". And it too is making a big white space above all my videos.

What do you suggest I do? I can't sit and manually go through 600 articles removing this CSS that's appeared out of nowhere. Help.

mkaz commented 6 years ago

ok, good to see we were able to narrow it down.

Will need to investigate further to see what it was trying to address and figure out a solution.

Pikkals commented 6 years ago

You got me thinking.

I deactivated the Autoptimize plugin and taaadaaaa.... problem resolved.

I will play around with the settings and see if I can get Autoptimize to work without breaking my site...

Do you recommend another plugin similar to Autoptimize?

SolespireMarcus commented 6 years ago

You got me thinking.

I deactivated the Autoptimize plugin and taaadaaaa.... problem resolved.

I will play around with the settings and see if I can get Autoptimize to work without breaking my site...

Do you recommend another plugin similar to Autoptimize?

In the meantime of having the plugin deactivated, you should report that problem to the developers of Autoptimize: https://wordpress.org/plugins/autoptimize/#developers

Pikkals commented 6 years ago

Thank you Marcus.

I have contacted Autoptimiae support here: https://wordpress.org/support/topic/giant-space-above-all-youtube-video-when-viewing-posts-in-gutenberg-v-3-9-0/

futtta commented 6 years ago

Heya everyone; I'm Autoptimize's developer :-)

I actually still see the gap even with AO disabled, cfr. this screenshot;

image

There's a padding-top:75% and a padding-top:50% (both for .wp-block-embed__wrapper::before) in wp-content/plugins/gutenberg/build/block-library/style.css. Disabling those rules in the inspector "fixes" the problem?

Happy hunting! frank

Pikkals commented 6 years ago

Goodgrief, the error is back... I'm now confused... Never the less...

Frank you are a superstar!!!! 🥇

mkaz commented 6 years ago

Thanks for jumping in and looking @futtta - agreed, I don't think it is a conflict with Autoptimize.

To help troubleshoot:

@Pikkals what steps did you do when adding the YouTube embed? It looks like older posts do not have that same issue with embeds, likely because they were not created in Gutenberg. Can you try to delete and add the YouTube block again in that post?

For @notnownikki and @jasmussen who worked on the issue:

It looks like an unintended consequence from this PR #9770 in 3.9.0 the iframe width/height is being detected properly and the additional aspect ratio class is added, however, when the new class is added it includes a large padding at the top.

It looks like the styles in #9550 are what is causing the issue here, but I'm not able to reproduce in my tests when adding a block, using same theme as user.

chrisvanpatten commented 6 years ago

Hi all -

I took a look at this and I think this may be a plugin conflict although I don't believe it's Autoptimize in this case.

When I insert a YouTube embed on my own site, I see this HTML rendered out:

<figure class="wp-block-embed-youtube wp-block-embed is-type-video is-provider-youtube wp-has-aspect-ratio wp-embed-aspect-16-9">
    <div class="wp-block-embed__wrapper">
        <iframe width="660" height="371" src="https://www.youtube.com/embed/GLwz5IJ4AsY?feature=oembed" frameborder="0" allow="autoplay; encrypted-media" allowfullscreen=""></iframe>
    </div>
</figure>

However when I look at a YouTube embed on your site, I see the following:

<figure class="wp-block-embed-youtube wp-block-embed is-type-video is-provider-youtube wp-has-aspect-ratio wp-embed-aspect-4-3">
    <div class="wp-block-embed__wrapper">
        <div class="video-container">
            <span class="embed-youtube" style="text-align:center; display: block;">
                <iframe class="youtube-player" type="text/html" width="640" height="360" src="https://www.youtube.com/embed/Bp00Wh-oaOQ?version=3&amp;rel=1&amp;fs=1&amp;autohide=2&amp;showsearch=0&amp;showinfo=1&amp;iv_load_policy=1&amp;start=108&amp;wmode=transparent" allowfullscreen="true" style="border:0;"></iframe>
            </span>
        </div>
    </div>
</figure>

Note all the extra <div> and <span> tags and CSS classes.

@Pikkals could you provide a list of plugins on your site? I would guess that another plugin is attempting to handle the responsive video, and is colliding with the way Gutenberg handles it.

Jeanneavelo commented 6 years ago

Hi,

Same problem on my blog with Hueman free theme. I don't use any editor plugin (could other kind of plugins be involved ?).

See for example at the bottom of that post modified with Gutenberg 3.9.0 today : http://jeanneavelo.fr/2018/02/22/un-carrefour-urbain-ordinaire-aux-pays-bas/

Older posts (i.e. not edited with Gutenbegrg 3.9.0) are not concerned.

chrisvanpatten commented 6 years ago

Sounds then like it's an issue with the Hueman Free theme in particular then. Perhaps worth reaching out to the theme developer about this.

Also possibly worth looking at whether this should be another opt-in theme feature via add_theme_support to avoid potential conflicts. That ship may have sailed since this feature is now in Gutenberg, but might at least be worth thinking about.

Pikkals commented 6 years ago

Hi guys

I am using the Hueman Pro theme. I will contact the developer and send them here to have a look.

And yes you are right Jeanneavelo posts that have not been edited with Gutenberg are fine.

Ok, I have logged a ticket with Hueman support.

Pikkals commented 6 years ago

Hi chrisvanpatten

Here is a list of all my plugins:

THEME | VERSION : Hueman Pro | v1.1.3 WP VERSION : 4.9.8

jasmussen commented 6 years ago

Good ticket, thank you.

So this issue happens because embeds created in Gutenberg are now responsive out of the box. The issue here is that if the theme itself also provides responsiveness for embeds through filtering the output, you end up with a double fix, which makes this extra space appear.

There are a number of ways to fix this, @chrisvanpatten I would appreciate your thoughts here as well.

Option 1: Move the responsive Gutenblock CSS from style.scss to theme.scss. The pro is this means Gutenblocks are only responsive if the theme author asks for it, and is therefore aware of it. Con: it means we can't provide responsive videos out of the box.

Option 2: We make it a user toggle, so a user can toggle the responsiveness off on a per-embed basis, if we default to On.

Option 3: Here's some CSS that seems to fix it in the case of THIS particular theme:

.wp-block-embed__wrapper div, .wp-block-embed__wrapper span {
    padding: inherit;
    position: static;
}

But this is a little hacky because it assumes a particular way of adding responsiveness to the videos.

What do you think? CC: @mtias

Pikkals commented 6 years ago

Hmmmm

I see I have a plugin called - WP Edit (version 4.0.3) installed,

I am going to deactivate this and see what happens.

Ok, I see I still have the problem. I'll keep this plugin deactivated as I don't see the need for it.

notnownikki commented 6 years ago

Option 2: We make it a user toggle, so a user can toggle the responsiveness off on a per-embed basis, if we default to On.

I had this on my list to suggest too, thinking of wider themes and narrower videos where it might not look that great.

Jeanneavelo commented 6 years ago

Ok, I have logged a ticket with Hueman support.

So did I : https://wordpress.org/support/topic/display-issue-with-youtube-embedded-video-in-gutenberg-3-9-0-edited-post/

Pikkals commented 6 years ago

I logged my ticket with support@presscustomizr.com and asked the developers to please come to this thread.

notnownikki commented 6 years ago

As much as it disappoints me, I think we might have to make this opt in. Or, perhaps we make responsive videos opt-in (or out?) at the document level instead of the block level?

jasmussen commented 6 years ago

As much as it disappoints me, I think we might have to make this opt in. Or, perhaps we make responsive videos opt-in (or out?) at the document level instead of the block level?

This PR makes it opt-in through the block styles. But I agree, I would consider that our last option, because I sort of feel a responsibility for Gutenberg to provide responsive features by default, so let's make sure we have a fix in the 4.0, but let's keep thinking of ways to do this without making it opt-in.

Presumably themes which provide their own responsiveness do so with a filter that adds additional wrapping elements and classes, no? Would it be possible/feasible/not break things, if we apply a differently named filter for responsive videos?

jasmussen commented 6 years ago

Additional thought on why it would be good to find a solution that did not require opt in via add_theme_support( 'wp-block-styles' ): many themes are likely going to never opt in, even post-merge, because they want to provide their own blockquote styles and not have to override those provided by core blocks. These themes should still be able to benefit from what I would call structural CSS, such as that necessary for responsive embeds.

notnownikki commented 6 years ago

Would it be possible/feasible/not break things, if we output a different filter for responsive videos?

All this goes through oembed, I think trying to intercept what themes do there will end up in priority fighting heck.

... it would be good to find a solution that did not require opt in via add_theme_support( 'wp-block-styles' ) ..

Ugh, I agree with all of that too. Perhaps a document level option is the way to go? Or perhaps a site-wide option, so if you change your theme, you can toggle the G supplied responsiveness across all posts?

jasmussen commented 6 years ago

I haven't totally thrown in the towel on a code fix. https://github.com/WordPress/gutenberg/issues/10109#issuecomment-423898890 fixes it but I would love for us to try it against a bunch of themes that provide responsive embed support, perhaps even plugins, to see if it works well enough.

Pikkals commented 6 years ago

Email from support on Hueman theme

Hi Deborah, thanks for reporting this!

We've identified the compatibility issue, it happens because both the theme and Gutenberg apply their own code to make the video responsive, but these two codes conflict.

https://imgur.com/a/1BzGIu5

A new theme version containing a fix for it will be released pretty soon.

Meantime, since both the theme, by default, and Gutenberg make the video responsive with the same aspect ratio (16:9), you can act on the Gutenberg block additional classes and remove the plugin CSS class which "produces" the conflict: wp-embed-aspect-16-9

Hope this helps.

Rocco, Support Team @presscustomizr

Jeanneavelo commented 6 years ago

Meantime, since both the theme, by default, and Gutenberg make the video responsive with the same aspect ratio (16:9), you can act on the Gutenberg block additional classes and remove the plugin CSS class which "produces" the conflict: wp-embed-aspect-16-9

Hope this helps.

Thanks for the tweak : it works perfectly well.

caouic commented 6 years ago

I confirm. The conflict between Gutenberg and Hueman Theme Pro occurs too since Gutenberg version 3.9.0 . Thanks for the tweak @Pikkals You save my day Au passage, sympa le blog ;)

Jeanneavelo commented 6 years ago

Au passage, sympa le blog ;)

Si le compliment m'est adressé, je dis merci ! :-)

chrisvanpatten commented 6 years ago

Awesome! Since there seems to be a fix I’ll close this issue but if anyone still has problems we can always reopen :)

notnownikki commented 6 years ago

Just a quick mention of the PR that would let you toggle responsiveness on a block-by-block basis :) https://github.com/WordPress/gutenberg/pull/10161

sandnsurf commented 6 years ago

Same issue with the Lensacap theme by Array (https://arraythemes.com/themes/lenscap-wordpress-theme/), not limited to the Hueman Pro theme

Does not occur with old posts with video, just when I edit the page and update (e.g. https://litfl.com/tee-essentials-making-sense-of-the-views/)

When I remove the "wp-embed-aspect-16-9" as suggested all looks good again. https://litfl.com/tee-essentials-assessment-of-the-left-ventricle/

Realise thread closed, just wanted to give feedback from an additional theme.

joopringelberg commented 6 years ago

The very same problem applies to the SiteOrigin Vantage theme. I've created a thread on their forum.

I don't understand how to apply the fix suggested above. Should I edit the CSS file of the installed theme? And just how can I do that?

notnownikki commented 6 years ago

Hey folks, we just merged https://github.com/WordPress/gutenberg/pull/10161 which will allow you to turn off the responsiveness on a per-block basis. That'll be in the 4.0 release.

Jeanneavelo commented 6 years ago

I don't understand how to apply the fix suggested above. Should I edit the CSS file of the installed theme? And just how can I do that?

You have to edit each block concerned in html to remove the "wp-embed-aspect-16-9" CSS value. It isn't a global tweak.

C47Joe commented 6 years ago

I'm on Divi. Just got same issue. Copy and pasted video URLs into post and let it convert to an embed link. Narrowed it down the the 50% padding in wp-block-embed__wrapper::before

If I go into the block's settings to advanced and remove the added css code wp-has-aspect-ratio wp-embed-aspect-16-9 it seems to fix it. But I have to do that for every single video.

notnownikki commented 6 years ago

I just want to confirm with @jasmussen that this is fixed for the 4.0 release

jasmussen commented 6 years ago

Yep there is a feature in the 4.0 to let you easily switch off the baked in responsiveness for videos:

screen shot 2018-10-04 at 09 56 17

This will be in 4.0. If we need to go further, then there is a "nuclear option" in https://github.com/WordPress/gutenberg/pull/10133 which we hope to not have to use.

C47Joe commented 6 years ago

@jasmussen thanks for the screenshot. Is that a global setting or per YouTube block?

notnownikki commented 6 years ago

It's per block, but the workaround mentioned in https://github.com/WordPress/gutenberg/issues/10109#issuecomment-424289255 will also be 4.0 so turning off the responsiveness is a last resort and shouldn't be needed with 4

davisshaver commented 6 years ago

Bumping this for reconsideration, I ran into the style issue on my theme and the style rules seem a bit too clever, thankfully the snippet above was a good override but it's clearly not a universal solution. This is different from the block styles such as colored blocks because the responsive video styling more drastically changes the page flow. While I appreciate the feature in principle, what is the cost of shipping an ancillary experience that has a demonstrated high likelihood of breaking third-party themes? Could we hold this to a release after 4.0, or make it more opt-in?

davisshaver commented 6 years ago

This is the override style that ended up working for me. @jasmussen's snippet does not override for me.

.wp-block-embed.wp-embed-aspect-16-9 .wp-block-embed__wrapper::before {
padding-top: inherit;
}
jasmussen commented 6 years ago

ran into the style issue on my theme and the style rules seem a bit too clever

Can you elaborate a bit?

I'd prefer to make the block styles solid.

davisshaver commented 6 years ago

@jasmussen I wonder if the feature could be deferred until more bulletproof CSS can be developed. Regarding the actual CSS it seems to be a idiomatic way of achieving the fixed aspect ratio without using JS, but for a tertiary feature that appears to have at least some probability of causing a site to "break" upon first Gutenberg publish, and given the timeline for merge, I wonder if it's necessary for the initial release. I know there's some impetus on me as a commenter to offer a better solution, but I don't have one at-hand unfortunately. Too bad we can't do visual regression with Gutenberg and themes, that would be nice, as admittedly I don't have any way to estimate how many themes would be affected.

chrisvanpatten commented 6 years ago

@davisshaver Could you share the site in question? A bit of extra context would be very valuable!

jasmussen commented 6 years ago

I wonder if the feature could be deferred until more bulletproof CSS can be developed.

While anything is possible, this is the sort of feature that only really makes sense if actually shipped, and I doubt delaying the feature will do anything but delay the pain.

That's not to say we have to force it through, I'd like to make the CSS more "bulletproof", but the only way to do so is see what themes actually do, which is why I inquired about the method you're using.

If the method used is the same as that used for previously mentioned themes, perhaps I can make a pull request that leverages this to provide the responsiveness in a way that overrides a theme.

At this point I think it's very crucial to remember that this responsiveness is applied only to embeds created in Gutenberg, not to embeds created in the classic editor.

On a tangent, depending on how this plays out, themes can provide back compatible responsiveness by scoping the responsive embed CSS to :not(.wp-has-aspect-ratio) ... { }.

davisshaver commented 6 years ago

@jasmussen I am glad to help.. The patch is applied on this site but you can remove the override with dev tools. https://leb.town/2018/09/19/lebanon-teen-bringing-lego-based-stem-education-to-the-region/

In this case rather than other styles I believe the gap is actually being caused by the behavior of AMP version of YouTube embed. I am using AMP plugin v1.x and testing AMP native mode.

If we wanted to patch for this specifically I suppose something like :not(:has(amp-youtube)) {...} could work, or alternatively including the patch in blocks stylesheet with :has(amp-youtube) {...}.

jasmussen commented 6 years ago

Thanks so much for the additional context. Here's the markup from the YouTube AMP plugin:

screenshot 2018-10-09 at 14 13 16

It appears the AMP YouTube plugin is using basically the same technique, a 56.something% padding element.

So to summarize where we are: many WordPress themes and plugins provide some responsive video embed support. When Gutenberg also provides this, there's a conflict as shown in this thread.

Outside of simply rolling this feature back or making it off by default, here are some ideas:

  1. make it opt-in via the block-styles opt-in, i.e. https://github.com/WordPress/gutenberg/pull/10133
  2. make it opt-in via a brand new function call, such as add_theme_support('responsive-embeds')
  3. change the toggle so embeds are not responsive by default out of Gutenberg
  4. find a way to make a global user-facing setting to choose the Gutenberg default
  5. try and make a CSS hack that hopefully catches more methods than making themes responsive than the snippet shared here https://github.com/WordPress/gutenberg/issues/10109#issuecomment-423898890

I would love additional opinions on this. On the one hand I strongly feel like the fact that both themes and plugins add this support after the fact, is a strong indication that it's been done wrong all these years and we should fix it at the source.

On the other hand I deeply understand that users won't care about that, they'll just see a giant space above embeds and think they did something wrong.

I would love for us to find a middle-ground that allowed us to ship this by default, but still give some more power into the hands of both themers and users both.

Exploring 5, I wonder if the following could work better than previously shared snippet?

.wp-has-aspect-ratio>.wp-block-embed__wrapper>*:not(.i-amphtml-layout-responsive),
.wp-has-aspect-ratio>.wp-block-embed__wrapper>*:not(.i-amphtml-layout-responsive)::before,
.wp-has-aspect-ratio>.wp-block-embed__wrapper>*:not(.i-amphtml-layout-responsive)::after,
.wp-has-aspect-ratio>.wp-block-embed__wrapper>*:not(.i-amphtml-layout-responsive) *,
.wp-has-aspect-ratio>.wp-block-embed__wrapper>*:not(.i-amphtml-layout-responsive) *::before,
.wp-has-aspect-ratio>.wp-block-embed__wrapper>*:not(.i-amphtml-layout-responsive) *::after {
    padding-top: inherit !important;
    padding-bottom: inherit !important;
    position: static !important;
}
.wp-has-aspect-ratio>.wp-block-embed__wrapper>*:not(.i-amphtml-layout-responsive) iframe {
    position: absolute !important;
}

It would:

On the flipside, that CSS is a little crazy and heavy handed.

Would love your thoughts, @pento @mtias

chrisvanpatten commented 6 years ago

Another thing to consider is themes that bundle FitVids. That's where I encountered the problem. You could probably detect if jQuery.fn.fitVids is accessible and conditionally disable based on that but it seems like a hack.

I think an opt-in add_theme_support is the best call, even though it sadly means a lot of sites won't get it out of the box. It's the safest option though, and allows themes with specific needs to provide their own responsive solution without needing to ask content editors to remember to disable the responsive embed toggle.

davisshaver commented 6 years ago

@jasmussen I totally agree with you, this should have been done better sooner, but keeping legacy sites working is the hard balance of WordPress... I think that moving to add_theme_support could be a good balance of creating a canonical solution and minimizing risk for legacy users.

For reference, some other responsive embed examples I've seen in the wild include the .shortcake-bakery-responsive implementation as well as Pym.js.