Introduce Bento sanitizer to handle with `bento`-prefixed components #6722

westonruter · 2021-11-16T06:04:42Z

Summary

Bento components will be available not only as amp-prefixed components, but also with the bento prefix. Consider this following example which can be pasted into a Custom HTML block on a site without the AMP plugin running at all:

<script
	type="module"
	async
	src="https://cdn.ampproject.org/bento.mjs"
></script>
<script nomodule src="https://cdn.ampproject.org/bento.js"></script>
<script
	type="module"
	async
	src="https://cdn.ampproject.org/v0/bento-accordion-1.0.mjs"
></script>
<script
	nomodule
	async
	src="https://cdn.ampproject.org/v0/bento-accordion-1.0.js"
></script>
<link
	rel="stylesheet"
	type="text/css"
	href="https://cdn.ampproject.org/v0/bento-accordion-1.0.css"
/>

<p>The following should have a green outline.</p>

<bento-accordion id="my-accordion">
	<section>
		<h2>Section 1</h2>
		<div>Content in section 1.</div>
	</section>
	<section>
		<h2>Section 2</h2>
		<div>Content in section 2.</div>
	</section>
	<!-- Expanded on page load due to attribute: -->
	<section expanded>
		<h2>Section 3</h2>
		<div>Content in section 3.</div>
	</section>
</bento-accordion>

<!-- Note the use of bento-accordion tag in the selector. When converted to amp-accordion, the selector must also be converted. -->
<style>
	#my-accordion {
		display: block;
		outline: solid 2px red;
	}
	bento-accordion#my-accordion {
		outline: solid 2px green;
	}
</style>

On a non-AMP page, this is output as expected:

However, when viewing the AMP page it completely breaks:

Validating the page shows that there are many errors detected, resulting in the bento-accordion and Bento stylesheet and scripts being removed:

At the moment, all of the Bento components are just aliases for corresponding v1.0 AMP components. Therefore, in order to seamlessly incorporate Bento components into valid AMP pages, what is needed is to rename the bento prefix to amp when there is a corresponding component available. So there is an bento-accordion component and a corresponding amp-accordion v1.0 which is also the Bento version, as opposed to v0.1 which is the classic version. So we can just rename bento-accordion to amp-accordion which will ensure that it is recognized as valid AMP markup and thus won't be sanitized from the page. Any CSS selectors referencing bento-accordion are also updated to use amp-accordion. When such a conversion is done, any Bento-specific stylesheets and scripts can also be removed from the document since the auto-extension importing AMP will then automatically add the required markup.

So the above HTML is converted into the following on a valid AMP page:

<style amp-custom>#my-accordion{display:block;outline:solid 2px red}amp-accordion#my-accordion{outline:solid 2px green}</style>
<script src="https://cdn.ampproject.org/v0/amp-accordion-1.0.mjs" async="" custom-element="amp-accordion" type="module" crossorigin="anonymous"></script>
<script async nomodule src="https://cdn.ampproject.org/v0/amp-accordion-1.0.js" crossorigin="anonymous" custom-element="amp-accordion"></script>

...

<p>The following should have a green outline.</p>

<amp-accordion id="my-accordion" class="i-amphtml-layout-container" i-amphtml-layout="container">
	<section>
		<h2>Section 1</h2>
		<div>Content in section 1.</div>
	</section>
	<section>
		<h2>Section 2</h2>
		<div>Content in section 2.</div>
	</section>
	<!-- Expanded on page load due to attribute: -->
	<section expanded>
		<h2>Section 3</h2>
		<div>Content in section 3.</div>
	</section>
</amp-accordion>

<!-- Note the use of bento-accordion tag in the selector. When converted to amp-accordion, the selector must also be converted. -->

This PR also accounts for future Bento components which do not have AMP aliases. When such a component is encountered, the component along with its dependent scripts and stylesheet will not marked with data-px-verified so that the plugin's sanitizer will not later strip it out. Such pages with non-AMP Bento components will not be be marked as amp pages as they are not valid AMP, although they will still incorporate the AMP runtime and will go through the AMP Optimizer. This is the “Moderate” sandboxing level introduced in #6443.

Fix handling of ID attributes during sanitizer conversion

In 263501f I worked around Unexpected Handling of Element IDs in PHP DOM. In short, it turns out that PHP DOM does not handle element IDs the same way as browser DOM does. Therefore, in order to element conversions successfully, it's important to remove the id attribute from the old element before adding it to the new element.

Fix JS error on Validated URL screen when there are no Validation Errors

I discovered on the Validated URL screen that the rows wouldn't expand in the Stylesheets metabox. Looking at the console I saw an error coming from:

amp-wp/assets/src/amp-validation/amp-validated-url-post-edit-screen.js

Line 278 in 5405daa

    
           document.getElementById( 'search-submit' ).addEventListener( 'click', onClick );

This is for this search box:

The issue is that when there are no Validation Errors, then no search box is added to the page:

And this causes an error. Fixed in e13cc99.

Checklist

My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

westonruter · 2021-11-16T06:10:20Z

The CSS tree-shaker is actually removing #my-accordion{display:block;outline:solid 2px red} when it shouldn't be.

westonruter · 2021-11-16T17:26:27Z

Ensure compatibility with Gutenberg Bento. See Update AMP compatibility logic swissspidy/gutenberg-bento#33.

…esent

* Fix exempting non-AMP Bento stylesheet * Ensure scripts are removed if no Bento components present * Prefer bento when any Bento component is discovered (not just when conversion happened)

github-actions · 2021-11-23T18:01:09Z

Plugin builds for 58139b2 are ready 🛎️!

Download development build
Download production build

codecov · 2021-11-23T18:03:12Z

Codecov Report

Merging #6722 (99b419a) into develop (5405daa) will increase coverage by 0.03%.
The diff coverage is 93.54%.

❗ Current head 99b419a differs from pull request most recent head ad497cb. Consider uploading reports for the commit ad497cb to get more accurate results

@@              Coverage Diff              @@
##             develop    #6722      +/-   ##
=============================================
+ Coverage      77.00%   77.03%   +0.03%     
- Complexity      6548     6577      +29     
=============================================
  Files            264      265       +1     
  Lines          20901    20988      +87     
=============================================
+ Hits           16095    16169      +74     
- Misses          4806     4819      +13

