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 theme support scenarios: allow paired mode to serve some content exclusively in AMP #934

Closed
westonruter opened this issue Feb 6, 2018 · 23 comments
Assignees
Milestone

Comments

@westonruter
Copy link
Member

westonruter commented Feb 6, 2018

The plugin in amp theme support currently supports two modes:

  1. Canonical: Everything is canonical AMP.
  2. Paired: Everything is available in non-AMP and some content is also available in AMP, with different templates used in AMP mode.

There is a third case that should be supported and that is:

  1. Mixed-Some-Optionally-AMP: All content is available in non-AMP, and some content is also available in AMP, but they use the same theme templates.

Now that we have full-document sanitization in #929, the a non-AMP template can actually be served as totally valid AMP, with amp attribute added to the html element, meta viewport being added, and CSS boilerplate code. So we can actually use the exact same templates in AMP as non-AMP. We don't readily make this easy in paired mode, as to do paired mode the amp theme support must define a template_dir for where to locate the AMP templates, like:

add_theme_support( 'amp', array(
    'template_dir' => 'amp-templates/'
) );

When a paired-mode site only allows a subset of content to be shown in AMP (e.g. just singular posts), they can explicitly indicate this with the available_callback property:

add_theme_support( 'amp', array(
    'template_dir' => 'amp-templates/'
    'available_callback' => 'is_single',
) );

However, in order to support re-using templates between AMP and non-AMP we should also be able to do:

add_theme_support( 'amp', array(
    'available_callback' => 'is_single',
) );

Without this, you have to do a needless hack of setting the template_dir to . or add a single.php in the template directory that just does require __DIR__ . '../single.php';

Lastly, there is a fourth mode that should be doable:

  1. Mixed-Some-Only-AMP: Some content is not available in AMP, and everything else is only available in AMP.

In the fourth mode there should not be two versions of any one URL. There would be no /amp/ endpoints, and no amphtml links on AMP content. Content in AMP and non-AMP would be disjoint sets. To do this, there would need to probably need to be something different from available_callback in the theme support, or rather instead of this callback returning a boolean flag it should actually be tri-state:

  • AMP Omitted
  • AMP Optional
  • AMP Only

For example, if you wanted to serve AMP-only posts, AMP optionally for pages, and omit AMP everywhere else, you could do:

add_theme_support( 'amp', array(
    'available_callback' => function() {
        if ( is_single() ) {
            return AMP_ONLY; // No amphtml version offered. AMP 100%.
        } elseif (  is_page()) {
            return AMP_OPTIONAL; // Non-AMP by default. Link to amphtml URL offered to opt-in.
        } else {
            return AMP_OMITTED; // Non-AMP always. No link to amphtml URL offered at all.
        }
    }
) );
@amedina
Copy link
Member

amedina commented Feb 6, 2018

This scenario would work best in cases where the quality of the content is not diminished by the preprocessor. Combined with the opt-in capability it would empower the site owner.

@amedina
Copy link
Member

amedina commented Feb 6, 2018

Alternatively (or additionally), we could have a similar scenario where instead of having a single version of the content (the AMP version) we can generate a paired mode version, based on the original theme. In cases where the plugin can process 100% of the content without loss, the result is in two identical pages; but in cases where the fidelity is not 100%, the result would be a paired mode with the same look and feel as the original theme (because is the same) but with some functionality stripped down.

@westonruter
Copy link
Member Author

Yes, this would be accounted for here too more or less:

add_theme_support( 'amp', array(
    'active_callback' => function() { return AMP_OPTIONAL; }
) );

By not defining the template_dir then the theme's own regular templates would be used and converted to AMP by the sanitizer when the amphtml version of a post is requested.

@westonruter
Copy link
Member Author

@amedina Something else to note here that this optional amp theme support could be added by the plugin itself. This would allow for a user to opt-in to using their main theme templates to generate AMP instead of using the existing paired mode post templates from ≤0.6. In 1.0 we could make the switch to having this be the default when you install the plugin and activate it, with the old paired mode post templates only remaining active for legacy installs. This could be a natural migration path and deprecation strategy.

@amedina
Copy link
Member

amedina commented Feb 6, 2018

@westonruter that is a good strategy. An additional case (optimization) could be derived in cases where the user opts in for the new pair mode (based on the original theme) and when converting a given content type, if the preprocessor detects 100% fidelity (i.e. no functionality stripped out) the plugin generates a single version of the content (i.e. AMP canonical); otherwise stays in pair mode. This would be sort of a hybrid pair-canonical mode.

@westonruter
Copy link
Member Author

