New validation rule for A4A meta tags. by calebcordry · Pull Request #22917 · ampproject/amphtml

calebcordry · 2019-06-18T23:19:20Z

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 · 2019-06-18T23:19:56Z

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

Gregable · 2019-06-18T23:51:52Z

validator/validator-main.protoascii

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


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">

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

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-*

Gregable · 2019-06-18T23:53:56Z

validator/validator-main.protoascii

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


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.

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

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

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

Gregable

pressed wrong button, unapprove

lannka · 2019-06-18T23:55:42Z

naming bikeshed: amp4ads-vars

calebcordry · 2019-06-19T23:20:28Z

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

calebcordry · 2019-06-25T19:17:26Z

@Gregable Friendly ping.

Gregable · 2019-06-25T20:49:15Z

validator/validator-main.protoascii

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


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.

Gregable · 2019-06-25T20:51:27Z

validator/testdata/amp4ads_feature_tests/amp_story_ad_errors.out

+|      <!-- 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]


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.

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.

Gregable · 2019-06-25T20:55:55Z

validator/testdata/amp4ads_feature_tests/amp_story_ad_errors.out

+|      <!-- 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]


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.

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 · 2019-07-02T23:34:39Z

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

* 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

new rule.

27356bb

calebcordry requested review from Gregable and lannka June 18, 2019 23:19

googlebot added the cla: yes label Jun 18, 2019

typo

883baaa

Gregable approved these changes Jun 18, 2019

View reviewed changes

Gregable requested changes Jun 18, 2019

View reviewed changes

calebcordry added 2 commits June 19, 2019 12:21

comments

4330b9a

tests

620b411

Gregable requested changes Jun 25, 2019

View reviewed changes

comments

7f01e6e

Gregable approved these changes Jul 3, 2019

View reviewed changes

calebcordry merged commit b66f2fa into ampproject:master Jul 3, 2019

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

cl/257092328 Revision bump for ampproject#22917

6822316

twifkak mentioned this pull request Jul 9, 2019

Validator rollup #23227

Merged

pm-shashank-jain mentioned this pull request Apr 23, 2021

✨ RTC vendors: add query Param for Consent and ConsetMetadata fields to PubMatic OpenWrap #33973

Closed

Conversation

calebcordry commented Jun 18, 2019

Uh oh!

calebcordry commented Jun 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gregable left a comment

Choose a reason for hiding this comment

Uh oh!

lannka commented Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

calebcordry commented Jun 19, 2019

Uh oh!

calebcordry commented Jun 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calebcordry commented Jul 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lannka commented Jun 18, 2019 •

edited

Loading