Ensure a Web container is available for selection during Tag Manager module setup if using Standard mode AMP - when not all URLs are AMP

[jamesozzie]

Bug Description

As raised by one user in the support forums some users have AMP active in standard mode. For some URLs they may have AMP disabled, as configured in the AMP plugins "Supported Templates" configuration. If they wish to enable the Tag Manager module within Site Kit this results in no Web Container dropdown available - resulting in no GTM container for non AMP URLs.

Rather than just checking the active AMP mode the plugin should check the AMP Plugins "Supported Templates" configuration. There is a "Serve all templates as AMP?" flag as evident in a users Site Health information with the AMP plugin active, this could be used to allow the disable of a web container.

AMP plugin GitHub PR with "Support Templates" details

Steps to reproduce

1. Setup AMP in standard mode
2. Disable AMP for any template modes or content types (ie. homepage)
3. Setup Tag Manager
4. Web Container doesn't appear

Screenshots

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• The Site Kit logic to detect the AMP mode relevant for the site should be improved as follows:
  • If the AMP plugin option "Serve all templates as AMP" is not enabled, the AMP mode returned should be "secondary" instead of "primary". That is because, even though the site is mainly in standard mode ("primary"), there may still be non-AMP content present.
  • If the Web Stories plugin is active, the AMP mode returned should be "secondary" instead of false. In other words, if e.g. the AMP plugin is not active at all but the Web Stories plugin is, we still need to assume the site to be "secondary" AMP (since Web Stories uses AMP, while in that case the rest of the site wouldn't).

Implementation Brief

Update the function get_amp_mode() to work as follows:

site-kit-wp/includes/Context.php

Line 321 in d6ea055

public function get_amp_mode() {

• Always self::AMP_MODE_SECONDARY; in place of self::AMP_MODE_PRIMARY;
• Simplify conditionals if necessary where several paths lead to the same return statement.

Test Coverage

• Update integration tests if necessary in /tests/phpunit/integration/ContextTest.php for the new conditions.

Visual Regression Changes

NA

QA Brief

• Ensure Site Kit always operates in "AMP Secondary" mode, regardless of which setting is used in the AMP plugin, when Site Kit, WebStories and AMP plugins are installed. Make sure you use the released versions of Webstories/AMP from plugins.wordpress.org and a version of the AMP plugin pre-1.0 as well.

Changelog entry

• Fix bug where AMP mode detection would not consider the AMP plugin's template mode setting when the Web Stories plugin is active.

[asafm7]

Thanks, @jamesozzie .

Just to note that AMP can also be disabled on a specific page level (not just on a template level):

Maybe a good (and simple solution) is to always show both the option to choose AMP and non-AMP containers. Maybe when AMP is on the Standard mode, the non-AMP container field can include an "Optional" label with some explanation.

Thanks again

[jamesozzie]

Great point, thanks @asafm7

[jamesozzie]

@ivankruchkoff @felixarntz In the case whereby a user has all templates served as AMP but has disabled AMP on a particular post or page will any change made to this account for such situations? More details in one of the comments above?

[tofumatt]

Given the AMP rendering can still be disabled on a per-page as stated above, I think the ACs/IB here are a bit incomplete. They're an improvement, yes, but we still aren't accounting for a site in self::AMP_MODE_PRIMARY mode but with a single page that has AMP disabled.

If there is a flag or function that we can use to check is any page has AMP rendering disabled that'd be useful here to know if we can truly, reliably set self::AMP_MODE_PRIMARY. If the AMP plugin has something available that'd be good, but I suppose otherwise we could check through post_meta for a setting if it's easily query-able.

Can we see if the AMP plugin exposes a way to check if any page or post has opted out of AMP?

[felixarntz]

@westonruter Is it possible to disable AMP for a post even if the site is configured in Standard Mode with "Serve all templates in AMP" enabled? If that is the case, then we basically could never reliably assume that a site is only using AMP, so we would essentially always need to show options for both AMP and non-AMP. For a primary AMP site this would be confusing, so it wouldn't be a great experience; but if that's what's necessary 🤔

[asafm7]

@felixarntz Yes, it is possible. I'm using it this way. Maybe it's possible to reduce confusion with a short explanation under the non-AMP container field.

[westonruter]

If that is the case, then we basically could never reliably assume that a site is only using AMP, so we would essentially always need to show options for both AMP and non-AMP. For a primary AMP site this would be confusing, so it wouldn't be a great experience; but if that's what's necessary 🤔

This is correct. You can always disable AMP even when using Standard mode, either when All Templates are served as AMP, by turning off AMP for an individual post/page. This is important for e-commerce, for example, so the shopping cart page can have custom JS.

As for the UI, if you detect it's a Standard mode site with All Templates enabled for AMP, you could show the AMP setting first. Then you could show the non-AMP setting second and mention that it is only used in cases where you turn off AMP for specific posts/pages.

[ivankruchkoff]

Can we see if the AMP plugin exposes a way to check if any page or post has opted out of AMP?

This will do it:
SELECT count(*) FROM $wpdb->postmeta WHERE meta_key = 'amp_status' and meta_value = 'disabled' LIMIT 1

[westonruter]

Actually, no, because AMP can also be disabled programmatically via the amp_skip_post filter.

[ivankruchkoff]

Would it help to see if there are any registered hooks on the filter amp_skip_post or is that a rabbit hole?

I'll admit I'm a little confused in what the AC here is at this point to update the IB to address it.

[felixarntz]

@ivankruchkoff Given @westonruter's responses above, it seems to me that we can never reliably assume that the site is only AMP (i.e. what we call primary AMP). Potentially we'll need to therefore always assume that the site is secondary AMP (if it has AMP at all) - that's not great for users that truly want to do primary AMP, but it seems it's the only reliable approach.

@westonruter Given the above, it would be great to somehow indicate to the AMP plugin that you really only want AMP, everywhere. Almost like "strict mode". That may be a good enhancement, potentially just as a simple filter initially which would prevent any opt-outs, as it would give certain guarantees which would result in better UX for cases like ours here.

[westonruter]

@westonruter Given the above, it would be great to somehow indicate to the AMP plugin that you really only want AMP, everywhere. Almost like "strict mode". That may be a good enhancement, potentially just as a simple filter initially which would prevent any opt-outs, as it would give certain guarantees which would result in better UX for cases like ours here.

Yeah, it's something we've thought about. We had an issue for it: ampproject/amp-wp#2314. But we decided against it. The reality is that the majority of sites need the ability for AMP to be turned off in certain cases. Given that the AMP-compatible ecosystem of themes and plugins are not complete, the number of sites that run in Standard mode is still small in relation to sites in a paired mode (Reader/Transitional). And then among the Standard mode sites, there is a need to often turn off AMP for individual posts/pages (e.g. shopping cart), or to only have AMP enabled for certain templates (singular). So even if we added the ability for a site to only ever serve AMP pages no matter what without any ability to turn it off, the number would likely be very small who would access this streamlined analytics screen in Site Kit. It's not a scenario that is yet common enough to worry about, IMO.

See also ampproject/amp-wp#1864 and ampproject/amp-wp#2724 (comment).

[ivankruchkoff]

@felixarntz I've updated IB based on this new info.

[wpdarren]

@ivankruchkoff @felixarntz I get the gist of this from the AC but not overly familiar with AMP, so would love some clarification on the QAB, particularly Ensure Site Kit always operates in "AMP Secondary" mode - where would I look to see that's secondary mode? If you could write the steps that would be helpful. Thanks!

[aaemnnosttv]

@wpdarren the easiest way to see the current AMP mode used by Site Kit, check the Site Health. The mode itself is set in the AMP settings as the "Template Mode" here:

Standard = primary and the other two would be secondary 👍 Of course the AMP mode only applies when the AMP plugin is active as well 😄

[aaemnnosttv]

@felixarntz it seems that the ACs here weren't revised after the discussion above with @westonruter regarding always returning AMP secondary instead of primary.

However, the IB (and related PR) do not include the second point in the ACs which I believe is still necessary to include:

• If the Web Stories plugin is active, the AMP mode returned should be "secondary" instead of false. In other words, if e.g. the AMP plugin is not active at all but the Web Stories plugin is, we still need to assume the site to be "secondary" AMP (since Web Stories uses AMP, while in that case the rest of the site wouldn't).

Can you confirm that this is still necessary @felixarntz ?

cc: @ivankruchkoff @Hazlitte @tofumatt

Edit: regarding the point above for Web Stories, should this not also be specific to a is_singular( 'web-story' ) like we do in is_amp() rather than only the plugin being active?

[tofumatt]

I've created #3619 as a PR to address the Web Stories part of the ACs that I missed in review.

I think that's all that's needed to address the ACs, so if after having a look @felixarntz, can you assign this back to me and I'll make sure before moving this to Code Review?

[felixarntz]

@aaemnnosttv I'm not sure I follow your comment #2998 (comment) - for Web Stories without the AMP plugin, the AMP mode needs to be "secondary" since probably the rest of the side is non-AMP.

Otherwise, I think we've discussed earlier, so just leaving the feedback here that basically for now if AMP is relevant it should always be "secondary". We should keep the other code (and constant) around "primary" in the codebase, but for now it should never apply.

[aaemnnosttv]

@felixarntz currently in Context::is_amp, we only check that the current post is a web-story, but the bullet in the ACs reads like this should actually be based on whether the plugin is active at all rather than the context of a specific post – which makes sense for things like settings which would never have that context.

So just to confirm, we need to update is_amp and get_amp_mode for their web stories related parts (the latter doesn't exist yet) to be conditional based on the activation status of the plugin, and not is_singular correct?

[tofumatt]

Moving this back to @felixarntz for the question, but after that please assign back to me and I'll wrap up the PR 🙂

[felixarntz]

@aaemnnosttv @tofumatt Now I realize where the confusion is coming from - I think it is unclear currently what the intended purpose for is_amp is - possibly because the intention for isAMP in JavaScript is different. Let me clarify:

• Context::is_amp in PHP should check whether the current (usually frontend) context is in AMP. In other words, the current implementation is correct, this shouldn't return true just because the plugin is active.
• Context::get_amp_mode in PHP should check the status of AMP on the site in general. This is what the ACs focus on, so what matters here in terms of Web Stories is whether the plugin is active or not.
• In JS, all selectors that we have deal with the "AMP mode", rather than whether the current context is AMP (probably because in the Site Kit admin, this is irrelevant, and AMP doesn't allow custom JS anyway). In other words, the isAMP selector here does not have the same purpose as Context::is_amp - this was a confusing naming decision originally I guess. isAMP just tells us whether the site's "AMP mode" is in any way using AMP (i.e. primary or secondary).

[aaemnnosttv]

Thanks @felixarntz, that makes more sense now 👍

[wpdarren]

QA Update: Pass ✅

• Site Kit operates in "AMP Secondary" mode when Site Kit, WebStories and AMP plugins are installed.

• Verified that a version of AMP 0.7.2 (latest pre 1.0) still had AMP Secondary in Site Kit Health settings.

• Reading through the AC I noticed that the issue also arises when Serve all templates as AMP is disabled. The AMP mode should be secondary and can confirm that this is the case with this fix.