Try: Fix embed iframe sizing issue. #40213
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jasmussen
added
[Type] Bug
|
Size Change: -5 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
jasmussen
requested review from
a team,
oandregal and
noahtallen
and removed request for
a team
|
Hey, I've tested this with the pinterest embed and a few others (youtube, loom). Also with the HTML block (preview), the other use of the sandbox component I could find. It works fine:
The last change I saw was #26842 (Nov 2020) and the reason for it was the loom block. Not sure if some host-specific setup. I've tested that block and it still works as expected with this change (because it gets attached the aspect ratio class). I then tried to change to a theme that doesn't support responsive embeds to see how this fared. Used TwentyTwentyOne and removed support in its The logic seems fine, but I reckon I haven't been involved in this problem space, so may miss something. Do you think I should test any other thing? |
|
Thank you for testing, I appreciate that. In general testing the embeds, thanks for also testing Loom and pointing out that PR! Perhaps @retrofox can chime in and see if there's any context I might be missing? Overall I think this one should be fairly safe to land since it's mainly a CSS change that makes the editor look the same as the frontend. |
ntsekouras
reviewed
Apr 25, 2022
| width: 100%; | ||
| } | ||
| html.wp-has-aspect-ratio, | ||
| body.wp-has-aspect-ratio, | ||
| body.wp-has-aspect-ratio > div, | ||
| body.wp-has-aspect-ratio > div iframe { | ||
| width: 100%; |
There was a problem hiding this comment.
It seems that wp-has-aspect-ratio class is added from embed block. Not added here of course, but should the related styles live in that block? 🤔
We are making a change here that affects an exposed component(Sandbox) and I'm not 100% sure that the style removed above couldn't affect some existing usages from third party plugins, etc..
There was a problem hiding this comment.
Good point. What's the best way forward, you think?
I'll try to take a look, Joen. |
|
Thank you! Please note Nik's comment here, #40213 (comment) — and if you'd like to push directly to this branch, feel free. |
jasmussen
force-pushed
the
try/fix-embed-issue
branch
from
7ed4943 to
9d97a8b
Compare
|
I've rebased this one just to keep it current. It still solves the issue in question, and it's such a small bugfix, I'd love to land it. CC: @WordPress/gutenberg |
|
This is working great for me! 🚢 Before:
After:
|
oandregal
approved these changes
Sep 6, 2022
|
Thank you. I'm going to land this because it's such a small change that's easy to revert if there are unintended consequences. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Pinterest embeds are broken in the block editor, they appear cropped:
This happens because we have a rule that sets the iframe to be 100% wide inside, regardless of what original dimensions are set. This is fine for when the responsive embedding property is set so it can scale to an aspect ratio, but from what I can tell in the code, we apply this rule regardless of that.
This PR moves the width rule to only apply when responsive embed classes are present. Which appears to have changed recently, so I could use a sanity check both on the code and the assumed behavior.
Nevertheless, this PR fixes the issue in the process:
Perhaps most importantly, this makes it match the frontend:
Right now in trunk, the edit view doesn't match the frontend at all. That feels like a baseline to restore.
Testing Instructions
Paste a Pinterest embed such as
https://www.pinterest.com/pin/1023865296507001702/in the editor, and it should not be cropped and look the same on the frontend.