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

Propagate width/height styles as width/height attributes when setting layout #6662

Merged
merged 4 commits into from Dec 3, 2021

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Oct 26, 2021

Summary

Addresses issue raised in support topic.

The AMP_Base_Sanitizer::set_layout() method was extracting width/height styles, but it was only considering the 100% value case. This ensures that other values are also propagated to the resulting width/height attributes on the AMP element.

Given this markup:

<iframe style="width: 100%; height: 200px;" frameborder="no" scrolling="no" src="https://player.captivate.fm/episode/495621d8-6f05-4d95-bbaa-369a9a6189e1"></iframe>

Before:

<amp-iframe
		style="width: 100%; height: 200px;"
		frameborder="0"
		scrolling="no"
		src="https://player.captivate.fm/episode/495621d8-6f05-4d95-bbaa-369a9a6189e1" 
		height="400"
		layout="fixed-height" 
		width="auto"
		sandbox="allow-downloads allow-forms allow-modals allow-orientation-lock allow-pointer-lock allow-popups allow-popups-to-escape-sandbox allow-presentation allow-same-origin allow-scripts allow-top-navigation-by-user-activation">
</amp-iframe>

After:

<amp-iframe 
		style="width: 100%; height: 200px;"
		frameborder="0"
		scrolling="no" 
		src="https://player.captivate.fm/episode/495621d8-6f05-4d95-bbaa-369a9a6189e1" 
		height="200"
		width="auto" 
		layout="fixed-height" 
		sandbox="allow-downloads allow-forms allow-modals allow-orientation-lock allow-pointer-lock allow-popups allow-popups-to-escape-sandbox allow-presentation allow-same-origin allow-scripts allow-top-navigation-by-user-activation">
</amp-iframe>

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@westonruter westonruter added this to the v2.2 milestone Oct 26, 2021
@westonruter westonruter added this to In Progress in Ongoing Oct 26, 2021
@westonruter westonruter marked this pull request as ready for review October 26, 2021 22:28
@github-actions
Copy link
Contributor

github-actions bot commented Oct 26, 2021

Plugin builds for 7f04164 are ready 🛎️!

@westonruter westonruter moved this from In Progress to Ready for Review in Ongoing Oct 26, 2021
@westonruter
Copy link
Member Author

Rebasing against develop.

@westonruter westonruter force-pushed the fix/iframe-width-and-height-style-detection branch from 95fcdb5 to 7f04164 Compare December 2, 2021 23:23
@dhaval-parekh
Copy link
Collaborator

Change look good to me 👍

@westonruter westonruter merged commit 451c588 into develop Dec 3, 2021
@westonruter westonruter deleted the fix/iframe-width-and-height-style-detection branch December 3, 2021 19:04
@westonruter westonruter moved this from Ready for Review to Ready for QA in Ongoing Dec 3, 2021
@bartoszgadomski
Copy link
Contributor

QA passed

I tested this with three iframes:

Original styles Attributes Screenshot
width:100% height:200px width="auto" height="200 Screenshot 2021-12-8 at 22 11 50
width:25vh height:50vh width="25vh" height="50vh" Screenshot 2021-12-8 at 22 13 16
width:123pt height:50pt width="auto" height="400" Screenshot 2021-12-8 at 22 13 58

@bartoszgadomski bartoszgadomski moved this from Ready for QA to QA Passed in Ongoing Dec 8, 2021
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. Sandboxing Experiment Sanitizers
Projects
Ongoing
  
QA Passed
Development

Successfully merging this pull request may close these issues.

None yet

3 participants