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

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

Merged
merged 10 commits into from
Sep 7, 2018
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9255d29
Set aspect ratio preserving CSS on previews with fixed size iframes
notnownikki Aug 31, 2018
5baab35
Add aspect ratio CSS.
Sep 4, 2018
fdfa437
Add embed wrapper div in saved content for responsive classes
notnownikki Sep 4, 2018
3720784
Fix things up.
jasmussen Sep 4, 2018
a81372e
Remove the aspect ratio preservation from sandbox
notnownikki Sep 4, 2018
f75ac26
Fix embed tests
notnownikki Sep 4, 2018
6ed5c58
Fix for responsiveness in the editor also.
jasmussen Sep 4, 2018
27c15c7
Fix height for video sandboxing
notnownikki Sep 4, 2018
ea6d484
Make the aspect ratio generic again
notnownikki Sep 4, 2018
037607d
Fix review points
notnownikki Sep 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 110 additions & 19 deletions packages/block-library/src/embed/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
*/
import { parse } from 'url';
import { includes, kebabCase, toLower } from 'lodash';
import classnames from 'classnames';
import classnames from 'classnames/dedupe';

/**
* WordPress dependencies
Expand Down Expand Up @@ -136,6 +136,28 @@ export function getEmbedEdit( title, icon ) {
return false;
}

/**
* Finds the first iframe with a width and height and returns
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few minor things about the doc block.

  • Should it read 'or an empty object if there is no iframe with width and height.'?
  • My understanding is there should be a line separating the function description from the params.
  • The param description should be capitalised -> 'The preview HTML'
  • Typo in the param description - 'possible' -> 'possibly

* an object with width and height attributes, empty object
* if there is no iframe with width and height.
* @param {string} html the preview HTML that possible contains an iframe with width and height set.
* @return {Object} Object with extracted height and width if available.
*/
getiFrameHeightWidth( html ) {
Copy link
Contributor

@talldan talldan Sep 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be camel-cased as Iframe instead of iFrame. It looks like there's a FocusableIframe component in the codebase so would be good to be consistent in terms of case.

const previewDom = document.createElement( 'div' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

previewDom.innerHTML = html;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed this to use a regex in #9770

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const walker = document.createTreeWalker( previewDom );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be wrong, but I think it should be possible to simplify this to something like:

const iframe = previewDom.querySelector( 'iframe' );
if ( iframe ) {
	return {
		height: iframe.height,
		width: iframe.width,
	};
}

while ( walker.nextNode() ) {
if ( 'IFRAME' === walker.currentNode.tagName ) {
return {
height: walker.currentNode.height,
width: walker.currentNode.width,
};
}
}
return {};
}

/***
* Sets block attributes based on the preview data.
*/
Expand All @@ -156,6 +178,46 @@ export function getEmbedEdit( title, icon ) {
if ( html || 'photo' === type ) {
setAttributes( { type, providerNameSlug } );
}

// If the embedded content is in an iframe with fixed width and height, we
// calculate the aspect ratio and set an extra css class so that the rendered
// content keeps the correct height no matter how wide the block is set to be.
const { height, width } = this.getiFrameHeightWidth( html );
if ( undefined !== height && undefined !== width ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if most of this logic could be moved into getIframeHeightWidth and that could become getAspectRatioClassName so that setAttributesFromPreview isn't doing so much. Or they could be separate functions.

const aspectRatio = ( width / height ).toFixed( 2 );
let aspectRatioClassName;

switch ( aspectRatio ) {
// Common video resolutions.
case '2.33':
aspectRatioClassName = 'wp-embed-aspect-21-9';
break;
case '2.00':
aspectRatioClassName = 'wp-embed-aspect-18-9';
break;
case '1.78':
aspectRatioClassName = 'wp-embed-aspect-16-9';
break;
case '1.33':
aspectRatioClassName = 'wp-embed-aspect-4-3';
break;
// Vertical video and instagram square video support.
case '1.00':
aspectRatioClassName = 'wp-embed-aspect-1-1';
break;
case '0.56':
aspectRatioClassName = 'wp-embed-aspect-9-16';
break;
case '0.50':
aspectRatioClassName = 'wp-embed-aspect-1-2';
break;
}

if ( aspectRatioClassName ) {
const className = classnames( this.props.attributes.className, aspectRatioClassName );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could avoid the concatenation here by making classnames do the work:

const className = classnames( this.props.attributes.className, 'wp-has-aspect-ratio', aspectRatioClassName );

this.props.setAttributes( { className } );
}
}
}

switchBackToURLInput() {
Expand Down Expand Up @@ -257,6 +319,24 @@ export function getEmbedEdit( title, icon ) {
};
}

const embedAttributes = {
url: {
type: 'string',
},
caption: {
type: 'array',
source: 'children',
selector: 'figcaption',
default: [],
},
type: {
type: 'string',
},
providerNameSlug: {
type: 'string',
},
};

function getEmbedBlockSettings( { title, description, icon, category = 'embed', transforms, keywords = [] } ) {
// translators: %s: Name of service (e.g. VideoPress, YouTube)
const blockDescription = description || sprintf( __( 'Add a block that displays content pulled from other sites, like Twitter, Instagram or YouTube.' ), title );
Expand All @@ -266,23 +346,7 @@ function getEmbedBlockSettings( { title, description, icon, category = 'embed',
icon,
category,
keywords,
attributes: {
url: {
type: 'string',
},
caption: {
type: 'array',
source: 'children',
selector: 'figcaption',
default: [],
},
type: {
type: 'string',
},
providerNameSlug: {
type: 'string',
},
},
attributes: embedAttributes,

supports: {
align: true,
Expand Down Expand Up @@ -320,11 +384,38 @@ function getEmbedBlockSettings( { title, description, icon, category = 'embed',

return (
<figure className={ embedClassName }>
{ `\n${ url }\n` /* URL needs to be on its own line. */ }
<div className="wp-block-embed__wrapper">
{ `\n${ url }\n` /* URL needs to be on its own line. */ }
</div>
{ ! RichText.isEmpty( caption ) && <RichText.Content tagName="figcaption" value={ caption } /> }
</figure>
);
},

deprecated: [
{
attributes: embedAttributes,
save( { attributes } ) {
const { url, caption, type, providerNameSlug } = attributes;

if ( ! url ) {
return null;
}

const embedClassName = classnames( 'wp-block-embed', {
[ `is-type-${ type }` ]: type,
[ `is-provider-${ providerNameSlug }` ]: providerNameSlug,
} );

return (
<figure className={ embedClassName }>
{ `\n${ url }\n` /* URL needs to be on its own line. */ }
{ ! RichText.isEmpty( caption ) && <RichText.Content tagName="figcaption" value={ caption } /> }
</figure>
);
},
},
],
};
}

Expand Down
55 changes: 55 additions & 0 deletions packages/block-library/src/embed/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,59 @@
figcaption {
@include caption-style();
}

// Add responsiveness to common aspect ratios.
&.wp-embed-aspect-21-9 .wp-block-embed__wrapper,
&.wp-embed-aspect-18-9 .wp-block-embed__wrapper,
&.wp-embed-aspect-16-9 .wp-block-embed__wrapper,
&.wp-embed-aspect-4-3 .wp-block-embed__wrapper,
&.wp-embed-aspect-1-1 .wp-block-embed__wrapper,
&.wp-embed-aspect-9-16 .wp-block-embed__wrapper,
&.wp-embed-aspect-1-2 .wp-block-embed__wrapper {
position: relative;

&::before {
content: "";
display: block;
padding-top: 50%; // Default to 2:1 aspect ratio.
}

iframe {
position: absolute;
top: 0;
right: 0;
bottom: 0;
left: 0;
width: 100%;
height: 100%;
}
}

&.wp-embed-aspect-21-9 .wp-block-embed__wrapper::before {
padding-top: 42.85%; // 9 / 21 * 100
}

&.wp-embed-aspect-18-9 .wp-block-embed__wrapper::before {
padding-top: 50%; // 9 / 18 * 100
}

&.wp-embed-aspect-16-9 .wp-block-embed__wrapper::before {
padding-top: 56.25%; // 9 / 16 * 100
}

&.wp-embed-aspect-4-3 .wp-block-embed__wrapper::before {
padding-top: 75%; // 3 / 4 * 100
}

&.wp-embed-aspect-1-1 .wp-block-embed__wrapper::before {
padding-top: 100%; // 1 / 1 * 100
}

&.wp-embed-aspect-9-6 .wp-block-embed__wrapper::before {
padding-top: 66.66%; // 6 / 9 * 100
}

&.wp-embed-aspect-1-2 .wp-block-embed__wrapper::before {
padding-top: 200%; // 2 / 1 * 100
}
}
31 changes: 4 additions & 27 deletions packages/components/src/sandbox/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,26 +79,13 @@ class Sandbox extends Component {
const observeAndResizeJS = `
( function() {
var observer;
var aspectRatio = false;
var iframe = false;

if ( ! window.MutationObserver || ! document.body || ! window.parent ) {
return;
}

function sendResize() {
var clientBoundingRect = document.body.getBoundingClientRect();
var height = aspectRatio ? Math.ceil( clientBoundingRect.width / aspectRatio ) : clientBoundingRect.height;

if ( iframe && aspectRatio ) {
// This is embedded content delivered in an iframe with a fixed aspect ratio,
// so set the height correctly and stop processing. The DOM mutation will trigger
// another event and the resize message will get posted.
if ( iframe.height != height ) {
iframe.height = height;
return;
}
}

window.parent.postMessage( {
action: 'resize',
Expand Down Expand Up @@ -139,20 +126,6 @@ class Sandbox extends Component {
document.body.style.width = '100%';
document.body.setAttribute( 'data-resizable-iframe-connected', '' );

// Make embedded content in an iframe with a fixed size responsive,
// keeping the correct aspect ratio.
var potentialIframe = document.body.children[0];
if ( 'DIV' === potentialIframe.tagName || 'SPAN' === potentialIframe.tagName ) {
potentialIframe = potentialIframe.children[0];
}
if ( potentialIframe && 'IFRAME' === potentialIframe.tagName ) {
if ( potentialIframe.width ) {
iframe = potentialIframe;
aspectRatio = potentialIframe.width / potentialIframe.height;
potentialIframe.width = '100%';
}
}

sendResize();

// Resize events can change the width of elements with 100% width, but we don't
Expand All @@ -164,8 +137,12 @@ class Sandbox extends Component {
body {
margin: 0;
}
html,
body,
body > div,
body > div > iframe {
width: 100%;
height: 100%;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this height property is necessary in order for the responsiveness to work, basically because every element that contains the embed needs an explicit height in order for a single percentage height to work properly.

But if this component is used for things that aren't aspect ratio responsive, we need to scope it. Something like:

html.is-video,
html.is-video body,
html.is-video body > div,
html.is-video body > div > iframe {
height: 100%;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see :) Ok, I'll see how it can be changed around to only get applied to things we're doing aspect ratio preservation on.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to send this PR right back at me if we can have a CSS class or separate style declaration for aspect ratio responsive stuff only.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be cool to have an .is-responsive or .has-aspect-ratio class to target here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I was thinking .wp-embed-has-aspect-ratio and then the editor should be able to use that to apply the height to the sandbox iframe.

}
body > div > * {
margin-top: 0 !important; /* has to have !important to override inline styles */
Expand Down
4 changes: 3 additions & 1 deletion test/integration/full-content/fixtures/core__embed.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
<!-- wp:core/embed {"url":"https://example.com/"} -->
<figure class="wp-block-embed">
https://example.com/
<div class="wp-block-embed__wrapper">
https://example.com/
</div>
<figcaption>Embedded content from an example URL</figcaption>
</figure>
<!-- /wp:core/embed -->
2 changes: 1 addition & 1 deletion test/integration/full-content/fixtures/core__embed.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,6 @@
]
},
"innerBlocks": [],
"originalContent": "<figure class=\"wp-block-embed\">\n https://example.com/\n <figcaption>Embedded content from an example URL</figcaption>\n</figure>"
"originalContent": "<figure class=\"wp-block-embed\">\n <div class=\"wp-block-embed__wrapper\">\n https://example.com/\n </div>\n <figcaption>Embedded content from an example URL</figcaption>\n</figure>"
}
]
2 changes: 1 addition & 1 deletion test/integration/full-content/fixtures/core__embed.parsed.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"url": "https://example.com/"
},
"innerBlocks": [],
"innerHTML": "\n<figure class=\"wp-block-embed\">\n https://example.com/\n <figcaption>Embedded content from an example URL</figcaption>\n</figure>\n"
"innerHTML": "\n<figure class=\"wp-block-embed\">\n <div class=\"wp-block-embed__wrapper\">\n https://example.com/\n </div>\n <figcaption>Embedded content from an example URL</figcaption>\n</figure>\n"
},
{
"attrs": {},
Expand Down
4 changes: 2 additions & 2 deletions test/integration/full-content/fixtures/core__embed.serialized.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<!-- wp:embed {"url":"https://example.com/"} -->
<figure class="wp-block-embed">
<figure class="wp-block-embed"><div class="wp-block-embed__wrapper">
https://example.com/
<figcaption>Embedded content from an example URL</figcaption></figure>
</div><figcaption>Embedded content from an example URL</figcaption></figure>
<!-- /wp:embed -->