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

Extend Sandboxing experiment to Paired AMP modes #7268

Closed
4 tasks done
westonruter opened this issue Sep 22, 2022 · 3 comments · Fixed by #7288
Closed
4 tasks done

Extend Sandboxing experiment to Paired AMP modes #7268

westonruter opened this issue Sep 22, 2022 · 3 comments · Fixed by #7288
Labels
Changelogged Whether the issue/PR has been added to release notes. Enhancement New feature or improvement of an existing one
Milestone

Comments

@westonruter
Copy link
Member

westonruter commented Sep 22, 2022

Feature Description

Currently the Sandboxing experiment is limited to Standard mode. This is because pages using the experiment are not valid AMP, meaning they will not get indexed. Nevertheless, invalid paired AMP pages are still useful for one important case: a mobile-optimized version of the site. When mobile redirection is enabled in particular, being able to serve pages using the AMP runtime and our optimizations (e.g. Reader theme) will enable the plugin to be usable as a "mobile theme" plugin.

  • Show the Sandboxing experiment panel when any template mode is selected on the Settings screen, not just when the Standard mode is selected.
  • When a non-strict sandboxing level is selected, never output the link[rel=amphtml] element but always output link[rel=alternate] which is output when mobile redirection is enabled. The reason here is that we do not want to advertise to Googlebot in this case that there is an AMP page because then it will complain if there are validation errors.
  • Prevent redirecting from AMP to non-AMP when in a paired mode if the Sandboxing level is not set to Strict (and there is kept invalid markup).
  • Show link to exit mobile version on AMP pages even when mobile redirection is not enabled  #5293

Acceptance Criteria

No response

Implementation Brief

No response

QA Testing Instructions

No response

Demo

No response

Changelog Entry

No response

@westonruter westonruter added the Enhancement New feature or improvement of an existing one label Sep 22, 2022
@westonruter westonruter added this to the v2.4 milestone Sep 22, 2022
@thelovekesh thelovekesh self-assigned this Sep 29, 2022
@westonruter
Copy link
Member Author

  • Prevent redirecting from AMP to non-AMP when in a paired mode if the Sandboxing level is not set to Strict.

This specifically has in mind this code:

/*
* In AMP-first, documents with invalid AMP markup can still be served. The amp attribute will be omitted in
* order to prevent GSC from complaining about a validation error already surfaced inside of WordPress.
* This is intended to not serve dirty AMP, but rather a non-AMP document (intentionally not valid AMP) that
* contains the AMP runtime and AMP components.
*
* Otherwise, if in Paired AMP then redirect to the non-AMP version if the current user isn't an user who
* can manage validation error statuses (access developer tools) and change the AMP options for the template
* mode. Such users should be able to see kept invalid markup on the AMP page even though it is invalid.
*/
if ( amp_is_canonical() ) {
return true;
}
// Otherwise, since it is in a paired mode, only allow showing the dirty AMP page if the user is authorized.
// If not, normally the result is redirection to the non-AMP version.
return self::has_cap() || is_customize_preview();
}

Note the check for amp_is_canonical(). Essentially this needs to be changed to also return true if sandboxing is enabled and it is set to level 1 or 2. This will prevent redirecting to the non-AMP version when there is invalid markup that is kept making the page invalid AMP.

@westonruter
Copy link
Member Author

Prevent redirecting from AMP to non-AMP when in a paired mode if the Sandboxing level is not set to Strict (and there is kept invalid markup).

This can be seen by creating a post with the following content:

<!-- wp:paragraph -->
<p>This is a script using <code>document.write()</code>:</p>
<!-- /wp:paragraph -->

<!-- wp:html -->
<script>document.write('HELLO WORLD');</script><noscript>SCRIPT WAS BLOCKED</noscript>
<!-- /wp:html -->

If I then set template mode to Reader mode and enable loose sandboxing, I see:

image

Notice how even though there is invalid AMP markup, it is not redirecting to the non-AMP page. (Actually, this should rather be tested when the user is logged-out, and indeed I see that is the case.) Instead the amp attribute is removed from the html element.

@pavanpatil1
Copy link

QA Passed ✅

Show the Sandboxing experiment panel when any template mode is selected on the Settings screen, not just when the Standard mode is selected.

Before
The sandboxing experiment panel was visible only in standard mode.

image

After fix:
Now the sandboxing experiment panel is visible in all three modes.

image


When a non-strict sandboxing level is selected, never output the link[rel=amphtml] element but always output link[rel=alternate] which is output when mobile redirection is enabled. The reason here is that we do not want to advertise to Googlebot in this case that there is an AMP page because then it will complain if there are validation errors

Now, In transitional mode with the sandboxing strict mode, the link[rel=amphtml] is added and with the loose/moderate mode the link[rel=alternate] is displayed and link[rel=amphtml] is not visible.

sandboxing strict mode:
image

sandboxing Loose/moderate mode:
image


Prevent redirecting from AMP to non-AMP when in a paired mode if the Sandboxing level is not set to Strict (and there is kept invalid markup).

After setting the reader sandboxing to lose/moderate mode, the page is not rendering to non-AMP mode. working fine.

image

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. Enhancement New feature or improvement of an existing one
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants