Always allow switching to Reader mode, but default to mode defined by theme

[westonruter]

This is a follow-up on #2550.

When using a core theme, a notice is displayed encouraging a user to switch to Standard or Transitional mode:

If using a non-core theme without any special theme support, it looks like:

However, when using a theme like Neve which has built-in paired AMP support, the Reader mode option is removed:

The user can still switch to Transitional mode (see #2312), but the Reader mode is gone.

Additionally, if a theme is active which is AMP-first (non-paired amp theme support declared), then they see no options at all:

This can be undesirable because while the theme itself has amp support, the plugins being used by the theme may very well not. For such sites, having the option to stay in Reader mode is important until AMP-compatible plugins can be activated. As it stands today, the lack of Reader mode availability could cause theme makers hesitate from adding AMP compatibility, since it limits the options available to their users. It could cause users to either switch to a different theme if they want AMP, or turn off AMP if they really want the theme.

So we should let Reader mode always be an available option. Nevertheless, upon activating the AMP plugin and the theme has amp support, then the default option should be Standard when paired flag is present and Transitional when paired flag is present. As explained in #2312, if paired is absent then the Transitional mode should still not be made available.

This is related to #1384: Default to transitional mode with auto-sanitization for new activations, and to native mode when compatible theme/plugins active.

Proposed Changes

No change for when a core theme is being used, continuing to default to Reader mode or when using a theme that lacks amp theme support.

However, when switching to a theme like Neve which has paired theme support, then for existing plugin activations (which had previously been on Reader) then the Reader mode remains active and a green notice is displayed encouraging them to switch:

For new AMP plugin activations, the default mode is Transitional, and again the Reader mode is still presented as an option (the notice is removed because the user switched):

Similarly, if the user has activated a theme that has amp theme support without the paired flag (i.e. an AMP-first theme), then upon switching to such a theme from an existing Reader mode install they would see defaulting to Reader with a notice:

And brand new AMP plugin activations would default to Standard for themes that have non-paried amp theme support, but with the ability to switch to Reader:

[westonruter]

Best to suppress whitespace changes when reviewing: https://github.com/ampproject/amp-wp/pull/2622/files?w=1

[amedina]

LGTM. Tested the branch varying the theme support options. The changes make the UX more intuitive and I think it captures important use cases for the plugin.

[kienstra]

Hi @westonruter,
Hope your day's off to a great start.

What do you think about moving this to 'Ready for Merging,' without more functional testing? I could test this also, if that would help.

Thanks, Weston!

[westonruter]

I did test various scenarios, including variables:

• Upgrading from 1.0 to 1.2
• Testing with DB that had theme_support option saved for native, paired, and disabled.
• Activating a theme with theme support in 1.0 and ensuring the same mode is selected upon upgrade.
• On fresh installs, activating the plugin should have default mode that corresponds to mode defined in the theme.

Having your set of eyes on testing to make sure it works as expected would be helpful.

[kienstra]

Good idea. Like you mentioned, the theme might have support, but plugins might not.

[kienstra]

Upgrading from 1.0 to 1.2
theme_support option looks good

1. 'Classic' mode to 'Reader' mode:

2. 'Paired' mode to 'Transitional' mode:

3. 'Native' mode to 'Standard' mode:

Steps Taken

1. Activate Twenty Nineteen
2. Run wp option delete amp-options
3. Use the 1.0.0-built tag
4. In the AMP Options menu, observe that 'Classic' mode is selected
5. Use the `1.2-RC1-built tag
6. Verify that the right template options are present, like 'Reader' mode
7. Repeat steps 2-6 for 'Paired' and 'Native' modes (though in step 4, change the template mode)

[kienstra]

Theme with declared AMP support
Worked as expected

Activating a theme with theme support in 1.0 and ensuring the same mode is selected upon upgrade.

Steps Taken

1. Activate Twenty Fifteen child theme that has add_theme_support( 'amp' )
2. wp option delete amp-options
3. git fetch --tags && git checkout 1.0.0-built
4. Observe the AMP Settings page:

5. git checkout 1.2-RC1-built
6. In the AMP Options menu, observe that 'Standard' mode is selected, as expected:

7. Repeat steps 2-5, but in the child theme change add_theme_support( 'amp' ) to add_theme_support( 'amp', array( 'paired' => true ) )

8. As expected, on checking out 1.2-RC1-built, it's in 'Transitional' mode:

9. 'Transitional' mode works as expected, with a non-AMP URL and an AMP URL

[kienstra]

Testing on (simulated) fresh install
Also worked as expected

On fresh installs, activating the plugin should have default mode that corresponds to mode defined in the theme.

Steps Taken

1. wp option delete amp-options # simulate a fresh install
2. git checkout 1.2-RC1-built
3. Activate a Twenty Fifteen child theme that has add_theme_support( 'amp' )
4. As expected, 'Standard' mode is selected:

5. wp option delete amp-options
6. In the child theme, change add_theme_support( ‘amp’ ) to add_theme_support( ‘amp’, array( ‘paired’ => true ) )
7. As expected, 'Transitional' is selected, but the user can change it:

[kienstra]

Looks Good!

Hi @westonruter,
Sorry for the delay. All of the variables that you mentioned above also looked good in my testing.

See you soon!

[westonruter]

Thank you for confirming!