Add theme support settings to admin screen; prevent serving dirty AMP #1199

westonruter · 2018-06-06T01:29:17Z

Add admin settings for picking between disabled, paired, and native theme support. See Add theme support mode selection in AMP general settings page #1196.
Prevent making AMP Gutenberg blocks available when AMP is not native.
Prevent serving dirty AMP by forcibly sanitizing all validation errors when amp_is_canonical(). See Avoid serving dirty AMP until it is supported by the runtime #1192.
Add checkbox for automatically allowing tree shaking. This is not shown when native/canonical because dirty AMP is not allowed.
Add checkbox for automatically sanitizing all validation errors (including tree shaking).
Make explicitly clear that unaccepted validation errors will block rendering on AMP
When validation errors are automatically sanitized, ensure the terms' term_group is updated when re-checking.
Update AMP_Options_Manager::get_options() and to return default values.
Prevent showing amphtml link in paired mode when it is known that the URL has blocking validation errors.
Add link to General settings admin screen in plugin links.
Improve UI for distinguishing between validation errors which are rejected but which are also forced to be sanitized (such as in native AMP mode). These are shown as flagged.
Fix: Remove menu-toggle in Twenty Fifteen if there is nothing to open.
Add an AMP status item to the admin bar to give easy access to the Invalid URL post for any given URL, with an indicator for whether AMP is available, or else to initiate a validation check if no such post exists.

Fixes #1196.
Fixes #1192.

… handling * Add admin settings for picking between disabled, paired, and native theme support. See #1196. * Add checkbox for automatically allowing tree shaking. * Add checkbox for automatically sanitizing all validation errors (including tree shaking). * Make explicitly clear that unaccepted validation errors will block rendering on AMP * Prevent serving dirty AMP by forcibly sanitizing all validation errors when amp_is_canonical(). See #1192. * When validation errors are automatically sanitized, ensure the terms' term_group is updated when re-checking. * Update AMP_Options_Manager::get_options() and to return default values.

kienstra

Approved

Hi @westonruter,
This looks really good, especially the new 'AMP Settings' page. There are a few small points, but no blocker.

kienstra · 2018-06-06T04:48:53Z

includes/class-amp-theme-support.php

+			if ( 'paired' === $theme_support_option ) {
+				$args['template_dir'] = './';
+			}
+			add_theme_support( 'amp', $args );


It's nice how this can force theme support.

kienstra · 2018-06-06T04:51:19Z

includes/options/class-amp-options-menu.php

+					</label>
+				</dt>
+				<dd>
+					<?php esc_html_e( 'Display AMP responses in classic (legacy) post templates in a basic design that does not match your theme\'s templates.', 'amp' ); ?>


Good description of legacy templating.

kienstra · 2018-06-06T05:06:02Z

includes/options/class-amp-options-menu.php

+					<p>
+						<label for="force_sanitization">
+							<input id="force_sanitization" type="checkbox" name="<?php echo esc_attr( AMP_Options_Manager::OPTION_NAME . '[force_sanitization]' ); ?>" <?php checked( AMP_Options_Manager::get_option( 'force_sanitization' ) ); ?>>
+							<?php esc_html_e( 'Automatically accept sanitization for any AMP validation error that is encountered.', 'amp' ); ?>


It's great how this conditionally displays on selecting "Paired":

kienstra · 2018-06-06T05:10:22Z

includes/options/class-amp-options-menu.php

+				<dt>
+					<input type="radio" id="theme_support_disabled" name="<?php echo esc_attr( AMP_Options_Manager::OPTION_NAME . '[theme_support]' ); ?>" value="disabled" <?php checked( $theme_support, 'disabled' ); ?> <?php disabled( ! $theme_support_mutable ); ?>>
+					<label for="theme_support_disabled">
+						<strong><?php esc_html_e( 'Disabled', 'amp' ); ?></strong>


If somebody looked at this really quickly, it might look like AMP is disabled, not just theme support:

Maybe "Disabled" could be "Classic Templates (Disabled)."

That's a great point. Agreed.

And “Paired” doesn't really make sense to users at first look either, nor does “Native”.

Maybe:

Classic Templates (Legacy)

Active Theme's Templates with Separate URLs (Paired)

Active Theme's Templates without Separate URLs (Canonical/Native)

I don't feel great about these these latter two.

Renamed Disabled to Classic in 5bae33e. Needs more iteration.

