Skip to content

Commit ba15800

Browse files
westonruterwestonruter
committed
Eliminate CSS tree shaking from being optional
1 parent b22a32d commit ba15800

9 files changed

+75
-246
lines changed

‎includes/options/class-amp-options-manager.php

-2
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@ class AMP_Options_Manager {
2727
'supported_post_types' => array( 'post' ),
2828
'analytics' => array(),
2929
'auto_accept_sanitization' => true,
30-
'accept_tree_shaking' => true,
3130
'all_templates_supported' => true,
3231
'supported_templates' => array( 'is_singular' ),
3332
'enable_response_caching' => true,
@@ -135,7 +134,6 @@ public static function validate_options( $new_options ) {
135134
}
136135

137136
$options['auto_accept_sanitization'] = ! empty( $new_options['auto_accept_sanitization'] );
138-
$options['accept_tree_shaking'] = ! empty( $new_options['accept_tree_shaking'] );
139137
$options['enable_amp_stories'] = ! empty( $new_options['enable_amp_stories'] );
140138

141139
// Validate post type support.

‎includes/options/class-amp-options-menu.php

-30
Original file line numberDiff line numberDiff line change
@@ -270,15 +270,8 @@ public function render_validation_handling() {
270270
'code' => 'non_existent',
271271
)
272272
);
273-
remove_filter( 'amp_validation_error_sanitized', array( 'AMP_Validation_Manager', 'filter_tree_shaking_validation_error_as_accepted' ) );
274-
$tree_shaking_sanitization = AMP_Validation_Error_Taxonomy::get_validation_error_sanitization(
275-
array(
276-
'code' => AMP_Style_Sanitizer::TREE_SHAKING_ERROR_CODE,
277-
)
278-
);
279273

280274
$forced_sanitization = 'with_filter' === $auto_sanitization['forced'];
281-
$forced_tree_shaking = $forced_sanitization || 'with_filter' === $tree_shaking_sanitization['forced'];
282275
?>
283276

284277
<?php if ( $forced_sanitization ) : ?>
@@ -320,22 +313,6 @@ public function render_validation_handling() {
320313
</div>
321314
<?php endif; ?>
322315

323-
<?php if ( $forced_tree_shaking ) : ?>
324-
<input type="hidden" name="<?php echo esc_attr( AMP_Options_Manager::OPTION_NAME . '[accept_tree_shaking]' ); ?>" value="<?php echo AMP_Options_Manager::get_option( 'accept_tree_shaking' ) ? 'on' : ''; ?>">
325-
<?php else : ?>
326-
<div class="amp-tree-shaking">
327-
<p>
328-
<label for="accept_tree_shaking">
329-
<input id="accept_tree_shaking" type="checkbox" name="<?php echo esc_attr( AMP_Options_Manager::OPTION_NAME . '[accept_tree_shaking]' ); ?>" <?php checked( AMP_Options_Manager::get_option( 'accept_tree_shaking' ) ); ?>>
330-
<?php esc_html_e( 'Automatically remove CSS rules that are not relevant to a given page (tree shaking).', 'amp' ); ?>
331-
</label>
332-
</p>
333-
<p class="description">
334-
<?php esc_html_e( 'AMP limits the total amount of CSS to no more than 50KB; any more than this will cause a validation error. The need to tree shake the CSS is not done by default because in some situations (in particular for dynamic content) it can result in CSS rules being removed that are needed.', 'amp' ); ?>
335-
</p>
336-
</div>
337-
<?php endif; ?>
338-
339316
<script>
340317
(function( $ ) {
341318
var getThemeSupportMode = function() {
@@ -346,21 +323,14 @@ public function render_validation_handling() {
346323
return checkedInput.val();
347324
};
348325

349-
var updateTreeShakingHiddenClass = function() {
350-
var checkbox = $( '#auto_accept_sanitization' );
351-
$( '.amp-tree-shaking' ).toggleClass( 'hidden', checkbox.prop( 'checked' ) && 'native' !== getThemeSupportMode() );
352-
};
353-
354326
var updateHiddenClasses = function() {
355327
var themeSupportMode = getThemeSupportMode();
356328
$( '.amp-auto-accept-sanitize' ).toggleClass( 'hidden', 'native' === themeSupportMode );
357329
$( '.amp-validation-field' ).toggleClass( 'hidden', 'disabled' === themeSupportMode );
358330
$( '.amp-auto-accept-sanitize-canonical' ).toggleClass( 'hidden', 'native' !== themeSupportMode );
359-
updateTreeShakingHiddenClass();
360331
};
361332

362333
$( 'input[type=radio][name="amp-options[theme_support]"]' ).change( updateHiddenClasses );
363-
$( '#auto_accept_sanitization' ).change( updateTreeShakingHiddenClass );
364334

365335
updateHiddenClasses();
366336
})( jQuery );

‎includes/sanitizers/class-amp-style-sanitizer.php

+42-79
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,6 @@
2626
*/
2727
class AMP_Style_Sanitizer extends AMP_Base_Sanitizer {
2828

29-
/**
30-
* Error code for tree shaking.
31-
*
32-
* @var string
33-
*/
34-
const TREE_SHAKING_ERROR_CODE = 'removed_unused_css_rules';
35-
3629
/**
3730
* Error code for illegal at-rule.
3831
*
@@ -87,15 +80,13 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer {
8780
* Array of flags used to control sanitization.
8881
*
8982
* @var array {
90-
* @type string $remove_unused_rules Enum 'never', 'sometimes' (default), 'always'. If total CSS is greater than max_bytes, whether to strip selectors (and then empty rules) when they are not found to be used in doc. A validation error will be emitted when stripping happens since it is not completely safe in the case of dynamic content.
9183
* @type string[] $dynamic_element_selectors Selectors for elements (or their ancestors) which contain dynamic content; selectors containing these will not be filtered.
9284
* @type bool $use_document_element Whether the root of the document should be used rather than the body.
9385
* @type bool $require_https_src Require HTTPS URLs.
9486
* @type bool $allow_dirty_styles Allow dirty styles. This short-circuits the sanitize logic; it is used primarily in Customizer preview.
9587
* @type callable $validation_error_callback Function to call when a validation error is encountered.
9688
* @type bool $should_locate_sources Whether to locate the sources when reporting validation errors.
9789
* @type string $parsed_cache_variant Additional value by which to vary parsed cache.
98-
* @type bool $accept_tree_shaking Whether to accept tree-shaking by default and bypass a validation error.
9990
* @type string $include_manifest_comment Whether to show the manifest HTML comment in the response before the style[amp-custom] element. Can be 'always', 'never', or 'when_excessive'.
10091
* }
10192
*/
@@ -107,7 +98,6 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer {
10798
* @var array
10899
*/
109100
protected $DEFAULT_ARGS = array(
110-
'remove_unused_rules' => 'sometimes',
111101
'dynamic_element_selectors' => array(
112102
'amp-list',
113103
'amp-live-list',
@@ -116,7 +106,6 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer {
116106
),
117107
'should_locate_sources' => false,
118108
'parsed_cache_variant' => null,
119-
'accept_tree_shaking' => false,
120109
'include_manifest_comment' => 'always',
121110
);
122111

@@ -298,7 +287,6 @@ public static function get_css_parser_validation_error_codes() {
298287
self::ILLEGAL_AT_RULE_ERROR_CODE,
299288
'illegal_css_important',
300289
'illegal_css_property',
301-
self::TREE_SHAKING_ERROR_CODE,
302290
'unrecognized_css',
303291
'disallowed_file_extension',
304292
'file_path_not_found',
@@ -2388,7 +2376,7 @@ private function collect_inline_styles( $element ) {
23882376
/**
23892377
* Finalize stylesheets for style[amp-custom] and style[amp-keyframes] elements.
23902378
*
2391-
* Concatenate all pending stylesheets, remove unused rules if necessary, and add to style elements in doc.
2379+
* Concatenate all pending stylesheets, remove unused rules, and add to AMP style elements in document.
23922380
* Combine all amp-keyframe styles and add them to the end of the body.
23932381
*
23942382
* @since 1.0
@@ -2398,19 +2386,15 @@ private function finalize_styles() {
23982386
$stylesheet_groups = array(
23992387
'custom' => array(
24002388
'source_map_comment' => "\n\n/*# sourceURL=amp-custom.css */",
2401-
'total_size' => 0,
24022389
'cdata_spec' => $this->style_custom_cdata_spec,
24032390
'pending_stylesheets' => array(),
2404-
'remove_unused_rules' => $this->args['remove_unused_rules'],
24052391
'included_count' => 0,
24062392
'import_front_matter' => '', // Extra @import statements that are prepended when fetch fails and validation error is rejected.
24072393
),
24082394
'keyframes' => array(
24092395
'source_map_comment' => "\n\n/*# sourceURL=amp-keyframes.css */",
2410-
'total_size' => 0,
24112396
'cdata_spec' => $this->style_keyframes_cdata_spec,
24122397
'pending_stylesheets' => array(),
2413-
'remove_unused_rules' => 'never', // Not relevant.
24142398
'included_count' => 0,
24152399
'import_front_matter' => '',
24162400
),
@@ -2433,7 +2417,6 @@ private function finalize_styles() {
24332417
$size += strlen( $part[1] ); // Declaration block.
24342418
}
24352419
}
2436-
$stylesheet_groups[ $pending_stylesheet['group'] ]['total_size'] += $size; // Used in finalize_stylesheet_group() to determine if tree shaking is needed.
24372420

24382421
if ( ! empty( $pending_stylesheet['imported_font_urls'] ) ) {
24392422
$imported_font_urls = array_merge( $imported_font_urls, $pending_stylesheet['imported_font_urls'] );
@@ -2819,25 +2802,8 @@ function ( $selector_language ) {
28192802
* @return int Number of included stylesheets in group.
28202803
*/
28212804
private function finalize_stylesheet_group( $group, $group_config ) {
2822-
$included_count = 0;
2823-
$max_bytes = $group_config['cdata_spec']['max_bytes'] - strlen( $group_config['source_map_comment'] );
2824-
$is_too_much_css = $group_config['total_size'] > $max_bytes;
2825-
$should_tree_shake = (
2826-
'always' === $group_config['remove_unused_rules'] || (
2827-
$is_too_much_css
2828-
&&
2829-
'sometimes' === $group_config['remove_unused_rules']
2830-
)
2831-
);
2832-
2833-
if ( $is_too_much_css && $should_tree_shake && empty( $this->args['accept_tree_shaking'] ) ) {
2834-
$should_tree_shake = $this->should_sanitize_validation_error(
2835-
array(
2836-
'code' => self::TREE_SHAKING_ERROR_CODE,
2837-
'type' => AMP_Validation_Error_Taxonomy::CSS_ERROR_TYPE,
2838-
)
2839-
);
2840-
}
2805+
$included_count = 0;
2806+
$max_bytes = $group_config['cdata_spec']['max_bytes'] - strlen( $group_config['source_map_comment'] );
28412807

28422808
$previously_seen_stylesheet_index = array();
28432809
$indices_by_stylesheet_element_id = array();
@@ -2861,53 +2827,50 @@ private function finalize_stylesheet_group( $group, $group_config ) {
28612827
}
28622828

28632829
list( $selectors_parsed, $declaration_block ) = $stylesheet_part;
2864-
if ( $should_tree_shake ) {
2865-
$selectors = array();
2866-
foreach ( $selectors_parsed as $selector => $parsed_selector ) {
2867-
$should_include = (
2830+
2831+
$selectors = array();
2832+
foreach ( $selectors_parsed as $selector => $parsed_selector ) {
2833+
$should_include = (
2834+
(
2835+
// If all class names are used in the doc.
28682836
(
2869-
// If all class names are used in the doc.
2870-
(
2871-
empty( $parsed_selector[ self::SELECTOR_EXTRACTED_CLASSES ] )
2872-
||
2873-
$this->has_used_class_name( $parsed_selector[ self::SELECTOR_EXTRACTED_CLASSES ] )
2874-
)
2875-
&&
2876-
// If all IDs are used in the doc.
2877-
(
2878-
empty( $parsed_selector[ self::SELECTOR_EXTRACTED_IDS ] )
2879-
||
2880-
0 === count(
2881-
array_filter(
2882-
$parsed_selector[ self::SELECTOR_EXTRACTED_IDS ],
2883-
function( $id ) {
2884-
return ! $this->dom->getElementById( $id );
2885-
}
2886-
)
2837+
empty( $parsed_selector[ self::SELECTOR_EXTRACTED_CLASSES ] )
2838+
||
2839+
$this->has_used_class_name( $parsed_selector[ self::SELECTOR_EXTRACTED_CLASSES ] )
2840+
)
2841+
&&
2842+
// If all IDs are used in the doc.
2843+
(
2844+
empty( $parsed_selector[ self::SELECTOR_EXTRACTED_IDS ] )
2845+
||
2846+
0 === count(
2847+
array_filter(
2848+
$parsed_selector[ self::SELECTOR_EXTRACTED_IDS ],
2849+
function( $id ) {
2850+
return ! $this->dom->getElementById( $id );
2851+
}
28872852
)
28882853
)
2889-
&&
2890-
// If tag names are present in the doc.
2891-
(
2892-
empty( $parsed_selector[ self::SELECTOR_EXTRACTED_TAGS ] )
2893-
||
2894-
$this->has_used_tag_names( $parsed_selector[ self::SELECTOR_EXTRACTED_TAGS ] )
2895-
)
2896-
&&
2897-
// If all attribute names are used in the doc.
2898-
(
2899-
empty( $parsed_selector[ self::SELECTOR_EXTRACTED_ATTRIBUTES ] )
2900-
||
2901-
$this->has_used_attributes( $parsed_selector[ self::SELECTOR_EXTRACTED_ATTRIBUTES ] )
2902-
)
29032854
)
2904-
);
2905-
if ( $should_include ) {
2906-
$selectors[] = $selector;
2907-
}
2855+
&&
2856+
// If tag names are present in the doc.
2857+
(
2858+
empty( $parsed_selector[ self::SELECTOR_EXTRACTED_TAGS ] )
2859+
||
2860+
$this->has_used_tag_names( $parsed_selector[ self::SELECTOR_EXTRACTED_TAGS ] )
2861+
)
2862+
&&
2863+
// If all attribute names are used in the doc.
2864+
(
2865+
empty( $parsed_selector[ self::SELECTOR_EXTRACTED_ATTRIBUTES ] )
2866+
||
2867+
$this->has_used_attributes( $parsed_selector[ self::SELECTOR_EXTRACTED_ATTRIBUTES ] )
2868+
)
2869+
)
2870+
);
2871+
if ( $should_include ) {
2872+
$selectors[] = $selector;
29082873
}
2909-
} else {
2910-
$selectors = array_keys( $selectors_parsed );
29112874
}
29122875
$stylesheet_part = implode( ',', $selectors ) . $declaration_block;
29132876
$original_size += strlen( $stylesheet_part );

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

-3
Original file line numberDiff line numberDiff line change
@@ -797,9 +797,6 @@ public static function get_validated_environment() {
797797
return array(
798798
'theme' => get_stylesheet(),
799799
'plugins' => get_option( 'active_plugins', array() ),
800-
'options' => array(
801-
'accept_tree_shaking' => ( AMP_Options_Manager::get_option( 'accept_tree_shaking' ) || AMP_Options_Manager::get_option( 'auto_accept_sanitization' ) ),
802-
),
803800
);
804801
}
805802

‎includes/validation/class-amp-validation-manager.php

+2-22
Original file line numberDiff line numberDiff line change
@@ -206,11 +206,6 @@ function( $query_vars ) {
206206

207207
add_action( 'admin_bar_menu', array( __CLASS__, 'add_admin_bar_menu_items' ), 101 );
208208

209-
// Add filter to auto-accept tree shaking validation error.
210-
if ( AMP_Options_Manager::get_option( 'accept_tree_shaking' ) || AMP_Options_Manager::get_option( 'auto_accept_sanitization' ) ) {
211-
add_filter( 'amp_validation_error_sanitized', array( __CLASS__, 'filter_tree_shaking_validation_error_as_accepted' ), 10, 2 );
212-
}
213-
214209
if ( self::$should_locate_sources ) {
215210
self::add_validation_error_sourcing();
216211
}
@@ -240,20 +235,6 @@ public static function is_sanitization_auto_accepted() {
240235
return amp_is_canonical() || AMP_Options_Manager::get_option( 'auto_accept_sanitization' );
241236
}
242237

243-
/**
244-
* Filter a tree-shaking validation error as accepted for sanitization.
245-
*
246-
* @param bool $sanitized Sanitized.
247-
* @param array $error Error.
248-
* @return bool Sanitized.
249-
*/
250-
public static function filter_tree_shaking_validation_error_as_accepted( $sanitized, $error ) {
251-
if ( AMP_Style_Sanitizer::TREE_SHAKING_ERROR_CODE === $error['code'] ) {
252-
$sanitized = true;
253-
}
254-
return $sanitized;
255-
}
256-
257238
/**
258239
* Add menu items to admin bar for AMP.
259240
*
@@ -761,8 +742,8 @@ public static function add_validation_error( array $error, array $data = array()
761742
);
762743

763744
/*
764-
* Ignore validation errors which are forcibly sanitized by filter. This includes tree shaking error
765-
* accepted by options and via AMP_Validation_Error_Taxonomy::accept_validation_errors()).
745+
* Ignore validation errors which are forcibly sanitized by filter. This includes errors accepted via
746+
* AMP_Validation_Error_Taxonomy::accept_validation_errors(), such as the acceptable_errors in core themes.
766747
* This was introduced in <https://github.com/ampproject/amp-wp/pull/1413> to prevent forcibly-sanitized
767748
* validation errors from being reported, to avoid noise and wasted storage. It was inadvertently
768749
* reverted in de7b04b but then restored as part of <https://github.com/ampproject/amp-wp/pull/1413>.
@@ -1742,7 +1723,6 @@ public static function filter_sanitizer_args( $sanitizers ) {
17421723
if ( ! empty( $css_validation_errors ) ) {
17431724
$sanitizers['AMP_Style_Sanitizer']['parsed_cache_variant'] = md5( wp_json_encode( $css_validation_errors ) );
17441725
}
1745-
$sanitizers['AMP_Style_Sanitizer']['accept_tree_shaking'] = AMP_Options_Manager::get_option( 'accept_tree_shaking' );
17461726
}
17471727

17481728
return $sanitizers;

0 commit comments

Comments
 (0)