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.

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

Set aspect ratio preserving CSS on embedded content with fixed size iframes #9500

Merged
merged 10 commits into from Sep 7, 2018

Conversation

Projects
None yet
5 participants
@notnownikki
Member

notnownikki commented Aug 31, 2018

Description

To support the wide styles, we need to preserve the aspect ratio of fixed size embeds when they are resized to full width.

This applies a css class to embedded content that uses iframes (e.g. YouTube, Vimeo) to preserve aspect ratio.

How has this been tested?

Embed the following videos:

https://www.youtube.com/watch?v=XT13ijUfSts

https://www.youtube.com/watch?v=uAE6Il6OTcs

The first should have wp-embed-aspect-16-9 applied, and the second should have wp-embed-aspect-4-3.

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 4, 2018

Contributor

Okay, I pushed some CSS to make videos responsive. It's good, but it's not perfect, which I'll get to in a second. Just a heads up, don't merge quite yet.

First off, the aspect ratios are solid, and I included the math in comments. See also this codepen: https://codepen.io/joen/pen/pOwoGV?editors=1100

The responsiveness works well because it doesn't require recalculating the embed widths, and is more performant. It also presumably is only applied to video where an aspect ratio is detected, which is perfect in other words.

This also works perfectly in the editor:

screen shot 2018-09-04 at 13 17 15

I also put this in a themes.scss file, as an "opinionated style". That means a theme has to opt into this. Your thoughts here are welcome, I'd be fine with moving it to style.scss as it's a nice feature to have for the stock offering. But for now I made it opt-in because it's rather big CSS, and a theme might want to do its own thing with the CSS classes made available.

However, and this is probably why the responsiveness was reverted or phased out since I last added it many moons ago: it doesn't work on the front-end when there are captions:

screen shot 2018-09-04 at 13 17 15

Why? Because due to how the responsiveness works, the video (and only the video) needs its own container which isn't an iframe.

This explains why it works in the editor, where the markup is — (largely, I sanitized a bit) — this:

<figure class="wp-block-embed-youtube wp-embed-aspect-16-9 wp-block-embed is-video">
	<div class="wp-block-embed__wrapper">
		<iframe ... />
	</div>
	<figcaption>Caption</figcaption>
</figure>

Note how the video has the additional wp-block-embed__wrapper.

On the frontend, however, we get this:

<figure class="wp-block-embed-youtube wp-embed-aspect-16-9 wp-block-embed is-video">
	<iframe ... />
	<figcaption>Caption</figcaption>
</figure>

That means we can't really use the responsive trick, because that will cover the caption.

@notnownikki Can you make it so that same wrapper that exists in the editor, exists on the frontend? Doesn't have to be for all embeds (but can be), just ones that need to be responsive (i.e. video). If you can do that, I can fix things up so the responsiveness is solid in both places.

Eventually if we can get this working, should we consider removing the onResize responsiveness that exists for embeds right now? The idea being that a CSS only solution might be more performant?

Contributor

jasmussen commented Sep 4, 2018

Okay, I pushed some CSS to make videos responsive. It's good, but it's not perfect, which I'll get to in a second. Just a heads up, don't merge quite yet.

First off, the aspect ratios are solid, and I included the math in comments. See also this codepen: https://codepen.io/joen/pen/pOwoGV?editors=1100

The responsiveness works well because it doesn't require recalculating the embed widths, and is more performant. It also presumably is only applied to video where an aspect ratio is detected, which is perfect in other words.

This also works perfectly in the editor:

screen shot 2018-09-04 at 13 17 15

I also put this in a themes.scss file, as an "opinionated style". That means a theme has to opt into this. Your thoughts here are welcome, I'd be fine with moving it to style.scss as it's a nice feature to have for the stock offering. But for now I made it opt-in because it's rather big CSS, and a theme might want to do its own thing with the CSS classes made available.

However, and this is probably why the responsiveness was reverted or phased out since I last added it many moons ago: it doesn't work on the front-end when there are captions:

screen shot 2018-09-04 at 13 17 15

Why? Because due to how the responsiveness works, the video (and only the video) needs its own container which isn't an iframe.

This explains why it works in the editor, where the markup is — (largely, I sanitized a bit) — this:

<figure class="wp-block-embed-youtube wp-embed-aspect-16-9 wp-block-embed is-video">
	<div class="wp-block-embed__wrapper">
		<iframe ... />
	</div>
	<figcaption>Caption</figcaption>
</figure>

Note how the video has the additional wp-block-embed__wrapper.

On the frontend, however, we get this:

<figure class="wp-block-embed-youtube wp-embed-aspect-16-9 wp-block-embed is-video">
	<iframe ... />
	<figcaption>Caption</figcaption>
</figure>

That means we can't really use the responsive trick, because that will cover the caption.

@notnownikki Can you make it so that same wrapper that exists in the editor, exists on the frontend? Doesn't have to be for all embeds (but can be), just ones that need to be responsive (i.e. video). If you can do that, I can fix things up so the responsiveness is solid in both places.

Eventually if we can get this working, should we consider removing the onResize responsiveness that exists for embeds right now? The idea being that a CSS only solution might be more performant?

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Sep 4, 2018

Member

I also put this in a themes.scss file, as an "opinionated style". That means a theme has to opt into this. Your thoughts here are welcome, I'd be fine with moving it to style.scss as it's a nice feature to have for the stock offering.

I think this should be in style.scss so that no matter what the theme has as its content size, the embeds will always fit as they should.

Can you make it so that same wrapper that exists in the editor, exists on the frontend? Doesn't have to be for all embeds (but can be), just ones that need to be responsive (i.e. video).

I'll look at it now :)

Eventually if we can get this working, should we consider removing the onResize responsiveness that exists for embeds right now? The idea being that a CSS only solution might be more performant?

This is exactly what I want to do!

Member

notnownikki commented Sep 4, 2018

I also put this in a themes.scss file, as an "opinionated style". That means a theme has to opt into this. Your thoughts here are welcome, I'd be fine with moving it to style.scss as it's a nice feature to have for the stock offering.

I think this should be in style.scss so that no matter what the theme has as its content size, the embeds will always fit as they should.

Can you make it so that same wrapper that exists in the editor, exists on the frontend? Doesn't have to be for all embeds (but can be), just ones that need to be responsive (i.e. video).

I'll look at it now :)

Eventually if we can get this working, should we consider removing the onResize responsiveness that exists for embeds right now? The idea being that a CSS only solution might be more performant?

This is exactly what I want to do!

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Sep 4, 2018

Member

@jasmussen I added the wrapper div and added a block migration so that existing embeds will update when the posts are edited. There seem to be some lint issues with the new css though,

0] [2] packages/block-library/src/embed/theme.scss
[0] [2]  12:4   ✖  Expected double colon pseudo-element notation   selector-pseudo-element-colon-notation
[0] [2]  29:24  ✖  Expected double colon pseudo-element notation   selector-pseudo-element-colon-notation
[0] [2]  33:24  ✖  Expected double colon pseudo-element notation   selector-pseudo-element-colon-notation
[0] [2]  37:24  ✖  Expected double colon pseudo-element notation   selector-pseudo-element-colon-notation
[0] [2]  41:23  ✖  Expected double colon pseudo-element notation   selector-pseudo-element-colon-notation
[0] [2]  45:23  ✖  Expected double colon pseudo-element notation   selector-pseudo-element-colon-notation
[0] [2]  49:23  ✖  Expected double colon pseudo-element notation   selector-pseudo-element-colon-notation
[0] [2]  53:23  ✖  Expected double colon pseudo-element notation   selector-pseudo-element-colon-notation

Would you be able to fix those if you move the css over to style.scss?

Member

notnownikki commented Sep 4, 2018

@jasmussen I added the wrapper div and added a block migration so that existing embeds will update when the posts are edited. There seem to be some lint issues with the new css though,

0] [2] packages/block-library/src/embed/theme.scss
[0] [2]  12:4   ✖  Expected double colon pseudo-element notation   selector-pseudo-element-colon-notation
[0] [2]  29:24  ✖  Expected double colon pseudo-element notation   selector-pseudo-element-colon-notation
[0] [2]  33:24  ✖  Expected double colon pseudo-element notation   selector-pseudo-element-colon-notation
[0] [2]  37:24  ✖  Expected double colon pseudo-element notation   selector-pseudo-element-colon-notation
[0] [2]  41:23  ✖  Expected double colon pseudo-element notation   selector-pseudo-element-colon-notation
[0] [2]  45:23  ✖  Expected double colon pseudo-element notation   selector-pseudo-element-colon-notation
[0] [2]  49:23  ✖  Expected double colon pseudo-element notation   selector-pseudo-element-colon-notation
[0] [2]  53:23  ✖  Expected double colon pseudo-element notation   selector-pseudo-element-colon-notation

Would you be able to fix those if you move the css over to style.scss?

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 4, 2018

Contributor

Behold, our magic at work:

behold

Isn't that amazing? I think it's amazing. That's Twenty Seventeen, which has no concept of blocks.

🤘

There seem to be some lint issues with the new css though,

I could swear I fixed those. They blocked my commit and I had to fix them before I could push. So I'm confused why they regressed. But I've fixed them again in 3720784

Contributor

jasmussen commented Sep 4, 2018

Behold, our magic at work:

behold

Isn't that amazing? I think it's amazing. That's Twenty Seventeen, which has no concept of blocks.

🤘

There seem to be some lint issues with the new css though,

I could swear I fixed those. They blocked my commit and I had to fix them before I could push. So I'm confused why they regressed. But I've fixed them again in 3720784

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Sep 4, 2018

Member

@jasmussen Last commit removes the aspect ratio preservation javascript from the sandbox. It's working brilliantly on the front end, but in the editor there's something throwing it off and I'm not sure what. If you embed a 16:9 video you'll see it's not as high as it needs to be in the editor. Perhaps the caption editable is overlapping somehow?

Member

notnownikki commented Sep 4, 2018

@jasmussen Last commit removes the aspect ratio preservation javascript from the sandbox. It's working brilliantly on the front end, but in the editor there's something throwing it off and I'm not sure what. If you embed a 16:9 video you'll see it's not as high as it needs to be in the editor. Perhaps the caption editable is overlapping somehow?

notnownikki and others added some commits Sep 4, 2018

Fix for responsiveness in the editor also.
Note that this changes the sandbox component a little bit. We want to make sure that the changes we made here don't negatively affect other things that use the same component.
@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 4, 2018

Contributor

Okay I pushed a fix:

behold again

However as noted in the commit note, I had to make a change to the sandbox component in order to make this work. But I'm not sure where else that component is being made — for example if it's being used for embeds that are not supposed to be responsive, the height property I added is undesirable. Do we have a way to scope this a bit so only videos receive the height stuff?

Contributor

jasmussen commented Sep 4, 2018

Okay I pushed a fix:

behold again

However as noted in the commit note, I had to make a change to the sandbox component in order to make this work. But I'm not sure where else that component is being made — for example if it's being used for embeds that are not supposed to be responsive, the height property I added is undesirable. Do we have a way to scope this a bit so only videos receive the height stuff?

notnownikki added some commits Sep 4, 2018

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Sep 4, 2018

Member

Alrighty, we have a class applied to embeds that have a fixed aspect ration, wp-has-aspect-ratio. That gets applied to the type of the Sandbox component, so the CSS inside the sandbox can apply the height fix to content that needs it for the aspect ratio.

Test by embedding videos with different aspect ratios, and a tweet to test normal content, because the new behaviour should only depend on their being an iframe with fixed dimensions, and not the content type.

Member

notnownikki commented Sep 4, 2018

Alrighty, we have a class applied to embeds that have a fixed aspect ration, wp-has-aspect-ratio. That gets applied to the type of the Sandbox component, so the CSS inside the sandbox can apply the height fix to content that needs it for the aspect ratio.

Test by embedding videos with different aspect ratios, and a tweet to test normal content, because the new behaviour should only depend on their being an iframe with fixed dimensions, and not the content type.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 6, 2018

Contributor

If this is ready for review, would you mind extending the review range to for example gutenberg-core, and perhaps assigning a 3.9 milestone?

Contributor

jasmussen commented Sep 6, 2018

If this is ready for review, would you mind extending the review range to for example gutenberg-core, and perhaps assigning a 3.9 milestone?

@notnownikki notnownikki requested a review from WordPress/gutenberg-core Sep 6, 2018

@notnownikki notnownikki added this to the 3.9 milestone Sep 6, 2018

@talldan

This works really nicely, great work 😁

I've added a few minor comments.

Show outdated Hide outdated packages/block-library/src/embed/index.js Outdated
Show outdated Hide outdated packages/block-library/src/embed/index.js Outdated
Show outdated Hide outdated packages/block-library/src/embed/index.js Outdated
Show outdated Hide outdated packages/block-library/src/embed/index.js Outdated
Show outdated Hide outdated packages/block-library/src/embed/index.js Outdated
Show outdated Hide outdated packages/block-library/src/embed/index.js Outdated
@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Sep 7, 2018

Member

@talldan thank you! I'll make all those changes. querySelector has slipped my mind and it would do exactly what we need.

Member

notnownikki commented Sep 7, 2018

@talldan thank you! I'll make all those changes. querySelector has slipped my mind and it would do exactly what we need.

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Sep 7, 2018

Member

Ok, think I fixed all those points. I moved all of the aspect ratio CSS code into maybeSetAspectRatioClassName.

Member

notnownikki commented Sep 7, 2018

Ok, think I fixed all those points. I moved all of the aspect ratio CSS code into maybeSetAspectRatioClassName.

@talldan

talldan approved these changes Sep 7, 2018

@talldan

This comment has been minimized.

Show comment
Hide comment
@talldan

talldan Sep 7, 2018

Contributor

Looks good to me. Thanks for making those changes. Looks like codecov has failed, not sure what can be done about that.

Contributor

talldan commented Sep 7, 2018

Looks good to me. Thanks for making those changes. Looks like codecov has failed, not sure what can be done about that.

@notnownikki notnownikki merged commit 8d46362 into master Sep 7, 2018

1 of 2 checks passed

codecov/project 49.07% (-1.17%) compared to 9f2b3e3
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/embed-responsive-iframed-content branch Sep 7, 2018

@notnownikki notnownikki changed the title from Set aspect ratio preserving CSS on previews with fixed size iframes to Set aspect ratio preserving CSS on embedded content with fixed size iframes Sep 7, 2018

* @param {string} html The preview HTML that possibly contains an iframe with width and height set.
*/
maybeSetAspectRatioClassName( html ) {
const previewDom = document.createElement( 'div' );

This comment has been minimized.

@aduth

aduth Sep 10, 2018

Member

Minor: DOM is an acronym and as such should be capitalized as previewDOM (reference)

@aduth

aduth Sep 10, 2018

Member

Minor: DOM is an acronym and as such should be capitalized as previewDOM (reference)

@@ -176,7 +158,7 @@ class Sandbox extends Component {
// put the html snippet into a html document, and then write it to the iframe's document
// we can use this in the future to inject custom styles or scripts
const htmlDoc = (
<html lang={ document.documentElement.lang }>
<html lang={ document.documentElement.lang } className={ this.props.type }>

This comment has been minimized.

@aduth

aduth Sep 10, 2018

Member

What is the type prop meant to represent?

@aduth

aduth Sep 10, 2018

Member

What is the type prop meant to represent?

This comment has been minimized.

@notnownikki

notnownikki Sep 10, 2018

Member

it's the type that the oembed response contains, it was meant for styling video and image content differently, however I'm not sure it's actually used any more.

@notnownikki

notnownikki Sep 10, 2018

Member

it's the type that the oembed response contains, it was meant for styling video and image content differently, however I'm not sure it's actually used any more.

*/
maybeSetAspectRatioClassName( html ) {
const previewDom = document.createElement( 'div' );
previewDom.innerHTML = html;

This comment has been minimized.

@aduth

aduth Sep 10, 2018

Member

I am very wary of the security implications of this line. This will allow JavaScript in the preview markup to be evaluated, even though we're never including it in the DOM. As a demonstration, paste the following into your console while viewing the editor:

document.createElement( 'div' ).innerHTML = '<img src=/ onerror=\'alert("haxd")\'>'

Now it's a question as to whether we consider embed preview markup to be "safe". Given that the Sandbox component exists, I'm operating on the assumption that the answer is no. Therefore, this is a security vulnerability if it comes to be released.

@aduth

aduth Sep 10, 2018

Member

I am very wary of the security implications of this line. This will allow JavaScript in the preview markup to be evaluated, even though we're never including it in the DOM. As a demonstration, paste the following into your console while viewing the editor:

document.createElement( 'div' ).innerHTML = '<img src=/ onerror=\'alert("haxd")\'>'

Now it's a question as to whether we consider embed preview markup to be "safe". Given that the Sandbox component exists, I'm operating on the assumption that the answer is no. Therefore, this is a security vulnerability if it comes to be released.

This comment has been minimized.

@aduth

aduth Sep 10, 2018

Member

Previously: #1334 (comment) (not as fun as it was previously since Photobucket appears to have since let their oEmbed API languish and die)

@aduth

aduth Sep 10, 2018

Member

Previously: #1334 (comment) (not as fun as it was previously since Photobucket appears to have since let their oEmbed API languish and die)

This comment has been minimized.

@notnownikki

notnownikki Sep 10, 2018

Member

Looking at this now. It might have to be a regular expression that picks out the iframes, so we avoid creating actual elements.

@notnownikki

notnownikki Sep 10, 2018

Member

Looking at this now. It might have to be a regular expression that picks out the iframes, so we avoid creating actual elements.

This comment has been minimized.

@notnownikki

notnownikki Sep 10, 2018

Member

Changed this to use a regex in #9770

@notnownikki

notnownikki Sep 10, 2018

Member

Changed this to use a regex in #9770

This comment has been minimized.

@aduth

aduth Sep 11, 2018

Member

Another simpler option might be document.implementation.createHTMLDocument as a "sandbox" of sorts.

Example: https://github.com/aduth/hpq/blob/f17b3fdc9c5692e9f676f94c33a003d191c81bd6/src/index.js#L21-L23

@aduth

aduth Sep 11, 2018

Member

Another simpler option might be document.implementation.createHTMLDocument as a "sandbox" of sorts.

Example: https://github.com/aduth/hpq/blob/f17b3fdc9c5692e9f676f94c33a003d191c81bd6/src/index.js#L21-L23

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 14, 2018

Member

The demo content should have been updated here. While the deprecation migration saves it from being flagged as invalid, it does cause the demo post to trigger an autosave after delay.

validation.js:150 Block validation: Block validation failed for `core-embed/vimeo` ({name: "core-embed/vimeo", title: "Vimeo", description: "Add a block that displays content pulled from other sites, like Twitter, Instagram or YouTube.", icon: {…}, category: "embed", …}).

Expected:

<figure class="wp-block-embed-vimeo alignwide wp-block-embed is-type-video is-provider-vimeo"><div class="wp-block-embed__wrapper">
https://vimeo.com/22439234
</div></figure>

Actual:

<figure class="wp-block-embed-vimeo wp-block-embed alignwide is-type-video is-provider-vimeo">https://vimeo.com/22439234</figure>
Member

aduth commented Sep 14, 2018

The demo content should have been updated here. While the deprecation migration saves it from being flagged as invalid, it does cause the demo post to trigger an autosave after delay.

validation.js:150 Block validation: Block validation failed for `core-embed/vimeo` ({name: "core-embed/vimeo", title: "Vimeo", description: "Add a block that displays content pulled from other sites, like Twitter, Instagram or YouTube.", icon: {…}, category: "embed", …}).

Expected:

<figure class="wp-block-embed-vimeo alignwide wp-block-embed is-type-video is-provider-vimeo"><div class="wp-block-embed__wrapper">
https://vimeo.com/22439234
</div></figure>

Actual:

<figure class="wp-block-embed-vimeo wp-block-embed alignwide is-type-video is-provider-vimeo">https://vimeo.com/22439234</figure>
@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Sep 23, 2018

Contributor

There's a report in #10109 about this CSS causing a conflict with a theme that's doing its own responsive video handling.

It's only in one theme so far (that we know of) so maybe just something the theme dev needs to patch, but if there's a more widespread practice of themes including some kind of responsive video functionality, it may be worth revisiting a way to make this more robust or opt-in.

Contributor

chrisvanpatten commented Sep 23, 2018

There's a report in #10109 about this CSS causing a conflict with a theme that's doing its own responsive video handling.

It's only in one theme so far (that we know of) so maybe just something the theme dev needs to patch, but if there's a more widespread practice of themes including some kind of responsive video functionality, it may be worth revisiting a way to make this more robust or opt-in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment