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

Integrate admin bar with AMP dev mode #3187

Merged
merged 23 commits into from
Sep 11, 2019
Merged

Integrate admin bar with AMP dev mode #3187

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
3380451
Revert "Suppress admin bar from being shown during validation requests"
westonruter Sep 4, 2019
e3d4f57
Skip sanitizing admin bar (and removing it) in favor of ignoring (in …
westonruter Sep 5, 2019
8640d1a
Prevent tree-shaking under #wpadminbar when admin bar is showing
westonruter Sep 6, 2019
f75586f
Prevent admin bar from even attempting to be displayed on story template
westonruter Sep 6, 2019
a81fb73
Remove redundant document argument from is_document_in_dev_mode()
westonruter Sep 6, 2019
bfad786
Skip converting elements in dev mode; skip processing style attribute…
westonruter Sep 6, 2019
5bb7466
Add filters for whether dev mode is enabled and what elements get it
westonruter Sep 6, 2019
d615d39
Bring back remove_admin_bar_if_css_excluded and admin-bar CSS priorit…
westonruter Sep 7, 2019
277131f
Prevent dev mode when admin bar shown for unauthenticated users
westonruter Sep 7, 2019
4b533d2
Fix placement of dev_mode_enabled variable definition
westonruter Sep 7, 2019
e3025cd
Fix undefined dev_mode_xpaths being used in cache key
westonruter Sep 8, 2019
a726d41
Fix typos in comments and add array casting
westonruter Sep 9, 2019
58f5eb3
Merge branch 'develop' of github.com:ampproject/amp-wp into add/dev-m…
westonruter Sep 10, 2019
d1ac164
Move dev mode mutations into new sanitizer
westonruter Sep 10, 2019
5ba10b4
Remove now-unnecessary admin_bar_showing arg for style sanitizer
westonruter Sep 10, 2019
38fa7ba
Add tests for init_admin_bar
westonruter Sep 10, 2019
71c195c
Add test for amp_is_dev_mode()
westonruter Sep 10, 2019
2471036
Add test for amp_get_content_sanitizers()
westonruter Sep 10, 2019
7c57708
Add tests for AMP_Dev_Mode_Sanitizer
westonruter Sep 10, 2019
e15bbe3
Include dashicons in dev mode but only if exclusively connected to ad…
westonruter Sep 10, 2019
d79be8d
Remove amp_dev_mode_enabled filter in test
westonruter Sep 10, 2019
5de22ff
Remove useless variable
schlessera Sep 11, 2019
290adf7
Add asserts to make sure we are not enqueueing both versions of dashi…
schlessera Sep 11, 2019
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
63 changes: 63 additions & 0 deletions includes/amp-helper-functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -815,6 +815,42 @@ function amp_get_content_embed_handlers( $post = null ) {
);
}

/**
* Determine whether AMP dev mode is enabled.
*
* When enabled, the <html> element will get the data-ampdevmode attribute and the plugin will add the same attribute
* to elements associated with the admin bar and other elements that are provided by the `amp_dev_mode_element_xpaths`
* filter.
*
* @since 1.3
*
* @return bool Whether AMP dev mode is enabled.
*/
function amp_is_dev_mode() {

/**
* Filters whether AMP mode is enabled.
*
* When enabled, the data-ampdevmode attribute will be added to the document element and it will allow the
* attributes to be added to the admin bar. It will also add the attribute to all elements which match the
* queries for the expressions returned by the 'amp_dev_mode_element_xpaths' filter.
*
* @since 1.3
* @param bool Whether AMP dev mode is enabled.
*/
return apply_filters(
'amp_dev_mode_enabled',
(
// For the few sites that forcibly show the admin bar even when the user is logged out, only enable dev
// mode if the user is actually logged in. This prevents the dev mode from being served to crawlers
// when they index the AMP version.
( is_admin_bar_showing() && is_user_logged_in() )
||
is_customize_preview()
)
);
}

