Issue #864: Fix 'Video' widget for YouTube and Vimeo, simplify 'Gallery' solution #921

kienstra · 2018-01-31T08:37:38Z

Request For Code Review

Hi @westonruter,
Could you please review this pull request for #864? It uses your suggestions to:

Fix YouTube and Vimeo 'Video' widgets with the filter wp_video_shortcode_override
Simplify the 'Gallery' widget, using the filter post_gallery

Using the post_gallery filter, like you suggested, enables deleting the subclass for the 'Gallery' widget.

Of course, another plugin is free to override the behavior here by either adding a new shortcode callback for gallery, or filtering post_gallery later.

These widgets have different logic in: WP_Widget_Media_Video::render() So override the value of wp_video_shortcode(), which renders these. Produce 'youtube' or 'vimeo' shortcodes. And this plugin's handlers will create <amp-youtube> and <amp-vimeo>. Another plugin is also free to override those shortcodes.

Similarly to how the other widget overrides work. Also, adjust tests for this.

This will allow another plugin to override this. Inspired by the Jetpack plugin's return of the initial value.

Thanks to Weston for mentioning how we could use 'post_gallery.' Using this removes the need to subclass that widget. The widget calls gallery_shortcode(), which has the filter: 'post_gallery' As Weston mentioned, Jetpack filters 'post_gallery'.

kienstra · 2018-01-31T08:41:02Z

includes/embeds/class-amp-gallery-embed.php

-	public function unregister_embed() {
-		remove_shortcode( 'gallery' );
-	}
+	public function unregister_embed() {}


This extends an abstract class, so this method has to be implemented. This isn't ideal, but editing this embed handler enables deleting the entire 'Gallery' widget subclass.

westonruter · 2018-01-31T22:04:28Z

includes/class-amp-theme-support.php

@@ -177,6 +177,7 @@ public static function register_hooks() {
 		add_action( 'wp_head', array( __CLASS__, 'add_amp_custom_style_placeholder' ), 8 ); // Because wp_print_styles() normally happens at 8.
 		add_action( 'wp_head', array( __CLASS__, 'add_amp_component_scripts' ), 10 );
 		add_action( 'wp_head', 'amp_add_generator_metadata', 20 );
+		add_filter( 'wp_video_shortcode_override', array( __CLASS__, 'video_override' ), 10, 2 );


I'm thinking that video_override actually would be better put into either a new video embed class, or else to add the method to the Vimeo and YouTube. This would ensure that Vimeo and YouTube videos used with the video shortcode will work in the old paired mode templates as well. It shouldn't be limited to theme support.

Hi @westonruter,
This commit moves the video_override callback into the YouTube and Vimeo embed classes, like you suggested.

westonruter · 2018-01-31T22:05:41Z

includes/class-amp-theme-support.php

+	 * @param array  $attr The shortcode attributes.
+	 * @return string $markup The markup to output.
+	 */
+	public static function video_override( $html, $attr ) {


In other words, add this method to both the AMP_Vimeo_Embed_Handler and AMP_YouTube_Embed_Handler classes, but in each one only account for the Vimeo or YouTube URLs respectively.

Thanks, @westonruter. This commit from above applies your suggestion.

westonruter · 2018-01-31T22:12:06Z

@kienstra one other thing would be great to fix here if you get a chance. Video widgets that show a regular amp-video get poor aspect ratio:

See also https://core.trac.wordpress.org/ticket/42203

I'm not sure if there is a perfect solution, but at the very least I should think the layout should be responsive and it should get a width/height of an average video on YouTube like 16:9? The markup currently is:

<amp-video class="wp-video-shortcode" controls="" height="400" layout="fixed-height"><source type="video/mp4" src="https://src.wordpress-develop.test/wp-content/uploads/2017/12/VID_20171203_165418.mp4?_=1"><source type="video/mp4" src="https://src.wordpress-develop.test/wp-content/uploads/2017/12/VID_20171203_165418.mp4?_=1"></amp-video>

Maybe fixed-height is wrong?

pbakaus · 2018-02-01T18:19:54Z

fixed-height definitely seems wrong here. It should likely either be fixed, if you want a fixed size one that isn't responsive, or responsive, but responsive needs a specified width and height.

Are there cases were we wouldn't know the aspect ratio? I guess external videos from some random URL? In those cases, I agree with @westonruter, I'd assume a 16/9 ratio and use responsive. The way the <video> tag in HTML works luckily means it won't be stretched, and fill up with blank space.

These are now in the embed classes. Also, create test classes for them.

kienstra · 2018-02-01T23:01:43Z

Aspect Ratio

Hi @westonruter and @pbakaus,
Thanks for pointing out the issue and possible solutions for the <amp-video> aspect ratio. I'll work on that either later today or tomorrow, if that's alright.

westonruter · 2018-02-02T23:54:14Z

@pbakaus:

Are there cases were we wouldn't know the aspect ratio? I guess external videos from some random URL? In those cases, I agree with @westonruter, I'd assume a 16/9 ratio and use responsive. The way the

Yes, the external URLs won't known aspect ratios, unless we try to fetch them to read the dimensions (which we're actually doing for images now).

Otherwise, videos uploaded into the media library do get their dimensions parsed. The wp_get_attachment_metadata() function will return the width and height of the video if it was able to be parsed.

@kienstra These dimensions do get added to the HTML output by WordPress, but the Video widget specifically is stripping them out for the sake of letting MediaElement.js do it. We need to stop that from happening. This is the offending code in the Video and Text widgets:

Once the dimensions are restored, then we'd have the desired aspect ratios. But the videos will at that point be too large for the container. So then I think the main thing left will be to make sure the video gets a max-width:100% style. This could be added as a style attribute by the Video sanitizer since the Style sanitizer could then later move the inline style to be in the amp-custom style element. For this to work, amp_get_content_sanitizers() should be modified to move AMP_Style_Sanitizer be run immediately before AMP_Tag_And_Attribute_Sanitizer. I think it is not ideal as it is right now to be run as the first sanitizer.

When they use 'Media Library' videos, they output <amp-video>. The AMP_Video_Sanitizer added a 'layout' value of 'fixed-height'. The height of 400px didn't look good, as the aspect ratio was wrong. Instead, make video widgets that use <amp-video> use layout=responsive. Detect this by checking that neight 'width' or 'height' are present. inject_video_max_width_style() removes these attributes for the widget. Otherwise, retain the previous logic.

kienstra · 2018-02-03T00:09:06Z

Thanks, Will Apply Suggestion

Hi @westonruter,
Thanks for your suggestion above. I coincidentally pushed a commit at about the same time 😄
But your idea looks to be cleaner.

This reverts commit a330342.

@westonruter

Props @westonruter for the details of how to do this. Overrides the widget WP_Widget_Media_Video, in order to override the callback that strips the height and width. Also, add a style of 'width:100%' to the <amp-video>. This ensures that the widget doesn't overflow the container.

Override the 'Text' widget. Prevent the stripping of <video> width and height attributes. Also, move the addition of the 'style' attribute. Before, it wasn't added.

kienstra · 2018-02-03T02:49:34Z

Applied Suggestions, Question About Issue

Hi @westonruter,
The commits above apply your suggestions for the 'Video' and 'Text' widgets, and they now appear as expected.

There looks to be an issue in the 'Video' widget when using URLs that don't have embed handlers, like:

https://videopress.com/v/DK5mLrbr

These use <amp-iframe> elements, which seem to overflow the container:

Placing this line here fixes the issue with these widgets:

$out['style'] = 'max-width:100%';

This is similar to this fix for the <amp-video>. But this will mean that every <amp-iframe> will have style="max-width:100%". Is that alright?

westonruter · 2018-02-03T07:43:38Z

But this will mean that every will have style="max-width:100%". Is that alright?

Yes, that's OK. Maybe it would be better to work it into some default CSS that gets output, but this should work great for now.

pbakaus · 2018-02-04T19:05:11Z

This all makes sense with the exception of max-width: 100%. In AMP land, this should never be required as soon as the layout attribute is set to responsive. I'm curious – why is that not an option here, or is there something I'm missing?

westonruter · 2018-02-05T05:50:14Z

@pbakaus This doesn't seem to be the case…

The following <amp-iframe> is being sent in the response:

<amp-iframe width="694" height="390" src="https://videopress.com/embed/DK5mLrbr?hd=0" frameborder="0" allowfullscreen="" sandbox="allow-scripts allow-same-origin" sizes="(min-width: 694px) 694px, 100vw" class="amp-wp-enforced-sizes"><div placeholder="" class="amp-wp-iframe-placeholder"></div></amp-iframe>

This is in a container that is ~250 pixels wide. When I open dev tools I see the i-amphtml-layout-responsive class which indicates that it has the implied layout=responsive:

<amp-iframe width="694" height="390" src="https://videopress.com/embed/DK5mLrbr?hd=0" frameborder="0" allowfullscreen="" sandbox="allow-scripts allow-same-origin" sizes="(min-width: 694px) 694px, 100vw" class="amp-wp-enforced-sizes i-amphtml-element i-amphtml-layout-responsive i-amphtml-layout-size-defined i-amphtml-layout" style="width: 694px;"><i-amphtml-sizer style="display: block; padding-top: 56.196%;"></i-amphtml-sizer><div placeholder="" class="amp-wp-iframe-placeholder amp-hidden"></div><i-amphtml-scroll-container class="amp-active"><iframe class="i-amphtml-fill-content" name="amp_iframe0" allowfullscreen="" frameborder="0" sandbox="allow-scripts allow-same-origin" src="https://videopress.com/embed/DK5mLrbr?hd=0#amp=1" style="z-index: 0;"></iframe></i-amphtml-scroll-container></amp-iframe>

Even when I explicitly add layout=responsive, I get the same result, where it is not sized to fit the container:

pbakaus · 2018-02-05T18:41:27Z

@westonruter that sure sounds like a bug on our part (I suspect it is related to our implementation of the sizes attribute, it did some weird stuff when I last checked). @aghassemi, can you take a look?

cvializ · 2018-02-05T22:56:55Z

I looked into this. I'm not 100% sure how we should expect layout="responsive" to behave with sizes, given that sizes is designed to set the width of the element directly, while the responsive layout sizer element is more indirect in setting a sizer element inside the <amp-img> container. I am observing identical behavior with an <img> element with a sizes and srcset, and an <amp-img> element with sizes and srcset. https://ampb.in/#-L4bxLkhTCekphu1gZUc

I think for sizes and the <iframe> here, layout="responsive" causes the height to change with the width according to the aspect ratio, with the sizes evaluating to 100vw so the iframe is supposed to be the full width of the viewport, which has nothing to do with the container it's in. Let me know if I'm misunderstanding anything, my grasp of sizes is not 100%. /cc @aghassemi

pbakaus · 2018-02-05T23:30:32Z

This is an interesting case. The MDN doc for sizes behavior says the following:

The selected source size affects the intrinsic size of the image (the image’s display size if no CSS styling is applied). If the srcset attribute is absent, or contains no values with a width (w) descriptor, then the sizes attribute has no effect.

This means that srcset affects only the size if no CSS overrides it. The layout attribute to me seems comparable to an "explicit" setting of the layout, similar to CSS, so I'm fairly determined that the right behavior should be to prioritize the layout property, and ignore the sizes attribute for styling.

To further clarify, the sizes attribute primary use-case is to tell the user agent what the displayed size will be at a certain browser width. The primary function is not to enforce said display.

@cvializ and @aghassemi, what are your thoughts? Does this make sense? @bpaduch has run into this before when doing a demo and was equally confused by our sizes behavior. I strongly suggest changing it.

aghassemi · 2018-02-05T23:39:08Z

Agree, Dima and I were chatting about this last week, we have decided to remove AMP custom logic from sizes/srcset completely and use native. Issue to track is ampproject/amphtml#11575 which is planned for end of February.

pbakaus · 2018-02-05T23:58:56Z

Great thanks for clarifying. @kienstra and @westonruter, as workaround I propose to temporarily get rid of the sizes attribute here (this should fix the behavior).

westonruter · 2018-02-06T01:07:39Z

@pbakaus ok, interesting. I just noticed that the plugin is adding the sizes attribute:

https://github.com/Automattic/amp-wp/blob/7b88ae0acd450e1658f1248d11319a01e7c5e920/includes/sanitizers/class-amp-iframe-sanitizer.php#L84

https://github.com/Automattic/amp-wp/blob/7b88ae0acd450e1658f1248d11319a01e7c5e920/includes/sanitizers/class-amp-base-sanitizer.php#L219-L253

Some background on this decision comes in ampproject/amphtml#1280 (comment) and #101.

Does this background change your recommendation at all? This change was introduced in 2b0ca8f with the comment:

We want elements to not grow beyond their width and shrink to fill the screen on viewports smaller than their width. ¶ We do this by adding a sizes attribute (when not already set) to
enforce this behavior.

@kienstra I can see actually in this commit that the max-width was actually part of the default stylesheet which we aren't using now in canonical (but apparently we need to incorporate it):

.wp-amp-enforced-sizes {
    /** Our sizes fallback is 100vw, and we have a padding on the container; the max-width here prevents the element from overflowing. **/
    max-width: 100%;
}

pbakaus · 2018-02-10T05:05:59Z

@westonruter sorry for the late response.

The way sizes has been used in the past in the plugin (described in the comment you pointed out) is not spec compliant, and will break with the changes the AMP team is planning for end of Feb.

Sizes, in the future is only telling the browser what the rendered sizes will be, not enforce it. Which means that in terms of actual styling, sizes will be doing absolutely nothing going forward.

Sizes is useful if you know exactly how an image will render – e.g. if you know that at 600px viewport it will render full width, and at 1200px you set up a grid in CSS, so it will render next to two others, you can tell the browser that through the sizes attribute.

The problem is that in WordPress, you only know this precisely if you know the exact CSS used to drive the styling. Thus, I'd say it's safer to remove for now, and bring back once we can parse the entire CSS and make an accurate prediction of the actual rendered size.

This is definitely a job for CSS.

Ryan Kienstra added 5 commits January 31, 2018 00:44

Issue #864: Only override video widget output if is_amp_endpoint().

cb0f071

Similarly to how the other widget overrides work. Also, adjust tests for this.

Issue #864: Align equals signs in response to Travis error.

6608df5

Issue #864: Simply return the initial value, instead of ''.

76cf75f

This will allow another plugin to override this. Inspired by the Jetpack plugin's return of the initial value.

kienstra commented Jan 31, 2018

View reviewed changes

kienstra requested a review from westonruter January 31, 2018 08:43

kienstra assigned westonruter Jan 31, 2018

kienstra changed the title ~~Issue #864: Fix 'Video' widget for YouTube and Vimeo, simplify 'Gallery' widget~~ Issue #864: Fix 'Video' widget for YouTube and Vimeo, simplify 'Gallery' solution Jan 31, 2018

westonruter requested changes Jan 31, 2018

View reviewed changes

Issue #864: Move video override logic into embed handlers.

7c1f8fe

These are now in the embed classes. Also, create test classes for them.

Ryan Kienstra added 3 commits February 2, 2018 19:08

Revert "Issue #864: Handle a 'fixed-height' issue with 'Video' widgets."

13b52da

This reverts commit a330342.

Issue #864: Fix an issue with the style placement, override 'Text.'

2e197d3

Override the 'Text' widget. Prevent the stripping of <video> width and height attributes. Also, move the addition of the 'style' attribute. Before, it wasn't added.

Prevent suppressing inject_video_max_width_style if not in AMP endpoint

a6e7c36

westonruter approved these changes Feb 3, 2018

View reviewed changes

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

westonruter merged commit 4089c33 into develop Feb 3, 2018

westonruter deleted the fix/864-video-widget-youtube-vimeo branch February 3, 2018 19:45

kienstra mentioned this pull request Feb 3, 2018

Issue #864: Fix issue in 'Video' widget width in <amp-iframe>. #928

Merged

westonruter mentioned this pull request Feb 6, 2018

Add default stylesheet for AMP content when in theme support mode #933

Closed

kienstra mentioned this pull request Feb 6, 2018

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #864: Fix 'Video' widget for YouTube and Vimeo, simplify 'Gallery' solution #921

Issue #864: Fix 'Video' widget for YouTube and Vimeo, simplify 'Gallery' solution #921

kienstra commented Jan 31, 2018 •

edited

Loading

kienstra Jan 31, 2018 •

edited

Loading

westonruter Jan 31, 2018

kienstra Feb 1, 2018

westonruter Jan 31, 2018

kienstra Feb 1, 2018

westonruter commented Jan 31, 2018

pbakaus commented Feb 1, 2018

kienstra commented Feb 1, 2018

westonruter commented Feb 2, 2018

kienstra commented Feb 3, 2018 •

edited

Loading

kienstra commented Feb 3, 2018

westonruter commented Feb 3, 2018

pbakaus commented Feb 4, 2018

westonruter commented Feb 5, 2018

pbakaus commented Feb 5, 2018

cvializ commented Feb 5, 2018

pbakaus commented Feb 5, 2018

aghassemi commented Feb 5, 2018

pbakaus commented Feb 5, 2018

westonruter commented Feb 6, 2018

pbakaus commented Feb 10, 2018

Issue #864: Fix 'Video' widget for YouTube and Vimeo, simplify 'Gallery' solution #921

Issue #864: Fix 'Video' widget for YouTube and Vimeo, simplify 'Gallery' solution #921

Conversation

kienstra commented Jan 31, 2018 • edited Loading

kienstra Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

westonruter Jan 31, 2018

Choose a reason for hiding this comment

kienstra Feb 1, 2018

Choose a reason for hiding this comment

westonruter Jan 31, 2018

Choose a reason for hiding this comment

kienstra Feb 1, 2018

Choose a reason for hiding this comment

westonruter commented Jan 31, 2018

pbakaus commented Feb 1, 2018

kienstra commented Feb 1, 2018

westonruter commented Feb 2, 2018

kienstra commented Feb 3, 2018 • edited Loading

kienstra commented Feb 3, 2018

westonruter commented Feb 3, 2018

pbakaus commented Feb 4, 2018

westonruter commented Feb 5, 2018

pbakaus commented Feb 5, 2018

cvializ commented Feb 5, 2018

pbakaus commented Feb 5, 2018

aghassemi commented Feb 5, 2018

pbakaus commented Feb 5, 2018

westonruter commented Feb 6, 2018

pbakaus commented Feb 10, 2018

kienstra commented Jan 31, 2018 •

edited

Loading

kienstra Jan 31, 2018 •

edited

Loading

kienstra commented Feb 3, 2018 •

edited

Loading