Skip to content

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

Merged
swissspidy merged 7 commits intodevelopfrom
update/amp-bind-syntax
Aug 13, 2019
Merged

Normalize amp-bind syntax to non-bracketed data-amp-bind-* attributes#2974
swissspidy merged 7 commits intodevelopfrom
update/amp-bind-syntax

Conversation

@westonruter
Copy link
Copy Markdown
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 force-pushed the update/amp-bind-syntax branch from b0a2ea7 to 267abc2 Compare August 9, 2019 23:42
@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)
@westonruter westonruter requested a review from swissspidy August 12, 2019 20:24
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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