Remove margin from sandbox iframe to fix size calculation by westonruter · Pull Request #1688 · WordPress/gutenberg

westonruter · 2017-07-04T06:52:53Z

The sandboxed iframe was not getting resized enough to account for the margins of the body inside the iframed window. The result, as I'm working on adding sandboxing to the Custom HTML block (#1625), is the content being cut off:

With the fix, it looks like:

Amends #1392

…count for margins

notnownikki

I'm not sure this calculates correctly. I'm not sure the original does either, I did notice some cut-off even on twitter embeds...

This is how a twitter embed should look:

but changing to use scrollWidth and scrollHeight results in this:

The problem here is that it appears that the extra space can be clicked in to edit the caption, but it can't be. You have to click in a very specific area that is not visually delimited, as it clearly is in the first picture (the end of the tweet is the end of the embedded html, below it immediately is the caption).

The list of embeds we've been using to test is:

Short tweet:
https://twitter.com/notnownikki/status/876229494465581056

Tall tweet:
https://twitter.com/PattyJenks/status/874034832430424065

Youtube:
https://www.youtube.com/watch?v=PfKUdmTq2MI

Photo:
https://cloudup.com/cl3Oq5MU8Rm

Long tumblr post:
http://doctorwho.tumblr.com/post/162052108791

Perhaps we can add the margin sizes? Or in the document we create for the iframe, reset all margins?

aduth · 2017-07-05T21:10:26Z

In an alternate implementation of this behavior, we'd switched from offsetWidth / offsetHeight to getBoundingClientRect, which is documented as being more accurate than offset dimensions. At a glance it seems it could only be inaccurate by as much as a single pixel, but might be worth exploring as better accounting for margins.

…ffsets

westonruter · 2017-07-11T00:59:00Z

@aduth @notnownikki Using getBoundingClientRect to compute the width and height, combined with the computed style to obtain the margin dimensions, seems to do the trick.

…ensions

westonruter · 2017-07-11T01:17:12Z

A couple other fixes related to the sandbox component:

The width and height HTML attributes are integers, so 0b21a93 switches to specifying the dimensions using CSS so that the decimal parts won't be truncated.
I noticed that the top window was being used instead of the parent window. While this isn't causing a problem right now, it could eventually cause problems if the editor is embedded inside of a window that is itself inside of another frame. See 2f7f4cc.

westonruter · 2017-07-12T15:43:21Z

@notnownikki how's this look for you?

notnownikki · 2017-07-12T15:50:28Z

