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

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

Merged
merged 11 commits into from Feb 3, 2018

Conversation

Projects
None yet
5 participants
@kienstra
Copy link
Collaborator

commented Jan 31, 2018

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.

kienstra added some commits Jan 31, 2018

Issue #864: Address an issue in 'Video' widgets for YouTube and Vimeo.
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.
Issue #864: Only override video widget output if is_amp_endpoint().
Similarly to how the other widget overrides work.
Also, adjust tests for this.
Issue #864: Simply return the initial value, instead of ''.
This will allow another plugin to override this.
Inspired by the Jetpack plugin's return of the initial value.
Issue #864: Remove the 'Gallery' widget subclass, in favor of a filter.
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'.
public function unregister_embed() {
remove_shortcode( 'gallery' );
}
public function unregister_embed() {}

This comment has been minimized.

Copy link
@kienstra

kienstra Jan 31, 2018

Author Collaborator

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.

@kienstra kienstra requested a review from westonruter Jan 31, 2018

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

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

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 31, 2018

Member

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.

This comment has been minimized.

Copy link
@kienstra

kienstra Feb 1, 2018

Author Collaborator

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

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

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 31, 2018

Member

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.

This comment has been minimized.

Copy link
@kienstra

kienstra Feb 1, 2018

Author Collaborator

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

@westonruter

This comment has been minimized.

Copy link
Member

commented Jan 31, 2018

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

image

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

This comment has been minimized.

Copy link
Collaborator

commented Feb 1, 2018

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.

Issue #864: Move video override logic into embed handlers.
These are now in the embed classes.
Also, create test classes for them.
@kienstra

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 1, 2018

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

This comment has been minimized.

Copy link
Member

commented Feb 2, 2018

@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 tag in HTML works luckily means it won't be stretched, and fill up with blank space.

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.

Issue #864: Handle a 'fixed-height' issue with 'Video' widgets.
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

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 3, 2018

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.

kienstra added some commits Feb 3, 2018

Issue #864: Video widget fixes, including width and height.
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.
Issue #864: Fix an issue with the style placement, override 'Text.'
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

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 3, 2018

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:

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

This comment has been minimized.

Copy link
Member

commented Feb 3, 2018

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.

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

@westonruter westonruter merged commit 4089c33 into develop Feb 3, 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-video-widget-youtube-vimeo branch Feb 3, 2018

@pbakaus

This comment has been minimized.

Copy link
Collaborator

commented Feb 4, 2018

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

This comment has been minimized.

Copy link
Member

commented Feb 5, 2018

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

image

@pbakaus

This comment has been minimized.

Copy link
Collaborator

commented Feb 5, 2018

@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

This comment has been minimized.

Copy link

commented Feb 5, 2018

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

This comment has been minimized.

Copy link
Collaborator

commented Feb 5, 2018

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

This comment has been minimized.

Copy link
Member

commented Feb 5, 2018

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

This comment has been minimized.

Copy link
Collaborator

commented Feb 5, 2018

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

This comment has been minimized.

Copy link
Member

commented Feb 6, 2018

@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

This comment has been minimized.

Copy link
Collaborator

commented Feb 10, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.