/**
* Get content sanitizers.
*
Expand Down Expand Up @@ -899,6 +935,33 @@ function amp_get_content_sanitizers( $post = null ) {
*/
$sanitizers = apply_filters( 'amp_content_sanitizers', $sanitizers, $post );

if ( amp_is_dev_mode() ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be preferable to have this piece of logic be part of the AMP_Dev_Mode_Sanitizer class instead, and then make that sanitizer decide when it acts or not, instead of hardcoding it in here.

It should be the responsibility of the AMP_Dev_Mode_Sanitizer to know when to do something and what to do, not some helper function collection.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did think about that, but I think it's preferable here because this function is inherently connected directly with WordPress as it applies a filter. There are two reasons for why this should not be in the sanitizer:

  1. It is important that the array of sanitizer args returned by amp_get_content_sanitizers() be deterministic in what the resulting sanitized document be. In other words, we cannot apply filters inside of a sanitizer because then we cannot use the sanitizer args as a source of truth for computing a cache key for the post-processed output, as we can now.
  2. We should be trending the sanitizers toward being pure PHP classes that are do not connect with WordPress APIs (especially the plugin API per above) as this is the direction needed for abstracting the sanitizers onto a CMS-agnostic package.

Additionally, this amp_is_dev_mode() function would be useful outside of the context of a sanitizer. For example, a theme/plugin may want to call this function during template generation to determine whether it can output some dev code (e.g. Query Monitor).

The amp_is_dev_mode() is gatekeeping whether or not the AMP_Dev_Mode_Sanitizer should be used in the first place. So it is operating at a higher level.

Aside: should it rather be named is_amp_dev_mode()?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Aside: should it rather be named is_amp_dev_mode()?

I'm seeing the amp_ as a prefix here, as this could just as well be used outside of the AMP plugin, so I think the name is fine.

/**
* Filters the XPath queries for elements that should be enabled for dev mode.
*
* By supplying XPath queries to this filter, the data-ampdevmode attribute will automatically be added to the
* root HTML element as well as to any elements that match the expressions. The attribute is added to the
* elements prior to running any of the sanitizers.
*
* @since 1.3
* @param string[] XPath element queries. Context is the root element.
*/
$dev_mode_xpaths = (array) apply_filters( 'amp_dev_mode_element_xpaths', [] );
if ( is_admin_bar_showing() ) {
$dev_mode_xpaths[] = '//*[ @id = "wpadminbar" ]';
$dev_mode_xpaths[] = '//*[ @id = "wpadminbar" ]//*';
$dev_mode_xpaths[] = '//style[ @id = "admin-bar-inline-css" ]';
}
$sanitizers = array_merge(
[
'AMP_Dev_Mode_Sanitizer' => [
'element_xpaths' => $dev_mode_xpaths,
],
],
$sanitizers
);
}

