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

Introduce support for a more flexible AMP via Bento with levels of sandboxing enforcement #6443

Closed
7 tasks
westonruter opened this issue Jul 3, 2021 · 17 comments · Fixed by #6546 or #6769
Closed
7 tasks
Assignees
Labels
Bento Changelogged Whether the issue/PR has been added to release notes. Enhancement New feature or improvement of an existing one P0 High priority Sandboxing Experiment
Milestone

Comments

@westonruter
Copy link
Member

westonruter commented Jul 3, 2021

Feature description

The AMP plugin currently offers a binary choice: either you are 100% AMP or you disable AMP. The Bento AMP initiative seeks to make AMP more flexible by allowing you to use AMP components on non-AMP pages. In reality, the AMP plugin has supported this to a limited degree in Standard mode. When invalid markup is kept, the AMP plugin serves an AMP page without the amp attribute on the html element (cf. #5549). This in effect allows you to have a page with all the benefits of the AMP runtime, but also with additional non-AMP scripts you may need. The caveat here is that this is not officially supported and since AMP components lack encapsulation, there is no guarantee that it will work. Your mileage may vary. Bento solves this problem by encapsulating the components with shadow DOM so that other scripts on the page can't interfere with their internal behavior.

An additional purpose of the Bento project is to make components much easier to author via Preact. In addition to the AMP core components being available as Bento, site owners and plugin authors may create their own Bento components which may not be “certified” for AMP and yet still have an assurance of performance by being created with the Bento framework.

With Bento components in hand, we can relax AMP validation to make pages more flexible. In particular, we can introduce sandbox levels to indicate how strict a site owner wants to be in terms of the markup allowed on the page:

  • L1: The initial sandboxing level involves the plugin converting markup to Bento components where it is able (e.g. social embeds, videos, images), and then anything else it finds (custom scripts for a carousel) will be left in place. In other words, this level involves no sanitization (removal) of markup from the page. As such, this level should not break existing pages.
  • L2: The second level introduces sanitization by stripping out markup that is not part of a Bento component, either a Bento AMP component or a custom 3rd party component.
  • L3: The third level is the most strict and it corresponds to what is currently valid AMP. Only valid AMP components are allowed.

For the initial exploration, the sandbox levels would only be available in Standard mode. Initially it would default to L3, maintaining strict AMP validity. Once Bento matures, the plugin can switch from Reader mode being the default to instead having Standard mode with L1 sandboxing as the default.

Additionally, in the initial sandboxing implementation, the level would largely be static. If you select L1, then that is the level that would be reflected on the rendered document. However, in a subsequent iteration the sandboxing levels should dynamically adjust based on the content of the page. So if you selected L1 and yet there is no invalid AMP markup on the page, then it would automatically be upgraded to L3.

Lastly, in future releases we may allow the sandboxing levels to be used in non-Standard modes as well, in particular Reader mode. If the user selected L1 or L2 sandboxing for Reader mode, then the paired versions would not be linked to as AMP with link[rel=amphtml], since there would be no guarantee that the pages would be valid. Instead, only a link[rel=alternate][media] would appear as is today when mobile redirection is enabled, as is documented in the Separate URLs documentation on Google Search Console. In other words, Reader mode with these lesser sandboxing levels would provide a “mobile theme” for the site, which would be particularly useful for sites which have a primary theme which is fundamentally not responsive with a poor user experience.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

  • Remove amp attribute from html and add data-ampdevmode to Bento experiment opt-in script?
  • The image sanitizer no longer converts img to amp-img but instead just ensures dimensions are provided.
  • Bento versions of scripts are used instead of latest.
  • The amp-runtime is omitted as a dependency of any Bento scripts. (Eventually, but currently it is required.)
  • Bento stylesheets are enqueued for any Bento components used on the page. (Should these be excluded from CSS concat?)
  • Add sanitizer that upgrades AMP components to Bento, specifically where attributes change or where there are backward-incompatibilities. For example, amp-carousel should be converted to amp-base-carousel.
  • When there is a form POST on the page which didn't originally have action-xhr, do not include the amp-form component.

QA testing instructions

Demo

Changelog entry

@westonruter westonruter added Enhancement New feature or improvement of an existing one Bento labels Jul 3, 2021
@westonruter westonruter added this to the v2.2 milestone Jul 3, 2021
@westonruter westonruter self-assigned this Jul 3, 2021
@westonruter westonruter added the P0 High priority label Jul 3, 2021
@westonruter westonruter mentioned this issue Jul 3, 2021
11 tasks
@jwold
Copy link
Collaborator

jwold commented Jul 23, 2021

Supporting graphic:

Screen Shot 2021-07-23 at 9 44 22 AM

@jwold jwold added WS:UX Work stream for UX/Front-end and removed WS:UX Work stream for UX/Front-end labels Jul 29, 2021
@jwold
Copy link
Collaborator

jwold commented Jul 29, 2021

We discussed this in our plugin sync today, and per the initial release this will be an experimental feature. We won't have a user interface to reveal the feature, but will instead make it available as a PHP filter for initial testing.

@delawski
Copy link
Collaborator

QA Passed

After enabling the experimental feature (via the example plugin), I've followed the QA process described in #6546 (Testing section).

The sandboxing level selected on the AMP settings page was set to Loose. All three sample posts passed the specified tests:

Loose mode
sandboxing-level=1:1
Moderate mode
sandboxing-level=1:2
Strict mode
sandboxing-level=1:3
Screenshot 2021-11-22 at 19 36 23 Screenshot 2021-11-22 at 19 34 26 Screenshot 2021-11-22 at 19 39 53

Tested on AMP 2.2.0-alpha-20211117T230738Z-094aef8

I'm leaving this issue in "Ready for QA" so that @dhaval-parekh has a chance to do a QA separately.

@dhaval-parekh
Copy link
Collaborator

dhaval-parekh commented Nov 25, 2021

✅ Automatically upgrading to sandboxing level if page supports it.

✅ I have tested with the QA process described in PR #6546 (Testing section) and that looks good to me.

⚠️ While doing testing in loose sandbox mode for video. I found that adding dimensions may cause some issues specifically video with higher dimensions than container width. Check the below images.

None AMP Mode Loose mode
AMP-GH-6546-nonAMP-video AMP-GH-6546-Loose-Sandbox-video

However, It works perfectly in "Moderate" and "Strict" mode because of the AMP video component.
I believe we are setting height/width to reduce CLS. In that case, I think we can add a video container element ( The way amp-video component do ) and set height in percentage according to the aspect ratio of the video.
cc: @westonruter @delawski

Other than this all looks good to me.

@westonruter
Copy link
Member Author

@dhaval-parekh Yes, I'm seeing that as well. Thanks for raising that. This seems like it may be a Gutenberg bug. Specifically, I'm seeing that Gutenberg has this style rule:

.wp-block-video video {
    width: 100%;
}

The width and height attributes being present, along with a width:100% but a lack of height:auto is causing the aspect ratio to not be respected. This seems to have been something that was missed in the recent PR WordPress/gutenberg#30092.

@westonruter
Copy link
Member Author

I've opened WordPress/gutenberg#37052 to address this.

@milindmore22
Copy link
Collaborator

@westonruter

While testing sandboxing level with Strict Mode (develop branch), I noticed comment form is missing on attribute.

image

@westonruter
Copy link
Member Author

@milindmore22 I'm not seeing that in develop:

image

@westonruter
Copy link
Member Author

Were you testing with one of these test cases?

image

@milindmore22
Copy link
Collaborator

yes, I was testing those test cases

@westonruter
Copy link
Member Author

The fact that your screenshot is showing a red outline means that something is not working as expected. The test cases should only have green outlines if they are passing.

@milindmore22
Copy link
Collaborator

milindmore22 commented Dec 7, 2021

JFI : I installed production build v2.2.0-alpha-20211207T172207Z-0ab628a on the test site to confirm
https://plugins.dev6.rt.gw/sandboxing-level-strict/

@westonruter
Copy link
Member Author

Can you add me to that site?

@westonruter
Copy link
Member Author

Nevermind, I'm able to reproduce it now. It doesn't happen when I have the Sandboxing level set to Loose and I'm upgraded to Strict since there is nothing AMP-invalid on the page. However, if I've set the level to Strict then it fails to add the on attribute. Investigating...

@westonruter
Copy link
Member Author

Simple fix: #6769

@westonruter
Copy link
Member Author

(And good catch!)

@milindmore22
Copy link
Collaborator

QA Passed

Sandboxing Levels are all well tested and Look good as per the QA process #6546

  • Remove amp attribute from html and add data-ampdevmode to Bento experiment opt-in script?

  • The image sanitizer no longer converts img to amp-img but instead just ensures dimensions are provided.

  • Bento versions of scripts are used instead of latest.

  • The amp-runtime is omitted as a dependency of any Bento scripts. (Eventually, but currently it is required.)

  • Bento stylesheets are enqueued for any Bento components used on the page. (Should these be excluded from CSS concat?)

  • Add sanitizer that upgrades AMP components to Bento, specifically where attributes change or where there are backward-incompatibilities. For example, amp-carousel should be converted to amp-base-carousel.

  • When there is a form POST on the page which didn't originally have action-xhr, do not include the amp-form component.

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bento Changelogged Whether the issue/PR has been added to release notes. Enhancement New feature or improvement of an existing one P0 High priority Sandboxing Experiment
Projects
None yet
5 participants