Remove 'sizes' attribute for <amp-iframe> and <amp-video>; add appropriate layout #937

kienstra · 2018-02-06T21:58:28Z

Request For Code Review

Hi @westonruter,
Could you please review this pull request, which removes the sizes attribute for <amp-iframe> and <amp-video>?

Before, there was a workaround to ensure these didn't overflow. I had added style="max-width:100%"
This removes that workaround, and instead sets the layout to 'responsive.'

We don't need to remove sizes from <amp-img>, right? The discussion in PR #921 only briefly mentioned <amp-img>, though the referenced amphtml issue # 11575 discusses <amp-img>.

Update
Sorry, Weston, I might have missed the real need of removing sizes. It sounds like the main need was removing them from images. If there's still a need to remove them from images, I'd be happy to do that.

Before, there was a workaround to ensure these didn't overflow. I added style="max-width:1005" This removes that workaround, And instead sets the layout to 'responsive.' Also, this updates the PHPUnit tests.

I had copied this line into the block above. But it's not needed there.

kienstra · 2018-02-06T21:59:56Z

includes/sanitizers/class-amp-iframe-sanitizer.php

+			$new_attributes = $this->set_layout( $new_attributes );
+			if ( isset( $new_attributes['width'] ) && isset( $new_attributes['height'] ) ) {
+				$this->add_or_append_attribute( $new_attributes, 'class', 'amp-wp-enforced-sizes' );
+			}


These 3 lines are copied from enforce_sizes_attribute().

westonruter · 2018-02-08T21:56:19Z

I'm not clear on whether removing sizes is what we're supposed to do, and whether it is just for amp-iframe or amp-img. I'm waiting on confirmation for #921 (comment) I think. I'm honestly a bit lost. In any case, I've deployed your changes up to a new multidev environment at https://sizesfix-ampconfdemo.pantheonsite.io/

Would you get some QA support for check if the changes help or hinder?

westonruter · 2018-02-09T07:27:26Z

Would this potentially solve #919 as well?

pbakaus · 2018-02-10T05:06:36Z

Commented on the other one – for the reasons outlined, I'd definitely recommend removing.

westonruter · 2018-02-10T06:53:05Z

@pbakaus Thank you for clarifying.

@kienstra Please remove sizes for images as well.

On Paul Baukus and Weston Ruter's suggestion. The sizes attribute isn't relevant to this WP plugin, as Paul Baukus mentioned.

kienstra · 2018-02-13T04:54:38Z

Thanks, Sizes Removed From Images

Hi @pbakaus and @westonruter,
Thanks for your guidance on the sizes attribute. The commit above removes them from <amp-img> elements.

@westonruter, I still need to set up tests for the <amp-iframe>, using the test environment that you created.

This could lead to unexpected results. If the intended width or height is less than the container, This will increase it to fill the container. This fixed the issue of overflowing <ampiframe> in widgets. But it could create an issue in other places.

kienstra · 2018-02-14T04:45:34Z

amp-video, amp-iframe

Hi @westonruter,
The <amp-video> looks to display well with no sizes, and with layout="responsive". This ensures that it doesn't overflow its container, like a sidebar.

It also doesn't look to expand larger than its width or height, so users shouldn't have an unexpected change with this version:

But the <amp-iframe> does expand to fill its container when it has layout="responsive". This might cause surprises with the new version.

So I removed my addition of layout="responsive" to the <amp-iframe>, and instead re-added style="max-width:100%".

This is just a workaround, and it might help to eventually have .amp-wp-enforced-sizes in the <style amp-custom>.

This was present earlier. Weston added this, and it describes where it will appear.

westonruter · 2018-02-14T06:52:41Z

This is just a workaround, and it might help to eventually have .amp-wp-enforced-sizes in the <style amp-custom>.

See #933.

westonruter · 2018-02-14T07:20:42Z

I've deployed the changes to https://sizesfix-ampconfdemo.pantheonsite.io/

Question: Should this eliminate the need for https://github.com/xwp/ampconf/blob/992fa40c75b9be898326b186a4c3efd0dc788325/inc/template-functions.php#L72-L112 ?

An earlier commit only prevented adding it. This removes the attribute it it's present.

kienstra · 2018-02-15T00:20:53Z

