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 32 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
4 changes: 4 additions & 0 deletions includes/amp-helper-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -524,6 +524,10 @@ function amp_get_content_sanitizers( $post = null ) {
*/
$sanitizers = apply_filters( 'amp_content_sanitizers',
array(
'AMP_Core_Theme_Sanitizer' => array(
'template' => get_template(),
'stylesheet' => get_stylesheet(),
),
'AMP_Img_Sanitizer' => array(),
'AMP_Form_Sanitizer' => array(),
'AMP_Comments_Sanitizer' => array(),
Expand Down
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 @@ -159,6 +159,12 @@ public static function finish_init() {
self::$sanitizer_classes = amp_get_content_sanitizers();
self::$sanitizer_classes = AMP_Validation_Manager::filter_sanitizer_args( self::$sanitizer_classes );
self::$embed_handlers = self::register_content_embed_handlers();

foreach ( self::$sanitizer_classes as $sanitizer_class => $args ) {
if ( method_exists( $sanitizer_class, 'add_buffering_hooks' ) ) {
call_user_func( array( $sanitizer_class, 'add_buffering_hooks' ), $args );
}
}
}

/**
Expand Down Expand Up @@ -303,6 +309,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_action( 'wp_enqueue_scripts', function() {
wp_dequeue_script( 'comment-reply' ); // Handled largely by AMP_Comments_Sanitizer and *reply* methods in this class.
} );

// @todo Add character conversion.
}
Expand Down Expand Up @@ -1264,4 +1274,111 @@ 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
* @link https://github.com/WordPress/wordpress-develop/blob/d002fde80e5e3a083e5f950313163f566561517f/src/wp-includes/js/wp-custom-header.js#L78
* @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' => 'responsive',
'autoplay' => '',
'id' => 'wp-custom-header-video',
);

// Create image banner to stay behind the video.
$image_header = 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_header . $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.

$place_holder = AMP_HTML_Utils::build_tag( 'amp-img', $atts );

return $place_holder;

}
}
16 changes: 16 additions & 0 deletions includes/sanitizers/class-amp-base-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,22 @@ public function __construct( $dom, $args = array() ) {
}
}

/**
* Add filters to manipulate output during output buffering before the DOM is constructed.
*
* Add actions and filters before the page is rendered so that the sanitizer can fix issues during output buffering.
* This provides an alternative to manipulating the DOM in the sanitize method. This is a static function because
* it is invoked before the class is instantiated, as the DOM is not available yet. This method is only called
* when 'amp' theme support is present. It is conceptually similar to the AMP_Base_Embed_Handler class's register_embed
* method.
*
* @since 1.0
* @see \AMP_Base_Embed_Handler::register_embed()
*
* @param array $args Args.
*/
public static function add_buffering_hooks( $args = array() ) {}

/**
* Get mapping of HTML selectors to the AMP component selectors which they may be converted into.
*
Expand Down
Loading