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

Fix header image filtering and YouTube header video detection #1208

Merged
merged 2 commits into from Jun 12, 2018

Conversation

Projects
None yet
2 participants
@westonruter
Member

westonruter commented Jun 10, 2018

Amends #1074. See #1078.

Before 🚫

<div id="wp-custom-header" class="wp-custom-header">
    <amp-img 
        src="https://src.wordpress-develop.test/wp-content/uploads/2018/06/cropped-hoodoo-ski-area-2016-12-29.jpg"
        width="2000"
        height="1199"
        alt="WordPress Develop"
        srcset="
            https://src.wordpress-develop.test/wp-content/uploads/2018/06/cropped-hoodoo-ski-area-2016-12-29.jpg 2000w, 
            https://src.wordpress-develop.test/wp-content/uploads/2018/06/cropped-hoodoo-ski-area-2016-12-29-300x180.jpg 300w, 
            https://src.wordpress-develop.test/wp-content/uploads/2018/06/cropped-hoodoo-ski-area-2016-12-29-768x460.jpg 768w, 
            https://src.wordpress-develop.test/wp-content/uploads/2018/06/cropped-hoodoo-ski-area-2016-12-29-1024x614.jpg 1024w
        "
        sizes="(max-width: 767px) 89vw, (max-width: 1000px) 54vw, (max-width: 1071px) 543px, 580px"
    ></amp-img>
</div>

Notice the low resolution image:

image

After

<div id="wp-custom-header" class="wp-custom-header">
    <amp-img
        src="https://src.wordpress-develop.test/wp-content/uploads/2018/06/cropped-hoodoo-ski-area-2016-12-29.jpg"
        width="2000"
        height="1199"
        alt="WordPress Develop"
        srcset="
            https://src.wordpress-develop.test/wp-content/uploads/2018/06/cropped-hoodoo-ski-area-2016-12-29.jpg 2000w, 
            https://src.wordpress-develop.test/wp-content/uploads/2018/06/cropped-hoodoo-ski-area-2016-12-29-300x180.jpg 300w, 
            https://src.wordpress-develop.test/wp-content/uploads/2018/06/cropped-hoodoo-ski-area-2016-12-29-768x460.jpg 768w, 
            https://src.wordpress-develop.test/wp-content/uploads/2018/06/cropped-hoodoo-ski-area-2016-12-29-1024x614.jpg 1024w
        " 
        sizes="100vw" 
        class="amp-wp-enforced-sizes"
        layout="intrinsic"
    ></amp-img>
</div>

Notice high resolution image:

image

@westonruter westonruter added this to the v1.0 milestone Jun 10, 2018

@westonruter westonruter requested a review from kienstra Jun 10, 2018

Fix header image filtering and YouTube header video detection
* Make sure that any get_header_image_tag filters from theme get applied. Fixes quality issue in Twenty Seventeen.
* Fix detection of YouTube short URLs.
* Dequeue wp-custom-header instead of deregistering to prevent Query Monitor complaining.

@westonruter westonruter force-pushed the fix/header-media branch from c01fabd to 4445c18 Jun 10, 2018

@kienstra

Approved

Hi @westonruter,
This PR looks good. It's nice how it accommodates URLs like https://youtu.be/v5zg_Yv048s.

Nice "before" and "after" images and markup. I also saw how this PR enables outputting the layout attribute on the header image.

@@ -369,7 +369,10 @@ public static function add_hooks() {
add_action( 'comment_form', array( __CLASS__, 'amend_comment_form' ), 100 );
remove_action( 'comment_form', 'wp_comment_form_unfiltered_html_nonce' );
add_filter( 'wp_kses_allowed_html', array( __CLASS__, 'whitelist_layout_in_wp_kses_allowed_html' ), 10 );
add_filter( 'get_header_image_tag', array( __CLASS__, 'conditionally_output_header' ), 10, 3 );
add_filter( 'get_header_image_tag', array( __CLASS__, 'amend_header_image_with_video_header' ), PHP_INT_MAX );

This comment has been minimized.

@kienstra

kienstra Jun 12, 2018

Collaborator

It's nice how this allows all other filters to run first.

@@ -1338,15 +1338,15 @@ public function test_whitelist_layout_in_wp_kses_allowed_html() {
/**
* Test AMP_Theme_Support::conditionally_output_header().
*
* @see AMP_Theme_Support::conditionally_output_header()
* @see AMP_Theme_Support::amend_header_image_with_video_header()
*/
public function conditionally_output_header() {

This comment has been minimized.

@kienstra

kienstra Jun 12, 2018

Collaborator

This is an error I made in a commit, but this method conditionally_output_header() should have been test_conditionally_output_header().

But with the method renamed, this method should probably be test_amend_header_image_with_video_header().

This comment has been minimized.

@westonruter

westonruter Jun 12, 2018

Member

Thanks for noting that. Fixed in 97620b9.

$image_header = AMP_HTML_Utils::build_tag( 'amp-img', $atts );
$youtube_id = null;
if ( isset( $parsed_url['host'] ) && preg_match( '/(^|\.)(youtube\.com|youtu\.be)$/', $parsed_url['host'] ) ) {
if ( 'youtu.be' === $parsed_url['host'] && ! empty( $parsed_url['path'] ) ) {

This comment has been minimized.

@kienstra

kienstra Jun 12, 2018

Collaborator

It's nice how this accounts for URLs like https://youtu.be/v5zg_Yv048s

@westonruter westonruter merged commit a92f802 into develop Jun 12, 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/header-media branch Jun 12, 2018

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