Skip to content

Disable experimental Bento integration in Sandboxing #7267

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

Closed
westonruter opened this issue Sep 22, 2022 · 1 comment · Fixed by #7269
Closed

Disable experimental Bento integration in Sandboxing #7267

westonruter opened this issue Sep 22, 2022 · 1 comment · Fixed by #7269
Labels
Bento Changelogged Whether the issue/PR has been added to release notes. Enhancement New feature or improvement of an existing one Sandboxing Experiment
Milestone

Comments

@westonruter
Copy link
Member

westonruter commented Sep 22, 2022

Feature Description

As discovered in a support topic, the Bento implementation of some components (at least amp-accordion) is broken, at least when used on a page with the AMP runtime. Also, since the Bento AMP experiment is still present, pages using the Bento versions of components cannot be valid AMP. Therefore, we should go ahead and just disable the Bento integration for the Sandboxing levels experiment.

This can be done as simply as the following (with test updates not withstanding):

--- a/includes/sanitizers/class-amp-script-sanitizer.php
+++ b/includes/sanitizers/class-amp-script-sanitizer.php
@@ -144,7 +144,6 @@ public function sanitize() {
 		// When custom scripts are on the page, use Bento AMP components whenever possible and turn off some CSS
 		// processing is unnecessary for a valid AMP page and which can break custom scripts.
 		if ( $this->px_verified_kept_node_count > 0 || $this->kept_script_count > 0 ) {
-			$sanitizer_arg_updates[ AMP_Tag_And_Attribute_Sanitizer::class ]['prefer_bento']       = true;
 			$sanitizer_arg_updates[ AMP_Style_Sanitizer::class ]['transform_important_qualifiers'] = false;
 			$sanitizer_arg_updates[ AMP_Style_Sanitizer::class ]['allow_excessive_css']            = true;
 			$sanitizer_arg_updates[ AMP_Form_Sanitizer::class ]['native_post_forms_allowed']       = 'always';

In this way, the Bento integration will only be enabled if someone explicitly filters amp_bento_enabled to be true.


I was originally thinking of removing all the Bento code and I was adding the following steps to do, but I decided it was overkill for a minor release. But I'll leave it here for posterity:

  • Mark amp_is_bento_enabled() as deprecated and use apply_filters_deprecated for the amp_bento_enabled filter.
  • Remove Bento from being included in what bin/amphtml-update.py parses from the validator spec.
  • Remove the Bento check in amp_register_default_scripts() and amp_register_default_styles().
  • Eliminate the $is_using_bento check from AMP_Theme_Support::ensure_required_markup().
  • Remove Bento from amp_get_content_sanitizers().
  • Unregister the AMP_Bento_Sanitizer

Acceptance Criteria

Bento component scripts should not be served when using Loose/Moderate sandboxing level.

Implementation Brief

No response

QA Testing Instructions

No response

Demo

No response

Changelog Entry

No response

@pavanpatil1
Copy link

QA Passed ✅

Cross-checked the issue with the below Custom HTML snippet and fix is working as expected.

<button on="tap:mySelector.clear">Clear Selection</button>
<amp-selector id="mySelector" layout="container" multiple>
  <div option>Option One</div>
  <div option>Option Two</div>
  <div option>Option Three</div>
</amp-selector>

<!-- To keep sandboxing in loose mode  -->
<script>document.write('document write!')</script>

Before:
image

After fix:

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
Bento Changelogged Whether the issue/PR has been added to release notes. Enhancement New feature or improvement of an existing one Sandboxing Experiment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants