Fix AMP carousel for gallery block.

* Let gallery have be sized according to the max height. * Constrain height of AMP gallery carousel by ratio with max width * Remove duplicate data for jed_locale_data.
ampproject · May 30, 2018 · 201654f · 201654f
1 parent 46c3a50
commit 201654f
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 42 deletions.
diff --git a/assets/js/amp-editor-blocks.js b/assets/js/amp-editor-blocks.js
@@ -180,7 +180,7 @@ var ampEditorBlocks = ( function() { // eslint-disable-line no-unused-vars
 			ampAttributes[ 'data-amp-lightbox' ] = attributes.ampLightbox;
 		}
 		if ( attributes.ampCarousel ) {
-			ampAttributes[ 'data-amp-carousel' ] = attributes.ampLightbox;
+			ampAttributes[ 'data-amp-carousel' ] = attributes.ampCarousel;
 		}
 
 		return _.extend( ampAttributes, props );

diff --git a/includes/admin/class-amp-editor-blocks.php b/includes/admin/class-amp-editor-blocks.php
@@ -117,12 +117,6 @@ public function enqueue_block_editor_assets() {
 			AMP__VERSION
 		);
 
-		wp_add_inline_script(
-			'amp-editor-blocks-build',
-			'wp.i18n.setLocaleData( ' . wp_json_encode( gutenberg_get_jed_locale_data( 'amp' ) ) . ', "amp" );',
-			'before'
-		);
-
 		// Scripts.
 		wp_enqueue_script(
 			'amp-editor-blocks-build',

diff --git a/includes/sanitizers/class-amp-gallery-block-sanitizer.php b/includes/sanitizers/class-amp-gallery-block-sanitizer.php
@@ -78,35 +78,30 @@ public function sanitize() {
 				continue;
 			}
 
-			$num_images = 0;
+			$images = null;
 
 			// If it's not AMP lightbox, look for links first.
 			if ( ! $is_amp_lightbox ) {
-				$images     = $node->getElementsByTagName( 'a' );
-				$num_images = $images->length;
+				$images = $node->getElementsByTagName( 'a' );
 			}
 
-			if ( 0 === $num_images ) {
-
-				// If not linking to anything then look for <amp-img>.
-				$images     = $node->getElementsByTagName( 'amp-img' );
-				$num_images = $images->length;
+			// If not linking to anything then look for <amp-img>.
+			if ( ! $images || 0 === $images->length ) {
+				$images = $node->getElementsByTagName( 'amp-img' );
 			}
 
-			if ( 0 === $num_images ) {
+			// Skip if no images found.
+			if ( ! $images || 0 === $images->length ) {
 				continue;
 			}
 
-			$carousel_height     = $this->get_carousel_height( $node );
-			$carousel_attributes = array(
-				'height' => $carousel_height,
+			$amp_carousel = AMP_DOM_Utils::create_node( $this->dom, 'amp-carousel', array(
+				'height' => $this->get_carousel_height( $node ),
 				'type'   => 'slides',
 				'layout' => 'fixed-height',
-			);
-			$amp_carousel        = AMP_DOM_Utils::create_node( $this->dom, 'amp-carousel', $carousel_attributes );
-
-			for ( $j = $num_images - 1; $j >= 0; $j-- ) {
-				$amp_carousel->appendChild( $images->item( $j ) );
+			) );
+			foreach ( $images as $image ) {
+				$amp_carousel->appendChild( $image );
 			}
 
 			$node->parentNode->replaceChild( $amp_carousel, $node );
@@ -117,39 +112,47 @@ public function sanitize() {
 	/**
 	 * Get carousel height by containing images.
 	 *
-	 * @param DOMNode $node Node <ul>.
-	 * @return int
+	 * @param DOMElement $element The UL element.
+	 * @return int Height.
 	 */
-	protected function get_carousel_height( $node ) {
-		$images     = $node->getElementsByTagName( 'amp-img' );
+	protected function get_carousel_height( $element ) {
+		$images     = $element->getElementsByTagName( 'amp-img' );
 		$num_images = $images->length;
-		$height     = false;
+		$max_height = 0;
+		$max_width  = 0;
 		if ( 0 === $num_images ) {
 			return self::FALLBACK_HEIGHT;
 		}
-		for ( $i = $num_images - 1; $i >= 0; $i-- ) {
-			$image        = $images->item( $i );
+		foreach ( $images as $image ) {
+			/**
+			 * Image.
+			 *
+			 * @var DOMElement $image
+			 */
 			$image_height = $image->getAttribute( 'height' );
-			if ( ! $image_height || ! is_numeric( $image_height ) ) {
-				continue;
+			if ( is_numeric( $image_height ) ) {
+				$max_height = max( $max_height, $image_height );
 			}
-			if ( ! $height ) {
-				$height = $image_height;
-			} elseif ( $height > $image_height ) {
-				$height = $image_height;
+			$image_width = $image->getAttribute( 'height' );
+			if ( is_numeric( $image_width ) ) {
+				$max_width = max( $max_width, $image_width );
 			}
 		}
 
-		return false === $height ? self::FALLBACK_HEIGHT : $height;
+		if ( ! empty( $this->args['content_max_width'] ) && $max_height > 0 && $max_width > $this->args['content_max_width'] ) {
+			$max_height = ( $max_width * $this->args['content_max_width'] ) / $max_height;
+		}
+
+		return ! $max_height ? self::FALLBACK_HEIGHT : $max_height;
 	}
 
 	/**
 	 * Set lightbox related attributes to <amp-img> within gallery.
 	 *
-	 * @param DOMNode $node <ul> node.
+	 * @param DOMElement $element The UL element.
 	 */
-	protected function add_lightbox_attributes_to_image_nodes( $node ) {
-		$images     = $node->getElementsByTagName( 'amp-img' );
+	protected function add_lightbox_attributes_to_image_nodes( $element ) {
+		$images     = $element->getElementsByTagName( 'amp-img' );
 		$num_images = $images->length;
 		if ( 0 === $num_images ) {
 			return;

diff --git a/tests/test-class-amp-gallery-block-sanitizer.php b/tests/test-class-amp-gallery-block-sanitizer.php
@@ -58,7 +58,9 @@ public function get_data() {
 	 */
 	public function test_sanitizer( $source, $expected ) {
 		$dom       = AMP_DOM_Utils::get_dom_from_content( $source );
-		$sanitizer = new AMP_Gallery_Block_Sanitizer( $dom );
+		$sanitizer = new AMP_Gallery_Block_Sanitizer( $dom, array(
+			'content_max_width' => 600,
+		) );
 		$sanitizer->sanitize();
 		$content = AMP_DOM_Utils::get_content_from_dom( $dom );
 		$content = preg_replace( '/(?<=>)\s+(?=<)/', '', $content );