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.

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

Facilitate using AMP components outside of AMP documents (AMP in PWA, “dirty AMP”) #1013

Merged
merged 4 commits into from Mar 12, 2018

Conversation

Projects
None yet
2 participants
@westonruter
Copy link
Member

westonruter commented Mar 12, 2018

  • Add AMP scripts the WordPress way: register all AMP component scripts as defined in spec via WP_Scripts via new function amp_register_default_scripts(), allowing simple enqueueing via wp_enqueue_script( 'amp-list' ), for example.
  • Also register the AMP Shadow API as a default script.
  • Use wp_print_scripts() with filtering script_loader_tag to output scripts with necessary attributes. See https://core.trac.wordpress.org/ticket/12009
  • Update whitelist sanitizer to allow, validate, and sanitize AMP scripts.
  • Eliminate needless amp_component_scripts filter.

With this in place, you can leverage AMP components entirely outside of pages served as AMP. For example, to add the quadradic formula to the footer of a site, served as AMP or not, all that is needed is:

<?php
add_action( 'wp_enqueue_scripts', function() {
	wp_enqueue_script( 'amp-mathml' );
} );

add_action( 'wp_footer', function() {
	?>
	<amp-mathml
		layout="container"
		data-formula="\[x = {-b \pm \sqrt{b^2-4ac} \over 2a}.\]"
	></amp-mathml>
	<?php
} );

This not only adds the amp-mathml component script to the head, but it also automatically outputs the AMP runtime script, since it is marked as a dependency of all registered AMP component scripts.

Fixes #982.

@westonruter westonruter requested a review from kienstra Mar 12, 2018

@westonruter westonruter added this to the v0.7 milestone Mar 12, 2018

@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Mar 12, 2018

It would be nice if this were further enhanced to automatically move the AMP scripts from the footer or anywhere else in the body to the head automatically. This would allow us to simplify the above to as follows:

add_action( 'wp_footer', function() {
	wp_print_scripts( 'amp-mathml' );
	?>
	<amp-mathml layout="container" data-formula="\[x = {-b \pm \sqrt{b^2-4ac} \over 2a}.\]"></amp-mathml>
	<?php
} );

westonruter added some commits Mar 12, 2018

Facilitate using AMP components outside of AMP documents (dirty AMP)
* Add AMP scripts the WordPress way: register all AMP component scripts as defined in spec via WP_Scripts via new function amp_register_default_scripts(), allowing simple enqueueing via wp_enqueue_script( 'amp-list' ), for example.
* Also register the AMP Shadow API as a default script.
* Use wp_print_scripts() with filtering script_loader_tag to output scripts with necessary attributes.
* Update whitelist sanitizer to allow, validate, and sanitize AMP scripts.
* Eliminate needless amp_component_scripts filter.
Move any AMP component scripts from body into head
Call ensure_required_markup prior to sanitizing document

@westonruter westonruter force-pushed the add/amp-script-enqueues branch from 3860e77 to 0154620 Mar 12, 2018

@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Mar 12, 2018

It would be nice if this were further enhanced to automatically move the AMP scripts from the footer or anywhere else in the body to the head automatically.

This is done on 0154620.

*/
$custom_templates = array( 'amp-mustache' );
foreach ( $extensions as $extension ) {

This comment has been minimized.

Copy link
@westonruter

westonruter Mar 12, 2018

Author Member

Probably should do an array_unique call here.

continue;
}
$tag_spec = $rule_spec[ AMP_Rule_Spec::TAG_SPEC ];
if ( ! empty( $tag_spec['also_requires_tag_warning'] ) ) {

This comment has been minimized.

Copy link
@westonruter

westonruter Mar 12, 2018

Author Member

I don't think this statement is relevant here

} else {
$attributes['custom-element'] = $extension;
}
$wp_scripts->add_data( $extension, 'amp_script_attributes', $attributes );

This comment has been minimized.

Copy link
@westonruter

westonruter Mar 12, 2018

Author Member

This really isn't necessary. In the script_loader_tag filter we can just look to see if the script comes from the AMP CDN and if so then automatically add async and the required custom attribute.

Improve registration and printing of AMP scripts
* Eliminate amp_script_attributes script data in favor of just looking at the URL to determine whether to add additional script attributes.
* Add array_unique when iterating over extensions to add.
* Eliminate looking at also_requires_tag_warning when gathering extension scripts.

@westonruter westonruter changed the title Facilitate using AMP components outside of AMP documents (dirty AMP) Facilitate using AMP components outside of AMP documents (AMP in PWA, “dirty AMP”) Mar 12, 2018

@westonruter

This comment has been minimized.

Copy link
Member Author

westonruter commented Mar 12, 2018

Ready for review.

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Mar 12, 2018

Reviewing Now

Thanks, @westonruter. I'm reviewing this now.

@kienstra kienstra self-assigned this Mar 12, 2018

@kienstra
Copy link
Collaborator

kienstra left a comment

Approved, Minor Points

Hi @westonruter,
This looks good, and it's interesting how it uses DOMXPath.

There are a few minor questions here. If you're satisfied, feel free to merge this.

$attribute_string = sprintf( ' %s="%s"', esc_attr( $key ), esc_attr( $value ) );
}
$tag = preg_replace(
':(?=></script>):',

This comment has been minimized.

Copy link
@kienstra

kienstra Mar 12, 2018

Collaborator

Should the slash be escaped?

This comment has been minimized.

Copy link
@westonruter

westonruter Mar 12, 2018

Author Member

No. This is doing a lookahead search to find ></script> in the string to inject content before it.

wp_enqueue_script( 'amp-runtime' );
// Enqueue default styles expected by sanitizer.
wp_enqueue_style( 'amp-default', amp_get_asset_url( 'css/amp-default.css' ) );

This comment has been minimized.

Copy link
@kienstra

kienstra Mar 12, 2018

Collaborator

It looks like this is just moved from enqueue_amp_default_styles(), which only passed 2 arguments to wp_enqueue_style(). But could this use a $version as the 4th argument: AMP__VERSION?

This comment has been minimized.

Copy link
@westonruter

westonruter Mar 12, 2018

Author Member

Oh good point! Yes, this is needed.

$script_tags = ob_get_clean();
if ( ! empty( $script_tags ) ) {
$response = preg_replace(
'#(?=</head>)#',

This comment has been minimized.

Copy link
@kienstra

kienstra Mar 12, 2018

Collaborator

Should the slash be escaped?

@kienstra kienstra assigned westonruter and unassigned kienstra Mar 12, 2018

@westonruter westonruter merged commit 839d7ff into develop Mar 12, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the add/amp-script-enqueues branch Mar 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.