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

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Aug 7, 2019

Fixes #1162.

The libxml in PHP does not understand the bracketed syntax of bound attributes in AMP-bind. So before loading an HTML document into the DOM, we first have to convert the bracketed syntax into something that is not munged via \AMP_DOM_Utils::convert_amp_bind_attributes(). Originally this replacement was a placeholder that was replaced with the bracketed-syntax upon serialization (via \AMP_DOM_Utils::restore_amp_bind_attributes()).

However, AMP now supports an alternative syntax for bound attributes which is XML-compatible: instead of [foo] one does data-amp-bind-foo. Now that this is equivalent in AMP, there is no need to restore the bracketed syntax upon serialization. We can normalize to the XML-compatible syntax. A couple added benefits to this are:

  1. Less processing time doing search/replace.
  2. Consumers of the AMP documents don't themselves have to worry about the bracketed syntax (see support forum topic).
  3. We no longer have to handle both syntaxes when doing validation. At the time of spec ingestion, all bracketed-attributes can be converted to data-amp-bind-* attributes.

Build for testing: amp.zip - v1.2.1-beta1-20190812T202306Z-b41861fe

@westonruter westonruter changed the title Eliminate restoration of bracketed amp-bind syntax after parsing Normalize amp-bind syntax to non-bracketed data-amp-bind-* attributes Aug 10, 2019
@westonruter westonruter marked this pull request as ready for review August 10, 2019 00:24
…p-bind-syntax

* 'develop' of github.com:ampproject/amp-wp:
  RTLCSS all the things (#2977)
  Fix AMP Story editor compatibility with code editor (#3007)
  Update dependency core-js to v3.2.1 (#3011)
  Update amphtml validator spec to v1907301630320 (#3003)
  Improve handling of unlisted Vimeo videos (#2986)
  Always hide AMP admin menu item and compatibility tool menu ite… (#3005)
  Update dependency dom-scroll-into-view to v2.0.1 (#3008)
  Hide tooltips that should be hidden (#2988)
Copy link
Collaborator

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@swissspidy
Copy link
Collaborator

Less processing time doing search/replace.

Perhaps we could have some dedicated performance tests at some point?

@swissspidy swissspidy merged commit f213c49 into develop Aug 13, 2019
@swissspidy swissspidy deleted the update/amp-bind-syntax branch August 13, 2019 09:43
@westonruter
Copy link
Member Author

Perhaps we could have some dedicated performance tests at some point?

Yes. See #1017.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Utilize new data-amp-bind-* alternative syntax for amp-bind attributes
4 participants