Improve validation error listing with more context and links to file editors

assets/css/src/amp-validation-error-taxonomy.css

@@ -78,7 +78,38 @@ details.single-error-detail[open] .single-error-detail-summary::after {
 
 .notice .detailed details .detailed {
 	padding-left: 32px;
-	font-family: Consolas, Monaco, monospace;
+}
+
+dl.detailed dt {
+	font-weight: bold;
+	margin-bottom: 0.5em;
+}
+
+dl.detailed dd + dt {
+	margin-top: 1em;
+}
+
+dl.detailed .element-attributes th {
+	font-weight: bold;
+	text-align: right;
+}
+
+dl.detailed .element-attributes th.has-attr-value::after {
+	content: ":";
+}
+
+dl.detailed .element-attributes th,
+dl.detailed .element-attributes td {
+	padding: 2px;
+	vertical-align: top;
+}
+
+dl.detailed a {
+	text-decoration: underline;
+}
+
+dl.detailed {
+	padding-bottom: 16px;
 }
 
 .details-attributes__title code,
@@ -222,6 +253,14 @@ body.post-type-amp_validated_url .wp-list-table .new td {
 	background-color: #fef7f1;
 }
 
+body.taxonomy-amp_validation_error .wp-list-table .new.odd th,
+body.taxonomy-amp_validation_error .wp-list-table .new.odd td,
+tr.expanded.new.odd + tr > td:first-of-type,
+body.post-type-amp_validated_url .wp-list-table .new.odd th,
+body.post-type-amp_validated_url .wp-list-table .new.odd td {
+	background-color: #f9eee5;
+}
+
 body.taxonomy-amp_validation_error .wp-list-table .new th.check-column,
 tr.expanded.new + tr > td:first-of-type,
 body.post-type-amp_validated_url .wp-list-table .new th.check-column {
@@ -296,3 +335,24 @@ body.taxonomy-amp_validation_error .wp-list-table .new th.check-column input {
 	cursor: pointer;
 	text-decoration: underline;
 }
+
+.validation-error-sources th {
+	font-weight: bold;
+	text-align: right;
+}
+
+.validation-error-sources td,
+.validation-error-sources th {
+	vertical-align: top;
+	padding: 0 4px;
+}
+
+.validation-error-sources > li {
+	border-top: solid 1px #ddd;
+	margin: 0;
+	padding: 6px 0;
+}
+
+.validation-error-sources > li:first-child {
+	border-top: 0;
+}

assets/css/src/amp-validation-single-error-url.css

@@ -63,19 +63,9 @@ table.striped > tbody > tr.even {
 	cursor: pointer;
 }
 
-.details ul.detailed {
-	padding: 0 32px;
-	margin-top: 0;
-}
-
-.details div.detailed {
+.details dl.detailed {
 	padding-left: 30px;
 	margin-top: 10px;
-	font-family: Consolas, Monaco, monospace;
-}
-
-.details .detailed details {
-	padding-bottom: 16px;
 }
 
 .details .detailed summary code {

assets/src/amp-validation/amp-validated-url-post-edit-screen.js

@@ -245,7 +245,7 @@ const handleSearching = () => {
 		const detailsQuery = document.querySelectorAll( 'tbody .column-details' );
 
 		/*
-		 * Iterate through the 'Details' column of each row.
+		 * Iterate through the 'Context' (formerly 'Details') column of each row.
 		 * If the search query is not present, hide the row.
 		 */
 		let numberErrorsDisplaying = 0;

includes/sanitizers/class-amp-core-theme-sanitizer.php

@@ -198,8 +198,7 @@ public static function get_supported_themes() {
 	public static function get_acceptable_errors( $template ) {
 		if ( isset( self::$theme_features[ $template ] ) ) {
 			return [
-				'removed_unused_css_rules' => true,
-				'illegal_css_at_rule'      => [
+				'illegal_css_at_rule' => [
 					[
 						'at_rule' => 'viewport',
 					],

includes/validation/class-amp-validated-url-post-type.php

@@ -934,30 +934,30 @@ public static function add_single_post_columns() {
 		return [
 			'cb'                          => '<input type="checkbox" />',
 			'error'                       => __( 'Error', 'amp' ),
-			'status'                      => sprintf(
+			'error_type'                  => __( 'Type', 'amp' ),
+			'details'                     => sprintf(
 				'%s<span class="dashicons dashicons-editor-help tooltip-button" tabindex="0"></span><div class="tooltip" hidden data-content="%s"></div>',
-				esc_html__( 'Status', 'amp' ),
+				esc_html__( 'Context', 'amp' ),
 				esc_attr(
 					sprintf(
 						'<h3>%s</h3><p>%s</p>',
-						esc_html__( 'Status', 'amp' ),
-						esc_html__( 'An accepted validation error is one that will not block a URL from being served as AMP; the validation error will be sanitized, normally resulting in the offending markup being stripped from the response to ensure AMP validity.', 'amp' )
+						esc_html__( 'Context', 'amp' ),
+						esc_html__( 'The parent element of where the error occurred.', 'amp' )
 					)
 				)
 			),
-			'details'                     => sprintf(
+			'sources_with_invalid_output' => __( 'Sources', 'amp' ),
+			'status'                      => sprintf(
 				'%s<span class="dashicons dashicons-editor-help tooltip-button" tabindex="0"></span><div class="tooltip" hidden data-content="%s"></div>',
-				esc_html__( 'Details', 'amp' ),
+				esc_html__( 'Status', 'amp' ),
 				esc_attr(
 					sprintf(
 						'<h3>%s</h3><p>%s</p>',
-						esc_html__( 'Details', 'amp' ),
-						esc_html__( 'The parent element of where the error occurred.', 'amp' )
+						esc_html__( 'Status', 'amp' ),
+						esc_html__( 'An accepted validation error is one that will not block a URL from being served as AMP; the validation error will be sanitized, normally resulting in the offending markup being stripped from the response to ensure AMP validity.', 'amp' )
 					)
 				)
 			),
-			'sources_with_invalid_output' => __( 'Sources', 'amp' ),
-			'error_type'                  => __( 'Type', 'amp' ),
 		];
 	}
 
@@ -990,18 +990,18 @@ public static function output_custom_column( $column_name, $post_id ) {
 				if ( ! empty( $error_summary[ AMP_Validation_Error_Taxonomy::REMOVED_ELEMENTS ] ) ) {
 					foreach ( $error_summary[ AMP_Validation_Error_Taxonomy::REMOVED_ELEMENTS ] as $name => $count ) {
 						if ( 1 === (int) $count ) {
-							$items[] = sprintf( '<code>%s</code>', esc_html( $name ) );
+							$items[] = sprintf( '<code>&lt;%s&gt;</code>', esc_html( $name ) );
 						} else {
-							$items[] = sprintf( '<code>%s</code> (%d)', esc_html( $name ), $count );
+							$items[] = sprintf( '<code>&lt;%s&gt;</code> (%d)', esc_html( $name ), $count );
 						}
 					}
 				}
 				if ( ! empty( $error_summary[ AMP_Validation_Error_Taxonomy::REMOVED_ATTRIBUTES ] ) ) {
 					foreach ( $error_summary[ AMP_Validation_Error_Taxonomy::REMOVED_ATTRIBUTES ] as $name => $count ) {
 						if ( 1 === (int) $count ) {
-							$items[] = sprintf( '<code>[%s]</code>', esc_html( $name ) );
+							$items[] = sprintf( '<code>%s</code>', esc_html( $name ) );
 						} else {
-							$items[] = sprintf( '<code>[%s]</code> (%d)', esc_html( $name ), $count );
+							$items[] = sprintf( '<code>%s</code> (%d)', esc_html( $name ), $count );
 						}
 					}
 				}
@@ -1029,7 +1029,7 @@ public static function render_sources_column( $error_summary, $post_id ) {
 			return;
 		}
 
-		// Show nothing if there are no valudation errors.
+		// Show nothing if there are no validation errors.
 		if ( 0 === count( array_filter( $error_summary ) ) ) {
 			esc_html_e( '--', 'amp' );
 			return;
@@ -1043,22 +1043,19 @@ public static function render_sources_column( $error_summary, $post_id ) {
 
 		$sources = $error_summary[ AMP_Validation_Error_Taxonomy::SOURCES_INVALID_OUTPUT ];
 		$output  = [];
-		$plugins = get_plugins();
 		foreach ( wp_array_slice_assoc( $sources, [ 'plugin', 'mu-plugin' ] ) as $type => $slugs ) {
 			$plugin_names = [];
 			$plugin_slugs = array_unique( $slugs );
 			foreach ( $plugin_slugs as $plugin_slug ) {
 				if ( 'mu-plugin' === $type ) {
 					$plugin_names[] = $plugin_slug;
 				} else {
-					$name = $plugin_slug;
-					foreach ( $plugins as $plugin_file => $plugin_data ) {
-						if ( strtok( $plugin_file, '/' ) === $plugin_slug ) {
-							$name = $plugin_data['Name'];
-							break;
-						}
+					$plugin_name = $plugin_slug;
+					$plugin      = AMP_Validation_Error_Taxonomy::get_plugin_from_slug( $plugin_slug );
+					if ( $plugin ) {
+						$plugin_name = $plugin['data']['Name'];
 					}
-					$plugin_names[] = $name;
+					$plugin_names[] = $plugin_name;
 				}
 			}
 			$count = count( $plugin_names );
@@ -1580,11 +1577,7 @@ public static function handle_validation_error_status_update() {
 			'amp_taxonomy_terms_updated' => $updated_count,
 		];
 
-		/*
-		 * Re-check the post after the validation status change. This is particularly important for validation errors like
-		 * 'removed_unused_css_rules' since whether it is accepted will determine whether other validation errors are triggered
-		 * such as in this case 'excessive_css'.
-		 */
+		// Re-check the post after the validation status change.
 		if ( $updated_count > 0 ) {
 			$validation_results = self::recheck_post( $post->ID );
 			// @todo For WP_Error case, see <https://github.com/ampproject/amp-wp/issues/1166>.