// Force style sanitizer and whitelist sanitizer to be at end.
foreach ( [ 'AMP_Style_Sanitizer', 'AMP_Tag_And_Attribute_Sanitizer' ] as $class_name ) {
if ( isset( $sanitizers[ $class_name ] ) ) {
Expand Down
1 change: 1 addition & 0 deletions includes/class-amp-autoloader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ class AMP_Autoloader {
'AMP_Base_Sanitizer' => 'includes/sanitizers/class-amp-base-sanitizer',
'AMP_Blacklist_Sanitizer' => 'includes/sanitizers/class-amp-blacklist-sanitizer',
'AMP_Block_Sanitizer' => 'includes/sanitizers/class-amp-block-sanitizer',
'AMP_Dev_Mode_Sanitizer' => 'includes/sanitizers/class-amp-dev-mode-sanitizer',
'AMP_Gallery_Block_Sanitizer' => 'includes/sanitizers/class-amp-gallery-block-sanitizer',
'AMP_Iframe_Sanitizer' => 'includes/sanitizers/class-amp-iframe-sanitizer',
'AMP_Img_Sanitizer' => 'includes/sanitizers/class-amp-img-sanitizer',
Expand Down
123 changes: 1 addition & 122 deletions includes/class-amp-theme-support.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1363,14 +1363,6 @@ public static function init_admin_bar() {
$style = ob_get_clean();
$data = trim( preg_replace( '#<style[^>]*>(.*)</style>#is', '$1', $style ) ); // See wp_add_inline_style().
wp_add_inline_style( 'admin-bar', $data );

add_filter(
'amp_dev_mode_element_xpaths',
static function ( $queries ) {
$queries[] = '//style[ @id = "admin-bar-inline-css" ]';
return $queries;
}
);
}

// Emulate customize support script in PHP, to assume Customizer.
Expand Down Expand Up @@ -1877,106 +1869,6 @@ public static function filter_customize_partial_render( $partial ) {
return $partial;
}

/**
* Determine whether AMP dev mode is enabled.
*
* When enabled, the <html> element will get the data-ampdevmode attribute, and
*
* @since 1.3
*
* @return bool Whether AMP dev mode is enabled.
*/
public static function is_dev_mode_enabled() {

/**
* Filters whether AMP mode is enabled.
*
* When enabled, the data-ampdevmode attribute will be added to the document element and it will allow the
* attributes to be added to the admin bar. It will also add the attribute to all elements which match the
* queries for the expressions returned by the 'amp_dev_mode_element_xpaths' filter.
*
* @todo Should this be applied in add_hooks()? Ideally the admin-bar script would be skipped from being enqueued if dev mode is not going to be enabled.
*
* @since 1.3
* @param bool[] Whether AMP dev mode is enabled.
*/
return apply_filters(
'amp_dev_mode_enabled',
(
// For the few sites that forcibly show the admin bar even when the user is logged out, only enable dev
// mode if the user is actually logged in. This prevents the dev mode from being served to crawlers
// when they index the AMP version.
( is_admin_bar_showing() && is_user_logged_in() )
||
is_customize_preview()
)
);
}

/**
* Get the XPath expressions to query for elements to add data-ampdevmode attributes.
*
* @since 1.3
*
* @return string[] XPath queries for elements to add data-ampdevmode attributes.
*/
private static function get_dev_mode_xpaths() {
/**
* Filters the XPath queries for elements that should be enabled for dev mode.
*
* By supplying XPath queries to this filter, the data-ampdevmode attribute will automatically be added to the
* root HTML element as well as to any elements that match the expressions. The attribute is added to the
* elements prior to running any of the sanitizers.
*
* @since 1.3
* @param string[] XPath element queries. Context is the root element.
*/
$dev_mode_xpaths = (array) apply_filters( 'amp_dev_mode_element_xpaths', [] );
if ( is_admin_bar_showing() ) {
$dev_mode_xpaths[] = '//*[ @id = "wpadminbar" ]';
$dev_mode_xpaths[] = '//*[ @id = "wpadminbar" ]//*';
}
return $dev_mode_xpaths;
}

/**
* Apply dev mode mutations.
*
* @since 1.3
*
* @param DOMDocument $dom Document.
* @param string[] $dev_mode_xpaths Dev mode XPath expressions.
* @return array The original admin bar element and the placeholder admin bar element, if the admin bar is shown.
*/
private static function apply_dev_mode_mutations( DOMDocument $dom, array $dev_mode_xpaths ) {
$dom->documentElement->setAttribute( AMP_Rule_Spec::DEV_MODE_ATTRIBUTE, '' );
$xpath = new DOMXPath( $dom );

foreach ( $dev_mode_xpaths as $dev_mode_xpath ) {
foreach ( $xpath->query( $dev_mode_xpath ) as $node ) {
if ( $node instanceof DOMElement ) {
$node->setAttribute( AMP_Rule_Spec::DEV_MODE_ATTRIBUTE, '' );
}
}
}

// Replace admin bar with placeholder prior to sanitizing to prevent conversion of its elements.
$original_admin_bar_element = $dom->getElementById( 'wpadminbar' );
$placeholder_admin_bar_element = null;
if ( is_admin_bar_showing() && $original_admin_bar_element ) {
$placeholder_admin_bar_element = $dom->createElement( 'div' );
foreach ( [ 'id', 'class' ] as $attribute_name ) {
$placeholder_admin_bar_element->setAttribute(
$attribute_name,
$original_admin_bar_element->getAttribute( $attribute_name )
);
}
$original_admin_bar_element->parentNode->replaceChild( $placeholder_admin_bar_element, $original_admin_bar_element );
}

return [ $original_admin_bar_element, $placeholder_admin_bar_element ];
}

/**
* Process response to ensure AMP validity.
*
Expand Down Expand Up @@ -2058,9 +1950,6 @@ public static function prepare_response( $response, $args = [] ) {
! is_customize_preview()
);

$dev_mode_enabled = self::is_dev_mode_enabled();
$dev_mode_xpaths = $dev_mode_enabled ? self::get_dev_mode_xpaths() : [];

// When response caching is enabled, determine if it should be turned off for cache misses.
$caches_for_url = null;
if ( $enable_response_caching ) {
Expand All @@ -2078,7 +1967,7 @@ public static function prepare_response( $response, $args = [] ) {
'user_can_validate' => AMP_Validation_Manager::has_cap(),
],
$args,
compact( 'enable_response_caching', 'dev_mode_enabled', 'dev_mode_xpaths' )
compact( 'enable_response_caching' )
);

$current_url = amp_get_current_url();
Expand Down Expand Up @@ -2264,18 +2153,8 @@ public static function prepare_response( $response, $args = [] ) {
$dom->documentElement->setAttribute( 'amp', '' );
}

// Apply dev mode mutations when in dev mode.
if ( $dev_mode_enabled ) {
list( $original_admin_bar_element, $placeholder_admin_bar_element ) = self::apply_dev_mode_mutations( $dom, $dev_mode_xpaths );
}

$assets = AMP_Content_Sanitizer::sanitize_document( $dom, self::$sanitizer_classes, $args );

// Replace the original admin bar after sanitizers have all run.
if ( $dev_mode_enabled && isset( $original_admin_bar_element ) && isset( $placeholder_admin_bar_element ) ) {
$placeholder_admin_bar_element->parentNode->replaceChild( $original_admin_bar_element, $placeholder_admin_bar_element );
}

// Determine what the validation errors are.
$blocking_error_count = 0;
$validation_results = [];
Expand Down
45 changes: 45 additions & 0 deletions includes/sanitizers/class-amp-dev-mode-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
<?php
/**
* Class AMP_Dev_Mode_Sanitizer
*
* Add the data-ampdevmode to the document element and to the elements specified by the supplied args.
*
* @since 1.3
* @package AMP
*/

/**
* Class AMP_Dev_Mode_Sanitizer
*
* @since 1.3
*/
final class AMP_Dev_Mode_Sanitizer extends AMP_Base_Sanitizer {

/**
* Array of flags used to control sanitization.
*
* @var array {
* @type string[] $element_xpaths XPath expressions for elements to add the data-ampdevmode attribute to.
* }
*/
protected $args;

/**
* Sanitize document for dev mode.
*
* @since 1.3
*/
public function sanitize() {
$this->dom->documentElement->setAttribute( AMP_Rule_Spec::DEV_MODE_ATTRIBUTE, '' );

$xpath = new DOMXPath( $this->dom );
$element_xpaths = ! empty( $this->args['element_xpaths'] ) ? $this->args['element_xpaths'] : [];
foreach ( $element_xpaths as $element_xpath ) {
foreach ( $xpath->query( $element_xpath ) as $node ) {
if ( $node instanceof DOMElement ) {
$node->setAttribute( AMP_Rule_Spec::DEV_MODE_ATTRIBUTE, '' );
}
}
}
}
}