Improve Sandboxing

assets/src/settings-page/index.js

@@ -44,6 +44,7 @@ import { PluginsContextProvider } from '../components/plugins-context-provider';
 import { ThemesContextProvider } from '../components/themes-context-provider';
 import { SiteScanContextProvider } from '../components/site-scan-context-provider';
 import { Welcome } from './welcome';
+import { SandboxingLevel } from './sandboxing-level';
 import { TemplateModes } from './template-modes';
 import { SupportedTemplates } from './supported-templates';
 import { SettingsFooter } from './settings-footer';
@@ -221,6 +222,7 @@ function Root( { appRoot } ) {
 				<h2 id="advanced-settings">
 					{ __( 'Advanced Settings', 'amp' ) }
 				</h2>
+				<SandboxingLevel focusedSection={ focusedSection } />
 				<AMPDrawer
 					heading={ (
 						<h3>

assets/src/settings-page/sandboxing-level.js

@@ -0,0 +1,105 @@
+/**
+ * External dependencies
+ */
+import PropTypes from 'prop-types';
+
+/**
+ * WordPress dependencies
+ */
+import { useContext } from '@wordpress/element';
+import { __ } from '@wordpress/i18n';
+
+/**
+ * Internal dependencies
+ */
+import { Options } from '../components/options-context-provider';
+import { AMPDrawer } from '../components/amp-drawer';
+import { STANDARD } from '../common/constants';
+
+/**
+ * Component rendering the sandboxing level.
+ *
+ * @param {Object} props                Component props.
+ * @param {string} props.focusedSection Focused section.
+ */
+export function SandboxingLevel( { focusedSection } ) {
+	const { updateOptions, editedOptions: {
+		sandboxing_level: sandboxingLevel,
+		theme_support: themeSupport,
+	} } = useContext( Options );
+
+	if ( ! sandboxingLevel || STANDARD !== themeSupport ) {
+		return null;
+	}
+
+	return (
+		<AMPDrawer
+			heading={ (
+				<h3>
+					{ __( 'Sandboxing Level (Experimental)', 'amp' ) }
+				</h3>
+			) }
+			hiddenTitle={ __( 'Sandboxing Level (Experimental)', 'amp' ) }
+			id="sandboxing-level"
+			initialOpen={ 'sandboxing-level' === focusedSection }
+		>
+			<p>
+				{ __( 'Try out a more flexible AMP by generating pages that use AMP components without requiring AMP validity! By selecting a sandboxing level, you are indicating the minimum degree of sanitization. For example, if you selected the loose leve but have a page without any POST form and no custom scripts, it will still be served as valid AMP—the same as if you had selected the strict level.', 'amp' ) }
+			</p>
+			<ol>
+				<li>
+					<input
+						type="radio"
+						id="sandboxing-level-1"
+						checked={ 1 === sandboxingLevel }
+						onChange={ () => {
+							updateOptions( { sandboxing_level: 1 } );
+						} }
+					/>
+					<label htmlFor="sandboxing-level-1">
+						<strong>
+							{ __( 'Loose:', 'amp' ) }
+						</strong>
+						{ ' ' + __( 'Do not remove any AMP-invalid markup by default, including custom scripts. CSS processing is disabled.', 'amp' ) }
+					</label>
+				</li>
+				<li>
+					<input
+						type="radio"
+						id="sandboxing-level-2"
+						checked={ 2 === sandboxingLevel }
+						onChange={ () => {
+							updateOptions( { sandboxing_level: 2 } );
+						} }
+					/>
+					<label htmlFor="sandboxing-level-2">
+						<strong>
+							{ __( 'Moderate:', 'amp' ) }
+						</strong>
+						{ ' ' + __( 'Remove anything invalid AMP except for POST forms, excessive CSS, and other PX-verified markup.', 'amp' ) }
+					</label>
+				</li>
+				<li>
+					<input
+						type="radio"
+						id="sandboxing-level-3"
+						checked={ 3 === sandboxingLevel }
+						onChange={ () => {
+							updateOptions( { sandboxing_level: 3 } );
+						} }
+					/>
+					<label htmlFor="sandboxing-level-3">
+						<strong>
+							{ __( 'Strict:', 'amp' ) }
+						</strong>
+						{ ' ' + __( 'Require valid AMP, removing all markup that causes validation errors.', 'amp' ) }
+					</label>
+				</li>
+
+			</ol>
+		</AMPDrawer>
+	);
+}
+SandboxingLevel.propTypes = {
+	focusedSection: PropTypes.string,
+};

assets/src/settings-page/style.css

@@ -367,6 +367,7 @@ li.error-kept {
 	}
 }
 
+#sandboxing-level .amp-drawer__panel-body-inner,
 #site-scan .amp-drawer__panel-body-inner,
 #site-review .amp-drawer__panel-body-inner,
 #plugin-suppression .amp-drawer__panel-body-inner,
@@ -658,3 +659,31 @@ li.error-kept {
 .amp-other-settings p {
 	font-size: 0.875rem;
 }
+
+/* Sandboxing Level */
+#sandboxing-level .amp-drawer__panel-body-inner p {
+	font-size: 14px;
+	margin-top: 0;
+}
+
+#sandboxing-level .amp-drawer__panel-body-inner ol {
+	margin-top: 1.5rem;
+	margin-left: 0;
+}
+
+#sandboxing-level .amp-drawer__panel-body-inner li {
+	list-style-type: none;
+}
+
+#sandboxing-level .amp-drawer__panel-body-inner li:not(:last-child) {
+	margin-bottom: 12px;
+}
+
+#sandboxing-level .amp-drawer__panel-body-inner input[type="radio"] {
+	margin-right: 12px;
+}
+
+#sandboxing-level .amp-drawer__panel-body-inner input[type="radio"],
+#sandboxing-level .amp-drawer__panel-body-inner label {
+	vertical-align: middle;
+}

