ampproject · westonruter · Jul 6, 2020 · Jul 4, 2020 · Jul 4, 2020 · Jul 4, 2020
diff --git a/assets/css/src/amp-icons.css b/assets/css/src/amp-icons.css
@@ -26,6 +26,14 @@ body .amp-icon {
 	content: url('data:image/svg+xml;charset=utf8,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 20 20"><rect x="0" fill="none" width="20" height="20"/><g><path fill="%2346b450" d="M10 2c-4.42 0-8 3.58-8 8s3.58 8 8 8 8-3.58 8-8-3.58-8-8-8zm-.615 12.66h-1.34l-3.24-4.54 1.34-1.25 2.57 2.4 5.14-5.93 1.34.94-5.81 8.38z"/></g></svg>');
 }
 
+.amp-icon.amp-logo::before {
+	width: 20px;
+	height: 20px;
+	display: inline-block;
+
+	content: url('data:image/svg+xml;charset=utf8,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 30 30"><g fill="none" fill-rule="evenodd"><circle fill="%23FFF" cx="15" cy="15" r="10"/><path fill="%23005AF0" fill-rule="nonzero" d="M13.85 24.098h-1.14l1.128-6.823-3.49.005h-.05a.57.57 0 0 1-.568-.569c0-.135.125-.363.125-.363l6.272-10.46 1.16.005-1.156 6.834 3.508-.004h.056c.314 0 .569.254.569.568 0 .128-.05.24-.121.335L13.85 24.098zM15 0C6.716 0 0 6.716 0 15c0 8.284 6.716 15 15 15 8.285 0 15-6.716 15-15 0-8.284-6.715-15-15-15z"/></g></svg>');
+}
+
 .amp-icon.amp-invalid::before {
 	content: "\f153";
 

diff --git a/includes/amp-helper-functions.php b/includes/amp-helper-functions.php
@@ -1752,7 +1752,9 @@ function amp_add_admin_bar_view_link( $wp_admin_bar ) {
 		return;
 	}
 
-	if ( is_amp_endpoint() ) {
+	$is_amp_endpoint = is_amp_endpoint();
+
+	if ( $is_amp_endpoint ) {
 		$href = amp_remove_endpoint( amp_get_current_url() );
 	} elseif ( is_singular() ) {
 		$href = amp_get_permalink( get_queried_object_id() ); // For sake of Reader mode.
@@ -1762,22 +1764,42 @@ function amp_add_admin_bar_view_link( $wp_admin_bar ) {
 
 	$href = remove_query_arg( QueryVars::NOAMP, $href );
 
-	$parent = [
-		'id'    => 'amp',
-		'title' => sprintf(
-			'%s %s',
-			Icon::link()->to_html(
-				[
-					'id'    => 'amp-admin-bar-item-status-icon',
-					'class' => 'ab-icon',
-				]
+	if ( $is_amp_endpoint ) {
+		$icon = Icon::logo()->to_html(
+			[
+				'id'    => 'amp-admin-bar-item-status-icon',
+				'class' => 'ab-icon',
+			]
+		);
+	} else {
+		$icon = Icon::link()->to_html(
+			[
+				'id'    => 'amp-admin-bar-item-status-icon',
+				'class' => 'ab-icon',
+			]
+		);
+	}
+
+	$wp_admin_bar->add_node(
+		[
+			'id'    => 'amp',
+			'title' => sprintf(
+				'%s %s',
+				$icon,
+				esc_html__( 'AMP', 'amp' )
 			),
-			esc_html( is_amp_endpoint() ? __( 'Non-AMP', 'amp' ) : __( 'AMP', 'amp' ) )
-		),
-		'href'  => esc_url( $href ),
-	];
+			'href'  => esc_url( $href ),
+		]
+	);
 
-	$wp_admin_bar->add_node( $parent );
+	$wp_admin_bar->add_node(
+		[
+			'parent' => 'amp',
+			'id'     => 'amp-view',
+			'title'  => esc_html( $is_amp_endpoint ? __( 'View non-AMP version', 'amp' ) : __( 'View AMP version', 'amp' ) ),
+			'href'   => esc_url( $href ),
+		]
+	);
 }
 
 /**

diff --git a/includes/validation/class-amp-validation-manager.php b/includes/validation/class-amp-validation-manager.php
@@ -416,11 +416,12 @@ public static function add_admin_bar_menu_items( $wp_admin_bar ) {
 		$validate_item = [
 			'parent' => 'amp',
 			'id'     => 'amp-validity',
-			'title'  => esc_html__( 'Validate', 'amp' ),
+			'title'  => esc_html__( 'Validate URL', 'amp' ),
 			'href'   => esc_url( $validate_url ),
 		];
 
 		// Construct admin bar item to link to AMP version or non-AMP version.
+		$wp_admin_bar->remove_node( 'amp-view' ); // Remove so we can re-add in the right position.
 		$link_item = [
 			'parent' => 'amp',
 			'id'     => 'amp-view',
@@ -1851,7 +1852,7 @@ public static function finalize_validation( Document $dom ) {
 		 * Override AMP status in admin bar set in \AMP_Validation_Manager::add_admin_bar_menu_items()
 		 * when there are validation errors which have not been explicitly accepted.
 		 */
-		if ( is_admin_bar_showing() && self::$amp_admin_bar_item_added ) {
+		if ( is_admin_bar_showing() && self::$amp_admin_bar_item_added && $total_count > 0 ) {
 			self::update_admin_bar_item( $dom, $total_count, $kept_count, $unreviewed_count );
 		}
 
@@ -1899,7 +1900,7 @@ public static function finalize_validation( Document $dom ) {
 	 * when there are validation errors which have not been explicitly accepted.
 	 *
 	 * @param Document $dom              Document.
-	 * @param int      $total_count      Total count of validation errors.
+	 * @param int      $total_count      Total count of validation errors (more than 0).
 	 * @param int      $kept_count       Count of validation errors with invalid markup kept.
 	 * @param int      $unreviewed_count Count of unreviewed validation errors.
 	 */
@@ -1930,53 +1931,83 @@ private static function update_admin_bar_item( Document $dom, $total_count, $kep
 		 */
 		if ( $kept_count > 0 && ! amp_is_canonical() ) {
 			$admin_bar_icon->setAttribute( 'class', 'ab-icon amp-icon ' . Icon::INVALID );
-		} elseif ( $unreviewed_count > 0 ) {
+		} elseif ( $unreviewed_count > 0 || $kept_count > 0 ) {
 			$admin_bar_icon->setAttribute( 'class', 'ab-icon amp-icon ' . Icon::WARNING );
 		}
 
-		$text = sprintf(
-			/* translators: %s is total count of validation errors */
-			_n(
-				'Validate %s issue',
-				'Validate %s issues',
-				$total_count,
-				'amp'
-			),
-			number_format_i18n( $total_count )
-		);
-
+		// Update the text of the link to reflect the status of the validation error(s).
 		$items = [];
 		if ( $unreviewed_count > 0 ) {
-			$items[] = sprintf(
-				/* translators: %s the total count of unreviewed validation errors */
-				_n(
-					'%s unreviewed',
-					'%s unreviewed',
-					$total_count,
+			if ( $unreviewed_count === $total_count ) {
+				/* translators: text is describing validation issue(s) */
+				$items[] = _n(
+					'unreviewed',
+					'all unreviewed',
+					$unreviewed_count,
 					'amp'
-				),
-				number_format_i18n( $unreviewed_count )
-			);
+				);
+			} else {
+				$items[] = sprintf(
+					/* translators: %s the total count of unreviewed validation errors */
+					_n(
+						'%s unreviewed',
+						'%s unreviewed',
+						$unreviewed_count,
+						'amp'
+					),
+					number_format_i18n( $unreviewed_count )
+				);
+			}
 		}
 		if ( $kept_count > 0 ) {
-			$items[] = sprintf(
-				/* translators: %s the total count of unreviewed validation errors */
-				_n(
-					'%s kept',
-					'%s kept',
+			if ( $kept_count === $total_count ) {
+				/* translators: text is describing validation issue(s) */
+				$items[] = _n(
+					'kept',
+					'all kept',
 					$kept_count,
 					'amp'
-				),
-				number_format_i18n( $kept_count )
-			);
+				);
+			} else {
+				$items[] = sprintf(
+					/* translators: %s the total count of unreviewed validation errors */
+					_n(
+						'%s kept',
+						'%s kept',
+						$kept_count,
+						'amp'
+					),
+					number_format_i18n( $kept_count )
+				);
+			}
 		}
-		if ( $items ) {
-			$text .= sprintf( ' (%s)', implode( ', ', $items ) );
+		if ( empty( $items ) ) {
+			/* translators: text is describing validation issue(s) */
+			$items[] = _n(
+				'reviewed',
+				'all reviewed',
+				$total_count,
+				'amp'
+			);
 		}
 
-		if ( $validate_link->firstChild instanceof DOMText ) {
-			$validate_link->firstChild->nodeValue = $text;
-		}
+		$text = sprintf(
+			/* translators: %s is total count of validation errors */
+			_n(
+				'%s issue:',
+				'%s issues:',
+				$total_count,
+				'amp'
+			),
+			number_format_i18n( $total_count )
+		);
+		$text .= ' ' . implode( ', ', $items );
+
+		$validate_link->appendChild( $dom->createTextNode( ' ' ) );
+		$small = $dom->createElement( 'small' );
+		$small->setAttribute( 'style', 'font-size: smaller' );
+		$small->appendChild( $dom->createTextNode( sprintf( '(%s)', $text ) ) );
+		$validate_link->appendChild( $small );
 	}
 
 	/**

diff --git a/src/Icon.php b/src/Icon.php
@@ -34,6 +34,11 @@ final class Icon {
 	 */
 	const WARNING = 'amp-warning';
 
+	/**
+	 * Indicates being on an AMP page.
+	 */
+	const LOGO = 'amp-logo';
+
 	/**
 	 * Icon class name.
 	 *
@@ -86,6 +91,15 @@ public static function warning() {
 		return new self( self::WARNING );
 	}
 
+	/**
+	 * Logo icon
+	 *
+	 * @return Icon
+	 */
+	public static function logo() {
+		return new self( self::LOGO );
+	}
+
 	/**
 	 * Get color for current icon.
 	 *
@@ -114,10 +128,10 @@ public function get_color() {
 	 * @return string Rendered HTML.
 	 */
 	public function to_html( $attributes = [] ) {
-		$icon_class = ' amp-icon ' . $this->icon;
+		$icon_class = 'amp-icon ' . $this->icon;
 
-		$attributes['class'] = isset( $attributes['class'] )
-			? $attributes['class'] . $icon_class
+		$attributes['class'] = ! empty( $attributes['class'] )
+			? $attributes['class'] . ' ' . $icon_class
 			: $icon_class;
 
 		$attributes_string = implode(
@@ -126,8 +140,8 @@ public function to_html( $attributes = [] ) {
 				static function ( $key, $value ) {
 					return sprintf(
 						'%s="%s"',
-						htmlspecialchars( $key ),
-						htmlspecialchars( $value )
+						esc_attr( sanitize_key( $key ) ),
+						esc_attr( $value )
 					);
 				},
 				array_keys( $attributes ),

diff --git a/tests/php/src/Unit/IconTest.php b/tests/php/src/Unit/IconTest.php
@@ -0,0 +1,66 @@
+<?php
+
+namespace AmpProject\AmpWP\Tests\Unit;
+
+use AmpProject\AmpWP\Icon;
+use AmpProject\AmpWP\Tests\AssertContainsCompatibility;
+use PHPUnit\Framework\TestCase;
+
+final class IconTest extends TestCase {
+
+	use AssertContainsCompatibility;
+
+	/** @return array */
+	public function get_icon_types() {
+		$types = [
+			'invalid',
+			'link',
+			'valid',
+			'warning',
+			'logo',
+		];
+
+		$data = [];
+		foreach ( $types as $type ) {
+			$data[ $type ] = [ $type ];
+		}
+		return $data;
+	}
+
+	/**
+	 * @param string $type Icon type.
+	 * @dataProvider get_icon_types
+	 * @covers Icon::__construct()
+	 * @covers Icon::invalid()
+	 * @covers Icon::link()
+	 * @covers Icon::valid()
+	 * @covers Icon::warning()
+	 * @covers Icon::logo()
+	 * @covers Icon::get_color()
+	 * @covers Icon::to_html()
+	 */
+	public function test_types( $type ) {
+		/** @var Icon $icon */
+		$icon = Icon::$type();
+		$this->assertInstanceOf( Icon::class, $icon );
+
+		$this->assertInternalType( 'string', $icon->get_color() );
+
+		$html = $icon->to_html();
+		$this->assertStringStartsWith( '<span ', $html );
+		$this->assertStringEndsWith( '</span>', $html );
+		$this->assertStringContains( "class=\"amp-icon amp-{$type}\"", $html );
+
+		$html = $icon->to_html(
+			[
+				'id'          => 'amp-admin-bar-item',
+				'class'       => '" onclick="alert(\"evil\")">end',
+				'onmouseover' => 'alert("BAD")',
+			]
+		);
+		$this->assertStringContains( "class=\"&quot; onclick=&quot;alert(\&quot;evil\&quot;)&quot;&gt;end amp-icon amp-{$type}\"", $html );
+		$this->assertStringContains( 'id="amp-admin-bar-item"', $html );
+		$this->assertStringNotContains( 'onmouseover', $html );
+		$this->assertStringEndsWith( '</span>', $html );
+	}
+}
diff --git a/tests/php/validation/test-class-amp-validation-error-taxonomy.php b/tests/php/validation/test-class-amp-validation-error-taxonomy.php
@@ -1228,7 +1228,7 @@ public function test_filter_manage_custom_columns() {
 		// Test the 'status' block in the switch for the error taxonomy page.
 		$GLOBALS['pagenow'] = 'edit-tags.php';
 		$filtered_content   = AMP_Validation_Error_Taxonomy::filter_manage_custom_columns( $initial_content, 'status', $term_id );
-		$this->assertStringContains( '<span class="status-text"><span class=" amp-icon amp-invalid"></span> Kept</span>', $filtered_content );
+		$this->assertStringContains( '<span class="status-text"><span class="amp-icon amp-invalid"></span> Kept</span>', $filtered_content );
 
 		// Test the 'status' block switch for the single error page.
 		$GLOBALS['pagenow'] = 'post.php';