Looking better, but seems to have broken photo embeds from cloudup. The long tweet, short tweet, youtube, and long tumblr embed are all pretty much pixel perfect now, but the test embed from cloudup ( https://cloudup.com/cl3Oq5MU8Rm ) doesn't show anything.

notnownikki · 2017-07-12T16:07:42Z

Ah, I think I see the problem with photo embeds.

Because some sites return the full size photo for a thumbnail, we set the width of photos to 100%, so they just fill the block and get the right height automatically. But, this change adds style, which sets the width and height to whatever's in the state, which by default is 0. That means the image ends up 0px wide and 0px tall, and gets hidden completely.

notnownikki · 2017-07-12T16:21:25Z

Ok, I've just tested with removing the style and going back to width and height in the iframe, and using the following for the dimensions calculation in sendResize:

						width: document.body.offsetWidth + parseFloat( computedStyle.marginLeft ) + parseFloat( computedStyle.marginRight ),
						height: document.body.offsetHeight + parseFloat( computedStyle.marginTop ) + parseFloat( computedStyle.marginBottom )

... and things look good for all the embeds and the Custom HTML block.

westonruter · 2017-07-12T18:29:04Z

@notnownikki Very strange. I don't see why using style on the iframe as opposed to using width/height attributes would be any different. Nevertheless, I can see switching back to HTML attributes does fix the problem. I did retain the getClientBoundingRect part, however, so that we can obtain the fractional dimensions and then ceil that value for the width/height to ensure nothing is cropped, even by 1px.

notnownikki

Works with everything I threw at it, including the custom html!

westonruter · 2017-07-12T18:51:16Z

@notnownikki is there a reason why margin:0 is not being applied to the iframe's body? This also came up in #1658. If we added it, then there would be no need to get the computed style of the body in the first place to obtain the margins.

westonruter · 2017-07-12T18:51:45Z

Also, I found a core bug with the oEmbed proxy endpoint in core where the maxwidth param is not getting passed along properly. This is partly why the YouTube video appears cut off on the right.

notnownikki · 2017-07-12T19:00:54Z

@westonruter Looking at #1658, yeah, we should zero the margins. We generate the HTML document for the iframe in trySandbox so just adding the CSS to the <head> seems ok to me.

westonruter · 2017-07-12T19:10:23Z

Cool, I added it to body[style] since inline styles are also being set with JS.

notnownikki · 2017-07-12T19:13:38Z

Works great with everything I threw at it again! Happy to see this 🚢

Use body.scrollWidth/scrollHeight instead of offsetWidth/Height to ac…

8074a41

…count for margins

westonruter requested a review from notnownikki July 4, 2017 06:52

westonruter mentioned this pull request Jul 4, 2017

Make videos responsive #1658

Closed

westonruter force-pushed the fix/sandbox-dimension-calculation branch from 980b2e0 to 8074a41 Compare July 4, 2017 06:55

westonruter mentioned this pull request Jul 4, 2017

Add Custom HTML block description and use sandbox for preview #1625

Closed

westonruter changed the title ~~Use scrollWidth & scrollHeight instead of offsetWidth & Height to account for margins~~ Use scrollWidth & scrollHeight instead of offsetWidth & offsetHeight to account for margins Jul 4, 2017

notnownikki suggested changes Jul 4, 2017

View reviewed changes

mtias added the [Status] In Progress Tracking issues with work in progress label Jul 9, 2017

Use bounding client rect and computed margin styles instead of body o…

cf67b68

…ffsets

westonruter removed the [Status] In Progress Tracking issues with work in progress label Jul 11, 2017

westonruter added 2 commits July 10, 2017 18:08

Populate sandbox width and height via CSS to honor floating point dim…

0b21a93

…ensions

Use window.parent instead of window.top for sending resize messages

2f7f4cc

Use iframe width/height attributes instead of style props

f156566

notnownikki approved these changes Jul 12, 2017

View reviewed changes

Set margin:0 on sandbox body; remove margin computing

a394e9a

westonruter force-pushed the fix/sandbox-dimension-calculation branch from 487ea4d to a394e9a Compare July 12, 2017 19:27

westonruter changed the title ~~Use scrollWidth & scrollHeight instead of offsetWidth & offsetHeight to account for margins~~ Remove margin from sandbox iframe to fix size calculation Jul 12, 2017

westonruter merged commit 78e2233 into master Jul 12, 2017

westonruter deleted the fix/sandbox-dimension-calculation branch July 12, 2017 19:34

Conversation

westonruter commented Jul 4, 2017

Uh oh!

notnownikki left a comment

Choose a reason for hiding this comment

Uh oh!

aduth commented Jul 5, 2017

Uh oh!

westonruter commented Jul 11, 2017

Uh oh!

westonruter commented Jul 11, 2017

Uh oh!

westonruter commented Jul 12, 2017

Uh oh!

notnownikki commented Jul 12, 2017

Uh oh!

notnownikki commented Jul 12, 2017

Uh oh!

notnownikki commented Jul 12, 2017

Uh oh!

westonruter commented Jul 12, 2017

Uh oh!

notnownikki left a comment

Choose a reason for hiding this comment

Uh oh!

westonruter commented Jul 12, 2017

Uh oh!

westonruter commented Jul 12, 2017

Uh oh!

notnownikki commented Jul 12, 2017 • edited by westonruter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

westonruter commented Jul 12, 2017

Uh oh!

notnownikki commented Jul 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

notnownikki commented Jul 12, 2017 •

edited by westonruter

Loading