Skip to content

Commit

Permalink
Merge pull request #2508 from ampproject/remove/optional-tree-shaking
Browse files Browse the repository at this point in the history 
Always perform CSS tree shaking, regardless of total size
  • Loading branch information
@westonruter
westonruter authored Jun 5, 2019
2 parents e1c78fb + ba15800 commit b222d6f
Show file tree
Hide file tree
Showing 9 changed files with 75 additions and 246 deletions.
2 changes: 0 additions & 2 deletions includes/options/class-amp-options-manager.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ class AMP_Options_Manager {
'supported_post_types' => array( 'post' ),
'analytics' => array(),
'auto_accept_sanitization' => true,
'accept_tree_shaking' => true,
'all_templates_supported' => true,
'supported_templates' => array( 'is_singular' ),
'enable_response_caching' => true,
Expand Down Expand Up @@ -135,7 +134,6 @@ public static function validate_options( $new_options ) {
}

$options['auto_accept_sanitization'] = ! empty( $new_options['auto_accept_sanitization'] );
$options['accept_tree_shaking'] = ! empty( $new_options['accept_tree_shaking'] );
$options['enable_amp_stories'] = ! empty( $new_options['enable_amp_stories'] );

// Validate post type support.
Expand Down
30 changes: 0 additions & 30 deletions includes/options/class-amp-options-menu.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -270,15 +270,8 @@ public function render_validation_handling() {
'code' => 'non_existent',
)
);
remove_filter( 'amp_validation_error_sanitized', array( 'AMP_Validation_Manager', 'filter_tree_shaking_validation_error_as_accepted' ) );
$tree_shaking_sanitization = AMP_Validation_Error_Taxonomy::get_validation_error_sanitization(
array(
'code' => AMP_Style_Sanitizer::TREE_SHAKING_ERROR_CODE,
)
);

$forced_sanitization = 'with_filter' === $auto_sanitization['forced'];
$forced_tree_shaking = $forced_sanitization || 'with_filter' === $tree_shaking_sanitization['forced'];
?>

<?php if ( $forced_sanitization ) : ?>
Expand Down Expand Up @@ -320,22 +313,6 @@ public function render_validation_handling() {
</div>
<?php endif; ?>

<?php if ( $forced_tree_shaking ) : ?>
<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' : ''; ?>">
<?php else : ?>
<div class="amp-tree-shaking">
<p>
<label for="accept_tree_shaking">
<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' ) ); ?>>
<?php esc_html_e( 'Automatically remove CSS rules that are not relevant to a given page (tree shaking).', 'amp' ); ?>
</label>
</p>
<p class="description">
<?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' ); ?>
</p>
</div>
<?php endif; ?>

<script>
(function( $ ) {
var getThemeSupportMode = function() {
Expand All @@ -346,21 +323,14 @@ public function render_validation_handling() {
return checkedInput.val();
};

var updateTreeShakingHiddenClass = function() {
var checkbox = $( '#auto_accept_sanitization' );
$( '.amp-tree-shaking' ).toggleClass( 'hidden', checkbox.prop( 'checked' ) && 'native' !== getThemeSupportMode() );
};

var updateHiddenClasses = function() {
var themeSupportMode = getThemeSupportMode();
$( '.amp-auto-accept-sanitize' ).toggleClass( 'hidden', 'native' === themeSupportMode );
$( '.amp-validation-field' ).toggleClass( 'hidden', 'disabled' === themeSupportMode );
$( '.amp-auto-accept-sanitize-canonical' ).toggleClass( 'hidden', 'native' !== themeSupportMode );
updateTreeShakingHiddenClass();
};

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

updateHiddenClasses();
})( jQuery );
Expand Down
121 changes: 42 additions & 79 deletions includes/sanitizers/class-amp-style-sanitizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,6 @@
*/
class AMP_Style_Sanitizer extends AMP_Base_Sanitizer {

/**
* Error code for tree shaking.
*
* @var string
*/
const TREE_SHAKING_ERROR_CODE = 'removed_unused_css_rules';

/**
* Error code for illegal at-rule.
*
Expand Down Expand Up @@ -87,15 +80,13 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer {
* Array of flags used to control sanitization.
*
* @var array {
* @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.
* @type string[] $dynamic_element_selectors Selectors for elements (or their ancestors) which contain dynamic content; selectors containing these will not be filtered.
* @type bool $use_document_element Whether the root of the document should be used rather than the body.
* @type bool $require_https_src Require HTTPS URLs.
* @type bool $allow_dirty_styles Allow dirty styles. This short-circuits the sanitize logic; it is used primarily in Customizer preview.
* @type callable $validation_error_callback Function to call when a validation error is encountered.
* @type bool $should_locate_sources Whether to locate the sources when reporting validation errors.
* @type string $parsed_cache_variant Additional value by which to vary parsed cache.
* @type bool $accept_tree_shaking Whether to accept tree-shaking by default and bypass a validation error.
* @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'.
* }
*/
Expand All @@ -107,7 +98,6 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer {
* @var array
*/
protected $DEFAULT_ARGS = array(
'remove_unused_rules' => 'sometimes',
'dynamic_element_selectors' => array(
'amp-list',
'amp-live-list',
Expand All @@ -116,7 +106,6 @@ class AMP_Style_Sanitizer extends AMP_Base_Sanitizer {
),
'should_locate_sources' => false,
'parsed_cache_variant' => null,
'accept_tree_shaking' => false,
'include_manifest_comment' => 'always',
);

Expand Down Expand Up @@ -298,7 +287,6 @@ public static function get_css_parser_validation_error_codes() {
self::ILLEGAL_AT_RULE_ERROR_CODE,
'illegal_css_important',
'illegal_css_property',
self::TREE_SHAKING_ERROR_CODE,
'unrecognized_css',
'disallowed_file_extension',
'file_path_not_found',
Expand Down Expand Up @@ -2388,7 +2376,7 @@ private function collect_inline_styles( $element ) {
/**
* Finalize stylesheets for style[amp-custom] and style[amp-keyframes] elements.
*
* Concatenate all pending stylesheets, remove unused rules if necessary, and add to style elements in doc.
* Concatenate all pending stylesheets, remove unused rules, and add to AMP style elements in document.
* Combine all amp-keyframe styles and add them to the end of the body.
*
* @since 1.0
Expand All @@ -2398,19 +2386,15 @@ private function finalize_styles() {
$stylesheet_groups = array(
'custom' => array(
'source_map_comment' => "\n\n/*# sourceURL=amp-custom.css */",
'total_size' => 0,
'cdata_spec' => $this->style_custom_cdata_spec,
'pending_stylesheets' => array(),
'remove_unused_rules' => $this->args['remove_unused_rules'],
'included_count' => 0,
'import_front_matter' => '', // Extra @import statements that are prepended when fetch fails and validation error is rejected.
),
'keyframes' => array(
'source_map_comment' => "\n\n/*# sourceURL=amp-keyframes.css */",
'total_size' => 0,
'cdata_spec' => $this->style_keyframes_cdata_spec,
'pending_stylesheets' => array(),
'remove_unused_rules' => 'never', // Not relevant.
'included_count' => 0,
'import_front_matter' => '',
),
Expand All @@ -2433,7 +2417,6 @@ private function finalize_styles() {
$size += strlen( $part[1] ); // Declaration block.
}
}
$stylesheet_groups[ $pending_stylesheet['group'] ]['total_size'] += $size; // Used in finalize_stylesheet_group() to determine if tree shaking is needed.

if ( ! empty( $pending_stylesheet['imported_font_urls'] ) ) {
$imported_font_urls = array_merge( $imported_font_urls, $pending_stylesheet['imported_font_urls'] );
Expand Down Expand Up @@ -2819,25 +2802,8 @@ function ( $selector_language ) {
* @return int Number of included stylesheets in group.
*/
private function finalize_stylesheet_group( $group, $group_config ) {
$included_count = 0;
$max_bytes = $group_config['cdata_spec']['max_bytes'] - strlen( $group_config['source_map_comment'] );
$is_too_much_css = $group_config['total_size'] > $max_bytes;
$should_tree_shake = (
'always' === $group_config['remove_unused_rules'] || (
$is_too_much_css
&&
'sometimes' === $group_config['remove_unused_rules']
)
);

if ( $is_too_much_css && $should_tree_shake && empty( $this->args['accept_tree_shaking'] ) ) {
$should_tree_shake = $this->should_sanitize_validation_error(
array(
'code' => self::TREE_SHAKING_ERROR_CODE,
'type' => AMP_Validation_Error_Taxonomy::CSS_ERROR_TYPE,
)
);
}
$included_count = 0;
$max_bytes = $group_config['cdata_spec']['max_bytes'] - strlen( $group_config['source_map_comment'] );

$previously_seen_stylesheet_index = array();
$indices_by_stylesheet_element_id = array();
Expand All @@ -2861,53 +2827,50 @@ private function finalize_stylesheet_group( $group, $group_config ) {
}

list( $selectors_parsed, $declaration_block ) = $stylesheet_part;
if ( $should_tree_shake ) {
$selectors = array();
foreach ( $selectors_parsed as $selector => $parsed_selector ) {
$should_include = (

$selectors = array();
foreach ( $selectors_parsed as $selector => $parsed_selector ) {
$should_include = (
(
// If all class names are used in the doc.
(
// If all class names are used in the doc.
(
empty( $parsed_selector[ self::SELECTOR_EXTRACTED_CLASSES ] )
||
$this->has_used_class_name( $parsed_selector[ self::SELECTOR_EXTRACTED_CLASSES ] )
)
&&
// If all IDs are used in the doc.
(
empty( $parsed_selector[ self::SELECTOR_EXTRACTED_IDS ] )
||
0 === count(
array_filter(
$parsed_selector[ self::SELECTOR_EXTRACTED_IDS ],
function( $id ) {
return ! $this->dom->getElementById( $id );
}
)
empty( $parsed_selector[ self::SELECTOR_EXTRACTED_CLASSES ] )
||
$this->has_used_class_name( $parsed_selector[ self::SELECTOR_EXTRACTED_CLASSES ] )
)
&&
// If all IDs are used in the doc.
(
empty( $parsed_selector[ self::SELECTOR_EXTRACTED_IDS ] )
||
0 === count(
array_filter(
$parsed_selector[ self::SELECTOR_EXTRACTED_IDS ],
function( $id ) {
return ! $this->dom->getElementById( $id );
}
)
)
&&
// If tag names are present in the doc.
(
empty( $parsed_selector[ self::SELECTOR_EXTRACTED_TAGS ] )
||
$this->has_used_tag_names( $parsed_selector[ self::SELECTOR_EXTRACTED_TAGS ] )
)
&&
// If all attribute names are used in the doc.
(
empty( $parsed_selector[ self::SELECTOR_EXTRACTED_ATTRIBUTES ] )
||
$this->has_used_attributes( $parsed_selector[ self::SELECTOR_EXTRACTED_ATTRIBUTES ] )
)
)
);
if ( $should_include ) {
$selectors[] = $selector;
}
&&
// If tag names are present in the doc.
(
empty( $parsed_selector[ self::SELECTOR_EXTRACTED_TAGS ] )
||
$this->has_used_tag_names( $parsed_selector[ self::SELECTOR_EXTRACTED_TAGS ] )
)
&&
// If all attribute names are used in the doc.
(
empty( $parsed_selector[ self::SELECTOR_EXTRACTED_ATTRIBUTES ] )
||
$this->has_used_attributes( $parsed_selector[ self::SELECTOR_EXTRACTED_ATTRIBUTES ] )
)
)
);
if ( $should_include ) {
$selectors[] = $selector;
}
} else {
$selectors = array_keys( $selectors_parsed );
}
$stylesheet_part = implode( ',', $selectors ) . $declaration_block;
$original_size += strlen( $stylesheet_part );
Expand Down
3 changes: 0 additions & 3 deletions includes/validation/class-amp-validated-url-post-type.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -797,9 +797,6 @@ public static function get_validated_environment() {
return array(
'theme' => get_stylesheet(),
'plugins' => get_option( 'active_plugins', array() ),
'options' => array(
'accept_tree_shaking' => ( AMP_Options_Manager::get_option( 'accept_tree_shaking' ) || AMP_Options_Manager::get_option( 'auto_accept_sanitization' ) ),
),
);
}

Expand Down
24 changes: 2 additions & 22 deletions includes/validation/class-amp-validation-manager.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,11 +206,6 @@ function( $query_vars ) {

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

// Add filter to auto-accept tree shaking validation error.
if ( AMP_Options_Manager::get_option( 'accept_tree_shaking' ) || AMP_Options_Manager::get_option( 'auto_accept_sanitization' ) ) {
add_filter( 'amp_validation_error_sanitized', array( __CLASS__, 'filter_tree_shaking_validation_error_as_accepted' ), 10, 2 );
}

if ( self::$should_locate_sources ) {
self::add_validation_error_sourcing();
}
Expand Down Expand Up @@ -240,20 +235,6 @@ public static function is_sanitization_auto_accepted() {
return amp_is_canonical() || AMP_Options_Manager::get_option( 'auto_accept_sanitization' );
}

/**
* Filter a tree-shaking validation error as accepted for sanitization.
*
* @param bool $sanitized Sanitized.
* @param array $error Error.
* @return bool Sanitized.
*/
public static function filter_tree_shaking_validation_error_as_accepted( $sanitized, $error ) {
if ( AMP_Style_Sanitizer::TREE_SHAKING_ERROR_CODE === $error['code'] ) {
$sanitized = true;
}
return $sanitized;
}

/**
* Add menu items to admin bar for AMP.
*
Expand Down Expand Up @@ -761,8 +742,8 @@ public static function add_validation_error( array $error, array $data = array()
);

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

return $sanitizers;
Expand Down
Loading

0 comments on commit b222d6f

Please sign in to comment.