Remove obsolete ampPageHasErrors and improve isAmpDocument passing

ampproject · Nov 5, 2020 · 12378ae · 12378ae
1 parent 76c1caf
commit 12378ae
Show file tree

Hide file tree

Showing 4 changed files with 11 additions and 87 deletions.
diff --git a/assets/src/admin/paired-browsing/app.js b/assets/src/admin/paired-browsing/app.js
@@ -1,7 +1,7 @@
 /**
  * WordPress dependencies
  */
-import { addQueryArgs, hasQueryArg, removeQueryArgs } from '@wordpress/url';
+import { addQueryArgs, removeQueryArgs } from '@wordpress/url';
 /**
  * Internal dependencies
  */
@@ -79,7 +79,6 @@ class PairedBrowsingApp {
 	constructor() {
 		this.nonAmpIframe = document.querySelector( '#non-amp iframe' );
 		this.ampIframe = document.querySelector( '#amp iframe' );
-		this.ampPageHasErrors = false; // @todo This is obsolete. Remove this and invalid-amp span.
 
 		this.currentNonAmpUrl = this.nonAmpIframe.src;
 		this.currentAmpUrl = this.ampIframe.src;
@@ -90,10 +89,6 @@ class PairedBrowsingApp {
 
 		// Overlay that is displayed on the client that becomes disconnected.
 		this.disconnectOverlay = document.querySelector( '.disconnect-overlay' );
-		this.disconnectText = {
-			general: document.querySelector( '.disconnect-overlay .dialog-text span.general' ),
-			invalidAmp: document.querySelector( '.disconnect-overlay .dialog-text span.invalid-amp' ),
-		};
 		this.disconnectButtons = {
 			exit: document.querySelector( '.disconnect-overlay .button.exit' ),
 			goBack: document.querySelector( '.disconnect-overlay .button.go-back' ),
@@ -211,16 +206,9 @@ class PairedBrowsingApp {
 		const isClientConnected = this.isClientConnected( iframe );
 
 		if ( ! isClientConnected ) {
-			if ( this.ampIframe === iframe && this.ampPageHasErrors ) {
-				this.disconnectText.general.classList.toggle( 'hidden', true );
-				this.disconnectText.invalidAmp.classList.toggle( 'hidden', false );
-			} else {
-				this.disconnectText.general.classList.toggle( 'hidden', false );
-				this.disconnectText.invalidAmp.classList.toggle( 'hidden', true );
-			}
-
 			// Show the 'Go Back' button if the parent window has history.
 			this.disconnectButtons.goBack.classList.toggle( 'hidden', 0 >= window.history.length );
+
 			// If the document is not available, the window URL cannot be accessed.
 			this.disconnectButtons.exit.classList.toggle( 'hidden', null === iframe.contentDocument );
 
@@ -246,7 +234,7 @@ class PairedBrowsingApp {
 	 * @param {HTMLIFrameElement} iframe The iframe.
 	 */
 	isClientConnected( iframe ) {
-		if ( this.ampIframe === iframe && this.ampPageHasErrors ) {
+		if ( this.ampIframe === iframe ) {
 			return false;
 		}
 
@@ -292,16 +280,6 @@ class PairedBrowsingApp {
 		return parsedUrl.href;
 	}
 
-	/**
-	 * Checks if a URL has the 'amp_validation_errors' query variable.
-	 *
-	 * @param {string} url URL string.
-	 * @return {boolean} True if such query var exists, false if not.
-	 */
-	urlHasValidationErrorQueryVar( url ) {
-		return hasQueryArg( url, 'amp_validation_errors' );
-	}
-
 	/**
 	 * Replace location.
 	 *
@@ -365,37 +343,16 @@ class PairedBrowsingApp {
 				return;
 			}
 
+			// Force the AMP iframe to always have an AMP URL.
 			if ( ! isAmpDocument ) {
-				if ( this.urlHasValidationErrorQueryVar( ampUrl ) ) {
-					/*
-					 * If the AMP page has validation errors, mark the page as invalid so that the
-					 * 'disconnected' overlay can be shown.
-					 */
-					this.ampPageHasErrors = true;
-					this.toggleDisconnectOverlay( this.ampIframe );
-					return;
-				} else if ( ampUrl ) {
-					// Force the AMP iframe to always have an AMP URL, if an AMP version is available.
-					this.replaceLocation( sourceIframe, ampUrl );
-					return;
-				}
-
-				/*
-				 * If the AMP iframe has loaded a non-AMP page and none of the conditions above are
-				 * true, then explicitly mark it as having errors and display the 'disconnected
-				 * overlay.
-				 */
-				this.ampPageHasErrors = true;
-				// this.toggleDisconnectOverlay( this.ampIframe );
+				this.replaceLocation( sourceIframe, ampUrl );
 				return;
 			}
 
 			this.currentAmpUrl = ampUrl;
 
 			// Update the AMP link above the iframe used for exiting paired browsing.
 			this.ampLink.href = removeQueryArgs( ampUrl, noampQueryVar );
-
-			this.ampPageHasErrors = false;
 		} else {
 			// Stop if the URL has not changed.
 			if ( this.currentNonAmpUrl === nonAmpUrl ) {

diff --git a/assets/src/admin/paired-browsing/client.js b/assets/src/admin/paired-browsing/client.js
@@ -9,36 +9,7 @@ import domReady from '@wordpress/dom-ready';
 import { isNonAmpWindow, isAmpWindow } from './utils';
 
 const { parent, ampPairedBrowsingClientData } = window;
-const { ampUrl, nonAmpUrl } = ampPairedBrowsingClientData;
-
-/**
- * Validates whether or not the window document is AMP compatible.
- *
- * @return {boolean} True if AMP compatible, false if not.
- */
-function documentIsAmp() {
-	return Boolean( document.querySelector( 'head > script[src="https://cdn.ampproject.org/v0.js"]' ) );
-}
-
-// /**
-//  * Get amphtml link URL.
-//  *
-//  * @return {string|null} URL or null if link not present.
-//  */
-// function getAmphtmlLinkHref() {
-// 	const link = document.querySelector( 'head > link[rel=amphtml]' );
-// 	return link ? link.href : null;
-// }
-//
-// /**
-//  * Get canonical link URL.
-//  *
-//  * @return {string|null} URL or null if link not present.
-//  */
-// function getCanonicalLinkHref() {
-// 	const link = document.querySelector( 'head > link[rel=canonical]' );
-// 	return link ? link.href : null;
-// }
+const { ampUrl, nonAmpUrl, isAmpDocument } = ampPairedBrowsingClientData;
 
 /**
  * Modify document for paired browsing.
@@ -47,7 +18,7 @@ function modifyDocumentForPairedBrowsing() {
 	// Scrolling is not synchronized if `scroll-behavior` is set to `smooth`.
 	document.documentElement.style.setProperty( 'scroll-behavior', 'auto', 'important' );
 
-	if ( documentIsAmp() ) {
+	if ( isAmpDocument ) {
 		// Hide the paired browsing menu item.
 		const pairedBrowsingMenuItem = document.getElementById( 'wp-admin-bar-amp-paired-browsing' );
 		if ( pairedBrowsingMenuItem ) {
@@ -148,8 +119,7 @@ function sendHeartbeat() {
 		parent,
 		'heartbeat',
 		{
-			isAmpDocument: documentIsAmp(),
-			//locationHref: window.location.href,
+			isAmpDocument,
 			ampUrl,
 			nonAmpUrl,
 			documentTitle: document.title,

diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php
@@ -2370,8 +2370,9 @@ public static function setup_paired_browsing_client() {
 			'amp-paired-browsing-client',
 			'ampPairedBrowsingClientData',
 			[
-				'ampUrl'    => $amp_url,
-				'nonAmpUrl' => $non_amp_url,
+				'isAmpDocument' => $is_amp_request,
+				'ampUrl'        => $amp_url,
+				'nonAmpUrl'     => $non_amp_url,
 			]
 		);
 

diff --git a/includes/templates/amp-paired-browsing.php b/includes/templates/amp-paired-browsing.php
@@ -43,10 +43,6 @@
 						<span class="general">
 							<?php esc_html_e( 'The navigated URL is not available for paired browsing.', 'amp' ); ?>
 						</span>
-
-						<span class="invalid-amp">
-							<?php esc_html_e( 'The navigated page does not have an AMP counterpart due to invalid AMP markup being kept.', 'amp' ); ?>
-						</span>
 					</div>
 
 					<div class="dialog-buttons">