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

Add support for AMP dev mode #505

Merged
merged 13 commits into from Nov 20, 2019
Merged

Add support for AMP dev mode #505

merged 13 commits into from Nov 20, 2019

Conversation

@westonruter
Copy link
Member

@westonruter westonruter commented Sep 8, 2019

Addresses issue #438.

Summary

Depends on ampproject/amp-wp#3187.

As noted in ampproject/amp-wp#1921, AMP now supports a dev mode (ampproject/amphtml#20974) which allows a document to mark certain elements as being excluded for AMP validation. This is exactly what the AMP plugin has needed to allow the admin bar without fighting against the 50KB CSS limit. It also opens up the door to using scripts on AMP pages to add interactivity to the admin bar without worrying about AMP compatibility. This is exactly what Site Kit has needed for its admin bar integration on AMP pages.

This PR begins to implement support for that. I'll need help to complete it. In short, any markup that is being added by Site Kit to the frontend should be amended with the data-ampdevmode attribute to each element. The elements in the admin bar items will get this automatically, so the primary concern is the script, link, and style elements that are output to integrate Site Kit on the frontend admin bar.

Relevant technical choices

  • Add data-ampdevmode to elements for scripts and styles on AMP pages to exempt them from AMP validation.

Checklist

  • My code is tested and passes existing unit tests.
  • My code has an appropriate set of unit tests which all pass.
  • My code is backward-compatible with WordPress 4.7 and PHP 5.4.
  • My code follows the WordPress coding standards.
  • My code has proper inline documentation.
  • I have signed the Contributor License Agreement (see https://cla.developers.google.com/).
@westonruter
Copy link
Member Author

@westonruter westonruter commented Sep 10, 2019

FYI: I've attached a build of the AMP plugin with a link the PR description at ampproject/amp-wp#3187

@westonruter
Copy link
Member Author

@westonruter westonruter commented Sep 13, 2019

@felixarntz I forgot that I had created an issue for this. See #438. Updated description to indicate the relationship.

@westonruter
Copy link
Member Author

@westonruter westonruter commented Oct 1, 2019

AMP plugin v1.3 has now been released with the Admin Bar support. Can this be moved forward?

Copy link
Member

@felixarntz felixarntz left a comment

@westonruter This looks good, just one compatibility fix would be good - and maybe we can solve one piece a bit more elegantly.

Can you also make this a proper (non-draft) PR? :)

includes/Core/Admin_Bar/Admin_Bar.php Outdated Show resolved Hide resolved
* @return string[] XPath queries.
*/
public function add_amp_dev_mode( $xpath_queries ) {
$xpath_queries[] = '//script[ contains( text(), "googlesitekit" ) ]';
Copy link
Member

@felixarntz felixarntz Oct 27, 2019

Choose a reason for hiding this comment

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

Just to clarify, this is only for inline scripts then right? The non-inline scripts receive the attribute in their own *_loader_tag hooks I see.

Adding a random googlesitekit comment to all these scripts is not so great. Is there not a way to add the data-ampdevmode attribute to those inline scripts with a filter too?

Copy link
Member Author

@westonruter westonruter Oct 27, 2019

Choose a reason for hiding this comment

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

Just to clarify, this is only for inline scripts then right? The non-inline scripts receive the attribute in their own *_loader_tag hooks I see.

Yes, that is correct.

Adding a random googlesitekit comment to all these scripts is not so great. Is there not a way to add the data-ampdevmode attribute to those inline scripts with a filter too?

It's not awesome, but it is effective and somewhat nice for debugging to locate and identify scripts coming from Site Kit.

I did try to find a way to filter the inline scripts to inject this data-amp-dev-mode attribute, but I was unable to. The extra scripts get printed in \WP_Scripts::print_extra_script() which unfortunately has no filter like script_loader_tag.

westonruter added 2 commits Oct 27, 2019
…dev-mode-support

* 'develop' of github.com:google/site-kit-wp: (538 commits)
  keep array casting
  refactor earnings datapoint to not use array_merge
  Improve error handling in proxy mode by relying on the actual error response.
  Revert back to clientId in third-party code argument.
  fix: Always show proxy setup page when proxy is on
  fix: Rename TagManagerTest.php file
  fix docblocks in Data_Request
  add assertion for data request object immutability
  update docblock for get_batch_data
  revert changes to get_batch_data call in SC module
  Move Data_Request to REST_API namespace
  refactor Data Request getters with magic methods
  Fix incorrect Travis-CI script order.
  Ensure environment preparation failures result in errors in Travis-CI.
  Remove unnecessary array_map.
  Improve regex in PHP-Scoper configuration.
  fix: Rename third-party library
  preserve case change in Tag Manager datapoints
  update references to Google_Client
  fix intVal global prefixing
  ...
@westonruter westonruter marked this pull request as ready for review Oct 27, 2019
@westonruter
Copy link
Member Author

@westonruter westonruter commented Oct 27, 2019

Can you also make this a proper (non-draft) PR? :)

It was a draft because it wasn't fully working. There are a bunch of other script dependencies from Core/Gutenberg that need to get flagged as being in dev mode as well. They include:

  • escape-html
  • is-shallow-equal
  • deprecated
  • dom
  • keycodes
  • priority-queue
  • redux-routine
  • data
  • rich-text

As well as script localization inline scripts.

add_filter(
'script_loader_tag',
function ( $tag, $handle ) use ( $assets ) {
if ( $this->context->is_amp() && isset( $assets[ $handle ] ) && $assets[ $handle ] instanceof Script ) {
Copy link
Member Author

@westonruter westonruter Oct 27, 2019

Choose a reason for hiding this comment

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

This apparently needs to look to also look to see if the $handle is a dependency of any $assets as well.

Copy link
Member

@felixarntz felixarntz Oct 28, 2019

Choose a reason for hiding this comment

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

We can probably add a get_dependency_handles() method to Google\Site_Kit\Core\Assets\Asset, which we can then leverage here to check that.

Copy link
Collaborator

@aaemnnosttv aaemnnosttv Nov 20, 2019

Choose a reason for hiding this comment

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

Out of curiosity, why would the amp plugin not just add the attribute to all style and script tags? Why only whitelist Site Kit assets and dependencies? If needed, why wouldn't the amp plugin add the attributes and let other plugins hook in to whitelist its assets rather than each plugin reimplementing this itself?

Copy link
Member

@felixarntz felixarntz Nov 20, 2019

Choose a reason for hiding this comment

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

The attribute should only be added to JS that is related to functionality not relevant to the frontend (and which thus would not actually break AMP compatibility of the frontend, as long as you're logged-out). That's why it needs to be conservative, any plugin that has JS-powered admin bar functionality needs to explicitly declare that.

Regarding addition of the attributes, that's a good point. @westonruter Maybe the AMP plugin could include a central solution for adding the attribute that scripts/stylesheets could leverage via e.g. a wp_*_add_data( ... )?

Copy link
Member Author

@westonruter westonruter Nov 20, 2019

Choose a reason for hiding this comment

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

Note that any script or stylesheet that has admin-bar as a dependency will automatically get the dev mode attribute. The issue is with scripts/styles that do not have this explicit admin bar linkage, such as dependencies on general libraries.

Maybe the AMP plugin could include a central solution for adding the attribute that scripts/stylesheets could leverage via e.g. a wp_*_add_data( ... )?

I'm not sure I understand. Are you saying you could do something like:

wp_script_add_data( 'lodash', 'ampdevmode', true );

And that this could be used as a dev mode signal equivalent to dependence on the admin-bar script? Then the AMP plugin could include a script_loader_tag filter which adds the data-ampdevmode attribute automatically to any script that has the ampdevmode data or which depends on a script that has this attribute?

This could also be used to automatically add the comment for inline scripts as well. Whereas right now we are manually adding /* googlesitekit */ to the inline scripts, there could be a wp_print_scripts action which loops over all scripts prior to printing and any script which contains the ampdevmode data could also then do:

wp_add_inline_script( $handle, '/*ampdevmode*/' );

And this pattern could be automatically added to the list of amp_dev_mode_element_xpaths.

Does this sound right?

Copy link
Member

@felixarntz felixarntz Dec 6, 2019

Choose a reason for hiding this comment

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

@westonruter Yes, that would be great - really like including inline script support as well.

Copy link
Member Author

@westonruter westonruter Dec 20, 2019

Choose a reason for hiding this comment

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

Work in progress: ampproject/amp-wp#4001

add_filter(
'script_loader_tag',
function ( $tag, $handle ) use ( $assets ) {
if ( $this->context->is_amp() && isset( $assets[ $handle ] ) && $assets[ $handle ] instanceof Script ) {
Copy link
Member

@felixarntz felixarntz Oct 28, 2019

Choose a reason for hiding this comment

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

We can probably add a get_dependency_handles() method to Google\Site_Kit\Core\Assets\Asset, which we can then leverage here to check that.

@felixarntz
Copy link
Member

@felixarntz felixarntz commented Nov 20, 2019

With the latest commits, AMP dev mode works as expected. Some notes:

  • We now bundle wp-escape-html and wp-is-shallow-equal. Those scripts are needed because they are dependencies of wp-element and wp-compose respectively, both of which we use. Them not being there in WP<5.0 did not cause any issues, but only because we didn't use the respective functionality yet. The reason I even discovered that those two dependencies were missing is because they weren't whitelisted for AMP dev mode, although they were actually enqueued as a dependency of Site Kit dependencies.
  • The way we bundle and register these dependencies in PHP is still far from optimal. We should update our build scripts so that all WP dependencies that are dependencies of other dependencies we use are automatically bundled, without us needing to figure that out manually. The same applies to keeping the asset versions in PHP in sync - doing that manually as we currently do is a pain and error-prone.

@felixarntz felixarntz requested a review from aaemnnosttv Nov 20, 2019
@westonruter
Copy link
Member Author

@westonruter westonruter commented Nov 20, 2019

Awesome. I deployed it to my AMP-first site and I can see the Site Kit admin bar and there are no validation errors being reported!

Co-Authored-By: Evan Mattson <evan.mattson@10up.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants