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

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

Merged
merged 22 commits into from Apr 4, 2018

Conversation

Projects
7 participants
@kienstra
Copy link
Collaborator

kienstra commented Feb 6, 2018

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.

kienstra added some commits Feb 6, 2018

Issue #864: Remove 'sizes' attribute for <amp-iframe> and <amp-video>.
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().
I had copied this line into the block above.
But it's not needed there.
$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' );
}

This comment has been minimized.

@kienstra

kienstra Feb 6, 2018

Collaborator

These 3 lines are copied from enforce_sizes_attribute().

@kienstra kienstra requested a review from westonruter Feb 6, 2018

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

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Feb 8, 2018

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

This comment has been minimized.

Copy link
Member

westonruter commented Feb 9, 2018

Would this potentially solve #919 as well?

@pbakaus

This comment has been minimized.

Copy link
Collaborator

pbakaus commented Feb 10, 2018

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

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Feb 10, 2018

@pbakaus Thank you for clarifying.

@kienstra Please remove sizes for images as well.

Issue #864: Remove the 'sizes' attribute from <amp-img>.
On Paul Baukus and Weston Ruter's suggestion.
The sizes attribute isn't relevant to this WP plugin,
as Paul Baukus mentioned.
@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Feb 13, 2018

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.

kienstra added some commits Feb 14, 2018

Issue #864: Remove layout="responsive" from <amp-iframe>.
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

This comment has been minimized.

Copy link
Collaborator

kienstra commented Feb 14, 2018

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:

amp-video-no-sizes

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>.

Issue #864: Add comment after addition of 'style.'
This was present earlier.
Weston added this, and it describes where it will appear.
@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Feb 14, 2018

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

See #933.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Feb 14, 2018

Issue #864: Remove <amp-img> sizes attribute if present.
An earlier commit only prevented adding it.
This removes the attribute it it's present.
@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Feb 15, 2018

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

This comment has been minimized.

Copy link
Collaborator

kienstra commented Feb 15, 2018

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.

Issue #864: Add layout="responsive" to <amp-iframe>.
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

This comment has been minimized.

Copy link
Member

westonruter commented Feb 19, 2018

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

This comment has been minimized.

@delawski

delawski Mar 1, 2018

@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).

This comment has been minimized.

@kienstra

kienstra Mar 1, 2018

Collaborator

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

This comment has been minimized.

@westonruter

westonruter Mar 1, 2018

Member

@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.

This comment has been minimized.

@kienstra

kienstra Mar 1, 2018

Collaborator

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.

kienstra added some commits Mar 1, 2018

Issue #864: Convert 'data-amp-layout' to 'layout.'
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.'
Add this attribute to the allowed list for <img>.
The sanitizer now converts this to 'layout.'
public static function add_layout( $context, $context_type ) {
if ( 'post' !== $context_type ) {
return $context;
}

This comment has been minimized.

@kienstra

kienstra Mar 1, 2018

Collaborator

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

This comment has been minimized.

@westonruter

westonruter Mar 1, 2018

Member

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

This comment has been minimized.

@kienstra

kienstra Mar 1, 2018

Collaborator

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

);
return $context;
}

This comment has been minimized.

@westonruter

westonruter Mar 1, 2018

Member

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

This comment has been minimized.

@kienstra

kienstra Mar 1, 2018

Collaborator

Thanks, how about AMP_HTML_Utils?

This comment has been minimized.

@westonruter

westonruter Mar 1, 2018

Member

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.

This comment has been minimized.

@kienstra

kienstra Mar 1, 2018

Collaborator

Thanks, I'll move it to AMP_Theme_Support.

This comment has been minimized.

@kienstra

kienstra Mar 1, 2018

Collaborator

This commit moves add_layout() to AMP_Theme_Support.

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Mar 1, 2018

Will Resolve Merge Conflicts

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

@kienstra kienstra changed the base branch from develop to 0.7 Mar 29, 2018

mehigh and others added some commits Mar 29, 2018

Merge in 0.7, resolve conflicts.
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.
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.
In response to Travis errors.
Use parentheses in class instantiation.
Set default layout of <amp-iframe> and <amp-video> to intrinsic.
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

This comment has been minimized.

Copy link
Collaborator

kienstra commented Mar 29, 2018

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:

layout-intrinsic

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

This comment has been minimized.

Copy link
Collaborator

mehigh commented Mar 31, 2018

@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

mehigh approved these changes Mar 31, 2018

Copy link
Collaborator

mehigh left a comment

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

Remove my duplicate layout assignment in AMP_Video_Sanitizer.
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

This comment has been minimized.

Copy link
Collaborator

kienstra commented Mar 31, 2018

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".

Conditionally set layout to 'responsive' for <amp-video>.
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 kienstra added this to the v0.7 milestone Mar 31, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Apr 2, 2018

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

This comment has been minimized.

Copy link
Collaborator

mehigh commented Apr 2, 2018

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

@kienstra kienstra removed their assignment Apr 2, 2018

Merge in 0.7 branch, resolve conflict.
The conflict was in:
test-amp-img-sanitizer.php.
Both branches added a test in the same place.
So retain both edits.
@westonruter
Copy link
Member

westonruter left a comment

Let's get this in testing!

@westonruter westonruter merged commit fb65a9a into 0.7 Apr 4, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the fix/864-remove-sizes branch Apr 4, 2018

@westonruter 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 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

@westonruter westonruter added this to Ready for Review in v0.7 Apr 4, 2018

@westonruter westonruter moved this from Ready for Review to QA in v0.7 Apr 4, 2018

@MackenzieHartung MackenzieHartung moved this from QA to Ready for Merging in v0.7 Apr 18, 2018

@kevincoleman kevincoleman moved this from Ready for Merging to Production Release in v0.7 May 8, 2018

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