New Commit

Hi @westonruter,
Thanks for deploying the changes. This new commit removes the sizes attribute, where this previous commit only prevented it from being added if it wasn't present.

As you probably know, the filters you mentioned override the removal of the sizes. When they're present, this plugin's removal of the sizes doesn't apply.

Still, I need to look at the underlying cause causes of those filters, and whether it's appropriate to remove them.

kienstra · 2018-02-15T04:01:23Z

PR To Remove Workaround

Hi @westonruter,
The theme PR #81 removes the workaround filters you mentioned. In testing it, I noticed an issue in theme styling, and described it on the ticket.

Iframes from WordPress embeds usually have width and height. In AMP, the inferred value layout for this is fixed. This means that even if you apply max-width:100%, The height won't adjust to the new ratio. Setting the 'layout' to 'responsive' seems to be the only way to ensure the same aspect ratio. Of course, this has a risk that iframes will expand to fill their container, where they didn't before.

westonruter · 2018-02-19T19:27:26Z

Deployed latest to https://sizesfix-ampconfdemo.pantheonsite.io/

delawski · 2018-03-01T11:41:12Z

includes/sanitizers/class-amp-img-sanitizer.php

@@ -119,7 +119,6 @@ private function filter_attributes( $attributes ) {
 				case 'alt':
 				case 'class':
 				case 'srcset':
-				case 'sizes':


@kienstra We need to add layout attribute here so that it's possible to control the image sizing at the theme's level (see my comment in the AMPConf theme).

That sounds good, @delawski. If it's alright, I'll work on this in several hours.

@delawski @kienstra Adding a layout to an HTML img element makes me nervous. I think it would be better actually to introduce an data-amp-layout attribute which the sanitizer could convert into a layout attribute when converting an img into an amp-img. This would allow for regular plain HTML in post content to be decorated with the data-amp-layout attribute and for the content to be still served as valid HTML. This will also allow for such img[data-amp-layout] to be styled with CSS in non-AMP responses. This would play nicely into adding AMP support for Gutenberg blocks as described in #902 (comment)

We'll need to allow data-amp-layout attribute to Kses for it to be saved with unfiltered_html.

Thanks, @westonruter. The 2 commits below convert 'data-amp-layout' to 'layout' in the image sanitizer, and allow 'data-amp-layout' in wp_kses(), when in a 'post' context.

In the image preprocessor, Convert a 'data-amp-layout' attribute to 'layout.' As Weston mentioned, this won't change the styling on non-AMP pages, As the preprocessor won't modify them.

Add this attribute to the allowed list for <img>. The sanitizer now converts this to 'layout.'

kienstra · 2018-03-01T21:07:08Z

includes/utils/class-amp-wp-utils.php

+	public static function add_layout( $context, $context_type ) {
+		if ( 'post' !== $context_type ) {
+			return $context;
+		}


Is it alright to only filter the allowed attributes in a 'post' context?

I'd say it should be allowed in any context where an img is allowed with a width and height.

Thanks, this commit allows data-amp-layout whenever an <img> permits width and height.

westonruter · 2018-03-01T21:09:38Z

includes/utils/class-amp-wp-utils.php

+		);
+		return $context;
+	}
+


This shouldn't be added to AMP_WP_Utils since it is marked for deletion in #876.

Thanks, how about AMP_HTML_Utils?

Let's just put it in AMP_Theme_Support for now. It's the only class that is using it, and we'll probably break up that class into separate classes later anyway.

Thanks, I'll move it to AMP_Theme_Support.

This commit moves add_layout() to AMP_Theme_Support.

kienstra · 2018-03-01T21:12:06Z

Will Resolve Merge Conflicts

Next, I'll merge in develop and resolve the conflicts.

kienstra · 2018-03-29T17:24:08Z

@mehigh, thanks a lot for your commit and PR for this! Your screencast of this fix is also really helpful.

There were several conflicts in: class-amp-base-sanitizer.php. Mainly retain those in this branch: fix/864-remove-sizes

Add layout="intrinsic" to some <amp-img> tags. We might later consider using that value for other tags as a default value.

In response to Travis errors. Use parentheses in class instantiation.

Inspired by Mike Cranteas earlier commit, which applied to <amp-img> Before, this set the layout of those elements to responsive if the height and width weren't empty. This does the same thing, only with 'intrinsic.' Initial testing on Native AMP and paired mode shows that this displays as expected. According to the documentation, 'This layout works very well for most AMP elements, including amp-img, amp-video,' @see https://www.ampproject.org/docs/design/responsive/control_layout#what-if-the-layout-attribute-isn%E2%80%99t-specified?

kienstra · 2018-03-29T21:48:49Z

Thanks
Question About Doing Same Thing For Other Elements

Hi @mehigh,
Thanks a lot for your commit to fix the image size issue. If it's alright, I cherry-picked it into this pull request. I had to change the target branch of this PR to 0.7 for the upcoming release.

You already did us a big favor by working on this so quickly, so no pressure if you can't get to this.

But does the commit I made above look reasonable? It does something similar to what you did, but for <amp-video> and <amp-iframe>.

This could prevent the <amp-iframe> from expanding to fill its container. Before this PR, most embed iframes had a height and width, so I think AMP inferred a layout of fixed. And they wouldn't expand to fill their container.

By using layout: intrinsic, it looks like this avoids the iframe filling the container:

To test this, I made this change to AMP_Base_Sanitizer::set_layout() (only for testing):

	public function set_layout( $attributes ) {
+		$attributes['width']  = 300;
+		$attributes['height'] = 300;
		if ( empty( $attributes['height'] ) ) {

And pasted this in the post content:

<h1>Amazon Com Smile Embed</h1>
https://smile.amazon.com/dp/B00DPM7TIG
<h1>Amazon Co Smile Embed</h1>
https://smile.amazon.co.uk/dp/B00DPM7TIG

mehigh · 2018-03-31T09:14:49Z

@kienstra
amp-video with intrinsic works really well and it's even recommended in the docs, so no comments there.

As for iframes, there isn't a one-size-fits-all solution. Some iFrames can be responsive (hence intrinsic works for these types of iframes allowing them to grow and occupy the available space), and some can be fixed, it all depends on what rules are inside of that iFrame, data we can't know from outside.
And knowing that, intrinsic makes more sense than responsive for amp-iframe as well, but there can be scenarios where a fixed layout would make more sense.
That's why we need the iframes to receive a fixed-height layout when no width is present. Think the FB like button (it used to be an iframe eons ago) -> that was not responsive, as the button didn't grow to 400px width if you would have placed it in such a large container.
As long as it's catered for iframes with height-only dimension provided, I'm good with the default being intrinsic.

mehigh

amp-images and amp-video with intrinsic - check
amp-iframes - intrinsic default, needs to fall-back to fixed-height when only height is provided

This already conditionally sets a layout of responsive in set_layout(). And the removed logic sometimes reassigned the layout to responsive. So remove this extra logic.

kienstra · 2018-03-31T19:23:00Z

Thanks!

Hi @mehigh,
Thanks a lot for your comments above about <amp-iframe> and <amp-video>.

That's why we need the iframes to receive a fixed-height layout when no width is present.

Thanks, that helps a lot to clarify this. This line on this PR ensures that amp-iframes with no width have layout="fixed-height".

This used to be a layout of 'intrinsic.' Strangely, the layout documentation says about 'intrinsic': 'This layout works very well for most of AMP elements, including amp-img, amp-video' @see https://www.ampproject.org/docs/design/responsive/control_layout But the spec for <amp-video> doesn't allow layout="intrinsic". So use layout=responsive for <amp-video> Still use layout=intrinsic for <amp-iframe>

kienstra · 2018-04-02T00:03:47Z

Request For Final Review

Hi @westonruter,
It's a holiday weekend, so there's no hurry to review this. But when you have a chance, you please give this a final review?

@mehigh's comments and commit helped a lot to clarify this PR.

Also, this existing assertion ensures that amp-iframes with no width have layout="fixed-height".

The snipped that I posted which turned out to not work was luckily never part of the diff.

It turns out that my idea of using layout=“intrinsic” for <amp-video> also doesn’t work, as the specification doesn’t allow this. I reverted it.

Strangely, this document encourages using intrinsic for <amp-video>:

This layout works very well for most AMP elements, including amp-img, amp-video, etc.

Using layout=“responsive” for <amp-video> looks to preserve the size, which is good. For example, this video doesn’t fill its container, and retains its size:

[video src=https://videos.files.wordpress.com/DK5mLrbr/video-ca6dc0ab4a_hd.mp4 width=400 height=300]

Of course, we'll also validate all of this in the 3 testing environments for 0.7.

mehigh · 2018-04-02T05:35:36Z

The mixed messages around intrinsic being viable or not for amp-video is odd.

The conflict was in: test-amp-img-sanitizer.php. Both branches added a test in the same place. So retain both edits.

westonruter

Let's get this in testing!

Ryan Kienstra added 2 commits February 6, 2018 15:30

Issue #864: Remove 'sizes' attribute for <amp-iframe> and <amp-video>.

87e8501

Before, there was a workaround to ensure these didn't overflow. I added style="max-width:1005" This removes that workaround, And instead sets the layout to 'responsive.' Also, this updates the PHPUnit tests.

Issue #864: Remove extra line in set_layout().

936f302

I had copied this line into the block above. But it's not needed there.

kienstra commented Feb 6, 2018

View reviewed changes

kienstra requested a review from westonruter February 6, 2018 22:00

kienstra assigned westonruter Feb 6, 2018

westonruter added this to the v0.7 milestone Feb 7, 2018

Issue #864: Remove the 'sizes' attribute from <amp-img>.

6848541

On Paul Baukus and Weston Ruter's suggestion. The sizes attribute isn't relevant to this WP plugin, as Paul Baukus mentioned.

Ryan Kienstra added 2 commits February 13, 2018 19:49

Merge branch 'develop' into fix/864-remove-sizes

9348ed3

Issue #864: Add comment after addition of 'style.'

f95c0be

This was present earlier. Weston added this, and it describes where it will appear.

Issue #864: Remove <amp-img> sizes attribute if present.

6bd4326

An earlier commit only prevented adding it. This removes the attribute it it's present.

kienstra mentioned this pull request Feb 15, 2018

Plugin Issue #864: Remove <amp-img> workaround filters. xwp/ampnews#81

Merged

delawski reviewed Mar 1, 2018

View reviewed changes

Ryan Kienstra added 2 commits March 1, 2018 13:53

Issue #864: Convert 'data-amp-layout' to 'layout.'

878e023

In the image preprocessor, Convert a 'data-amp-layout' attribute to 'layout.' As Weston mentioned, this won't change the styling on non-AMP pages, As the preprocessor won't modify them.

Issue #864: Allow 'data-amp-layout' in wp_kses() for 'post.'

037f6ef

Add this attribute to the allowed list for <img>. The sanitizer now converts this to 'layout.'

kienstra commented Mar 1, 2018

View reviewed changes

westonruter reviewed Mar 1, 2018

View reviewed changes

kienstra changed the base branch from develop to 0.7 March 29, 2018 17:45

mehigh and others added 5 commits March 29, 2018 11:47

864 - Use intrinsic as default layout for images

adce308

Merge in 0.7, resolve conflicts.

81e20c5

There were several conflicts in: class-amp-base-sanitizer.php. Mainly retain those in this branch: fix/864-remove-sizes

Update unit tests for layout.

bc5c407

Add layout="intrinsic" to some <amp-img> tags. We might later consider using that value for other tags as a default value.

Align equals signs, use () after class.

ece0f86

In response to Travis errors. Use parentheses in class instantiation.

mehigh approved these changes Mar 31, 2018

View reviewed changes

Remove my duplicate layout assignment in AMP_Video_Sanitizer.

f983029

This already conditionally sets a layout of responsive in set_layout(). And the removed logic sometimes reassigned the layout to responsive. So remove this extra logic.

kienstra added this to the v0.7 milestone Mar 31, 2018

kienstra removed their assignment Apr 2, 2018

Merge in 0.7 branch, resolve conflict.

c347f2b

The conflict was in: test-amp-img-sanitizer.php. Both branches added a test in the same place. So retain both edits.

westonruter approved these changes Apr 4, 2018

View reviewed changes

westonruter merged commit fb65a9a into 0.7 Apr 4, 2018

westonruter deleted the fix/864-remove-sizes branch April 4, 2018 05:51

westonruter changed the title ~~Issue #864: Remove 'sizes' attribute for <amp-iframe> and <amp-video>.~~ Issue #864: Remove 'sizes' attribute for <amp-iframe> and <amp-video>; add appropriate layout Apr 4, 2018

westonruter mentioned this pull request Apr 4, 2018

Sanitizer: Request to convert <img> to <amp-img> correctly #919

Closed

westonruter changed the title ~~Issue #864: Remove 'sizes' attribute for <amp-iframe> and <amp-video>; add appropriate layout~~ Remove 'sizes' attribute for <amp-iframe> and <amp-video>; add appropriate layout Apr 4, 2018

kienstra mentioned this pull request Apr 30, 2018

Images unexpectedly expand to fill containers #1104

Closed

westonruter mentioned this pull request May 11, 2018

How to make small images stretch to full width #1137

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove 'sizes' attribute for <amp-iframe> and <amp-video>; add appropriate layout #937

Remove 'sizes' attribute for <amp-iframe> and <amp-video>; add appropriate layout #937

kienstra commented Feb 6, 2018 •

edited

Loading

kienstra Feb 6, 2018

westonruter commented Feb 8, 2018

westonruter commented Feb 9, 2018

pbakaus commented Feb 10, 2018

westonruter commented Feb 10, 2018

kienstra commented Feb 13, 2018 •

edited

Loading

kienstra commented Feb 14, 2018

westonruter commented Feb 14, 2018

westonruter commented Feb 14, 2018

kienstra commented Feb 15, 2018 •

edited

Loading

kienstra commented Feb 15, 2018 •

edited

Loading

westonruter commented Feb 19, 2018

delawski Mar 1, 2018

kienstra Mar 1, 2018

westonruter Mar 1, 2018

kienstra Mar 1, 2018

kienstra Mar 1, 2018

westonruter Mar 1, 2018

kienstra Mar 1, 2018

westonruter Mar 1, 2018

kienstra Mar 1, 2018

westonruter Mar 1, 2018

kienstra Mar 1, 2018

kienstra Mar 1, 2018

kienstra commented Mar 1, 2018

kienstra commented Mar 29, 2018 •

edited

Loading

kienstra commented Mar 29, 2018 •

edited

Loading

mehigh commented Mar 31, 2018

mehigh left a comment

kienstra commented Mar 31, 2018 •

edited

Loading

kienstra commented Apr 2, 2018 •

edited

Loading

mehigh commented Apr 2, 2018

westonruter left a comment

Remove 'sizes' attribute for <amp-iframe> and <amp-video>; add appropriate layout #937

Remove 'sizes' attribute for <amp-iframe> and <amp-video>; add appropriate layout #937

Conversation

kienstra commented Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

westonruter commented Feb 8, 2018

westonruter commented Feb 9, 2018

pbakaus commented Feb 10, 2018

westonruter commented Feb 10, 2018

kienstra commented Feb 13, 2018 • edited Loading

kienstra commented Feb 14, 2018

westonruter commented Feb 14, 2018

westonruter commented Feb 14, 2018

kienstra commented Feb 15, 2018 • edited Loading

kienstra commented Feb 15, 2018 • edited Loading

westonruter commented Feb 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra commented Mar 1, 2018

kienstra commented Mar 29, 2018 • edited Loading

kienstra commented Mar 29, 2018 • edited Loading

mehigh commented Mar 31, 2018

mehigh left a comment

Choose a reason for hiding this comment

kienstra commented Mar 31, 2018 • edited Loading

kienstra commented Apr 2, 2018 • edited Loading

mehigh commented Apr 2, 2018

westonruter left a comment

Choose a reason for hiding this comment

kienstra commented Feb 6, 2018 •

edited

Loading

kienstra commented Feb 13, 2018 •

edited

Loading

kienstra commented Feb 15, 2018 •

edited

Loading

kienstra commented Feb 15, 2018 •

edited

Loading

kienstra commented Mar 29, 2018 •

edited

Loading

kienstra commented Mar 29, 2018 •

edited

Loading

kienstra commented Mar 31, 2018 •

edited

Loading

kienstra commented Apr 2, 2018 •

edited

Loading