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.

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

<iframe> height="100%" stripped to empty value throwing validation error #3277

Closed
claudiopome opened this issue Sep 17, 2019 · 4 comments · Fixed by #3295
Closed

<iframe> height="100%" stripped to empty value throwing validation error #3277

claudiopome opened this issue Sep 17, 2019 · 4 comments · Fixed by #3295
Milestone

Comments

@claudiopome
Copy link

<iframe src="https://landbot.io/u/H-22078-X2B4IWABAFJE2ROQ/index.html" width="100%" height="100%" frameborder="0">

This code throws an error on the AMP Validator saying

The attribute 'height' in tag 'amp-iframe' is set to the invalid value ''.

I checked #402 and it seems that % values on height attributes get stripped to empty by default? Am I missing something here?

@swissspidy swissspidy added Support Help with setup, implementation, or "How do I?" questions. Sanitizers labels Sep 17, 2019
@westonruter
Copy link
Member

Percentage dimensions are not valid in AMP. The AMP plugin is currently passing through invalid units: #2146.

Nevertheless, there is currently auto-conversion of with=100% to a layout=fixed-height:

if ( empty( $attributes['width'] ) || '100%' === $attributes['width'] ) {
$attributes['layout'] = 'fixed-height';
$attributes['width'] = 'auto';
}

The question remains as to whether we can add some similar conversion for height=100%. This actually was done in #3209. Is this a similar scenario? Is your iframe absolutely positioned to stretch to its container's dimensions? If so, then that PR could be followed up with doing the same logic if an element has width=100% and height=100%.

@claudiopome
Copy link
Author

claudiopome commented Sep 18, 2019

Hi @westonruter,

Thank you for your reply.

Nevertheless, there is currently auto-conversion of width=100% to a layout=fixed-height:

Indeed. That's exactly what happens, layout gets converted into fixed-height but the height value is empty

Is your iframe absolutely positioned to stretch to its container's dimensions?

Yes, There's a container div with a width="100%" and height="500px" Here's is the actual code:

<div style="width: 100%; height: 500px; position: relative; overflow: hidden;">
    <iframe style="position: absolute; left: 0; right: 0; bottom: 0; top: 0; border: 0;" src="https://landbot.io/u/H-22078-X2B4IWABAFJE2ROQ/index.html" width="100%" height="100%" frameborder="0"></iframe>
</div>

@ethanclevenger91
Copy link
Contributor

ethanclevenger91 commented Sep 18, 2019

Same thing here, specifically in the case of responsive embeds. A very common implementation is probably Bootstrap's. Glad to see it's being addressed. Original embed code in case it helps the PR:

<div style="padding-top: 56.25%; position: relative;">
    <iframe style="position: absolute; top: 0; right: 0; bottom: 0; left: 0;" src="..." scrolling="no" allowfullscreen="allowfullscreen" data-embed="true" width="100%" height="100%" frameborder="0"></iframe>
</div>

@westonruter westonruter removed the Support Help with setup, implementation, or "How do I?" questions. label Sep 18, 2019
@westonruter westonruter added this to the v1.3 milestone Sep 18, 2019
@westonruter
Copy link
Member

Please test fix in #3295.

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

Successfully merging a pull request may close this issue.

4 participants