New validation rule for A4A meta tags. #22917
Conversation
|
@lannka last chance to bikeshed this name :) are we happy with |
validator/validator-main.protoascii
Outdated
| tag_name: "META" | ||
| spec_name: "meta name=amp-vars-*" | ||
| mandatory_parent: "HEAD" | ||
| unique: true |
There was a problem hiding this comment.
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">
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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-*
validator/validator-main.protoascii
Outdated
| name: "name" | ||
| mandatory: true | ||
| value_regex: "^amp-vars-.+" | ||
| dispatch_key: NAME_VALUE_DISPATCH |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This doesn't sound great. Is there any way to get better errors?
There was a problem hiding this comment.
The suggestion above would resolve this issue, but it's changing the syntax.
There was a problem hiding this comment.
Thanks for the info, decided to go with multiple tags. Removed dispatch_key.
|
naming bikeshed: |
|
Also added some tests to |
|
@Gregable Friendly ping. |
validator/validator-main.protoascii
Outdated
| attrs: { | ||
| name: "name" | ||
| mandatory: true | ||
| value_regex: "^amp4ads-vars-.+" |
There was a problem hiding this comment.
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.
| | <!-- 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] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@Gregable I think I addressed latest round of comments. Please take another look when you have the chance. |
* new rule. * typo * comments * tests * comments
* 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
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">