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

Add sanitizer to fixup issues with core themes #1074

Merged
merged 33 commits into from
Jun 4, 2018
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
8643b8b
Add initial AMP_Core_Theme_Sanitizer with partial Twenty Seventeen fi…
westonruter Apr 13, 2018
4692577
Begin AMP header video implementation.
kienstra May 17, 2018
51eb633
Merge branch 'develop' into header-video
kienstra May 29, 2018
fc0bea1
Begin PHPUnit test of AMP header video implementation.
kienstra May 29, 2018
7f738c1
create image and video image tags in separate methods
DavidCramer May 30, 2018
cbacd9d
add doc params
DavidCramer May 30, 2018
f186cd9
Merge branch 'header-video' into 'core-themes'
DavidCramer May 30, 2018
31cb154
fix unit tests for video header
DavidCramer May 30, 2018
a36a281
correct expected count in unit test
DavidCramer May 30, 2018
f5ca657
Merge branch 'develop' into core-themes
DavidCramer May 31, 2018
601c005
remove video script enqueue
DavidCramer May 31, 2018
80e2355
merge develop
DavidCramer Jun 1, 2018
dc9e628
Add required styles for amp components in the header image and video
DavidCramer Jun 1, 2018
54e025a
remove unused amp-youtube as styles work correctly
DavidCramer Jun 1, 2018
65aec1e
Merge branch 'develop' of https://github.com/Automattic/amp-wp into c…
westonruter Jun 2, 2018
a91f0e2
Rename header_banner_styles to add_header_media_styles
westonruter Jun 2, 2018
e47275c
Split out add_has_header_video_body_class feature as separate method
westonruter Jun 2, 2018
d41e498
Add mechanism to allow sanitizers to add hooks during output buffering
westonruter Jun 3, 2018
d3b8eae
Add nav menu support to Twenty Sixteen
westonruter Jun 3, 2018
d2247ff
Ensure nav menus are keyboard-accessible via :focus-within
westonruter Jun 4, 2018
3bb3dd8
Add core theme sanitizer features for dequeueing scripts and removing…
westonruter Jun 4, 2018
ac98dfc
Emulate adjustHeaderHeight() in Twenty Seventeen via CSS
westonruter Jun 4, 2018
93c87eb
Add support for smooth scrolling to content in Twenty Seventeen
westonruter Jun 4, 2018
7b9acc6
Add accept_validation_errors feature to pre-accept issues in core themes
westonruter Jun 4, 2018
df2e561
Support setQuotesIcon()
DavidCramer Jun 4, 2018
bca1ead
check post format for quote, rename methods
DavidCramer Jun 4, 2018
e1fc251
Simplify implementation of Twenty Seventeen's setQuotesIcon()
westonruter Jun 4, 2018
d55cf6a
Implement is_array_subset to have more fine-grained control over whet…
westonruter Jun 4, 2018
425c80a
Accept core theme validation errors in admin as well as frontend
westonruter Jun 4, 2018
70d1440
Implement sticky header for Twenty Seventeen
westonruter Jun 4, 2018
2cdb1c8
Merge branch 'develop' of https://github.com/Automattic/amp-wp into c…
westonruter Jun 4, 2018
1fdb325
Prevent showing cloned sticky nav menu on mobile
westonruter Jun 4, 2018
2fc015d
Remove unnecessary output_header_image method
westonruter Jun 4, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions includes/class-amp-autoloader.php
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ class AMP_Autoloader {
'AMP_Style_Sanitizer' => 'includes/sanitizers/class-amp-style-sanitizer',
'AMP_Tag_And_Attribute_Sanitizer' => 'includes/sanitizers/class-amp-tag-and-attribute-sanitizer',
'AMP_Video_Sanitizer' => 'includes/sanitizers/class-amp-video-sanitizer',
'AMP_Core_Theme_Sanitizer' => 'includes/sanitizers/class-amp-core-theme-sanitizer',
'AMP_Customizer_Design_Settings' => 'includes/settings/class-amp-customizer-design-settings',
'AMP_Customizer_Settings' => 'includes/settings/class-amp-customizer-settings',
'AMP_Content' => 'includes/templates/class-amp-content',
Expand Down
117 changes: 117 additions & 0 deletions includes/class-amp-theme-support.php
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,14 @@ public static function register_paired_hooks() {
*/
public static function add_hooks() {

add_filter( 'amp_content_sanitizers', function( $sanitizers ) {
$sanitizers['AMP_Core_Theme_Sanitizer'] = array(
'template' => get_template(),
'stylesheet' => get_stylesheet(),
);
return $sanitizers;
} );

// Remove core actions which are invalid AMP.
remove_action( 'wp_head', 'wp_post_preview_js', 1 );
remove_action( 'wp_head', 'print_emoji_detection_script', 7 );
Expand Down Expand Up @@ -303,6 +311,7 @@ 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 );

// @todo Add character conversion.
}
Expand Down Expand Up @@ -1243,4 +1252,112 @@ public static function enqueue_assets() {
// Enqueue default styles expected by sanitizer.
wp_enqueue_style( 'amp-default', amp_get_asset_url( 'css/amp-default.css' ), array(), AMP__VERSION );
}

/**
* Conditionally replace the header image markup with a header video or image.
*
* This is JS-driven in Core themes like Twenty Sixteen and Twenty Seventeen.
* So in order for the header video to display,
* this replaces the markup of the header image.
*
* @since 1.0
* @link https://github.com/WordPress/wordpress-develop/blob/d002fde80e5e3a083e5f950313163f566561517f/src/wp-includes/js/wp-custom-header.js#L54
* @param string $html The image markup to filter.
* @param array $header The header config array.
* @param array $atts The image markup attributes.
* @return string $html Filtered markup.
*/
public static function conditionally_output_header( $html, $header, $atts ) {
if ( ! is_header_video_active() ) {
return $html;
};

if ( ! has_header_video() ) {
return self::output_header_image( $atts );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice use of these helper functions to simplify conditionally_output_header()

}

return self::output_header_video( $atts );
}

/**
* Replace the header image markup with a header video.
*
* This is JS-driven in Core themes like Twenty Sixteen and Twenty Seventeen.
* So in order for the header video to display,
* this replaces the markup of the header image.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a blocker, but maybe this 3-line DocBlock description could be removed. It looks to be the same as in conditionally_output_header().

*
* @since 1.0
* @link https://github.com/WordPress/wordpress-develop/blob/d002fde80e5e3a083e5f950313163f566561517f/src/wp-includes/js/wp-custom-header.js#L54
* @param array $atts The header tag attributes array.
* @return string $html Filtered markup.
*/
public static function output_header_video( $atts ) {
// Remove the script for video.
wp_deregister_script( 'wp-custom-header' );
$video_settings = get_header_video_settings();

$parsed_url = wp_parse_url( $video_settings['videoUrl'] );
$query = isset( $parsed_url['query'] ) ? wp_parse_args( $parsed_url['query'] ) : null;
$video_attributes = array(
'media' => '(min-width: ' . $video_settings['minWidth'] . 'px)',
'width' => $video_settings['width'],
'height' => $video_settings['height'],
'layout' => 'fill',
'autoplay' => '',
'id' => 'wp-custom-header-video',
);

$atts['placeholder'] = '';
$image_placeholder = self::output_header_image( $atts );

// If the video URL is for YouTube, return an <amp-youtube> element.
if ( isset( $parsed_url['host'], $query['v'] ) && ( false !== strpos( $parsed_url['host'], 'youtube' ) ) ) {
$video_header = AMP_HTML_Utils::build_tag(
'amp-youtube',
array_merge(
$video_attributes,
array(
'data-videoid' => $query['v'],
'data-param-rel' => '0', // Don't show related videos.
'data-param-showinfo' => '0', // Don't show video title at the top.
'data-param-controls' => '0', // Don't show video controls.
)
)
);
} else {
$video_header = AMP_HTML_Utils::build_tag(
'amp-video',
array_merge(
$video_attributes,
array(
'src' => $video_settings['videoUrl'],
)
)
);
}
return $image_placeholder . $video_header;
}

/**
* Replace the header image markup with a header image.
*
* This is JS-driven in Core themes like Twenty Sixteen and Twenty Seventeen.
* So in order for the header video to display,
* this replaces the markup of the header image.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the suggestion above, maybe these 3 lines could be removed.

*
* @since 1.0
* @link https://github.com/WordPress/wordpress-develop/blob/d002fde80e5e3a083e5f950313163f566561517f/src/wp-includes/js/wp-custom-header.js#L54
* @param array $atts The image tag attributes.
* @return string $html Filtered markup.
*/
public static function output_header_image( $atts ) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a blocker, but this might be cleaner as:

public static function output_header_image( $atts ) {
        return AMP_HTML_Utils::build_tag( 'amp-img', $atts );
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm going to just remove the method entirely. There's no need for it.

$atts['layout'] = 'fill';
unset( $atts['width'] );

$place_holder = AMP_HTML_Utils::build_tag( 'img', $atts );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

img or amp-img? I think this should be creating an amp-img here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, sorry, it was, I removed it for testing something, but forgot to revert 601c005


return $place_holder;

}
}
91 changes: 91 additions & 0 deletions includes/sanitizers/class-amp-core-theme-sanitizer.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
<?php
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be great to eventually go back and add tests for this class, and other new methods.