There's one more variation on the canonical/native/AMP_ONLY scenario. When we introduce server-side rendering (#958) this will actually re-introduce the paired mode URLs even though the site is native AMP.

@westonruter
Copy link
Member Author

In “AMP Optional” mode, the opt-out option should be made available to the user in the post editor, where it is normally not available in native (non-paired) mode. The scenario here I'm thinking of is you have a site where everything is served as AMP normally, but you have one exception where you want to serve non-AMP content. In effect, this would be like having the available_callback return AMP_OMITTED (from above) for the one post URL.

@westonruter
Copy link
Member Author

/cc @ericlindley-g

@asadkn
Copy link

asadkn commented Jun 4, 2018

Glad to see this discussion. I am not sure if this warrants a new issue, but I have a related scenario.

The "paired" mode is just fine for most of our publishers with mostly mobile traffic landing on AMP. Serving AMP is just perfect for mobile. Projects like AMP for WP get it right for practical needs here where they have a whole "amp theme" just for mobile version of the site - sort of Jetpack mobile theme concept.

As much as some of would like to have native AMP everything, it's not practical to have AMP-only websites just yet in most of the advanced themes and websites in the wild. Too many custom elements/widgets, too much JS-based functionality, unsupported ad network, control over every element like popups, notifications, and so on.

It would have been great to have support for home, archives, and so on for paired mode with overridable templates for each by themes, like AMP for WP / similar plugins. Since that's probably not going to happen now, I would like to suggest a related approach.

Similar to 3 and 4 but not exactly the same:

  • Paired (/amp/ endpoints) but Native templates (as in 3): Enable canonical mode at /amp/ end-points rather than the main site. Users can use this with other plugins to setup redirects on mobile devices or whichever condition they need. This would stay compatible with Google indexing.

A way to enable the AMP Native mode for mobile devices only would be just fine, but that has discovery issue - the /amp/ would still have to be added as rel=amphtml link tag.

@westonruter
Copy link
Member Author

@asadkn What you're asking for is now possible as of 0.7, once you add amp theme support with paired mode: https://github.com/Automattic/amp-wp/wiki/Adding-Theme-Support#paired-mode

@westonruter
Copy link
Member Author

Related to #1196 which adds an options for enabling amp theme support from the admin:

there would also need to be a way to indicate a subset-native mode, where AMP is served on canonical URLs for some subset of the site (e.g. singular template) but on others there is no AMP version available, not even on a separate AMP-specific URL (in paired mode).

@westonruter
Copy link
Member Author

I'm going to start working on this to be included in 1.0-beta1.

@westonruter
Copy link
Member Author

I've found that adding support for allowing canonical/native AMP to be used on a subset of URLs for a site causes problems with is_amp_endpoint() to detect whether the current response is AMP. Namely, if an available_callback uses a template conditional like is_singular() then it won't be able to call the callback until parse_query. This is what we tried to get away from with #1194 for #1148. I don't know if it is feasible to re-introduce this restriction for the sake of being able to have a site where some URLs serve non-AMP content and others serve AMP content but there not being any specific query param to differentiate the two.

@westonruter
Copy link
Member Author

Another issue here is in regards to AMP-specific Gutenberg blocks (like MathML and Time-ago): since these blocks only work in AMP, the plugin only makes them available when in native mode. But if a site is only partially native, then it could very well be that an AMP component will end up getting served on a non-AMP page, and thus would be broken.

@westonruter
Copy link
Member Author

westonruter commented Jun 22, 2018

So yeah ampproject/amphtml#15583 is resolved (and it's not certain whether it will be accepted) then the plugin will not allow non-valid AMP markup to be served in AMP responses, and it will avoid serving any AMP runtime or component scripts in non-AMP responses. Until that time, it would be problematic for a site that only wants to AMP their single posts, since if a post contains any AMP markup then it would fail to render on non-singular templates, such as the homepage, category archive, or author page.

@westonruter
Copy link
Member Author

Otherwise, it's not so much of a challenge for the plugin but rather a problem for plugins and themes that want to integrate with AMP. In particular, they need to know as early as possible if the page is going to end up being rendered as AMP. This allows themes and plugins to swap out which hooks are used for rendering the response (e.g. whether AMP components or custom scripts will be used). WordPress only determines whether the current URL is for a singular post or something relatively late in the process, so that means plugins and themes need to “hold their breath”. This is something that we could make a requirement if needed, but it will require plugins and themes to do some more work to make themselves AMP-compatible. If this is going to be a requirement, then it is important that we make the decision now before 1.0 is released.

@gravityrail what are your thoughts on this in relation to making Jetpack AMP-compatible?

@westonruter
Copy link
Member Author

I talked it over with @pbakaus and the use case of having a native subset of URLs on a site is an important use case, such as for e-commerce. So we'll have re-impose the requirement that the parse_query action must be done prior to calling is_amp_endpoint(). On native sites, this is the only way to know if AMP is going to be used on the page or not, since an individual post/page may disable AMP and we won't know which post/page is being queried until this action.

In the case of AMP content from a post appearing on non-AMP pages, we'll go ahead and serve these as dirty AMP. That is, when there is AMP content in a post, we'll load the AMP runtime and the required component scripts.

We will, however, continue to limit the use of AMP-based Gutenberg blocks to when a site is in native mode, so this issue will hopefully be mitigated.

Also, in contrast with my initial proposal for available_callback to return a ternary value of disabled, paired, or native… I'll reduce this back to a binary true/false. There will then be a separate flag to indicate the mode. For example, to show all single blog posts as AMP but everything else as not AMP:

add_theme_support( 'amp', array(
    'available_callback' => 'is_single',
) );

Or if you want to make the single posts available in paired URLs, you could do:

add_theme_support( 'amp', array(
    'paired' => true,
    'available_callback' => 'is_single',
) );

The existing behavior of the plugin is to presume paired mode is intended if a template_dir is provided, so this will remain the case.

@morsssss
Copy link

Thanks for that! I really believe it will feel natural to many WordPress site owners to make certain pages all AMP all the time, and other pages never AMP. Most WP pages are simple, and as long as they look the same as AMP or non-AMP, why use paired mode? In such cases I think AMP is simply better. And then, if there are pages that truly require JS, those can simply be non-AMP.

I just hope this doesn't prove difficult for those who make themes, those who make plugins, or you, the fearless developers of this plugin!

@ericlindley-g
Copy link

ericlindley-g commented Jul 3, 2018

+1 to the e-comm use case needing support for some page types to be non-AMP

Just to make sure I understand: @westonruter , will this make it possible to publish "dirty AMP" pages for end users (which I think we still want to avoid), or will this just enable the use of "dirty AMP" during the page creation / authoring flow (which seems OK)?

/cc @cramforce for visibility on the use of invalid AMP in the AMP plugin for WordPress

@westonruter
Copy link
Member Author

westonruter commented Jul 3, 2018

@ericlindley-g It's actually not about using AMP components in the authoring interface; this is currently blocked because AMP components can't be easily used in React, for example.

This PR still tries to avoid dirty AMP as much as possible, but it does allow it on the frontend in a corner case.

If you have selected the Paired mode, where content is expected to be served in AMP and non-AMP pages, then the AMP-specific Gutenberg blocks are not made available to the user in the Inserter. (This would exclude blocks for AMP Stories in #968 since they by very nature are only served in AMP regardless of template mode.)

The AMP-specific Gutenberg blocks are only made available to the user if they have selected the native mode (aka canonical, aka AMP as foundation). In the native mode, it is expected that the majority of the site's templates would be served in AMP, thus it makes sense that AMP-specific content should be allowed. Now, the situation for dirty AMP can arise now where:

  1. The user selects native AMP mode.
  2. The user creates a post and adds an AMP-specific Gutenberg block to it.
  3. The user serves singular post templates as AMP.
  4. The user decides to not serve the category listing template (or other such non-singular templates) as AMP.

In this scenario, when the post appears on a non-singular template, it can include AMP-specific markup in it which requires the AMP runtime to be loaded to render. This PR does not allow for an AMP document to be served with invalid AMP markup. What it does allow is for AMP components (in post content) and the dependent AMP runtime & component scripts to be loaded on non-AMP documents so that the content will not be broken. But, this is not without issues either, as described in ampproject/amphtml#15583.

So its not dirty AMP documents (no html[amp]), but non-AMP documents with AMP content in them.

@ericlindley-g
Copy link

@westonruter Got it — thanks! That's really useful to understand.

I would take the same approach with the two cases you describe (using AMP components & runtime with or without html[amp]), since they both run into the same issues with resource management, but maybe there are reasons to think of them differently? Will discuss more in real-time.

@westonruter
Copy link
Member Author

@ericlindley-g Yeah, at the moment there could be broken content in either case: either an AMP component could be broken (unrecognized) in a non-AMP document, or it could be recognized but some non-AMP resources on the page could have issues initializing. Which is the lesser of two evils?

@pbakaus Any thoughts on this to follow-up on our conversation?

@kienstra
Copy link
Contributor

kienstra commented Aug 3, 2018

Moving To "Ready For Merging"

If it's alright, I'm moving this to "Ready For Merging." If you think this could use functional testing, feel free to move it back.

@westonruter westonruter mentioned this issue Jun 9, 2019
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants