Amphtml: amp-bind: XHTML-friendly syntax

Created on 28 Aug 2017  ·  15Comments  ·  Source: ampproject/amphtml

Square brackets, e.g [foo], are not valid attribute name starting chars in XML spec: https://www.w3.org/TR/REC-xml/#NT-NameStartChar

Perhaps we can allow a configuration to switch to an alternative syntax, e.g.

<!-- Generic solution. -->
<script async custom-element="amp-bind" src="..." data-syntax="xml"></script>

<!-- Or an amp-bind specific solution. -->
<amp-bind syntax="xml"></amp-bind>
High Priority Feature Request

All 15 comments

@choumx We use JavaScript as part of our build process, and the square brackets have caused some headaches. We've resorted to using work-arounds to set the square bracket amp-bind attributes on page elements, which is not the best. ☹️

This issue hasn't been updated in awhile. @choumx Do you have any updates?

Sorry for the delay on this! A lot of things got in the way. We're super happy to accept PRs from external contributors too. 😉

Also: the bracketed syntax makes it difficult to generate AMP bound attributes with JSX (e.g. via React).

Simply allowing a data- attribute as an alternative syntax seems like it should be easy. So if there is a [foo] then this could be equivalent to a markup containing data-amp-bind-foo. I don't think there would have to be any specific syntax variation declared? For instance, both <html ⚡️> and <html amp> are equally valid.

@choumx I have a proposal for adding such support for data-amp-bind-* attributes in the, here for a WordPress theme that is being built to serve the AMP Start Adventure template: https://github.com/xwp/travel/pull/13#discussion_r179028914

To copy here for reference:

Maybe this is overkill in the theme actually and this initial XHTML syntax could should perhaps be something that is added to the AMP plugin itself. In fact, it could be part of the conversion process of the convert_amp_bind_attributes method, except this part could look like this instead:

[...Some AMP plugin PHP code...]

That should allow content such as <amp-img [src]="..."> and <amp-img data-amp-bind-src="..."> to both alike get serialized out as <amp-img [src]="..."> when the page is rendered. It would allow us to freely use such data- attributes in HTML content in post_content or in React JSX without worrying either about Kses, shortcode handling, or JSX syntax errors from getting in the way.

Such data- attributes wouldn't be recognized by the AMP runtime, but they would allow for them to be safely generated and stored in a WordPress context to then be converted to the bracketed syntax when the page is rendered. Now, if the data- syntax could also be just honored by the runtime as well that would be 100x better.

Sounds good. The only thing we'd need to decide is how to toggle amp-bind between parsing either [attr] or data-amp-bind-attr and not both.

I notice that <html ⚡ amp> does not trigger a validation error. So while alternative_names are allowed, the validator isn't currently enforcing exclusive use of one over an alternative (at least here). I see there is also a MUTUALLY_EXCLUSIVE_ATTRS error code but that isn't applying to alternative_names. Could that be the way to go here? Then an attribute like data-amp-bind-src could be added as an alternative name to [src], and only one would be allowed at a time.

(Also, maybe amp-bind-* would be a better attribute name prefix rather than data-amp-bind-* since generally data-* attributes are whitelisted.)

IMO data-* is nice since we won't need to whitelist every attribute, and I'm not as concerned with the validation part. Just wanted to avoid amp-bind doing unnecessary work by parsing extra attributes.

Could the parser when it comes across data-amp-bind-:name look to see if there is a corresponding [:name] attribute, and if so, just skip parsing the former?

In regards to validation, I think there'd still need to be something in place or else someone could throw on data-amp-bind-* attribute onto an element for an attribute for which there is no binding allowed. Also beware of data-amp-bind-onclick 😄

Absolutely, we have bind-validator.js that contains a whitelist of bindable attributes.

I still think we should use an <amp-bind syntax="default|data-amp-bind"> configuration element. It would avoid the need of a lot of hasAttribute checks and mixing syntax is probably bad practice anyways.

My concern about <amp-bind syntax="default|data-amp-bind"> is that a CMS may be combining AMP content from multiple places. In WordPress a theme template would be free to use the current bracketed syntax, where as content coming from the DB would likely not. So we'd need to make sure we normalize the syntax to one or the other when finalizing output. This is something we can do in the context of the WordPress plugin, but on other systems it may be more difficult to know beforehand which syntax is going to be used.

Oh interesting. So CMS's often stitch together fragments of HTML (e.g. template + UGC) to form an AMP page? Isn't there a risk that the result won't pass the AMP Validator?

Yes and yes 😄 But let's just say that the various modules/plugins of a CMS are validating that the markup they're contributing is valid AMP… it would complicate things if they also had to communicate to each other that one specific syntax of amp-bind is being enforced. If a plugin/module is outputting content with the bracketed amp-bind syntax, then it would be a burden for them to perhaps convert to the other syntax if the CMS specifies it. This could also hurt reusability of libraries that output AMP. For example if there is some Composer package that is not aware of any CMS context at all, and it is outputting amp-bind content, then it may not even provide an option to switch between the two syntaxes. So I think it would be beneficial if both were allowed in the same document, though naturally not on the same element.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

samanthamorco picture samanthamorco  ·  3Comments

sryze picture sryze  ·  3Comments

radiovisual picture radiovisual  ·  3Comments

aghassemi picture aghassemi  ·  3Comments

choumx picture choumx  ·  3Comments