/**
* Class AMP_Core_Theme_Sanitizer.
*
* @package AMP
* @since 1.0
*/

/**
* Class AMP_Core_Theme_Sanitizer
*
* Fixes up common issues in core themes and others.
*
* @since 1.0
*/
class AMP_Core_Theme_Sanitizer extends AMP_Base_Sanitizer {

/**
* Config for features needed by themes.
*
* @var array
*/
public $theme_features = array(
'twentyseventeen' => array(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great idea to pass an array of arguments to each of the methods, depending on the theme.

'force_svg_support' => array(),
'force_fixed_background_support' => array(),
// @todo Header video probably needs special support in the plugin generally.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #1078

// @todo Header image is not styled properly. Needs layout=responsive and other things?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// @todo Dequeue scripts and replace with AMP functionality where possible.
),
);

/**
* Fix up core themes to do things in the AMP way.
*
* @since 1.0
*/
public function sanitize() {
$theme_features = array();

// Find theme features for core theme.
$theme_candidates = wp_array_slice_assoc( $this->args, array( 'stylesheet', 'template' ) );
foreach ( $theme_candidates as $theme_candidate ) {
if ( isset( $this->theme_features[ $theme_candidate ] ) ) {
$theme_features = $this->theme_features[ $theme_candidate ];
break;
}
}

// Allow specific theme features to be requested even if the theme is not in core.
if ( isset( $this->args['theme_features'] ) ) {
$theme_features = array_merge( $this->args['theme_features'], $theme_features );
}

foreach ( $theme_features as $theme_feature => $feature_args ) {
if ( method_exists( $this, $theme_feature ) ) {
call_user_func( array( $this, $theme_feature ), $feature_args );
}
}
}

/**
* Force SVG support, replacing no-svg class name with svg class name.
*
* @link https://github.com/WordPress/wordpress-develop/blob/1af1f65a21a1a697fb5f33027497f9e5ae638453/src/wp-content/themes/twentyseventeen/assets/js/global.js#L211-L213
* @link https://caniuse.com/#feat=svg
*/
public function force_svg_support() {
$this->dom->documentElement->setAttribute(
'class',
preg_replace(
'/(^|\s)no-svg(\s|$)/',
' svg ',
$this->dom->documentElement->getAttribute( 'class' )
)
);
}

/**
* Force support for fixed background-attachment.
*
* @link https://github.com/WordPress/wordpress-develop/blob/1af1f65a21a1a697fb5f33027497f9e5ae638453/src/wp-content/themes/twentyseventeen/assets/js/global.js#L215-L217
* @link https://caniuse.com/#feat=background-attachment
*/
public function force_fixed_background_support() {
$this->dom->documentElement->setAttribute(
'class',
$this->dom->documentElement->getAttribute( 'class' ) . ' background-fixed'
);
}
}
29 changes: 29 additions & 0 deletions tests/test-class-amp-theme-support.php
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ public function tearDown() {
parent::tearDown();
AMP_Validation_Manager::reset_validation_results();
remove_theme_support( 'amp' );
remove_theme_support( 'custom-header' );
$_REQUEST = array(); // phpcs:ignore WordPress.CSRF.NonceVerification.NoNonceVerification
$_SERVER['QUERY_STRING'] = '';
unset( $_SERVER['REQUEST_URI'] );
Expand Down Expand Up @@ -299,6 +300,7 @@ public function test_add_hooks() {
$this->assertEquals( 10, has_filter( 'cancel_comment_reply_link', array( self::TESTED_CLASS, 'filter_cancel_comment_reply_link' ) ) );
$this->assertEquals( 100, has_action( 'comment_form', array( self::TESTED_CLASS, 'amend_comment_form' ) ) );
$this->assertFalse( has_action( 'comment_form', 'wp_comment_form_unfiltered_html_nonce' ) );
$this->assertEquals( 10, has_filter( 'get_header_image_tag', array( self::TESTED_CLASS, 'conditionally_output_header' ) ) );
}

/**
Expand Down Expand Up @@ -1236,4 +1238,31 @@ public function test_whitelist_layout_in_wp_kses_allowed_html() {
$image = '<img data-amp-layout="fill">';
$this->assertEquals( $image, wp_kses_post( $image ) );
}

/**
* Test AMP_Theme_Support::conditionally_output_header().
*
* @see AMP_Theme_Support::conditionally_output_header()
*/
public function conditionally_output_header() {
$mock_image = '<img src="https://example.com/flower.jpeg">';

// If there's no theme support for 'custom-header', the callback should simply return the image.
$this->assertEquals(
$mock_image,
AMP_Theme_Support::conditionally_output_header( $mock_image )
);

// If theme support is present, but there isn't a header video selected, the callback should again return the image.
add_theme_support( 'custom-header', array(
'video' => true,
) );

// There's a YouTube URL as the header video.
set_theme_mod( 'external_header_video', 'https://www.youtube.com/watch?v=a8NScvBhVnc' );
$this->assertEquals(
'<amp-youtube width="0" height="0" layout="responsive" autoplay id="wp-custom-header-video" data-videoid="a8NScvBhVnc" data-param-rel="0" data-param-showinfo="0"></amp-youtube>',
AMP_Theme_Support::conditionally_output_header( $mock_image )
);
}
}