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

Normalize amp-bind syntax to non-bracketed data-amp-bind-* attributes #2974

Merged
merged 7 commits into from
Aug 13, 2019
2 changes: 1 addition & 1 deletion includes/sanitizers/class-amp-comments-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ protected function process_comment_form( $comment_form ) {
}
}

$amp_bind_attr_format = AMP_DOM_Utils::get_amp_bind_placeholder_prefix() . '%s';
$amp_bind_attr_format = AMP_DOM_Utils::AMP_BIND_DATA_ATTR_PREFIX . '%s';
foreach ( $form_fields as $name => $form_field ) {
foreach ( $form_field as $element ) {

Expand Down
20 changes: 10 additions & 10 deletions includes/sanitizers/class-amp-core-theme-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -1407,12 +1407,12 @@ public function add_twentyfourteen_slider_carousel() {
$amp_carousel_desktop_id = 'twentyFourteenSliderDesktop';
$amp_carousel_mobile_id = 'twentyFourteenSliderMobile';
$amp_carousel_attributes = [
'layout' => 'responsive',
'on' => "slideChange:AMP.setState( { $selected_slide_state_id: event.index } )",
'width' => '100',
'type' => 'slides',
'loop' => '',
'data-amp-bind-slide' => $selected_slide_state_id,
'layout' => 'responsive',
'on' => "slideChange:AMP.setState( { $selected_slide_state_id: event.index } )",
'width' => '100',
'type' => 'slides',
'loop' => '',
AMP_DOM_Utils::AMP_BIND_DATA_ATTR_PREFIX . 'slide' => $selected_slide_state_id,
];
$amp_carousel_desktop = AMP_DOM_Utils::create_node(
$this->dom,
Expand Down Expand Up @@ -1460,7 +1460,7 @@ public function add_twentyfourteen_slider_carousel() {
$li->setAttribute( 'selected', '' );
$a->setAttribute( 'class', 'slider-active' );
}
$a->setAttribute( 'data-amp-bind-class', "$selected_slide_state_id == $i ? 'slider-active' : ''" );
$a->setAttribute( AMP_DOM_Utils::AMP_BIND_DATA_ATTR_PREFIX . 'class', "$selected_slide_state_id == $i ? 'slider-active' : ''" );
$a->setAttribute( 'role', 'button' );
$a->setAttribute( 'on', "tap:AMP.setState( { $selected_slide_state_id: $i } )" );
$li->setAttribute( 'option', (string) $i );
Expand Down Expand Up @@ -1511,8 +1511,8 @@ public function add_twentyfourteen_search() {

// Set visibility and aria-expanded based of the link based on whether the search bar is expanded.
$search_toggle_link->setAttribute( 'aria-expanded', wp_json_encode( $hidden ) );
$search_toggle_link->setAttribute( 'data-amp-bind-aria-expanded', "$hidden_state_id ? 'false' : 'true'" );
$search_toggle_div->setAttribute( 'data-amp-bind-class', "$hidden_state_id ? 'search-toggle' : 'search-toggle active'" );
$search_container->setAttribute( 'data-amp-bind-class', "$hidden_state_id ? 'search-box-wrapper hide' : 'search-box-wrapper'" );
$search_toggle_link->setAttribute( AMP_DOM_Utils::AMP_BIND_DATA_ATTR_PREFIX . 'aria-expanded', "$hidden_state_id ? 'false' : 'true'" );
$search_toggle_div->setAttribute( AMP_DOM_Utils::AMP_BIND_DATA_ATTR_PREFIX . 'class', "$hidden_state_id ? 'search-toggle' : 'search-toggle active'" );
$search_container->setAttribute( AMP_DOM_Utils::AMP_BIND_DATA_ATTR_PREFIX . 'class', "$hidden_state_id ? 'search-box-wrapper hide' : 'search-box-wrapper'" );
}
}
6 changes: 3 additions & 3 deletions includes/sanitizers/class-amp-nav-menu-toggle-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ public function sanitize() {

if ( ! empty( $this->args['nav_container_toggle_class'] ) ) {
$nav_el->setAttribute(
AMP_DOM_Utils::get_amp_bind_placeholder_prefix() . 'class',
AMP_DOM_Utils::AMP_BIND_DATA_ATTR_PREFIX . 'class',
sprintf(
"%s + ( $state_id ? %s : '' )",
wp_json_encode( $nav_el->getAttribute( 'class' ) ),
Expand All @@ -112,10 +112,10 @@ public function sanitize() {
$button_on = sprintf( "tap:AMP.setState({ $state_id: ! $state_id })" );
$button_el->setAttribute( 'on', $button_on );
$button_el->setAttribute( 'aria-expanded', 'false' );
$button_el->setAttribute( AMP_DOM_Utils::get_amp_bind_placeholder_prefix() . 'aria-expanded', "$state_id ? 'true' : 'false'" );
$button_el->setAttribute( AMP_DOM_Utils::AMP_BIND_DATA_ATTR_PREFIX . 'aria-expanded', "$state_id ? 'true' : 'false'" );
if ( ! empty( $this->args['menu_button_toggle_class'] ) ) {
$button_el->setAttribute(
AMP_DOM_Utils::get_amp_bind_placeholder_prefix() . 'class',
AMP_DOM_Utils::AMP_BIND_DATA_ATTR_PREFIX . 'class',
sprintf( "%s + ( $state_id ? %s : '' )", wp_json_encode( $button_el->getAttribute( 'class' ) ), wp_json_encode( ' ' . $this->args['menu_button_toggle_class'] ) )
);
}
Expand Down
2 changes: 1 addition & 1 deletion includes/sanitizers/class-amp-style-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -451,7 +451,7 @@ private function get_used_class_names() {
}

// Find all [class] attributes and capture the contents of any single- or double-quoted strings.
foreach ( $this->xpath->query( '//*/@' . AMP_DOM_Utils::get_amp_bind_placeholder_prefix() . 'class' ) as $bound_class_attribute ) {
foreach ( $this->xpath->query( '//*/@' . AMP_DOM_Utils::AMP_BIND_DATA_ATTR_PREFIX . 'class' ) as $bound_class_attribute ) {
if ( preg_match_all( '/([\'"])([^\1]*?)\1/', $bound_class_attribute->nodeValue, $matches ) ) {
$classes .= ' ' . implode( ' ', $matches[2] );
}
Expand Down
7 changes: 4 additions & 3 deletions includes/sanitizers/class-amp-tag-and-attribute-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@ public function __construct( $dom, $args = [] ) {
'amp_allowed_tags' => AMP_Allowed_Tags_Generated::get_allowed_tags(),
'amp_globally_allowed_attributes' => AMP_Allowed_Tags_Generated::get_allowed_attributes(),
'amp_layout_allowed_attributes' => AMP_Allowed_Tags_Generated::get_layout_attributes(),
'amp_bind_placeholder_prefix' => AMP_DOM_Utils::get_amp_bind_placeholder_prefix(),
];

parent::__construct( $dom, $args );
Expand Down Expand Up @@ -236,7 +235,7 @@ public function get_scripts() {
private function process_alternate_names( $attr_spec_list ) {
foreach ( $attr_spec_list as $attr_name => &$attr_spec ) {
if ( '[' === $attr_name[0] ) {
$placeholder_attr_name = $this->args['amp_bind_placeholder_prefix'] . trim( $attr_name, '[]' );
$placeholder_attr_name = AMP_DOM_Utils::AMP_BIND_DATA_ATTR_PREFIX . trim( $attr_name, '[]' );
if ( ! isset( $attr_spec[ AMP_Rule_Spec::ALTERNATIVE_NAMES ] ) ) {
$attr_spec[ AMP_Rule_Spec::ALTERNATIVE_NAMES ] = [];
}
Expand Down Expand Up @@ -597,6 +596,8 @@ private function process_node( $node ) {
$is_bind_attribute = (
'[' === $name[0]
||
AMP_DOM_Utils::AMP_BIND_DATA_ATTR_PREFIX === substr( $name, 0, 40 )
westonruter marked this conversation as resolved.
Show resolved Hide resolved
||
( isset( $this->rev_alternate_attr_name_lookup[ $name ] ) && '[' === $this->rev_alternate_attr_name_lookup[ $name ][0] )
);
if ( $is_bind_attribute ) {
Expand Down Expand Up @@ -1665,7 +1666,7 @@ private function is_amp_allowed_attribute( $attr_node, $attr_spec_list ) {
if (
isset( $attr_spec_list[ $attr_name ] )
||
'data-' === substr( $attr_name, 0, 5 )
( 'data-' === substr( $attr_name, 0, 5 ) && AMP_DOM_Utils::AMP_BIND_DATA_ATTR_PREFIX !== substr( $attr_name, 0, 14 ) )
||
// Allow the 'amp' or '⚡' attribute in <html>, like <html ⚡>.
( 'html' === $attr_node->parentNode->nodeName && in_array( $attr_node->nodeName, [ 'amp', '⚡' ], true ) )
Expand Down
26 changes: 15 additions & 11 deletions includes/utils/class-amp-dom-utils.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@
*/
class AMP_DOM_Utils {

/**
* Attribute prefix for AMP-bind data attributes.
*
* @var string
*/
const AMP_BIND_DATA_ATTR_PREFIX = 'data-amp-bind-';

/**
* HTML elements that are self-closing.
*
Expand Down Expand Up @@ -224,16 +231,14 @@ public static function is_valid_head_node( DOMNode $node ) {
* @since 0.7
* @see \AMP_DOM_Utils::convert_amp_bind_attributes()
* @see \AMP_DOM_Utils::restore_amp_bind_attributes()
* @deprecated Use AMP_DOM_Utils::AMP_BIND_DATA_ATTR_PREFIX alone.
* @link https://www.ampproject.org/docs/reference/components/amp-bind
*
* @return string HTML5 data-* attribute name prefix for AMP binding attributes.
*/
public static function get_amp_bind_placeholder_prefix() {
static $attribute_prefix;
if ( ! isset( $attribute_prefix ) ) {
$attribute_prefix = sprintf( 'amp-binding-%s-', md5( wp_rand() ) );
}
return $attribute_prefix;
_deprecated_function( __METHOD__, '1.2.1' );
return self::AMP_BIND_DATA_ATTR_PREFIX;
}

/**
Expand Down Expand Up @@ -284,7 +289,6 @@ private static function get_mustache_tag_placeholders() {
* @return string HTML with AMP binding attributes replaced with HTML5 data-* attributes.
*/
public static function convert_amp_bind_attributes( $html ) {
$amp_bind_attr_prefix = self::get_amp_bind_placeholder_prefix();

// Pattern for HTML attribute accounting for binding attr name, boolean attribute, single/double-quoted attribute value, and unquoted attribute values.
$attr_regex = '#^\s+(?P<name>\[?[a-zA-Z0-9_\-]+\]?)(?P<value>=(?:"[^"]*+"|\'[^\']*+\'|[^\'"\s]+))?#';
Expand All @@ -295,7 +299,7 @@ public static function convert_amp_bind_attributes( $html ) {
* @param array $tag_matches Tag matches.
* @return string Replacement.
*/
$replace_callback = static function( $tag_matches ) use ( $amp_bind_attr_prefix, $attr_regex ) {
$replace_callback = static function( $tag_matches ) use ( $attr_regex ) {

// Strip the self-closing slash as long as it is not an attribute value, like for the href attribute (<a href=/>).
$old_attrs = preg_replace( '#(?<!=)/$#', '', $tag_matches['attrs'] );
Expand All @@ -308,7 +312,7 @@ public static function convert_amp_bind_attributes( $html ) {
$offset += strlen( $attr_matches[0] );

if ( '[' === $attr_matches['name'][0] ) {
$new_attrs .= ' ' . $amp_bind_attr_prefix . trim( $attr_matches['name'], '[]' );
$new_attrs .= ' ' . self::AMP_BIND_DATA_ATTR_PREFIX . trim( $attr_matches['name'], '[]' );
if ( isset( $attr_matches['value'] ) ) {
$new_attrs .= $attr_matches['value'];
}
Expand Down Expand Up @@ -363,14 +367,16 @@ public static function convert_amp_bind_attributes( $html ) {
*
* @since 0.7
* @see \AMP_DOM_Utils::convert_amp_bind_attributes()
* @deprecated Allow the data-amp-bind-* attributes to be used instead.
* @link https://www.ampproject.org/docs/reference/components/amp-bind
*
* @param string $html HTML with amp-bind attributes converted.
* @return string HTML with amp-bind attributes restored.
*/
public static function restore_amp_bind_attributes( $html ) {
_deprecated_function( __METHOD__, '1.2.1' );
$html = preg_replace(
'#\s' . self::get_amp_bind_placeholder_prefix() . '([a-zA-Z0-9_\-]+)#',
'#\s' . self::AMP_BIND_DATA_ATTR_PREFIX . '([a-zA-Z0-9_\-]+)#',
' [$1]',
$html
);
Expand Down Expand Up @@ -560,8 +566,6 @@ public static function get_content_from_dom_node( $dom, $node ) {
);
}

$html = self::restore_amp_bind_attributes( $html );

/*
* Travis w/PHP 7.1 generates <br></br> and <hr></hr> vs. <br/> and <hr/>, respectively.
* Travis w/PHP 7.x generates <source ...></source> vs. <source ... />. Etc.
Expand Down
8 changes: 2 additions & 6 deletions tests/php/test-class-amp-dom-utils.php
Original file line number Diff line number Diff line change
Expand Up @@ -162,17 +162,13 @@ public function test__get_content_from_dom__br_no_closing_tag() {
* Test convert_amp_bind_attributes.
*
* @covers \AMP_DOM_Utils::convert_amp_bind_attributes()
* @covers \AMP_DOM_Utils::restore_amp_bind_attributes()
* @covers \AMP_DOM_Utils::get_amp_bind_placeholder_prefix()
*/
public function test_amp_bind_conversion() {
$original = '<amp-img width=300 height="200" data-foo="bar" selected src="/img/dog.jpg" [src]="myAnimals[currentAnimal].imageUrl"></amp-img>';
$converted = AMP_DOM_Utils::convert_amp_bind_attributes( $original );
$this->assertNotEquals( $converted, $original );
$this->assertContains( AMP_DOM_Utils::get_amp_bind_placeholder_prefix() . 'src="myAnimals[currentAnimal].imageUrl"', $converted );
$this->assertContains( AMP_DOM_Utils::AMP_BIND_DATA_ATTR_PREFIX . 'src="myAnimals[currentAnimal].imageUrl"', $converted );
$this->assertContains( 'width=300 height="200" data-foo="bar" selected', $converted );
$restored = AMP_DOM_Utils::restore_amp_bind_attributes( $converted );
$this->assertEquals( $original, $restored );

// Check tag with self-closing attribute.
$original = '<input type="text" role="textbox" class="calc-input" id="liens" name="liens" [value]="(result1 != null) ? result1.liens : \'verifying…\'" />';
Expand All @@ -191,7 +187,7 @@ public function test_amp_bind_conversion() {
];
foreach ( $malformed_html as $html ) {
$converted = AMP_DOM_Utils::convert_amp_bind_attributes( $html );
$this->assertNotContains( AMP_DOM_Utils::get_amp_bind_placeholder_prefix(), $converted, "Source: $html" );
$this->assertNotContains( AMP_DOM_Utils::AMP_BIND_DATA_ATTR_PREFIX, $converted, "Source: $html" );
}
}

Expand Down
4 changes: 2 additions & 2 deletions tests/php/test-class-amp-nav-menu-toggle-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,10 @@ public function data_converter() {
if ( empty( $toggle_class ) ) {
return '';
}
return ' [class]="&quot;' . $class . '&quot; + ( navMenuToggledOn ? &quot; ' . $toggle_class . '&quot; : \'\' )"';
return ' data-amp-bind-class="&quot;' . $class . '&quot; + ( navMenuToggledOn ? &quot; ' . $toggle_class . '&quot; : \'\' )"';
};
$amp_get_toggle_attrs = function( $class = '', $toggle_class = 'toggled-on' ) {
return ' on="tap:AMP.setState({ navMenuToggledOn: ! navMenuToggledOn })" aria-expanded="false" [aria-expanded]="navMenuToggledOn ? \'true\' : \'false\'"' . ( ! empty( $toggle_class ) ? ' [class]="&quot;' . $class . '&quot; + ( navMenuToggledOn ? &quot; ' . $toggle_class . '&quot; : \'\' )"' : '' );
return ' on="tap:AMP.setState({ navMenuToggledOn: ! navMenuToggledOn })" aria-expanded="false" data-amp-bind-aria-expanded="navMenuToggledOn ? \'true\' : \'false\'"' . ( ! empty( $toggle_class ) ? ' data-amp-bind-class="&quot;' . $class . '&quot; + ( navMenuToggledOn ? &quot; ' . $toggle_class . '&quot; : \'\' )"' : '' );
};

return [
Expand Down
16 changes: 8 additions & 8 deletions tests/php/test-tag-and-attribute-sanitizer.php
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,7 @@ static function () {

'carousel_lightbox' => [
'<amp-carousel width="450" height="300" delay="100" arrows [slide]="foo" autoplay loop lightbox></amp-carousel>',
null,
'<amp-carousel width="450" height="300" delay="100" arrows data-amp-bind-slide="foo" autoplay loop lightbox></amp-carousel>',
[ 'amp-bind', 'amp-carousel', 'amp-lightbox-gallery' ],
],

Expand Down Expand Up @@ -981,19 +981,19 @@ static function() {

'amp_bind_attr' => [
'<p [text]="\'Hello \' + foo">Hello World</p><button on="tap:AMP.setState({foo: \'amp-bind\'})">Update</button>',
null, // No change.
'<p data-amp-bind-text="\'Hello \' + foo">Hello World</p><button on="tap:AMP.setState({foo: \'amp-bind\'})">Update</button>',
[ 'amp-bind' ],
],

'amp_bind_with_greater_than_symbol' => [
'<div class="home page-template-default page page-id-7 logged-in wp-custom-logo group-blog" [class]="minnow.bodyClasses.concat( minnow.navMenuExpanded ? \'sidebar-open\' : \'\' ).filter( className => \'\' != className )">hello</div>',
'<div class="home page-template-default page page-id-7 logged-in wp-custom-logo group-blog" [class]="minnow.bodyClasses.concat( minnow.navMenuExpanded ? \'sidebar-open\' : \'\' ).filter( className =&gt; \'\' != className )">hello</div>',
'<div class="home page-template-default page page-id-7 logged-in wp-custom-logo group-blog" data-amp-bind-class="minnow.bodyClasses.concat( minnow.navMenuExpanded ? \'sidebar-open\' : \'\' ).filter( className =&gt; \'\' != className )">hello</div>',
[ 'amp-bind' ],
],

'amp_bad_bind_attr' => [
'<a [href]=\'/\' [hidden]>test</a><p [text]="\'Hello \' + name" [unrecognized] title="Foo"><button [disabled]="" [type]=\'\'>Hello World</button></p>',
'<a [href]="/" [hidden]>test</a><p [text]="\'Hello \' + name" title="Foo"><button [disabled]="" [type]="">Hello World</button></p>',
'<a data-amp-bind-href="/" data-amp-bind-hidden>test</a><p data-amp-bind-text="\'Hello \' + name" title="Foo"><button data-amp-bind-disabled="" data-amp-bind-type="">Hello World</button></p>',
[ 'amp-bind' ],
],

Expand Down Expand Up @@ -1081,7 +1081,7 @@ static function() {

'amp_date_picker_range' => [
'<amp-date-picker type="range" minimum-nights="2" maximum-nights="4" mode="overlay" id="range-date-picker" on=" select: AMP.setState({ dates: event.dates, startDate: event.start, endDate: event.end })" format="YYYY-MM-DD" open-after-select min="2017-10-26" start-input-selector="#range-start" end-input-selector="#range-end" class="example-picker space-between"><div class="ampstart-input"><input class="border-none p0" id="range-start" placeholder="Start date"></div><div class="ampstart-input"><input class="border-none p0" id="range-end" placeholder="End date"></div><button class="ampstart-btn caps" on="tap:range-date-picker.clear">Clear</button><template type="amp-mustache" info-template><span [text]="(startDate &amp;&amp; endDate ? \'You picked \' + startDate.date + \' as start date and \' + endDate.date + \' as end date.\' : \'You will see your chosen dates here.\')"> You will see your chosen dates here.</span></template></amp-date-picker>',
null, // No change.
'<amp-date-picker type="range" minimum-nights="2" maximum-nights="4" mode="overlay" id="range-date-picker" on=" select: AMP.setState({ dates: event.dates, startDate: event.start, endDate: event.end })" format="YYYY-MM-DD" open-after-select min="2017-10-26" start-input-selector="#range-start" end-input-selector="#range-end" class="example-picker space-between"><div class="ampstart-input"><input class="border-none p0" id="range-start" placeholder="Start date"></div><div class="ampstart-input"><input class="border-none p0" id="range-end" placeholder="End date"></div><button class="ampstart-btn caps" on="tap:range-date-picker.clear">Clear</button><template type="amp-mustache" info-template><span data-amp-bind-text="(startDate &amp;&amp; endDate ? \'You picked \' + startDate.date + \' as start date and \' + endDate.date + \' as end date.\' : \'You will see your chosen dates here.\')"> You will see your chosen dates here.</span></template></amp-date-picker>',
[ 'amp-date-picker', 'amp-bind', 'amp-mustache' ],
],

Expand Down Expand Up @@ -1307,7 +1307,7 @@ static function() {

'amp-lightbox' => [
'<amp-lightbox id="my-lightbox" [open]="true" animate-in="fly-in-top" layout="nodisplay"><div class="lightbox" on="tap:my-lightbox.close" role="button" tabindex="0"><h1>Hello World!</h1></div></amp-lightbox>',
null,
'<amp-lightbox id="my-lightbox" data-amp-bind-open="true" animate-in="fly-in-top" layout="nodisplay"><div class="lightbox" on="tap:my-lightbox.close" role="button" tabindex="0"><h1>Hello World!</h1></div></amp-lightbox>',
[ 'amp-lightbox', 'amp-bind' ],
],

Expand All @@ -1331,7 +1331,7 @@ static function() {

'amp_textarea_with_autoexpand_and_defaulttext' => [
'<textarea name="with-autoexpand" autoexpand [defaulttext]="hello" [text]="goodbye">hello</textarea>',
null,
'<textarea name="with-autoexpand" autoexpand data-amp-bind-defaulttext="hello" data-amp-bind-text="goodbye">hello</textarea>',
[ 'amp-form', 'amp-bind' ],
],

Expand Down Expand Up @@ -1432,7 +1432,7 @@ static function() {

'details' => [
'<details open [open]="foo.state"><summary>Learn more</summary><p>You are educated</p></details>',
null,
'<details open data-amp-bind-open="foo.state"><summary>Learn more</summary><p>You are educated</p></details>',
[ 'amp-bind' ],
],

Expand Down