assets/src/settings-page/template-modes.js

@@ -62,10 +62,8 @@ function NotRecommendedNotice() {
 export function TemplateModes( { focusReaderThemes } ) {
 	const {
 		editedOptions: {
-			sandboxing_level: sandboxingLevel,
 			theme_support: editedThemeSupport,
 		},
-		updateOptions,
 	} = useContext( Options );
 	const { selectedTheme, templateModeWasOverridden } = useContext( ReaderThemes );
 	const { templateModeRecommendation, staleTemplateModeRecommendation } = useTemplateModeRecommendation();
@@ -106,71 +104,7 @@ export function TemplateModes( { focusReaderThemes } ) {
 				initialOpen={ false }
 				mode={ STANDARD }
 				labelExtra={ getLabelForTemplateMode( STANDARD ) }
-			>
-				{
-					sandboxingLevel && (
-						<fieldset>
-							<h4 className="title">
-								{ __( 'Sandboxing Level (Experimental)', 'amp' ) }
-							</h4>
-							<p>
-								{ __( 'Try out a more flexible AMP by generating pages that use AMP components without requiring AMP validity! By selecting a sandboxing level, you are indicating the minimum degree of sanitization. For example, if you selected level 1 but have a page without any POST form and no custom scripts, it will still be served as valid AMP, the same as if you had selected level 3.', 'amp' ) }
-							</p>
-							<ol>
-								<li>
-									<input
-										type="radio"
-										id="sandboxing-level-1"
-										checked={ 1 === sandboxingLevel }
-										onChange={ () => {
-											updateOptions( { sandboxing_level: 1 } );
-										} }
-									/>
-									<label htmlFor="sandboxing-level-1">
-										<strong>
-											{ __( 'Loose:', 'amp' ) }
-										</strong>
-										{ ' ' + __( 'Do not remove any AMP-invalid markup by default, including custom scripts. CSS tree-shaking is disabled.', 'amp' ) }
-									</label>
-								</li>
-								<li>
-									<input
-										type="radio"
-										id="sandboxing-level-2"
-										checked={ 2 === sandboxingLevel }
-										onChange={ () => {
-											updateOptions( { sandboxing_level: 2 } );
-										} }
-									/>
-									<label htmlFor="sandboxing-level-2">
-										<strong>
-											{ __( 'Moderate:', 'amp' ) }
-										</strong>
-										{ ' ' + __( 'Remove non-AMP markup, but allow POST forms. CSS tree shaking is enabled.', 'amp' ) }
-									</label>
-								</li>
-								<li>
-									<input
-										type="radio"
-										id="sandboxing-level-3"
-										checked={ 3 === sandboxingLevel }
-										onChange={ () => {
-											updateOptions( { sandboxing_level: 3 } );
-										} }
-									/>
-									<label htmlFor="sandboxing-level-3">
-										<strong>
-											{ __( 'Strict:', 'amp' ) }
-										</strong>
-										{ ' ' + __( 'Require valid AMP.', 'amp' ) }
-									</label>
-								</li>
-
-							</ol>
-						</fieldset>
-					)
-				}
-			</TemplateModeOption>
+			/>
 			<TemplateModeOption
 				details={ templateModeRecommendation?.[ TRANSITIONAL ]?.details }
 				detailsUrl="https://amp-wp.org/documentation/getting-started/transitional/"

includes/class-amp-theme-support.php

@@ -2177,9 +2177,16 @@ static function( Optimizer\Error $error ) {
 			}
 		}
 
-		if ( Sandboxing::is_needed() ) {
-			Services::get( 'sandboxing' )->finalize_document( $dom, $effective_sandboxing_level );
-		}
+		/**
+		 * Fires immediately before the DOM is serialized to send as the response body.
+		 *
+		 * @since 2.2
+		 * @internal
+		 *
+		 * @param Document $dom                        Document prior to serialization.
+		 * @param int      $effective_sandboxing_level Effective sandboxing level.
+		 */
+		do_action( 'amp_finalize_dom', $dom, $effective_sandboxing_level );
 
 		$response = $dom->saveHTML();
 

includes/sanitizers/class-amp-comments-sanitizer.php

@@ -59,69 +59,90 @@ public function init( $sanitizers ) {
 	 * @since 0.7
 	 */
 	public function sanitize() {
-		foreach ( $this->dom->getElementsByTagName( Tag::FORM ) as $comment_form ) {
-			$action = $comment_form->getAttribute( Attribute::ACTION_XHR );
+
+		// Find the comment form (which may not have the commentform ID).
+		$comment_form = null;
+		foreach ( $this->dom->getElementsByTagName( Tag::FORM ) as $form ) {
+			$action = $form->getAttribute( Attribute::ACTION_XHR );
 			if ( ! $action ) {
-				$action = $comment_form->getAttribute( Attribute::ACTION );
+				$action = $form->getAttribute( Attribute::ACTION );
 			}
 			$action_path = wp_parse_url( $action, PHP_URL_PATH );
 			if ( $action_path && 'wp-comments-post.php' === basename( $action_path ) ) {
-				$this->ampify_threaded_comments( $comment_form );
+				$comment_form = $form;
+				break;
 			}
 		}
 
-		if ( $this->args['comments_live_list'] ) {
-			$comments = $this->dom->xpath->query( '//amp-live-list/*[ @items ]/*[ starts-with( @id, "comment-" ) ]' );
-
-			foreach ( $comments as $comment ) {
-				$this->add_amp_live_list_comment_attributes( $comment );
-			}
-		}
-	}
-
-	/**
-	 * Ampify threaded comments by utilizing amp-bind to implement comment reply functionality.
-	 *
-	 * The logic here is only needed if:
-	 * 1. Threaded comments is enabled, and
-	 * 2. The comment-reply script was not added to the page.
-	 *
-	 * @param Element $comment_form Comment form.
-	 */
-	protected function ampify_threaded_comments( Element $comment_form ) {
-		if ( ! get_option( 'thread_comments' ) ) {
-			return;
-		}
-
-		$comment_reply_script = $this->dom->xpath->query( '//script[ @id = "comment-reply-js" ]' )->item( 0 );
-		if ( 'never' === $this->args['ampify_comment_threading'] ) {
-			if ( $comment_reply_script instanceof Element ) {
-				$this->prepare_native_comment_reply( $comment_reply_script );
-			}
-			return;
-		}
-
+		// Handle the comment reply script.
+		$comment_reply_script            = $this->get_comment_reply_script();
+		$should_ampify_comment_threading = false;
 		if ( $comment_reply_script instanceof Element ) {
-			if (
+			if ( 'never' === $this->args['ampify_comment_threading'] ) {
+				$this->prepare_native_comment_reply( $comment_reply_script );
+				$should_ampify_comment_threading = false;
+			} elseif (
 				'always' === $this->args['ampify_comment_threading']
 				||
 				(
 					'conditionally' === $this->args['ampify_comment_threading']
 					&&
-					$comment_form->hasAttribute( Attribute::ACTION_XHR )
+					(
+						// If there isn't even a comment form on the page, then the comment-reply script shouldn't be here at all.
+						! $comment_form instanceof Element
+						||
+						// If the form has an action-xhr attribute, then it's going to be an AMP page and we need ampified comment threading.
+						$comment_form->hasAttribute( Attribute::ACTION_XHR )
+					)
 				)
 			) {
 				// Remove the script and then proceed with the amp-bind implementation below.
 				$comment_reply_script->parentNode->removeChild( $comment_reply_script );
+				$should_ampify_comment_threading = true;
 			} else {
 				// This is the conditionally-no case.
 				$this->prepare_native_comment_reply( $comment_reply_script );
 
 				// Do not proceed with the AMP-bind implementation for threaded comments since the comment-reply script was included.
-				return;
+				$should_ampify_comment_threading = false;
 			}
 		}
 
+		// Now based on the comment reply script handling above, ampify the comment form.
+		if ( get_option( 'thread_comments' ) && $comment_form && $should_ampify_comment_threading ) {
+			$this->ampify_threaded_comments( $comment_form );
+		}
+
+		if ( $this->args['comments_live_list'] ) {
+			$comments = $this->dom->xpath->query( '//amp-live-list/*[ @items ]/*[ starts-with( @id, "comment-" ) ]' );
+
+			foreach ( $comments as $comment ) {
+				$this->add_amp_live_list_comment_attributes( $comment );
+			}
+		}
+	}
+
+	/**
+	 * Get comment reply script.
+	 *
+	 * @return Element|null
+	 */
+	protected function get_comment_reply_script() {
+		$element = $this->dom->getElementById( 'comment-reply-js' );
+		if ( $element instanceof Element && Tag::SCRIPT === $element->tagName ) {
+			return $element;
+		} else {
+			return null;
+		}
+	}
+
+	/**
+	 * Ampify threaded comments by utilizing amp-bind to implement comment reply functionality.
+	 *
+	 * @param Element $comment_form Comment form.
+	 */
+	protected function ampify_threaded_comments( Element $comment_form ) {
+
 		// Create reply state.
 		$amp_state = $this->dom->createElement( Extension::STATE );
 		$comment_form->insertBefore( $amp_state, $comment_form->firstChild );