Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Restore admin bar on AMP pages and improve AMP menu items #1219

Merged
merged 5 commits into from
Jun 20, 2018
Merged

Restore admin bar on AMP pages and improve AMP menu items #1219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1be7ae1
Add recognition of the style element's media attribute
westonruter Jun 19, 2018
475642d
Automatically tree shake `* html` and `*+html` IE6/IE7 hack selectors
westonruter Jun 19, 2018
eed2c26
Insert important rulesets adjacent to the original ruleset to preserv…
westonruter Jun 19, 2018
3188501
Show admin bar in AMP
westonruter Jun 19, 2018
80ebbcb
Improve AMP admin bar menu item
westonruter Jun 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1,058 changes: 1,058 additions & 0 deletions assets/css/admin-bar.css
View file
Open in desktop

Large diffs are not rendered by default.

18 changes: 9 additions & 9 deletions composer.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

45 changes: 39 additions & 6 deletions includes/class-amp-theme-support.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,11 @@ public static function add_hooks() {
* Disable admin bar because admin-bar.css (28K) and Dashicons (48K) alone
* combine to surpass the 50K limit imposed for the amp-custom style.
*/
add_filter( 'show_admin_bar', '__return_false', 100 );
if ( AMP_Options_Manager::get_option( 'disable_admin_bar' ) ) {
add_filter( 'show_admin_bar', '__return_false', 100 );
} else {
add_action( 'admin_bar_init', array( __CLASS__, 'init_admin_bar' ) );
}

/*
* Start output buffering at very low priority for sake of plugins and themes that use template_redirect
Expand Down Expand Up @@ -916,6 +920,37 @@ public static function filter_cancel_comment_reply_link( $formatted_link, $link,
);
}

/**
* Configure the admin bar for AMP.
*
* @since 1.0
*/
public static function init_admin_bar() {

// Replace admin-bar.css in core with forked version which makes use of :focus-within among other change for AMP-compat.
wp_styles()->registered['admin-bar']->src = amp_get_asset_url( 'css/admin-bar.css' );
wp_styles()->registered['admin-bar']->ver = AMP__VERSION;

// Remove script which is almost entirely made obsolete by :focus-inside in the forked admin-bar.css.
wp_dequeue_script( 'admin-bar' );

// Remove customize support script since not valid AMP.
add_action( 'admin_bar_menu', function() {
remove_action( 'wp_before_admin_bar_render', 'wp_customize_support_script' );
}, 41 );

// Emulate customize support script in PHP, to assume Customizer.
add_filter( 'body_class', function( $body_classes ) {
return array_merge(
array_diff(
$body_classes,
array( 'no-customize-support' )
),
array( 'customize-support' )
);
} );
}

/**
* Print AMP boilerplate and custom styles.
*/
Expand Down Expand Up @@ -1253,11 +1288,9 @@ public static function prepare_response( $response, $args = array() ) {
trigger_error( esc_html( sprintf( __( 'The database has the %s encoding when it needs to be utf-8 to work with AMP.', 'amp' ), get_bloginfo( 'charset' ) ) ), E_USER_WARNING ); // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_trigger_error
}

if ( AMP_Validation_Manager::should_validate_response() ) {
AMP_Validation_Manager::finalize_validation( $dom, array(
'remove_source_comments' => ! isset( $_GET['amp_preserve_source_comments'] ), // WPCS: CSRF.
) );
}
AMP_Validation_Manager::finalize_validation( $dom, array(
'remove_source_comments' => ! isset( $_GET['amp_preserve_source_comments'] ), // WPCS: CSRF.
) );

$response = "<!DOCTYPE html>\n";
$response .= AMP_DOM_Utils::get_content_from_dom_node( $dom, $dom->documentElement );
Expand Down
2 changes: 2 additions & 0 deletions includes/options/class-amp-options-manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ class AMP_Options_Manager {
'analytics' => array(),
'force_sanitization' => false,
'accept_tree_shaking' => false,
'disable_admin_bar' => false,
);

/**
Expand Down Expand Up @@ -117,6 +118,7 @@ public static function validate_options( $new_options ) {

$options['force_sanitization'] = ! empty( $new_options['force_sanitization'] );
$options['accept_tree_shaking'] = ! empty( $new_options['accept_tree_shaking'] );
$options['disable_admin_bar'] = ! empty( $new_options['disable_admin_bar'] );

// Validate post type support.
if ( isset( $new_options['supported_post_types'] ) ) {
Expand Down
10 changes: 10 additions & 0 deletions includes/options/class-amp-options-menu.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,16 @@ public function render_validation_handling() {
jQuery( '.amp-tree-shaking' ).toggleClass( 'hidden', this.checked && 'native' !== jQuery( 'input[type=radio][name="amp-options[theme_support]"]:checked' ).val() );
} ).trigger( 'change' );
</script>

<p>
<label for="disable_admin_bar">
<input id="disable_admin_bar" type="checkbox" name="<?php echo esc_attr( AMP_Options_Manager::OPTION_NAME . '[disable_admin_bar]' ); ?>" <?php checked( AMP_Options_Manager::get_option( 'disable_admin_bar' ) ); ?>>
Copy link
Contributor

@kienstra kienstra Jun 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's great to have the admin bar enabled by default, with this checkbox to disable it.

Admins might be willing to ignore the AMP validation error of CSS over 50KB in order to have the admin bar. Of course, that error wouldn't even appear for non logged-in users

<?php esc_html_e( 'Disable admin bar on AMP pages.', 'amp' ); ?>
</label>
</p>
<p class="description">
<?php esc_html_e( 'An additional stylesheet is required to properly render the admin bar. If the additional stylesheet causes the total CSS to surpass 50KB then the admin bar should be disabled to prevent a validation error or an unstyled admin bar in AMP responses.', 'amp' ); ?>
</p>
</fieldset>
<?php
}
Expand Down
8 changes: 6 additions & 2 deletions includes/sanitizers/class-amp-core-theme-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,14 +600,18 @@ public function add_twentyseventeen_sticky_nav_menu() {

$navigation_top->parentNode->insertBefore( $navigation_top_fixed, $navigation_top->nextSibling );

$position_observer = AMP_DOM_Utils::create_node( $this->dom, 'amp-position-observer', array(
$attributes = array(
'layout' => 'nodisplay',
'intersection-ratios' => 1,
'on' => implode( ';', array(
'exit:navigationTopShow.start',
'enter:navigationTopHide.start',
) ),
) );
);
if ( is_admin_bar_showing() ) {
$attributes['viewport-margins'] = '32px 0';
}
$position_observer = AMP_DOM_Utils::create_node( $this->dom, 'amp-position-observer', $attributes );
$navigation_top->appendChild( $position_observer );

$animations = array(
Expand Down
40 changes: 32 additions & 8 deletions includes/sanitizers/class-amp-style-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -542,6 +542,12 @@ private function process_style_element( DOMElement $element ) {
$stylesheet = trim( $element->textContent );
$cdata_spec = $is_keyframes ? $this->style_keyframes_cdata_spec : $this->style_custom_cdata_spec;

// Honor the style's media attribute.
$media = $element->getAttribute( 'media' );
if ( $media && 'all' !== $media ) {
$stylesheet = sprintf( '@media %s { %s }', $media, $stylesheet );
}

$stylesheet = $this->process_stylesheet( $stylesheet, array(
'allowed_at_rules' => $cdata_spec['css_spec']['allowed_at_rules'],
'property_whitelist' => $cdata_spec['css_spec']['allowed_declarations'],
Expand Down Expand Up @@ -693,7 +699,7 @@ private function fetch_external_stylesheet( $url ) {
private function process_stylesheet( $stylesheet, $options = array() ) {
$parsed = null;
$cache_key = null;
$cache_group = 'amp-parsed-stylesheet-v8';
$cache_group = 'amp-parsed-stylesheet-v9';

$cache_impacting_options = array_merge(
wp_array_slice_assoc(
Expand Down Expand Up @@ -1306,6 +1312,10 @@ private function process_css_declaration_block( RuleSet $ruleset, CSSList $css_l

if ( $ruleset instanceof DeclarationBlock ) {
$this->ampify_ruleset_selectors( $ruleset );
if ( 0 === count( $ruleset->getSelectors() ) ) {
$css_list->remove( $ruleset );
return $results;
}
}

// Remove disallowed properties.
Expand Down Expand Up @@ -1606,7 +1616,13 @@ function( Selector $old_selector ) {
$ruleset->getSelectors()
) );
$important_ruleset->setRules( $importants );
$css_list->append( $important_ruleset ); // @todo It would be preferable if the important ruleset were inserted adjacent to the original rule.

$i = array_search( $ruleset, $css_list->getContents(), true );
if ( false !== $i ) {
$css_list->splice( $i + 1, 0, array( $important_ruleset ) );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was originally an error from this line, but running composer install took care of it:

( ! ) Error: Call to undefined method Sabberworm\CSS\CSSList\Document::splice() in /srv/www/loc/public_html/wp-content/plugins/amp/includes/sanitizers/class-amp-style-sanitizer.php on line 1622

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is expected 👍

} else {
$css_list->append( $important_ruleset );
}

return $results;
}
Expand Down Expand Up @@ -1827,15 +1843,23 @@ private function finalize_styles() {
}

/**
* Convert CSS selectors.
* Convert CSS selectors and remove obsolete selector hacks for IE.
*
* @param DeclarationBlock $ruleset Ruleset.
*/
private function ampify_ruleset_selectors( $ruleset ) {
$selectors = array();
$replacements = 0;
$selectors = array();
$changes = 0;
foreach ( $ruleset->getSelectors() as $old_selector ) {
$edited_selectors = array( $old_selector->getSelector() );
$selector = $old_selector->getSelector();

// Automatically tree-shake IE6/IE7 hacks for selectors with `* html` and `*+html`.
if ( preg_match( '/^\*\s*\+?\s*html/', $selector ) ) {
$changes++;
continue;
}

$edited_selectors = array( $selector );
foreach ( $this->selector_mappings as $html_selector => $amp_selectors ) { // Note: The $selector_mappings array contains ~6 items.
$html_pattern = '/(?<=^|[^a-z0-9_-])' . preg_quote( $html_selector ) . '(?=$|[^a-z0-9_-])/i';
foreach ( $edited_selectors as &$edited_selector ) { // Note: The $edited_selectors array contains only item in the normal case.
Expand All @@ -1850,7 +1874,7 @@ private function ampify_ruleset_selectors( $ruleset ) {
if ( ! $count ) {
continue;
}
$replacements += $count;
$changes += $count;
while ( ! empty( $amp_selectors ) ) { // Note: This array contains only a couple items.
$amp_selector = array_shift( $amp_selectors );
$edited_selectors[] = preg_replace( $html_pattern, $amp_selector, $original_selector, -1, $count );
Expand All @@ -1860,7 +1884,7 @@ private function ampify_ruleset_selectors( $ruleset ) {
$selectors = array_merge( $selectors, $edited_selectors );
}

if ( $replacements > 0 ) {
if ( $changes > 0 ) {
$ruleset->setSelectors( $selectors );
}
}
Expand Down
4 changes: 1 addition & 3 deletions includes/validation/class-amp-invalid-url-post-type.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1355,11 +1355,9 @@ public static function print_url_as_title( $post ) {
return;
}

// Remember URL is stored in post_title. Adding query var prevents redirection to non-AMP page.
$view_url = add_query_arg( AMP_Validation_Manager::VALIDATE_QUERY_VAR, '', $url );
?>
<h2 class="amp-invalid-url">
<a href="<?php echo esc_url( $view_url ); ?>"><?php echo esc_html( $url ); ?></a>
<a href="<?php echo esc_url( $url ); ?>"><?php echo esc_html( $url ); ?></a>
</h2>
<?php
}
Expand Down
Loading