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

New validation rule for A4A meta tags. #22917

Merged
merged 5 commits into from
Jul 3, 2019
Merged

New validation rule for A4A meta tags. #22917

merged 5 commits into from
Jul 3, 2019

Conversation

calebcordry
Copy link
Member

Introducing a new meta tag that advertisers can use to pass information to the amp runtime. These new meta tags will be read by the a4a transformer, and their data will be added to the created <script type="application/json" amp-ad-metadata> object.

format:
<meta name="amp-vars-id" content="123">

@calebcordry
Copy link
Member Author

@lannka last chance to bikeshed this name :) are we happy with amp-vars- ?

tag_name: "META"
spec_name: "meta name=amp-vars-*"
mandatory_parent: "HEAD"
unique: true
Copy link
Member

Choose a reason for hiding this comment

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

The PR description mentions "These new meta tags". Do you expect more than one of these on the same document? If so, I would not set unique here.

At the risk of bikeshedding (I have no strong opinion), if you expect multiple it might make sense instead to structure this like viewport, using commas to separate multiple values:

<meta name=amp-vars value="foo=bar;baz=lemur">

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, this looks interesting too. although there will be some escaping needed for = & ; (which is probably fine).

BTW, given we have amp4ads-ids, we should call this amp4ads-vars

Copy link
Member Author

Choose a reason for hiding this comment

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

Talked offline. Our partner teams prefer multiple tags, and since we don't feel strongly we decided to go with that.

Removed unique, changed to amp4ads-vars-*

name: "name"
mandatory: true
value_regex: "^amp-vars-.+"
dispatch_key: NAME_VALUE_DISPATCH
Copy link
Member

Choose a reason for hiding this comment

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

This won't work, since the value is a regex. You will need to drop the dispatch key, though it may cause some error-message quality issues with any <meta> errors giving error messages related to this one tagspec.

Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't sound great. Is there any way to get better errors?

Copy link
Member

Choose a reason for hiding this comment

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

The suggestion above would resolve this issue, but it's changing the syntax.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the info, decided to go with multiple tags. Removed dispatch_key.

Copy link
Member

@Gregable Gregable left a comment

Choose a reason for hiding this comment

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

pressed wrong button, unapprove

@lannka
Copy link
Contributor

lannka commented Jun 18, 2019

naming bikeshed: amp4ads-vars

@calebcordry
Copy link
Member Author

Also added some tests to amp_story_ad.html and amp_story_ad_errors.html

@calebcordry
Copy link
Member Author

@Gregable Friendly ping.

attrs: {
name: "name"
mandatory: true
value_regex: "^amp4ads-vars-.+"
Copy link
Member

Choose a reason for hiding this comment

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

value_regex always uses full-match rules. In javascript this is implemented as new RegExp('^(' + spec.value_regex + ')$'); so you don't need the ^ prefix.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

| <!-- Should fail due unrecognized `name` -->
| <meta name="amp4ads-vars" content="bar">
>> ^~~~~~~~~
amp4ads_feature_tests/amp_story_ad_errors.html:40:4 The attribute 'name' in tag 'meta name= and content=' is set to the invalid value 'amp4ads-vars'. [DISALLOWED_HTML]
Copy link
Member

Choose a reason for hiding this comment

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

Note that you end up with the same problem here. The error message is choosing the tagspec for 'meta name= and content=' to produce an error message, rather than the tagspec for 'meta name=amp4ads-vars-*'.

It's not so bad in this case, I suppose as it still reads sanely.

I would prefer to see a design where the name attribute value was a fixed set of strings or a single string, but I can live with this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see what you mean, I agree this could be better but there are tradeoffs with the all in one approach. Thanks for the clarity.

| <!-- Should fail due unrecognized `name` -->
| <meta name="amp4ads-vars" content="bar">
>> ^~~~~~~~~
amp4ads_feature_tests/amp_story_ad_errors.html:40:4 The attribute 'name' in tag 'meta name= and content=' is set to the invalid value 'amp4ads-vars'. [DISALLOWED_HTML]
Copy link
Member

Choose a reason for hiding this comment

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

Another thing to consider. Will you ever have a case where you want to have a specific rule for the content attribute value?

For example, something like: "If name=amp4ads-vars-visible, then content must have a value of true or false.". If you imagine something like that, then you should plan for it now, because after this change, such a rule would be making the validator rules more strict, which is generally hard to do.

Copy link
Member Author

Choose a reason for hiding this comment

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

Talked to @lannka offline. We will keep this as a generic API as far as validation is concerned, with the understanding that this pushes the responsibility to the runtime to handle any content related contracts.

@calebcordry
Copy link
Member Author

@Gregable I think I addressed latest round of comments. Please take another look when you have the chance.

@calebcordry calebcordry merged commit b66f2fa into ampproject:master Jul 3, 2019
@calebcordry calebcordry deleted the meta-validation branch July 3, 2019 22:24
twifkak added a commit to twifkak/amphtml that referenced this pull request Jul 9, 2019
@twifkak twifkak mentioned this pull request Jul 9, 2019
twifkak added a commit that referenced this pull request Jul 9, 2019
* cl/257092185 Revision bump for #23137

* cl/257092258 Revision bump for #23143

* cl/257092328 Revision bump for #22917

* cl/257092384 Revision bump for #23180

* cl/257092442 Revision bump for #23173

* cl/257092491 Revision bump for #23195
thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
* new rule.

* typo

* comments

* tests

* comments
thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
* cl/257092185 Revision bump for ampproject#23137

* cl/257092258 Revision bump for ampproject#23143

* cl/257092328 Revision bump for ampproject#22917

* cl/257092384 Revision bump for ampproject#23180

* cl/257092442 Revision bump for ampproject#23173

* cl/257092491 Revision bump for ampproject#23195
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants