Validate height, width & layout attributes

[pierlon]

Summary

As noted by @westonruter, there are no constraints on the width and height attributes as defined in the Validator protoascii, as the conditional logic is not feasible for the protoascii format.

This PR extends the method AMP_Tag_And_Attribute_Sanitizer::sanitize_disallowed_attribute_values_in_node so that the logic for the supported layouts of a given element are considered, and constraints for the width and height attributes are applied not only according to the layout, but also if a layout is not given.

The logic for validating these attributes is adapted from validator.js of the amproject/amphtml repository.

Video Cover Block

This PR also fixes the Cover Block when a video is selected as the background; the video in a Cover Block is filtered to get the fill layout and to have a cover object fit.

Todo:

• Remove the element if the width or height is invalid
• Remove the element if the width or height does not conform to the specified layout
• Set the width of the element to 'auto' if the width is not already set to 'auto' & the layout is 'fixed_height'

Fixes #2146.

Checklist

• My pull request is addressing an open issue (please create one otherwise).
• My code is tested and passes existing tests.
• My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

[westonruter]

Something important needing to be done here is conversion of HTML dimensions to AMP layout where necessary. One example of this which was done before is #2776 where width=100% gets converted into layout=fixed-height and width=auto. Another recent example of this is conversion of width=100% height=100% to layout=fill, per #3543 and #3295.

The conversion logic should perhaps be performed in AMP_Base_Sanitizer::set_layout() which should then be used by all sanitizers, and the validation can be done by AMP_Tag_And_Attribute_Sanitizer.

Requested changes made.

[westonruter]

With the introduction of layout validation, this redundant/obsolete logic should get removed:

amp-wp/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php

Lines 536 to 545 in be67818

	// Amend spec list with layout.
	if ( isset( $tag_spec['amp_layout'] ) ) {
	$merged_attr_spec_list = array_merge( $merged_attr_spec_list, $this->layout_allowed_attributes );
	if ( isset( $tag_spec['amp_layout']['supported_layouts'] ) ) {
	$layouts = wp_array_slice_assoc( AMP_Rule_Spec::$layout_enum, $tag_spec['amp_layout']['supported_layouts'] );
	$merged_attr_spec_list['layout'][ AMP_Rule_Spec::VALUE_REGEX_CASEI ] = '(' . implode( '|', $layouts ) . ')';
	}
	}

[pierlon]

That would still be needed when AMP_Tag_And_Attribute_Sanitizer::sanitize_disallowed_attribute_values_in_node does its sanitization.

This is needed to make sure the layout attributes are allowed:

amp-wp/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php

Line 537 in a782d7e

$merged_attr_spec_list = array_merge( $merged_attr_spec_list, $this->layout_allowed_attributes );

And this is to ensure that the defined layout is supported by the element:

amp-wp/includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php

Lines 540 to 542 in a782d7e

	$layouts = wp_array_slice_assoc( AMP_Rule_Spec::$layout_enum, $tag_spec['amp_layout']['supported_layouts'] );
	$merged_attr_spec_list['layout'][ AMP_Rule_Spec::VALUE_REGEX_CASEI ] = '(' . implode( '|', $layouts ) . ')';

[westonruter]

@pierlon Please make sure that this scenario is accounted for as reported at https://wordpress.org/support/topic/invalid-layout-specified-by-its-attributes-height/

Namely, code like this:

<div class="embed-vidweb"><iframe id="NATCOM_iframe" src="//vidweb.com/embedcfg/izmqP6DO9n/&amp;idioma=ES&amp;producto=FYI&amp;p=40932" width="100%" height="100%" frameborder="0" scrolling="no" allowfullscreen="allowfullscreen"></iframe></div>

When an iframe or any element has width/height of 100%, either specified via HTML attributes or style, that it gets converted to layout=fill and the width/height are removed. You may be accounting for this already in the PR, but I didn't have a chance to check:

--- a/includes/sanitizers/class-amp-base-sanitizer.php
+++ b/includes/sanitizers/class-amp-base-sanitizer.php
@@ -304,8 +304,7 @@ abstract class AMP_Base_Sanitizer {
 			}
 
 			// Apply fill layout if width & height are 100%.
-			if ( isset( $styles['position'], $attributes['width'], $attributes['height'] )
-				&& 'absolute' === $styles['position']
+			if ( isset( $attributes['width'], $attributes['height'] )
 				&& '100%' === $attributes['width']
 				&& '100%' === $attributes['height']
 			) {
@@ -321,7 +320,10 @@ abstract class AMP_Base_Sanitizer {
 			$attributes['height'] = self::FALLBACK_HEIGHT;
 		}
 
-		if ( empty( $attributes['width'] ) || '100%' === $attributes['width'] ) {
+		if ( isset( $attributes['width'], $attributes['height'] ) && '100%' === $attributes['width'] && '100%' === $attributes['height'] ) {
+			$attributes['layout'] = 'fill';
+			unset( $attributes['width'], $attributes['height'] );
+		} elseif ( empty( $attributes['width'] ) || '100%' === $attributes['width'] ) {
 			$attributes['layout'] = 'fixed-height';
 			$attributes['width']  = 'auto';
 		}

[pierlon]

The layout sanitizer wasn't accounting for the dimensions being in style, will add that in.

[pierlon]

Please make sure that this scenario is accounted for as reported at https://wordpress.org/support/topic/invalid-layout-specified-by-its-attributes-height/

That's already accounted for 😄. I also accounted for the dimensions being defined within the inline style in a85af72.

[westonruter]

Note: Test failure should be fixed if pulling latest changes from develop.

[pierlon]

⚠️ Rebased onto develop.

[westonruter]

@pierlon Now that I've pushed to this branch, please refrain from rebasing as it will cause headaches.

[westonruter]

That's already accounted for 😄. I also accounted for the dimensions being defined within the inline style in a85af72.

Note that this didn't seem to be the case. I've addressed in 99c7b4a.

[westonruter]

This PR has dragged on long enough. Great work here. Let's get it into testing to shake out any bugs or things we didn't account for.

[westonruter]

@schlessera Please advise whether there are outstanding issues you wanted addressed with this PR prior to merging.

[rgllm]

@westonruter any timeline when this will be released?

[westonruter]

@rgllm The 1.5 release is due out by end of March to coincide with WordPress 5.4.

In the meantime, you can use a pre-release version from develop: amp.zip (v1.5.0-alpha-20200210T165753Z-094272913)