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

Utilize new data-amp-bind-* alternative syntax for amp-bind attributes #1162

Closed
westonruter opened this issue May 19, 2018 · 8 comments · Fixed by #2974
Closed

Utilize new data-amp-bind-* alternative syntax for amp-bind attributes #1162

westonruter opened this issue May 19, 2018 · 8 comments · Fixed by #2974
Assignees
Labels
Enhancement New feature or improvement of an existing one Sanitizers
Milestone

Comments

@westonruter
Copy link
Member

There is a new XHTML-compatible amp-bind attribute syntax: ampproject/amphtml#11115 via ampproject/amphtml#15408

Instead of:

<span class="foo" [class]="foo ? 'foo' : 'bar'">

We can now do:

<span class="foo" data-amp-bind-class="foo ? 'foo' : 'bar'">

What this means is that we now have an official way of representing amp-bind attributes in PHP's DOM internals. We can now eliminate AMP_DOM_Utils::get_amp_bind_placeholder_prefix() and AMP_DOM_Utils::restore_amp_bind_attributes() entirely, and AMP_DOM_Utils::convert_amp_bind_attributes() can be modified to just convert the bracketed syntax into the data-amp-bind-* syntax. There would be no need to restore the bracketed syntax when re-serializing. The result will be improved performance.

@westonruter westonruter added this to the v1.0 milestone May 19, 2018
@westonruter westonruter removed this from the v1.0 milestone Jun 26, 2018
@lucky42890
Copy link

Is this issue solved ?

@westonruter
Copy link
Member Author

No

@westonruter westonruter added this to the v1.1 milestone Dec 14, 2018
@westonruter
Copy link
Member Author

We need to update the whitelist sanitizer to automatically enqueue amp-bind when data-amp-bind-* attributes are found. Currently it is only doing so for bracketed attributes:

$is_bind_attribute = (
'[' === $name[0]
||
( isset( $this->rev_alternate_attr_name_lookup[ $name ] ) && '[' === $this->rev_alternate_attr_name_lookup[ $name ][0] )
);
if ( $is_bind_attribute ) {
$this->script_components[] = 'amp-bind';
break;
}

This should also be done in the process_alternate_names method.

@westonruter westonruter removed this from the v1.1 milestone Mar 6, 2019
@westonruter
Copy link
Member Author

We also need to make sure that the alternative syntax is supported in places like:

// 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 ) {
if ( preg_match_all( '/([\'"])([^\1]*?)\1/', $bound_class_attribute->nodeValue, $matches ) ) {
$classes .= ' ' . implode( ' ', $matches[2] );
}
}

Anywhere that AMP_DOM_Utils::get_amp_bind_placeholder_prefix() occurs we'd need to also supply 'data-amp-bind-' as an alternative prefix.

Alternatively, we could just rewrite all bracketed-syntax attributes with data-amp-bind-* ones, which is what I suggested above 😄

@conwaydev
Copy link

conwaydev commented Aug 6, 2019

Glad I found this, we recently found out about the wonderful data-amp-bind-class attribute, however the AMP plugin's UnCSS will strip out the classes we pass to it whereas [class] will work just fine. Which is exactly what you mention above.

@westonruter
Copy link
Member Author

I started work on this.

@conwaydev Initial PR ready for testing: #2974

@westonruter westonruter self-assigned this Aug 7, 2019
@westonruter
Copy link
Member Author

@westonruter
Copy link
Member Author

@conwaydev Please test: #2974

@westonruter westonruter added this to the v1.2.1 milestone Aug 10, 2019
@swissspidy swissspidy added Enhancement New feature or improvement of an existing one Sanitizers labels Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or improvement of an existing one Sanitizers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants