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

Implement support for off cache AMP #5378

Closed
westonruter opened this issue Sep 14, 2020 · 34 comments
Closed

Implement support for off cache AMP #5378

westonruter opened this issue Sep 14, 2020 · 34 comments
Labels
Bento Blocked Groomed P0 High priority Punted UX WS:Core Work stream for Plugin core

Comments

@westonruter
Copy link
Member

westonruter commented Sep 14, 2020

Feature description

AMP will soon support valid AMP pages opting out of being served from an AMP Cache. See ampproject/amphtml#29296. This will be an important ability to fix various issues caused by caching, including analytics and serving stale pages (ampproject/amphtml#24326), the latter of which is particularly important for e-commerce.

Opting out of AMP Cache by default seems also important for non-singular templates, or any post/page that has frequent updates. For example, the homepage of a site (and any other archive pages) should probably not be served by the AMP Cache by default until there is a reliable way to ensure the AMP Cache is flushed (see #598). We could also have a list of post types from popular e-commerce plugins that we disable by default for AMP Cache.

👉 This is blocked from being closed until the feature lands in upstream. However, work can begin on implementing support as well as figuring out what the right defaults are. We also need to determine what UI (if any) is added for this. For example, there may need to be a new advanced section for AMP Cache handling which contains whether AMP Cache is enabled/disabled overall. Nevertheless, there could be fine-grained controls over whether AMP Cache is enabled, down to the post type and template level. If so, then this could be combined with the Supported Templates section somehow, where the current on/off toggles are replaced with a multi-value dropdown:

  • Disabled
  • Enabled (without caching)
  • Enabled (with caching)

This could also extend to the "AMP Enabled" toggle on individual posts and pages.

Relation to Bento AMP

This is also closely related to how we might need to adjust terminology for Bento AMP. When themes and plugins can use AMP components on non-AMP pages, it's not accurate to say that “AMP” can be disabled. It's more whether or not “AMP validation” is disabled. In this way, the AMP plugin could continue to inject AMP components into the responses for non-AMP pages, such as in oEmbed handlers or in converting img into amp-img to get the speed benefits that AMP provides (although amp-img specifically is on a deprecation path now that lazy-loaded images are part of the platform, per ampproject/amphtml#29786).

There are also implications here for AMP Optimizer. if we serve AMP components on non-AMP pages, we should make sure they get server-side rendered (SSR) as much as possible since the AMP Boilerplate will not be present. Currently AMP Optimizer takes complete AMP documents in DOM as input. The oEmbed handlers, however, only manipulate HTML strings. So either the oEmbed handlers need to create transformed AMP manually, or we need to constrict DOM fragments and pass them into the Optimizer for processing.


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

Acceptance criteria

  • The desired pages on which AMP Cache are disabled should be served with amp="no-caching" in the html attribute.
  • 80% use cases of users needing control over disabling AMP Cache should be accounted for (TBD).

Implementation brief

  • Similar to AMP Dev Mode with its amp_dev_mode_enabled filter, introduce an amp_cache_enabled filter. The default value should be true for singular queries, and false for archives (and perhaps other select post types).
  • As with AMP Dev Mode's AMP_Dev_Mode_Sanitizer, introduce an AMP_Off_Cache_Sanitizer which is added to the list of amp_content_sanitizers when amp_cache_enabled is filtered to be false. This sanitizer will set the amp attribute on the html element to be off-cache (tentatively).
  • TBD on UI. Note that any UI would conflict with programmatic control via the amp_dev_mode_enabled filter. So if a site has_filter('amp_dev_mode_enabled') then any cache options must be disabled.

QA testing instructions

Demo

Changelog entry

@westonruter westonruter added this to the v2.1 milestone Sep 14, 2020
@westonruter westonruter added the P0 High priority label Sep 15, 2020
@westonruter
Copy link
Member Author

We may want to split this into two separate issues: one that implements just the low-level support for opting out of AMP Cache (but leaving it enabled by default), and then a second issue where we decide if certain templates should be off-cache by default and a UI for controlling it.

The first issue can be tackled ASAP in a 2.0.x release, whereas the UI can be part of 2.1.

@amedina
Copy link
Member

amedina commented Sep 19, 2020

One possible approach is having something like the "Template Support" capability, or may be expanding that, to include the ability of opting out from cache at the template or content type level. And this will be complemented with the ability of opting out from the cache at a fine granularity level, along the lines of the options you suggested (Enabled, Enabled with/without AMP cache).

@westonruter
Copy link
Member Author

westonruter commented Sep 20, 2020

Yes, the only thing I have in mind so far is replacing each checkbox with a dropdown with those three options. We'll need to figure out a better way of showing such a dropdown as this doesn't look good:

Checkboxes Dropdowns
image image

@amedina
Copy link
Member

amedina commented Sep 26, 2020

Yeah; I was thinking to have two check boxes, one for enabling and one for the cache, with the latter conditionally enabled when the former is checked.

@westonruter
Copy link
Member Author

How would the checkboxes be labelled?

@amedina
Copy link
Member

amedina commented Sep 26, 2020

One possibility would be along the lines of labels indicating (1) AMP content generation, and (2) opt in/out from the cache:

  • Content
  • AMP Cache

And we can update the heading of the section to encompass the dual function.

@westonruter
Copy link
Member Author

Yeah. Let's explore some options for the UI to figure out something that minimizes clutter. The way it is right now with the one checkbox appears nice and tidy as the checkboxes kinda serve as bullet points. So if we add a second checkbox or use a select dropdown, then that bullet-point feeling will be lost:

image

Maybe we need a new icon that can represent the three states of (for lack of better icons, consider these emoji placeholders):

  • 🚫 AMP Document Off (i.e. Bento mode)
  • ⚠️ AMP Cache Off
  • ✅ AMP Document with AMP Cache

If we have icons for those three states, then perhaps instead of what we have right now:

image

We could replace the simple checkbox with the icon representing the current state. When you click the icon, then a dropdown could appear which could let you choose from among the 3 options. Once selected and the dropdown is closed, the icon could be updated to reflect the chosen option. So something like this:

image

I think this might be a tidy way to avoid clutter.

@kmyram kmyram added the UX label Sep 29, 2020
@jwold
Copy link
Collaborator

jwold commented Sep 29, 2020

So I’ve been playing with two concepts, neither of which are perfect, but which add to the discussion for some other ways we could approach this:

IMG_0141

@westonruter
Copy link
Member Author

User who also has requested this: https://wordpress.org/support/topic/disable-amp-cache-on-certain-pages/

@westonruter
Copy link
Member Author

So I’ve been playing with two concepts, neither of which are perfect, but which add to the discussion for some other ways we could approach this:

I don't think that UI allows for enough information density. Basically, every checkbox you see in this screenshot needs to be able to have this multi-state selection:

image

Some kind of dropdown that is revealed by an icon, where the icon represents the current state seems the most promising so far.

@westonruter
Copy link
Member Author

Now that I think about it more, perhaps we should only have AMP Cache enabled by default for the post post type. This is the post type that is least likely to be frequently updated. More often than not it will be published and then won't see further updates. The page post type is much more likely to be updated on an ongoing basis. So I think we can default AMP Cache to be off for any non-hierarchical post type and any archive template.

Oh, this brings up something else just realized. Before I said that the tri-state toggle would apply to everywhere that a checkbox appears now. However, this doesn't really work when you consider the singular template vs the individual post types. Let's say you want the singular template to have AMP Cache Enabled, and yet you indicate that the Page post type should have AMP Cache Disabled. The singular template is used to render page post type. So the user is not going to necessarily going to know which takes precedence.

One option to deal with this would be to keep the Content Types as being a simple boolean checkbox, but then to add the template subpoints under "Singular" (for is_page, is_single, etc). The drawback there is that we then have the post types being listed twice. Nevertheless, this is currently the case for post type archives:

$post_type_args = [
'has_archive' => true,
'public' => true,
];
foreach ( get_post_types( $post_type_args, 'objects' ) as $post_type ) {
$templates[ sprintf( 'is_post_type_archive[%s]', $post_type->name ) ] = [
'label' => $post_type->labels->archives,
'parent' => 'is_archive',
'callback' => static function ( WP_Query $query ) use ( $post_type ) {
return $query->is_post_type_archive( $post_type->name );
},
];
}

Something I realize we're not doing now is omitting the post type archive for post types that have AMP disabled. In the same way, if we list out each post type under Singular template, then we should probably omit those templates which don't have corresponding post type enabled.

@jwold
Copy link
Collaborator

jwold commented Oct 1, 2020

@westonruter is this what you were talking about this morning?

Screen Capture on 2020-10-01 at 14-14-21

@westonruter
Copy link
Member Author

@jwold lol, yes! I wasn't aware that this is standard behavior in Chrome now.

So one option would be to use a regular select and then remove the border and background so it looks like:

image

This gets us part of the way. What is not possible, however, is the inclusion of icons in option, yet.

This would work when the toggle is just for "AMP Enabled", like on the edit post screen:

image

However, I need to correct something I said this morning. In the context of the Template Hierarchy, the selected option would not be shown the control is not selected. Instead, it should be the icon representing the status followed by the template name (e.g. Singular). I suppose this is why I tried to mock it up like:

Mock

So in this case, throw out what I was thinking of entirely in regards to the selected option maintaining its vertical position when you tap on the control. The line item would be the template label (e.g. Singular) and then preceding that would be an icon indicating what its current support status is. Clicking on the icon should then reveal a dropdown to show what the currently selected icon means, as well as what other options are available. (In my mock I used a select dropdown so I couldn't add the icons inside.)

The goal is to keep the template hierarchy primarily showing the names of the templates and/or post types. Then to add these new settings by replacing the ☑️ with another icon which is a UI element to select from these other options. The hope is that this prevents the UI from becoming too cluttered.

@schlessera
Copy link
Collaborator

Regarding AMP cache defaults, you should take into consideration why a lot of site builders use AMP: they want to profit from the additional exposure that Google Search offers to AMP results => provided they are served via the Google AMP Cache. If you remove caching by default for most of the site, a lot of people will wait for these benefits to kick in only to find out at some point that the site is seemingly "misconfigured", and thus wastes the "AMP benefit" on the homepage and the evergreen pages.

@schlessera
Copy link
Collaborator

Regarding the borderless select, we need to find a compromise between sleek look and accessibility/discoverability. If something doesn't look like a control, no one will click on it to find out what it does...

@westonruter
Copy link
Member Author

Regarding AMP cache defaults, you should take into consideration why a lot of site builders use AMP: they want to profit from the additional exposure that Google Search offers to AMP results => provided they are served via the Google AMP Cache. If you remove caching by default for most of the site, a lot of people will wait for these benefits to kick in only to find out at some point that the site is seemingly "misconfigured", and thus wastes the "AMP benefit" on the homepage and the evergreen pages.

That's true, and the intention behind off-cache AMP is that any pages that opt-out of the AMP cache will still look the same in search results. They'll still have the AMP badge. The only difference is they won't get the prerendering, so tapping on a result will be slower to load since the page has to be loaded and rendered from origin, rather than having been loaded and pre-rendered in the background from the AMP Cache.

@schlessera
Copy link
Collaborator

Yes, but without the AMP cache, they will not appear in the different forms of carousels on mobile search results.

See https://developers.google.com/search/docs/guides/about-amp

@westonruter
Copy link
Member Author

I believe that will soon be outdated, but we will need to wait until off-cache AMP is launched in a couple weeks at AMP Fest to be sure. In any case, we can move forward with just a filter that allows a site owner to turn off AMP Cache at a programmatic level (with a filter). We just should make sure that this is done in a way that we anticipate a UI being added as well. So if that filter is added, for example, we may need to revert to only showing a simple checkbox since at that point we can no longer be sure that the UI toggles will have any effect (as the filter would presumably override them, unless we make the filter only change the default).

@jwold
Copy link
Collaborator

jwold commented Oct 6, 2020

Since our call I've been looking at some concepts that I'd love to discus in our product sync tomorrow.

First, here are some initial sketches:

My Sketches - 2020-10-05 13 36 29 4

The important factor here is for users to be able to understand that they are changing the states for different content types. So, for instance, for the Posts I want to toggle between how AMP is served. It needs to be clear what's happening, and there should be a way of describing that action. Each of these sketches conveys that in a different way.

Option D feels like the simplest, where the Cache toggle appears conditionally. However, as with the entire Onboarding wizard revamp, there is sometimes the opportunity to bring interactivity into an interface, which makes using the thing more delightful. So, with that in mind I took a stab at fleshing out Option C more fully.

Here's a 📹 video of the prototype: https://v.usetapes.com/N97gx3310z

@westonruter
Copy link
Member Author

Screenshot from prototype:

Screen Shot 2020-10-06 at 14 26 49

I like the popover with the list. That's good. The only thing is that feels somewhat busy. What would it look like if only showed the selected state as opposed to the selected icon and the two that are not selected? I like that you're styling the UI as a button which will encourage users to hover/tap to discover the options.

We'll still need to look into the iconography, as turning off AMP Cache shouldn't be ⚠️ although it serves as a placeholder for now. Actually, there's some key overlap with the iconography you've been working on for the plugin sidebar in #3821 (comment).

Editor Icon Note
image AMP disabled
image AMP enabled without cache/CDN
image + 🌐 AMP enabled with cache/CDN

The last icon could be some illustration combining the AMP icon with a 🌐 to represent the AMP Cache. (Heh, better than 💵.)

@westonruter
Copy link
Member Author

We'll also need to re-think the “Serve all templates as AMP” toggle since it wouldn't make sense anymore. In fact, this could also solve the label problem. What if above the template list we had three labelled buttons for the three states with their icons. Clicking those buttons would be doing bulk actions to set all templates to have the state. In this way, it would be similar to the bulk actions to change the state of multiple validation errors as discussed in #5427 and #5431. In this way we could freely show just the selected icon for each item rather than the selected one and the others being grayed out.

@jwold
Copy link
Collaborator

jwold commented Oct 7, 2020

Here's a refinement on the previous idea. It simplifies the user experience quite a bit, reducing the amount of icons and making the button action more clear.

Screen Capture on 2020-10-07 at 09-29-32

What if above the template list we had three labelled buttons for the three states with their icons.

This may still be valuable. I can't quite envision how that would look though.

Figma prototype: https://d.pr/70XnlU

@westonruter
Copy link
Member Author

I like that dropdown.

This may still be valuable. I can't quite envision how that would look though.

The template hierarchy could have a horizontal line above it, and then above that the dropdown could be prefixed by a label like "Update all templates:", or something.

@jwold
Copy link
Collaborator

jwold commented Oct 7, 2020

Should the "update all" button also apply to content types, or just to template hierarchy options?

@westonruter
Copy link
Member Author

Only the template hierarchy like we have the single toggle today.

I'm not sure yet if we will have the tri-state toggle for the content types. Those may get the binary checkbox still.

@amedina
Copy link
Member

amedina commented Oct 7, 2020

I like the dropdown, but I would prefer to have the Label first then the dropdown.

@westonruter
Copy link
Member Author

An issue I see with moving the dropdown after the label is that the label widths are variable, as each has a different template name. If the dropdowns were on the right then they wouldn't be neatly liked up vertically making it more difficult to repeatedly click down the list of templates with a mouse. An alternative could be to put the dropdowns vertically in a column to the right, but then we may need to add zebra striping to make sure users are clear which template is for which dropdown.

@jwold
Copy link
Collaborator

jwold commented Oct 8, 2020

Screen Shot 2020-10-08 at 9 20 29 AM

@westonruter here's how I envision the "update all" button could work.

And a note on the labeling, I agree that having the button before the label works better, but we also could setup a grid system of sorts as Weston described.

We could try something like this:

Screen Shot 2020-10-08 at 9 39 17 AM

@johnwatkins0
Copy link
Contributor

From Weston: "All archives should by default be not on AMP cache. Singular posts should be on AMP cache by default." Singular pages -- not sure yet.

@westonruter
Copy link
Member Author

The filter that is used for off-cache AMP should also be designed to also resolve filtering whether AMP is disabled for arbitrary queries (see #2817).

@westonruter
Copy link
Member Author

On that note, see #2817 (comment) for implications of filter. In particular, the plugin currently has AMP_Theme_Support::get_template_availability() which is passed a WP_Query or a WP_Post and that is used to look up the template and then that is checked for being supported. If a filter were added to this method to control whether a template is supported and is only passed this WP_Post or WP_Query as context, then that would be problematic because then in the UI then it seems problematic because we'd need to create a bunch of these WP_Query instances to pass into a bunch of filter calls, for example if we had:

return apply_filters( 'amp_template_supported', $supported, $query );

So instead of this, the AMP_Theme_Support::get_supportable_templates() method returns an array of supportable templates which includes a template ID which takes the format of $template_conditional[$argument], for example is_singular[post]. So we could use the template ID only in filtering like so:

return apply_filters( 'amp_template_supported', $supported, $template_id );

But that would limit flexibility for the filtering, since the reality is that the template hierarchy that we currently expose in the UI is a subset of all the template hierarchy. Nevertheless, it's true that users can add their own templates to the list via the amp_supportable_templates filter. But then there is an arbitrary number of query vars that can be registered as well that go beyond the template hierarchy which a user may want to use for toggling whether AMP is supported. So maybe we do need to figure out an efficient way to pass WP_Query when applying the filter for each template entry in the UI.


The $supported being filtered here would an enumeration of 4 values:

  • database — Use whatever the DB-saved setting indicates.
  • amp_disabled — Override the DB-saved setting by disabling AMP. This is similar to returning true from the amp_skip_post filter.
  • amp_enabled_without_cache — Override the DB-saved setting to enable AMP but opt-out of AMP Cache.
  • amp_enabled_with_cache — Override the DB-saved setting to enable AMP and allow AMP Cache.

@westonruter
Copy link
Member Author

Note that off-cache AMP will essentially come as a side effect for sites in Standard mode using Bento with L1/L2 sandboxing levels.

@westonruter westonruter modified the milestones: v2.2, v2.3 Jun 18, 2021
@jwold
Copy link
Collaborator

jwold commented Jul 29, 2021

This will still be importance in a Bento world, since we can have a site off cache depending on its sandbox level. It’s currently blocked upstream in the AMP project.

@westonruter westonruter modified the milestones: v2.3, v2.4 Dec 23, 2021
@westonruter
Copy link
Member Author

See latest comment on upstream issue: ampproject/amphtml#29296 (comment)

@westonruter westonruter removed this from the v2.4 milestone Apr 14, 2022
@westonruter westonruter closed this as not planned Won't fix, can't repro, duplicate, stale Jul 26, 2022
@github-project-automation github-project-automation bot moved this to Backlog in Ongoing Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bento Blocked Groomed P0 High priority Punted UX WS:Core Work stream for Plugin core
Projects
Archived in project
Development

No branches or pull requests

6 participants