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

1094: Transform CSS selectors according to sanitizer HTML element to AMP component conversions #1175

Merged
merged 17 commits into from
May 31, 2018
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
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
2 changes: 1 addition & 1 deletion assets/css/amp-default.css
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
.amp-wp-unknown-size img {
.amp-wp-unknown-size .i-amphtml-replaced-content {
Copy link
Member

Choose a reason for hiding this comment

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

It is illegal in AMP to use i-amphtml-replaced-content class names. See:

Internal AMP class names prefixed with -amp- and i-amp- are disallowed in AMP HTML.

https://www.ampproject.org/docs/fundamentals/spec#classes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, think I even saw a todo note recently somewhere as well to remove these from CSS within style sanitizer and realized that it's illegal. Will look into which selector to use.

Copy link
Member

Choose a reason for hiding this comment

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

Since tree shaking isn't done for attribute values, I suggest the way to fix this is to use an attribute selector like [src].

/** Worst case scenario when we can't figure out dimensions for an image. **/
/** Force the image into a box of fixed dimensions and use object-fit to scale. **/
object-fit: contain;
Expand Down
2 changes: 1 addition & 1 deletion assets/js/amp-editor-blocks.js
Original file line number Diff line number Diff line change
Expand Up @@ -731,7 +731,7 @@ var ampEditorBlocks = ( function() { // eslint-disable-line no-unused-vars
return element;
}
} else if ( ! component.hasGalleryShortcodeCarouselAttribute( attributes.text || '' ) ) {
// Add amp-carousel=false attribut to the shortcode.
// Add amp-carousel=false attribute to the shortcode.
text = attributes.text.replace( '[gallery', '[gallery amp-carousel=false' );
} else {
text = attributes.text;
Expand Down
11 changes: 11 additions & 0 deletions includes/sanitizers/class-amp-audio-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,17 @@ class AMP_Audio_Sanitizer extends AMP_Base_Sanitizer {
*/
public static $tag = 'audio';

/**
* Get mapping of HTML selectors to the AMP component selectors which they may be converted into.
*
* @return array Mapping.
*/
public function get_selector_conversion_mapping() {
return array(
'audio' => array( 'amp-audio' ),
);
}

/**
* Sanitize the <audio> elements from the HTML contained in this instance's DOMDocument.
*
Expand Down
36 changes: 36 additions & 0 deletions includes/sanitizers/class-amp-base-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ abstract class AMP_Base_Sanitizer {
* Value for <amp-image-lightbox> ID.
*
* @since 1.0
* @todo Move to AMP_Img_Sanitizer?
*
* @const string
*/
Expand Down Expand Up @@ -121,11 +122,41 @@ public function __construct( $dom, $args = array() ) {
}
}

/**
* Get mapping of HTML selectors to the AMP component selectors which they may be converted into.
*
* @return array Mapping.
*/
public function get_selector_conversion_mapping() {
return array();
}

/**
* Run logic before any sanitizers are run.
*
* After the sanitizers are instantiated but before calling sanitize on each of them, this
* method is called with list of all the instantiated sanitizers.
*
* @todo Call this init?
*
* @param AMP_Base_Sanitizer[] $sanitizers Sanitizers.
*/
public function before_sanitize( $sanitizers ) {}

/**
* Sanitize the HTML contained in the DOMDocument received by the constructor
*/
abstract public function sanitize();

/**
* Run logic after all sanitizers are finished.
*
* @todo Do we even need this?
*
* @param AMP_Base_Sanitizer[] $sanitizers Sanitizers.
*/
public function after_sanitize( $sanitizers ) {}

/**
* Return array of values that would be valid as an HTML `script` element.
*
Expand Down Expand Up @@ -444,6 +475,8 @@ public function get_data_amp_attributes( $node ) {
if ( isset( $parent_attributes['data-amp-noloading'] ) && true === filter_var( $parent_attributes['data-amp-noloading'], FILTER_VALIDATE_BOOLEAN ) ) {
$attributes['noloading'] = $parent_attributes['data-amp-noloading'];
}

// @todo Move to AMP_Img_Sanitizer subclassed method?
if ( isset( $parent_attributes['data-amp-lightbox'] ) && true === filter_var( $parent_attributes['data-amp-lightbox'], FILTER_VALIDATE_BOOLEAN ) ) {
$attributes['lightbox'] = true;
}
Expand All @@ -466,6 +499,7 @@ public function filter_data_amp_attributes( $attributes, $amp_data ) {
if ( isset( $amp_data['noloading'] ) ) {
$attributes['data-amp-noloading'] = '';
}
// @todo Move lightbox condition to subclassed method of AMP_Img_Sanitizer.
if ( isset( $amp_data['lightbox'] ) ) {
$attributes['data-amp-lightbox'] = '';
$attributes['on'] = 'tap:' . self::AMP_IMAGE_LIGHTBOX_ID;
Expand Down Expand Up @@ -514,6 +548,8 @@ public function filter_attachment_layout_attributes( $node, $new_attributes, $la

/**
* Add <amp-image-lightbox> element to body tag if it doesn't exist yet.
*
* @todo Move this to AMP_Img_Sanitizer?
*/
public function maybe_add_amp_image_lightbox_node() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this is also used by AMP_Gallery_Block_Sanitizer.


Expand Down
15 changes: 14 additions & 1 deletion includes/sanitizers/class-amp-iframe-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,19 @@ class AMP_Iframe_Sanitizer extends AMP_Base_Sanitizer {
'add_placeholder' => false,
);

/**
* Get mapping of HTML selectors to the AMP component selectors which they may be converted into.
*
* @return array Mapping.
*/
public function get_selector_conversion_mapping() {
return array(
'iframe' => array(
'amp-iframe',
),
);
}

/**
* Sanitize the <iframe> elements from the HTML contained in this instance's DOMDocument.
*
Expand Down Expand Up @@ -189,7 +202,7 @@ private function filter_attributes( $attributes ) {
private function build_placeholder( $parent_attributes ) {
$placeholder_node = AMP_DOM_Utils::create_node( $this->dom, 'div', array(
'placeholder' => '',
'class' => 'amp-wp-iframe-placeholder',
'class' => 'amp-wp-iframe-placeholder',
) );

return $placeholder_node;
Expand Down
14 changes: 14 additions & 0 deletions includes/sanitizers/class-amp-img-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,20 @@ class AMP_Img_Sanitizer extends AMP_Base_Sanitizer {
*/
private static $anim_extension = '.gif';

/**
* Get mapping of HTML selectors to the AMP component selectors which they may be converted into.
*
* @return array Mapping.
*/
public function get_selector_conversion_mapping() {
return array(
'img' => array(
'amp-img',
'amp-anim',
),
);
}

/**
* Sanitize the <img> elements from the HTML contained in this instance's DOMDocument.
*
Expand Down
12 changes: 12 additions & 0 deletions includes/sanitizers/class-amp-playbuzz-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,18 @@ class AMP_Playbuzz_Sanitizer extends AMP_Base_Sanitizer {
*/
private static $height = '500';

/**
* Get mapping of HTML selectors to the AMP component selectors which they may be converted into.
*
* @return array Mapping.
*/
public function get_selector_conversion_mapping() {
return array(
'div.pb_feed' => array( 'amp-playbuzz' ),
'.pb_feed' => array( 'amp-playbuzz' ),
);
}

/**
* Sanitize the Playbuzz elements from the HTML contained in this instance's DOMDocument.
*
Expand Down
71 changes: 70 additions & 1 deletion includes/sanitizers/class-amp-style-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,13 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer {
*/
private $processed_imported_stylesheet_urls = array();

/**
* Mapping of HTML element selectors to AMP selector elements.
*
* @var array
*/
private $selector_mappings = array();

/**
* Get error codes that can be raised during parsing of CSS.
*
Expand Down Expand Up @@ -308,6 +315,30 @@ private function get_used_tag_names() {
return $this->used_tag_names;
}

/**
* Run logic before any sanitizers are run.
*
* After the sanitizers are instantiated but before calling sanitize on each of them, this
* method is called with list of all the instantiated sanitizers.
*
* @param AMP_Base_Sanitizer[] $sanitizers Sanitizers.
*/
public function before_sanitize( $sanitizers ) {
parent::before_sanitize( $sanitizers );

foreach ( $sanitizers as $sanitizer ) {
foreach ( $sanitizer->get_selector_conversion_mapping() as $html_selectors => $amp_selectors ) {
if ( ! isset( $this->selector_mappings[ $html_selectors ] ) ) {
$this->selector_mappings[ $html_selectors ] = $amp_selectors;
} else {
$this->selector_mappings[ $html_selectors ] = array_unique(
array_merge( $this->selector_mappings[ $html_selectors ], $amp_selectors )
);
}
}
}
}

/**
* Sanitize CSS styles within the HTML contained in this instance's DOMDocument.
*
Expand Down Expand Up @@ -647,7 +678,7 @@ private function fetch_external_stylesheet( $url ) {
private function process_stylesheet( $stylesheet, $options = array() ) {
$parsed = null;
$cache_key = null;
$cache_group = 'amp-parsed-stylesheet-v6';
$cache_group = 'amp-parsed-stylesheet-v7';

$cache_impacting_options = array_merge(
wp_array_slice_assoc(
Expand Down Expand Up @@ -1236,6 +1267,10 @@ private function real_path_urls( $urls, $stylesheet_url ) {
private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_list, $options ) {
$results = array();

if ( $ruleset instanceof DeclarationBlock ) {
$this->ampify_ruleset_selectors( $ruleset );
}

// Remove disallowed properties.
if ( ! empty( $options['property_whitelist'] ) ) {
$properties = $ruleset->getRules();
Expand Down Expand Up @@ -1754,6 +1789,40 @@ private function finalize_styles() {
}
}

/**
* Convert CSS selectors.
*
* @param DeclarationBlock $ruleset Ruleset.
*/
private function ampify_ruleset_selectors( $ruleset ) {
$selectors = array();
$replacements = 0;
foreach ( $ruleset->getSelectors() as $old_selector ) {
$edited_selectors = array( $old_selector->getSelector() );
foreach ( $this->selector_mappings as $html_selector => $amp_selectors ) { // Note: The $selector_mappings array contains ~6 items.
Copy link
Member

Choose a reason for hiding this comment

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

These notes are here because 4 level of loop nesting is normally something that is a big red flag for performance.

$html_pattern = '/(?<=^|[^a-z0-9_-])' . preg_quote( $html_selector ) . '(?=$|[^a-z0-9_-])/i';
foreach ( $edited_selectors as &$edited_selector ) { // Note: The $edited_selectors array contains only item in the normal case.
$original_selector = $edited_selector;
$amp_selector = array_shift( $amp_selectors );
$edited_selector = preg_replace( $html_pattern, $amp_selector, $edited_selector, -1, $count );
if ( ! $count ) {
continue;
}
$replacements += $count;
while ( ! empty( $amp_selectors ) ) { // Note: This array contains only a couple items.
$amp_selector = array_shift( $amp_selectors );
$edited_selectors[] = preg_replace( $html_pattern, $amp_selector, $original_selector, -1, $count );
}
}
}
$selectors = array_merge( $selectors, $edited_selectors );
}

if ( $replacements > 0 ) {
$ruleset->setSelectors( $selectors );
}
}

/**
* Finalize a stylesheet set (amp-custom or amp-keyframes).
*
Expand Down
11 changes: 11 additions & 0 deletions includes/sanitizers/class-amp-video-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,17 @@ class AMP_Video_Sanitizer extends AMP_Base_Sanitizer {
*/
public static $tag = 'video';

/**
* Get mapping of HTML selectors to the AMP component selectors which they may be converted into.
*
* @return array Mapping.
*/
public function get_selector_conversion_mapping() {
return array(
'video' => array( 'amp-video' ),
);
}

/**
* Sanitize the <video> elements from the HTML contained in this instance's DOMDocument.
*
Expand Down
27 changes: 26 additions & 1 deletion includes/templates/class-amp-content-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,16 @@ public static function sanitize_document( &$dom, $sanitizer_classes, $args ) {

$return_styles = ! empty( $args['return_styles'] );
unset( $args['return_styles'] );

/**
* Sanitizers.
*
* @var AMP_Base_Sanitizer[] $sanitizers
*/
$sanitizers = array();

// Instantiate the sanitizers.
foreach ( $sanitizer_classes as $sanitizer_class => $sanitizer_args ) {
$sanitize_class_start = microtime( true );
if ( ! class_exists( $sanitizer_class ) ) {
/* translators: %s is sanitizer class */
_doing_it_wrong( __METHOD__, sprintf( esc_html__( 'Sanitizer (%s) class does not exist', 'amp' ), esc_html( $sanitizer_class ) ), '0.4.1' );
Expand All @@ -84,6 +92,18 @@ public static function sanitize_document( &$dom, $sanitizer_classes, $args ) {
continue;
}

$sanitizers[ $sanitizer_class ] = $sanitizer;
}

// Let the sanitizers know about each other prior to sanitizing.
foreach ( $sanitizers as $sanitizer ) {
$sanitizer->before_sanitize( $sanitizers );
Copy link
Member

Choose a reason for hiding this comment

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

I do think we should rename this to init

}

// Sanitize.
foreach ( $sanitizers as $sanitizer ) {
$sanitize_class_start = microtime( true );

$sanitizer->sanitize();

$scripts = array_merge( $scripts, $sanitizer->get_scripts() );
Expand All @@ -96,6 +116,11 @@ public static function sanitize_document( &$dom, $sanitizer_classes, $args ) {
AMP_Response_Headers::send_server_timing( 'amp_sanitize', -$sanitize_class_start, $sanitizer_class );
}

// Let the sanitizers.
foreach ( $sanitizers as $sanitizer ) {
$sanitizer->after_sanitize( $sanitizers );
}
Copy link
Member

Choose a reason for hiding this comment

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

Since we're not using this, I think we should just kill after_sanitize.


return compact( 'scripts', 'styles', 'stylesheets' );
}
}
Expand Down
2 changes: 1 addition & 1 deletion includes/utils/class-amp-dom-utils.php
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,7 @@ public static function create_node( $dom, $tag, $attributes ) {
*
* @since 0.2
*
* @param DOMNode $node Represents an HTML element for which to extract attributes.
* @param DOMElement $node Represents an HTML element for which to extract attributes.
*
* @return string[] The attributes for the passed node, or an
* empty array if it has no attributes.
Expand Down
Loading