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.

Sign up for GitHub

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

Jump to bottom

amp-bind: XHTML-friendly syntax #11115

Closed
dreamofabear opened this issue Aug 28, 2017 · 15 comments · Fixed by #15408
Closed

amp-bind: XHTML-friendly syntax #11115

dreamofabear opened this issue Aug 28, 2017 · 15 comments · Fixed by #15408
Assignees
@dreamofabear
Labels
P1: High Priority Type: Feature Request
Milestone
Sprint H2 May 2018

Comments

@dreamofabear
Copy link

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>
@dreamofabear dreamofabear added this to the New FRs milestone Aug 28, 2017
@dreamofabear dreamofabear self-assigned this Aug 28, 2017
@joshcp
Copy link

joshcp commented Sep 21, 2017

@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. ☹️

@ampprojectbot
Copy link
Member

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

@dreamofabear
Copy link
Author

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. 😉

@dreamofabear dreamofabear mentioned this issue Jan 25, 2018
Merged
4 tasks
@westonruter
Copy link
Member

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

@westonruter westonruter mentioned this issue Apr 4, 2018
Merged
@westonruter
Copy link
Member

westonruter commented Apr 4, 2018 •
edited
Loading

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.

@westonruter
Copy link
Member

westonruter commented Apr 4, 2018 •
edited
Loading

@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: xwp/travel#13 (comment)

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.

@dreamofabear
Copy link
Author

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.

@westonruter
Copy link
Member

westonruter commented Apr 4, 2018 •
edited
Loading

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.

@westonruter
Copy link
Member

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

@dreamofabear
Copy link
Author

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.

@westonruter
Copy link
Member

westonruter commented Apr 4, 2018 •
edited
Loading

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 😄

@dreamofabear
Copy link
Author

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.

@westonruter
Copy link
Member

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.

@dreamofabear
Copy link
Author

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?

@westonruter
Copy link
Member

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.

@dreamofabear dreamofabear mentioned this issue May 16, 2018
Merged
@dreamofabear dreamofabear mentioned this issue May 17, 2018
Merged
@westonruter westonruter mentioned this issue May 19, 2018
Closed
This was referenced Sep 5, 2018
Closed
Closed
westonruter added a commit to westonruter/amphtml that referenced this issue Dec 12, 2018
@westonruter
Add docs for XML-compatible binding attribute syntax
d713ebe 
See ampproject#11115 and ampproject#15408
@westonruter westonruter mentioned this issue Dec 12, 2018
Merged
dreamofabear pushed a commit that referenced this issue Jan 16, 2019
@westonruter
📖 Add docs for XML-compatible AMP bind attribute syntax (#19835)
e45b6f4 
* Add docs for XML-compatible binding attribute syntax

See #11115 and #15408

* Minor edits
@honeybadgerdontcare honeybadgerdontcare mentioned this issue Feb 8, 2019
Closed
noranazmy pushed a commit to noranazmy/amphtml that referenced this issue Mar 22, 2019
@westonruter
📖 Add docs for XML-compatible AMP bind attribute syntax (ampproject#1…
69f3818 
…9835)

* Add docs for XML-compatible binding attribute syntax

See ampproject#11115 and ampproject#15408

* Minor edits
@westonruter westonruter mentioned this issue Sep 20, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dreamofabear dreamofabear

Labels
P1: High Priority Type: Feature Request
Projects
None yet
Milestone
Sprint H2 May 2018
Development

Successfully merging a pull request may close this issue.

amp-bind: Support data-amp-bind-* attributes dreamofabear/amphtml
4 participants
@westonruter @joshcp @dreamofabear @ampprojectbot