To look at it from a different angle, perhaps we should consider using an other term than "Theme Support". I understand the reasoning behind it as it is technically defined using add_theme_support() but what this feature really does is controlling the AMP Mode. So perhaps the setting itself should be called "AMP Mode" and then have:

Legacy

Paired

Native

Yes, that makes perfect sense. 👍

Maybe “Template Mode” would be better since it's clearly related to AMP already.

kienstra · 2018-06-06T05:24:49Z

includes/admin/class-amp-editor-blocks.php

@@ -43,8 +43,16 @@ public function init() {
 		if ( function_exists( 'gutenberg_init' ) ) {
 			add_action( 'enqueue_block_editor_assets', array( $this, 'enqueue_block_editor_assets' ) );
 			add_filter( 'wp_kses_allowed_html', array( $this, 'whitelist_block_atts_in_wp_kses_allowed_html' ), 10, 2 );
-			add_filter( 'the_content', array( $this, 'tally_content_requiring_amp_scripts' ) );
-			add_action( 'wp_print_footer_scripts', array( $this, 'print_dirty_amp_scripts' ) );


Do you think the methods tally_content_requiring_amp_scripts() and print_dirty_amp_scripts() could be removed, given that they're not used anymore? Maybe the comment below could link to them.

You're probably right. It's easy enough to look back in history to find the removal of these methods to resurrect them. 👍

Done in 5bae33e.

westonruter · 2018-06-06T07:34:25Z

In a5756d9 added Settings link to plugins list table:

amedina

These changes are of paramount importance for success on the adoption of the plugin. Great work.

westonruter · 2018-06-07T16:48:28Z

BTW, the PR regarding URL handling (#1203) is a dependency for the PR regarding the new validation error screen because it ensures that the amp_invalid_url post for a given URL will always be stored with the canonical URL which will then allow for us to reliably add an “AMP Status” item to the admin bar. This is important because if there are unaccepted validation errors on a paired URL then they will get redirected to the non-AMP version and this admin bar item can indicate why. Likewise, we need to omit the amphtml link from the canonical page if there are unaccepted validation errors on the page.

…dd/admin-theme-support-options

…tion errors

… (and why) * Indicate the count of validation errors that are blocking AMP from being available. * Allow passing URL string in addition to amp_invalid_url post to AMP_Invalid_URL_Post_Type::get_invalid_url_validation_errors().

…ence

…ut which are still forcibly sanitized * Fix PHP 5.3 error * Remove automatically setting amp_validation_error term_group based on forced status. * Let preview URL include development=1 flag.

postphotos · 2018-06-08T08:13:52Z

This is great stuff, @westonruter! 🎂 🎉 I really like the thought you've put into this.

…s allowed

* Update post editor warnings to reflect when validation errors are forcibly sanitized but not explicitly accepted. * Allow tree shaking setting to be shown when in native mode. * Show notice when selecting native mode that sanitization is required. * Hide the preview button from the amp_invalid_url post if sanitization is required.

…te link Discontinue automatically-deleting an amp_invalid_url post when all errors are resolved.

westonruter · 2018-06-08T21:51:09Z

Now there is an admin bar menu item to link to the AMP version of the page (when available):

As well as the AMP validation status (when it is known):

If you click on the AMP link and there are validation errors, you'll get redirected back to the canonical URL with the admin bar updated to indicate this:

Clicking on the validate link takes you to the Invalid AMP URL edit post screen which lists out all of the errors so you can take action. This validate admin menu item is present even if there are no known validation errors:

kienstra · 2018-08-03T18:32:47Z

Moving To "Ready For Merging"

At some point, it'd be appropriate to test this. But as work on the Template Modes is ongoing, I'd propose that we don't need to test this now.

But if you have other ideas, feel free to move this back to "Ready For QA."

westonruter added 3 commits June 5, 2018 18:21

Prevent making AMP blocks available when AMP is not native

5a81073

Remove menu-toggle in Twenty Fifteen if there is nothing to open

92424fb

westonruter added this to the v1.0 milestone Jun 6, 2018

westonruter requested review from amedina, ThierryA and kienstra June 6, 2018 01:29

kienstra self-assigned this Jun 6, 2018

kienstra approved these changes Jun 6, 2018

View reviewed changes

westonruter added 2 commits June 5, 2018 22:57

Remove dirty AMP support; rename Disabled to Classic

5bae33e

Add link to settings screen among plugin action links

a5756d9

Rename Theme Support to Template Mode

0068e79

amedina approved these changes Jun 6, 2018

View reviewed changes

Fix undefined index notice

be0557b

westonruter added 5 commits June 7, 2018 09:51

Merge branch 'url-handling' into add/admin-theme-support-options

b94a830

Merge branch 'develop' of https://github.com/Automattic/amp-wp into a…

0c339d9

…dd/admin-theme-support-options

Omit displaying the amphtml link if there are known unaccepted valida…

9971e22

…tion errors

Simplify how validation errors get auto-accepted based on user prefer…

f864d67

…ence

kienstra removed their assignment Jun 8, 2018

Introduce "Flagged" status for validation errors which are Rejected b…

d890b6c

…ut which are still forcibly sanitized * Fix PHP 5.3 error * Remove automatically setting amp_validation_error term_group based on forced status. * Let preview URL include development=1 flag.

westonruter force-pushed the add/admin-theme-support-options branch from 4f44e7c to d890b6c Compare June 8, 2018 07:16

westonruter added 5 commits June 8, 2018 10:15

Merge flagged status into Rejected and Accepted

ec95351

Prevent raising removed_unused_css_rules as error when tree-shaking i…

365d571

…s allowed

Add AMP link to admin bar with indication of AMP status and re-valida…

5c6b486

…te link Discontinue automatically-deleting an amp_invalid_url post when all errors are resolved.

Omit AMP admin bar menu item from admin

616dad7

westonruter changed the title ~~[WIP] Add theme support settings to admin screen; prevent serving dirty AMP~~ Add theme support settings to admin screen; prevent serving dirty AMP Jun 8, 2018

Add duplicate admin bar sub item link for sake of mobile

421dc74

westonruter merged commit 634fbb8 into develop Jun 8, 2018

westonruter added this to Ready for review in v1.0 Jun 25, 2018

westonruter moved this from Ready for review to Ready for QA in v1.0 Jun 25, 2018

westonruter deleted the add/admin-theme-support-options branch July 5, 2018 15:35

kienstra moved this from Ready for QA to Ready for Merging in v1.0 Aug 3, 2018

kienstra moved this from Ready for Merging to In Production in v1.0 Dec 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add theme support settings to admin screen; prevent serving dirty AMP #1199

Add theme support settings to admin screen; prevent serving dirty AMP #1199

westonruter commented Jun 6, 2018 •

edited

kienstra left a comment

kienstra Jun 6, 2018

kienstra Jun 6, 2018

kienstra Jun 6, 2018

kienstra Jun 6, 2018

westonruter Jun 6, 2018 •

edited

westonruter Jun 6, 2018

ThierryA Jun 6, 2018 •

edited

westonruter Jun 6, 2018

kienstra Jun 6, 2018

westonruter Jun 6, 2018 •

edited

westonruter commented Jun 6, 2018

amedina left a comment

westonruter commented Jun 7, 2018

postphotos commented Jun 8, 2018

westonruter commented Jun 8, 2018

kienstra commented Aug 3, 2018

Add theme support settings to admin screen; prevent serving dirty AMP #1199

Add theme support settings to admin screen; prevent serving dirty AMP #1199

Conversation

westonruter commented Jun 6, 2018 • edited

kienstra left a comment

Choose a reason for hiding this comment

kienstra Jun 6, 2018

Choose a reason for hiding this comment

kienstra Jun 6, 2018

Choose a reason for hiding this comment

kienstra Jun 6, 2018

Choose a reason for hiding this comment

kienstra Jun 6, 2018

Choose a reason for hiding this comment

westonruter Jun 6, 2018 • edited

Choose a reason for hiding this comment

westonruter Jun 6, 2018

Choose a reason for hiding this comment

ThierryA Jun 6, 2018 • edited

Choose a reason for hiding this comment

westonruter Jun 6, 2018

Choose a reason for hiding this comment

kienstra Jun 6, 2018

Choose a reason for hiding this comment

westonruter Jun 6, 2018 • edited

Choose a reason for hiding this comment

westonruter commented Jun 6, 2018

amedina left a comment

Choose a reason for hiding this comment

westonruter commented Jun 7, 2018

postphotos commented Jun 8, 2018

westonruter commented Jun 8, 2018

kienstra commented Aug 3, 2018

westonruter commented Jun 6, 2018 •

edited

westonruter Jun 6, 2018 •

edited

ThierryA Jun 6, 2018 •

edited

westonruter Jun 6, 2018 •

edited