Fix AMP validation when tracking is disabled #1252
Conversation
includes/Modules/Analytics.php
Outdated
| 'groups' => 'default', | ||
| 'linker' => array( | ||
| 'domains' => array( $this->get_home_domain() ), | ||
| ), | ||
| ), | ||
| 'optoutElementId' => '__gaOptOutExtension', |
There was a problem hiding this comment.
This has to be on the top-level, together with vars. See e.g. https://github.com/ampproject/amphtml/blob/b0aff1b9a59d3ca8f10a6ebe4d6eaeda8c826e61/extensions/amp-analytics/0.1/vendors/googleanalytics.json#L65
includes/Modules/Analytics.php
Outdated
| @@ -1273,7 +1274,11 @@ private function get_home_domain() { | |||
| private function print_tracking_opt_out() { | |||
| ?> | |||
| <!-- <?php esc_html_e( 'Google Analytics user opt-out added via Site Kit by Google', 'google-site-kit' ); ?> --> | |||
| <script type="text/javascript">window["_gaUserPrefs"] = { ioo : function() { return true; } }</script> | |||
| <?php if ( $this->context->is_amp() ) : // TODO: update script type to `text/plain` when supported by AMP plugin. ?> | |||
There was a problem hiding this comment.
Let's remove the comment here per ampproject/amp-wp#4392
There was a problem hiding this comment.
Yeah, I suppose that is no longer relevant 😄
Done!
There was a problem hiding this comment.
@aaemnnosttv Your changes were looking good. During testing though, I noticed that we need to add the optoutElementId indicator to our Tag Manager snippet as well. amp-analytics will automatically combine that config object then with the one from the URL in the config attribute (see https://github.com/ampproject/amphtml/blob/b0aff1b9a59d3ca8f10a6ebe4d6eaeda8c826e61/extensions/amp-analytics/0.1/config.js#L208). It's necessary for us to specify it because the remote Tag Manager configuration also supports only the _gaUserPrefs.ioo way by default, not per outputElementId.
I've tested (and basically QAd this already), for both individual GA and GTM snippets in AMP, and it all appears to work as expected (I don't see myself in Analytics FE live stats when "Exclude logged-in users" is enabled, while I do see myself in there otherwise).
Summary
Addresses issue #1251
Relevant technical choices
masterbranch for mergeAMP render contexts will always have the
no-jsclass on thehtmlelement, even if it isn't disabled in the browser. However, if it is disabled in the browser, additional attributes will not be present on the element such asamp-version. By ensuring that this attribute is not present, when hiding with ifno-jswe can prevent the Site Kit admin bar menu item from being hidden when it shouldn't (i.e. front end AMP render).application/ld+json(which is valid HTML for AMP).Ideally this would use
text/plain(it's empty so it doesn't really matter) but this is done for compatibility with the AMP plugin which currently reports 2/3 of the valid script types as invalid and thus removes them.With JS enabled
With JS disabled
No AMP validation errors
Checklist