Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WordPress block has large whitespace at the bottom #14659

Open
notnownikki opened this issue Mar 27, 2019 · 23 comments
Open

WordPress block has large whitespace at the bottom #14659

notnownikki opened this issue Mar 27, 2019 · 23 comments
Labels
[Block] Embed Affects the Embed Block [Type] Enhancement A suggestion for improvement.

Comments

@notnownikki
Copy link
Member

Describe the bug

In the editor, inserting a WordPress block and embedding a post renders with a load of whitespace. It seems this happened because the html returned for embedding a post changed at some point.

cc @jasmussen
@jasmussen
Copy link
Contributor

Confirmed. In fact it's not just any random sized space, the block is being treated as "responsive 16:9".

Editor:

Screenshot 2019-03-28 at 09 51 43

Frontend:

Screenshot 2019-03-28 at 09 51 49

Actually... hmmmmmmmm. I think maybe the embed block is "fine", because the bug as shown above is actually because the theme I'm testing with is providing its own responsive embed styles, which should not be there.

In your testing environment, what theme did you use and which plugins did you have enabled?

It's possible that if a regression happened, it happened in the responsive-embeds opt-in function call — maybe it opts in for all themes now even when it shouldn't? Not at all sure, but right at this point my gut tells me this isn't a Gutenberg issue.

@notnownikki
Copy link
Member Author

notnownikki commented Mar 28, 2019 via email

@jasmussen
Copy link
Contributor

@kjellr can you recall if TwentyNineteen opts into responsive embeds? Or does it have its own embed responsiveness magic?

@kjellr
Copy link
Contributor

kjellr commented Mar 28, 2019

Yes, Twenty Nineteen does opt in to responsive embeds.

@notnownikki
Copy link
Member Author

notnownikki commented Mar 28, 2019 via email

@notnownikki
Copy link
Member Author

notnownikki commented Mar 29, 2019 •
edited

Checking this out, and found...

noresponsive

	{
		name: 'core-embed/wordpress',
		settings: {
			title: 'WordPress',
			icon: embedWordPressIcon,
			keywords: [ __( 'post' ), __( 'blog' ) ],
			responsive: false,
			description: __( 'Embed a WordPress post.' ),
		},
	},

The WordPress block does not have responsive styles applied, in the editor or the front end, and yet there's still a load of padding at the bottom. The iframe that the embed is rendered in has the padding...

embeddedWP

That iframe is coming from oembed, and there are no responsive styles applied to the wrapping figure.

@jasmussen
Copy link
Contributor

That padding is the responsive style, it's 56.something% right? That's the responsive style for a 16:9 video. Which obviously this isn't. This is freeform, so it should not have any of that. And I'm not sure where it's coming from, because that padding should only come from the 16:9 CSS classes

@swissspidy
Copy link
Member

Worth noting that WordPress post embeds (iframes) calculate their height and pass that information to the embedding site (host) via postMessage. That way, the host can re-adjust the height. There should be no need to do anything else except setting a width of 100% for the WordPress embed iframe.

@swissspidy swissspidy added the [Block] Embed Affects the Embed Block label Mar 29, 2019
@notnownikki
Copy link
Member Author

@jasmussen no, there are no responsive classes in the embedded html at all. Unless they're being applied to all iframes (which would be a horrible theme bug) then they're not applied there. Also, the dimensions shown there are the actual height and width attributes of the iframe in the html. It's not responsive styles making that size.

@swissspidy
Copy link
Member

Worth noting that WordPress embed iframes have a minimum height of 200px. See https://github.com/WordPress/wordpress-develop/blob/2a7aedc9e08a4a3701f908ffc981ac5a476b493c/src/js/_enqueues/wp/embed.js#L62-L72

@notnownikki This might be what you are running into at the moment.

@notnownikki
Copy link
Member Author

This is the HTML oembed generates:

<iframe class="wp-embedded-content" sandbox="allow-scripts" security="restricted" src="http://localhost:8888/?p=1&amp;embed=true#?secret=U0rOggtHDr" data-secret="U0rOggtHDr" width="600" height="200" title="“Hello world!” — Gutenberg Dev" frameborder="0" marginwidth="0" marginheight="0" scrolling="no"></iframe>

It's fixed height, and it's including the page as the source of the iframe, and if I remember correctly this is quite different to what used to happen. Now the iframe is a fixed height, which means shorter content going to have that white space, as the iframe does not resize.

In the editor and in the core blocks, there are no responsive styles applied. This looks like it's down to an oembed change which also meant we needed to change the WordPress HTML detection in #14658 (which needs a review ;) )

@swissspidy
Copy link
Member

Now the iframe is a fixed height, which means shorter content going to have that white space, as the iframe does not resize.

It should resize using postMessage calls. That's what wp-embed.js is for.

@notnownikki
Copy link
Member Author

notnownikki commented Mar 29, 2019 •
edited

Ah. It's not doing that here. For a post that simple says "hello world!" the content in the iframe is 600x132, but the containing iframe remains 600x200. So when we have an iframe that's 1000px high, it stays that high.

@swissspidy
Copy link
Member

Can you verify whether that wp-embed.js JS file is being enqueued on your site and that the iframe has a data-secret attribute? I'll try to reproduce on my end in the meantime.

@notnownikki
Copy link
Member Author

Yes, it's enqueued (the .min version) and there is a data-secret attribute in the iframe. #14659 (comment) has the HTML that appears in the post.

@jasmussen
Copy link
Contributor

jasmussen commented Mar 29, 2019 •
edited

I can't really reproduce in master + 2019:

embeds

What am I missing?

I can reproduce in my own theme because I haven't updated it in a while, and it provides its own heavy-handed responsive styles.

Edit: I mean, there's a LITTLE bit of extra padding below the "short" embed, but nothing that looks super wrong.

@notnownikki
Copy link
Member Author

I think you are seeing it there, can you inspect the iframe on the shorter embed there?, the "Learn something difficult" one? I'm pretty sure I can see the bottom whitespace when you highlight it. If the enclosing iframe is still 200px high, you've reproduced it.

@notnownikki
Copy link
Member Author

I mean, there's a LITTLE bit of extra padding below the "short" embed, but nothing that looks super wrong.

Yeah, that's a symptom. The problem is when the iframe is 1000px high, it should resize, so when a post is just enough to tip it over 200px, it'll go to 1000px and you'll see the massive whitespace.

If you inspect the short embed and you see the wrapping iframe is still 200px, then it hasn't resized and that's the problem.

@jasmussen
Copy link
Contributor

Yep, I see it now. There's definitely that 200px min-height that Pascal is referring to. Which would be nice to fix, but is also not horrible since it doesn't crop anything.

But I still can't reproduce the latter thing you're referring to — do you have a link to a post that results in a 1000px embed?

@notnownikki
Copy link
Member Author

Only on my local machine. This is how embedding the default "Welcome to WordPress" post looks in my browser. I can't guarantee that it would render pixel-perfect identical for you though, because of different browsers, OSes, etc. and so it might not trigger the problem for you.

1000px

@notnownikki
Copy link
Member Author

notnownikki commented Mar 29, 2019 •
edited

If I remember we had a similar issue with the resizing iframe in Gutenberg, and it was that the initial messages posted weren't caught by the resizing listener, because of $REASON (I think it was some release of Chrome which broke it?) and so we had to fire off our own message when we registered the listener.

@jasmussen
Copy link
Contributor

That's interesting, I can repeat it now:

Screenshot 2019-03-29 at 14 05 49

That page is from the same dev site. It smells race-condition-y, without knowing virtually ANYTHING about the embeds (so don't take my word). In this case the iframe is 338px tall. Note it looks okayish on the frontend where it's 200px tall:

Screenshot 2019-03-29 at 14 08 05

Given that this is a freeform iframe where the height is actually the attribute set on the iframe, I'm not sure I can be of that much help here :(

Screenshot 2019-03-29 at 14 08 34

@notnownikki
Copy link
Member Author

Whew, ok, it's not just me 😁

@swissspidy if your resizing code isn't firing, it might be that it's missing the first message. We fixed it in the sandbox component by running an initial resize ourselves, because we were missing the very first resize message after we registered the listener. https://github.com/WordPress/gutenberg/blob/master/packages/components/src/sandbox/index.js#L131

@bobbingwide bobbingwide mentioned this issue Aug 28, 2020
Open
@jordesign jordesign added the [Type] Enhancement A suggestion for improvement. label Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
[Block] Embed Affects the Embed Block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@swissspidy @kjellr @jasmussen @jordesign @notnownikki