Flag	Coverage Δ
javascript	`63.20% <ø> (ø)`
php	`77.83% <93.54%> (+0.03%)`	⬆️
unit	`77.83% <93.54%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
includes/amp-helper-functions.php	`84.66% <ø> (ø)`
...ludes/sanitizers/class-amp-o2-player-sanitizer.php	`94.11% <50.00%> (-5.89%)`	⬇️
...cludes/sanitizers/class-amp-playbuzz-sanitizer.php	`93.02% <50.00%> (-4.54%)`	⬇️
includes/sanitizers/class-amp-style-sanitizer.php	`87.31% <66.66%> (-0.52%)`	⬇️
includes/sanitizers/class-amp-bento-sanitizer.php	`98.59% <98.59%> (ø)`
includes/sanitizers/class-amp-audio-sanitizer.php	`94.44% <100.00%> (+0.06%)`	⬆️
includes/sanitizers/class-amp-base-sanitizer.php	`79.29% <100.00%> (+0.16%)`	⬆️
includes/sanitizers/class-amp-iframe-sanitizer.php	`96.10% <100.00%> (+0.02%)`	⬆️
includes/sanitizers/class-amp-img-sanitizer.php	`96.82% <100.00%> (+0.01%)`	⬆️
includes/sanitizers/class-amp-object-sanitizer.php	`100.00% <100.00%> (ø)`
... and 3 more

…les to AMP layout

delawski

I went through the code and left a few minor comments.

I didn't get a chance to test it out locally, but will do so on Monday.

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

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

Co-authored-by: Piotr Delawski <[email protected]>

delawski · 2021-11-29T14:55:30Z

I've tested the code locally. The provided sample code displaying an accordion works as expected.

I also tried adding some bento-mathml formulas. I used a code from the documentation (inside a Custom HTML block):

<script type="module" src="https://cdn.ampproject.org/bento.mjs" crossorigin="anonymous"></script>
<script nomodule src="https://cdn.ampproject.org/bento.js" crossorigin="anonymous"></script>
<script type="module" src="https://cdn.ampproject.org/v0/bento-mathml-1.0.mjs" crossorigin="anonymous"></script>
<script nomodule src="https://cdn.ampproject.org/v0/bento-mathml-1.0.js" crossorigin="anonymous"></script>
<link rel="stylesheet" href="https://cdn.ampproject.org/v0/bento-mathml-1.0.css" crossorigin="anonymous">

<h2>The Quadratic Formula</h2>
<bento-mathml
  style="height: 40px"
  data-formula="\[x = {-b \pm \sqrt{b^2-4ac} \over 2a}.\]"
></bento-mathml>

<h2>Inline formula</h2>
<p>
  This is an example of a formula,
  <bento-mathml
    style="height: 40px; width: 147px"
    inline
    data-formula="\[x = {-b \pm \sqrt{b^2-4ac} \over 2a}.\]"
  ></bento-mathml>
  placed inline in the middle of a block of text.
</p>

The AMP version doesn't render the formulas.

Frontend	AMP Validated URL

When I removed the inline style attributes, and added CSS in a separate <style> element, I got no AMP error and the formulas were rendered as expected.

<style>
  bento-mathml {
    width: 150px;
    height: 40px;
  }
</style>

Is that expected behaviour?

westonruter · 2021-11-29T17:00:38Z

Is that expected behaviour?

No, that's not expected. The problem appears to be that the sanitizer is adding layouts that the component doesn't support:

I need to make sure that is factored into the sanitizer logic.

westonruter · 2021-11-29T17:05:39Z

When I removed the inline style attributes, and added CSS in a separate <style> element, I got no AMP error and the formulas were rendered as expected.

This is because the default layout then becomes container, but since the element has no initial content prior to rendering the formula, then it gets no height. But seems the amp-mathml component only supports the container layout so it should never have width/height attributes added to it.

Working on a fix.

…ht styles

…t for fill and nodisplay

westonruter · 2021-11-29T22:10:21Z

OK, this is now much more robust.

…-js-support * 'develop' of github.com:ampproject/amp-wp: Use CF7 instead of Autoptimize for AMP-incompatible plugin in E2E tests Replace autoptimize with akismet for E2E tests Pin NodeJS to v14 to match WP core and Gutenberg Regenerate package-lock.json at lockfileVersion 1 Update Gutenberg package dependencies Update Gutenberg package dependencies

delawski

With the latest changes, bento-mathml displays correctly when there is no inline style attribute.

I've also tested bento-instagram both with explicit dimensions provided in an inline style attribute and with position: absolute; width: 100%; height: 100% (in order to trigger a layout="fill"). In both cases the Instagram embed rendered as expected.

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

Co-authored-by: Piotr Delawski <[email protected]>

milindmore22 · 2021-12-01T15:08:37Z

All looks good, I tested few random components date-countdown, base carousel, twitter, WordPress Embed All looks good except base carousel
When I tried example for bento Carousel from the docs which has no layout or inline style defined it didn't show until I add layout.

here is what I tried

<script type="module" src="https://cdn.ampproject.org/bento.mjs" crossorigin="anonymous"></script>
<script nomodule="" src="https://cdn.ampproject.org/bento.js" crossorigin="anonymous"></script>

<script type="module" src="https://cdn.ampproject.org/v0/bento-base-carousel-1.0.mjs" crossorigin="anonymous"></script>
<script nomodule="" src="https://cdn.ampproject.org/v0/bento-base-carousel-1.0.js" crossorigin="anonymous"></script>

<link rel="stylesheet" href="https://cdn.ampproject.org/v0/bento-base-carousel-1.0.css" crossorigin="anonymous">
  <bento-base-carousel id="my-carousel">
    <div class="red"></div>
    <div class="blue"></div>
    <div class="green"></div>
  </bento-base-carousel>

CSS for it you can add it Customizer

bento-base-carousel,
    bento-base-carousel > div {
      aspect-ratio: 4/1;
}
.red {
      background: darkred;
}
.blue {
      background: steelblue;
}
.green {
      background: seagreen;
}

This won't show unless you add layout for it, it does support all layouts

westonruter · 2021-12-01T17:47:11Z

has no layout or inline style defined it didn't show until I add layout

@milindmore22 Yes, this is somewhat of a known limitation. In order for that bento-base-carousel example to automatically convert to amp-base-carousel, the aspect-ratio has to be added to as a style attribute on the element itself. Otherwise the sanitizer can't determine the layout to obtain. This is shown in the tests:

amp-wp/tests/php/test-amp-bento-sanitizer.php

Line 223 in 1d435a2

    
           							<bento-soundcloud id="sc1" data-trackid="89299804" data-visual="true" style="aspect-ratio: 16 / 9"></bento-soundcloud>

is converted to:

amp-wp/tests/php/test-amp-bento-sanitizer.php

Line 255 in 1d435a2

    
           							<amp-soundcloud id="sc1" data-trackid="89299804" data-visual="true" height="9" width="16" layout="responsive"></amp-soundcloud>

…-js-support * 'develop' of github.com:ampproject/amp-wp: (45 commits) Update Gutenberg package dependencies Update CODEOWNERS Update amphtml spec to 2111242025000 Stop mapping E2E test plugins dir to container volume Add demo plugin ZIP archive Fix script command Fix lint issues Ensure React state updates are not performed on unmounted components Use different Jest configs for E2E tests; use `.env` config file Refactor "Other Settings" E2E tests Ensure selector is available before scrolling element into view Use more robust internal activate/deactivate functions in E2E tests Introduce general `saveSettings` util function for E2E tests Remove unnecessary cleanup functions in E2E tests Install Hestia theme globally as it is used in multiple tests Use local demo plugin in E2E tests Ensure disabled back button is covered by E2E tests Add `SiteScanContextProvider` mock to prevent unit test errors Ensure Site Scan progress bar never goes backwards Prevent going back in Wizard if Site Scan is in progress ...

milindmore22 · 2021-12-10T13:23:58Z

QA Passed

Everything looks good 👍🏼

✅ Bento components are renamed to the AMP components.
✅ AMP 1.0 scripts are included for the bento components.
✅ CSS selector with bento alias are converted to amp alias.

westonruter changed the title ~~Introduce Bento sanitizer to deal with BentoJS components~~ Introduce Bento sanitizer to handle with bento-prefixed components Nov 16, 2021

westonruter added this to the v2.2 milestone Nov 16, 2021

westonruter added the P0 High priority label Nov 16, 2021

swissspidy mentioned this pull request Nov 16, 2021

Update AMP compatibility logic swissspidy/gutenberg-bento#33

Open

westonruter force-pushed the add/bento-js-support branch from 7167dc2 to 7f44f94 Compare November 18, 2021 20:03

Introduce Bento sanitizer to deal with BentoJS components

eaefc1e

westonruter force-pushed the add/bento-js-support branch from 7f44f94 to 810b25f Compare November 23, 2021 05:09

westonruter added 2 commits November 22, 2021 22:20

Fix handling of ID attributes during sanitizer conversion

263501f

Prevent JS error on validated URL screen when no validation errors pr…

e13cc99

…esent

westonruter force-pushed the add/bento-js-support branch from 810b25f to e13cc99 Compare November 23, 2021 06:37

westonruter added 2 commits November 23, 2021 09:09

Prevent bento components from being treated as light shadow DOM wrappers

bd114a0

Improve Bento sanitizer and add tests

99b419a

* Fix exempting non-AMP Bento stylesheet * Ensure scripts are removed if no Bento components present * Prefer bento when any Bento component is discovered (not just when conversion happened)

westonruter marked this pull request as ready for review November 23, 2021 17:56

westonruter requested review from milindmore22, dhaval-parekh and delawski November 23, 2021 17:56

westonruter added 3 commits November 23, 2021 11:19

Improve test coverage for ID attribute changes

ad497cb

Further improve test converage and remove dead code

4d3e363

Add support for converting Bento component dimensions from inline sty…

dbdb47c

…les to AMP layout

delawski reviewed Nov 26, 2021

View reviewed changes

westonruter and others added 3 commits November 28, 2021 17:51

Fix typos in comments

846acc0

Co-authored-by: Piotr Delawski <[email protected]>

Fix variable name

268ebfb

Harmonize Bento component name parsing

25c4e31

westonruter added 3 commits November 29, 2021 10:50

Update AMP_Base_Sanitier::set_layout() to account for 100% width/heig…

35dd74f

…ht styles

Validate supported layouts for converted Bento components; add suppor…

c60019c

…t for fill and nodisplay

Derive intrinsic layout when max-width defined

671c63a

westonruter requested a review from delawski November 29, 2021 22:10

delawski reviewed Nov 30, 2021

View reviewed changes

includes/sanitizers/class-amp-bento-sanitizer.php Outdated Show resolved Hide resolved

includes/sanitizers/class-amp-bento-sanitizer.php Outdated Show resolved Hide resolved

Use strict check for in_array() and improve comment

1d435a2

Co-authored-by: Piotr Delawski <[email protected]>

delawski approved these changes Dec 1, 2021

View reviewed changes

westonruter added 3 commits December 1, 2021 09:51

Fix phpcs alignment warning

8b82ffb

Add test case for Bento lacking layout attributes; improve test coverage

58139b2

westonruter merged commit 5f147a6 into develop Dec 1, 2021

westonruter deleted the add/bento-js-support branch December 1, 2021 20:31

maitreyie-chavan assigned milindmore22 Dec 10, 2021

westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 15, 2021

westonruter added the Sandboxing Experiment label Feb 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Bento sanitizer to handle with `bento`-prefixed components #6722

Introduce Bento sanitizer to handle with `bento`-prefixed components #6722

westonruter commented Nov 16, 2021 •

edited

Loading

westonruter commented Nov 16, 2021 •

edited

Loading

westonruter commented Nov 16, 2021 •

edited

Loading

github-actions bot commented Nov 23, 2021 •

edited

Loading

codecov bot commented Nov 23, 2021 •

edited

Loading

delawski left a comment

delawski commented Nov 29, 2021

westonruter commented Nov 29, 2021

westonruter commented Nov 29, 2021

westonruter commented Nov 29, 2021 •

edited

Loading

delawski left a comment

milindmore22 commented Dec 1, 2021

westonruter commented Dec 1, 2021

milindmore22 commented Dec 10, 2021 •

edited

Loading

Introduce Bento sanitizer to handle with bento-prefixed components #6722

Introduce Bento sanitizer to handle with bento-prefixed components #6722

Conversation

westonruter commented Nov 16, 2021 • edited Loading

Summary

Fix handling of ID attributes during sanitizer conversion

Fix JS error on Validated URL screen when there are no Validation Errors

Checklist

westonruter commented Nov 16, 2021 • edited Loading

westonruter commented Nov 16, 2021 • edited Loading

github-actions bot commented Nov 23, 2021 • edited Loading

codecov bot commented Nov 23, 2021 • edited Loading

Codecov Report

delawski left a comment

Choose a reason for hiding this comment

delawski commented Nov 29, 2021

westonruter commented Nov 29, 2021

westonruter commented Nov 29, 2021

westonruter commented Nov 29, 2021 • edited Loading

delawski left a comment

Choose a reason for hiding this comment

milindmore22 commented Dec 1, 2021

westonruter commented Dec 1, 2021

milindmore22 commented Dec 10, 2021 • edited Loading

Introduce Bento sanitizer to handle with `bento`-prefixed components #6722

Introduce Bento sanitizer to handle with `bento`-prefixed components #6722

westonruter commented Nov 16, 2021 •

edited

Loading

westonruter commented Nov 16, 2021 •

edited

Loading

westonruter commented Nov 16, 2021 •

edited

Loading

github-actions bot commented Nov 23, 2021 •

edited

Loading

codecov bot commented Nov 23, 2021 •

edited

Loading

westonruter commented Nov 29, 2021 •

edited

Loading

milindmore22 commented Dec 10, 2021 •

edited

Loading