505 introduced support for AMP dev mode so that Site Kit scripts and stylesheets are not considered AMP violations by the plugin, since they are only loaded when the user is logged in anyway and should continue to work in these circumstances.
There was a regression recently (reported multiple times) by no longer using wp_add_inline_script() to print the window.googlesitekit object definition and instead relying on WP_Scripts::add_data() (because this is the only way to make it work as a standalone script). Since this script is standalone, we need to add back a mechanism to highlight it as compatible with AMP dev mode. We can use a /*googlesitekit*/ comment for that like before (unfortunately modifying the rendered script tag is not possible).
Another issue is that the AMP plugin filter for that, amp_dev_mode_element_xpaths, fires rather early, but we only hook it in during wp_enqueue_scripts which is too late. It can be generally hooked in during init, there is nothing expensive in there anyway.
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
There should not be any AMP violations flagged by the AMP plugin when the Site Kit admin bar menu is loaded.
Implementation Brief
Introduce Assets::add_extra_script_amp_dev_mode() to add /*googlesitekit*/ comment to all 'data' scripts.
Hook in amp_dev_mode_element_xpaths in top-level Admin_Bar::register().
Include hoverintent-js fix so that it is flagged with data-ampdevmode. This was added to the AMP plugin via https://github.com/ampproject/amp-wp/pull/3928, but only in the very recent 1.4.2, so I think it's worth doing that here, with a TODO to remove in the future.
Merge #1143
QA Brief
Activate AMP plugin and set to AMP standard mode.
Go to a page in the frontend for which the Site Kit admin bar menu shows.
Ensure the plugin does not report any validation errors, and that the menu opens as expected when hovering over it.
Changelog entry
Fix AMP violations when user is logged in and Site Kit admin bar menu is active.
Bug Description
505 introduced support for AMP dev mode so that Site Kit scripts and stylesheets are not considered AMP violations by the plugin, since they are only loaded when the user is logged in anyway and should continue to work in these circumstances.
There was a regression recently (reported multiple times) by no longer using
wp_add_inline_script()
to print thewindow.googlesitekit
object definition and instead relying onWP_Scripts::add_data()
(because this is the only way to make it work as a standalone script). Since this script is standalone, we need to add back a mechanism to highlight it as compatible with AMP dev mode. We can use a/*googlesitekit*/
comment for that like before (unfortunately modifying the renderedscript
tag is not possible).Another issue is that the AMP plugin filter for that,
amp_dev_mode_element_xpaths
, fires rather early, but we only hook it in duringwp_enqueue_scripts
which is too late. It can be generally hooked in duringinit
, there is nothing expensive in there anyway.Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Implementation Brief
Assets::add_extra_script_amp_dev_mode()
to add/*googlesitekit*/
comment to all 'data' scripts.amp_dev_mode_element_xpaths
in top-levelAdmin_Bar::register()
.hoverintent-js
fix so that it is flagged withdata-ampdevmode
. This was added to the AMP plugin via https://github.com/ampproject/amp-wp/pull/3928, but only in the very recent 1.4.2, so I think it's worth doing that here, with a TODO to remove in the future.QA Brief
Changelog entry