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

Update amphtml validator spec to v1907301630320 #3003

Merged
merged 4 commits into from Aug 12, 2019

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Aug 8, 2019

Previously: #2816.

Todos specific to this update:

  • Use responsive layout for amp-soundcloud.
  • When AMP_Block_Sanitizer removes classes like wp-embed-aspect-16-9, have it push the width and height onto the child element if it has layout=responsive.

Changelog

SoundCloud Embed Layout

This is revisiting #2722 since amp-soundcloud now supports layout=responsive per ampproject/amphtml#23144.

Before

Only layout=fixed-height was supported, resulting in squished-looking embeds:

Screen Shot 2019-08-09 at 15 41 26

After

Now with layout=responsive, the embed looks identical to the non-AMP version, including the preservation of the aspect ratio defined by the block class name:

Screen Shot 2019-08-09 at 15 41 08

Details

(
    PREV_VERSION=1907022322580;
    THIS_VERSION=1907301630320; 
    git diff $PREV_VERSION...$THIS_VERSION -- $( git ls-files $THIS_VERSION -- . | grep '.protoascii' )
)
Compare 1907022322580...1907301630320
diff --git a/extensions/amp-ad/validator-amp-ad.protoascii b/extensions/amp-ad/validator-amp-ad.protoascii
index cfc0e8018..67cce32bb 100644
--- a/extensions/amp-ad/validator-amp-ad.protoascii
+++ b/extensions/amp-ad/validator-amp-ad.protoascii
@@ -77,6 +77,42 @@ tags: {  # <amp-ad>
     supported_layouts: RESPONSIVE
   }
 }
+tags: {  # <amp-ad type="custom">
+  html_format: AMP  # Ads are not allowed inside ads
+  tag_name: "AMP-AD"
+  spec_name: "amp-ad with type=custom"
+  requires_extension: "amp-ad"  # See note at top of file
+  also_requires_tag_warning: "amp-ad extension .js script"
+  # If modifying disallowed_ancestors, then please also edit
+  # extensions/amp-auto-ads/*/placement.js
+  disallowed_ancestor: "AMP-APP-BANNER"
+  attrs: {
+    name: "data-url"
+    mandatory: true
+    value_url: {
+      protocol: "https"
+    }
+  }
+  attrs: { name: "template" }
+  attrs: {
+    name: "type"
+    mandatory: true
+    value: "custom"
+    dispatch_key: NAME_VALUE_DISPATCH
+  }
+  attr_lists: "extended-amp-global"
+  spec_url: "https://github.com/ampproject/amphtml/blob/master/ads/custom.md"
+  amp_layout: {
+    supported_layouts: FILL
+    supported_layouts: FIXED
+    supported_layouts: FIXED_HEIGHT
+    supported_layouts: FLEX_ITEM
+    supported_layouts: FLUID
+    supported_layouts: INTRINSIC
+    supported_layouts: NODISPLAY
+    supported_layouts: RESPONSIVE
+  }
+}
 tags: {  # <amp-ad data-multi-size>
   html_format: AMP  # Ads are not allowed inside ads
   tag_name: "AMP-AD"
@@ -185,7 +221,6 @@ tags: {  # <amp-embed>
     }
     blacklisted_value_regex: "__amp_source_origin"
   }
-  attrs: { name: "template" }
   attrs: { name: "type" mandatory: true }
   attr_lists: "extended-amp-global"
   spec_url: "https://amp.dev/documentation/components/amp-ad"
diff --git a/extensions/amp-carousel/validator-amp-carousel.protoascii b/extensions/amp-carousel/validator-amp-carousel.protoascii
index b40dab8a3..1b1f31e9c 100644
--- a/extensions/amp-carousel/validator-amp-carousel.protoascii
+++ b/extensions/amp-carousel/validator-amp-carousel.protoascii
@@ -37,13 +37,11 @@ attr_lists: {
     value: ""
   }
   attrs: {
-    disabled_by: "amp4email"
     name: "autoplay"
     value_regex: "(|[0-9]+)"
   }
   attrs: { name: "controls" }
   attrs: {
-    disabled_by: "amp4email"
     name: "delay"
     value_regex: "[0-9]+"
   }
@@ -59,6 +57,23 @@ attr_lists: {
   # <amp-bind>
   attrs: { name: "[slide]" }
 }
+# There are 4 variants here, due to two independent axis of differences:
+#  1) type=slides vs. type=carousel
+#     - type=slides is the implied default if type is unspecified. This is
+#       encoded in the rules by making type=carousel mandatory, and
+#       type=slides optional.
+#     - slides vs. carousel implies different sets up supported layouts.
+#  2) presence of `lightbox` attribute.
+#     - lightbox is only allowed in AMP html format.
+#     - lightbox implies additional reference_point requirements of the
+#       amp-lightbox children.
+# The attribute implied rules (different layouts or different children) is not
+# a feature of the validator rules engine. Thus the only supportable ruleset is
+# to implement 1 tagspec for each combination. This produces the desired rules
+# but has undesirable results for some of the error messages, as seen in some
+# of the test cases. The main issue is that since there are two tagspecs with
+# each combination, we cannot use the usual dispatch_key trick for either
+# attribute.
 tags: {  # <amp-carousel type=slides>
   html_format: AMP
   html_format: AMP4ADS
@@ -96,7 +111,6 @@ tags: {  # <amp-carousel type=carousel>
     name: "type"
     value: "carousel"
     mandatory: true
-    dispatch_key: NAME_VALUE_DISPATCH
   }
   attr_lists: "amp-carousel-common"
   attr_lists: "extended-amp-global"
diff --git a/extensions/amp-date-picker/validator-amp-date-picker.protoascii b/extensions/amp-date-picker/validator-amp-date-picker.protoascii
index 8beca0716..0e96a1eba 100644
--- a/extensions/amp-date-picker/validator-amp-date-picker.protoascii
+++ b/extensions/amp-date-picker/validator-amp-date-picker.protoascii
@@ -133,6 +133,10 @@ tags: {
 
 attr_lists: {
   name: "amp-date-picker-common-attributes"
+  attrs: {
+    name: "allow-blocked-end-date"
+    value: ""
+  }
   attrs: {
     name: "allow-blocked-ranges"
     value: ""
diff --git a/extensions/amp-iframe/validator-amp-iframe.protoascii b/extensions/amp-iframe/validator-amp-iframe.protoascii
index bb9475837..5b4c88fba 100644
--- a/extensions/amp-iframe/validator-amp-iframe.protoascii
+++ b/extensions/amp-iframe/validator-amp-iframe.protoascii
@@ -60,6 +60,10 @@ tags: {  # <amp-iframe>
     value: "no"
     value: "yes"
   }
+  attrs: {
+    name: "tabindex"
+    value_regex: "-?\\d+"
+  }
   attrs: {
     name: "src"
     mandatory_oneof: "['src', 'srcdoc']"
diff --git a/extensions/amp-image-lightbox/validator-amp-image-lightbox.protoascii b/extensions/amp-image-lightbox/validator-amp-image-lightbox.protoascii
index 25ca279e1..238852928 100644
--- a/extensions/amp-image-lightbox/validator-amp-image-lightbox.protoascii
+++ b/extensions/amp-image-lightbox/validator-amp-image-lightbox.protoascii
@@ -36,6 +36,8 @@ tags: {  # amp-image-lightbox
     version: "latest"
   }
   attr_lists: "common-extension-attrs"
+  deprecation: "amp-image-lightbox cannot be properly positioned in emails and will soon be invalid in AMP4EMAIL."
+  deprecation_url: "https://github.com/ampproject/amphtml/issues/23170"
 }
 tags: {  # <amp-image-lightbox>
   html_format: AMP
diff --git a/extensions/amp-lightbox/validator-amp-lightbox.protoascii b/extensions/amp-lightbox/validator-amp-lightbox.protoascii
index d433afe29..afb77bd7d 100644
--- a/extensions/amp-lightbox/validator-amp-lightbox.protoascii
+++ b/extensions/amp-lightbox/validator-amp-lightbox.protoascii
@@ -29,15 +29,27 @@ tags: {  # amp-lightbox
 }
 tags: {  # amp-lightbox
   html_format: AMP4ADS
+  tag_name: "SCRIPT"
+  spec_name: "SCRIPT[custom-element=amp-lightbox] (AMP4ADS)"
+  extension_spec: {
+    name: "amp-lightbox"
+    version: "0.1"
+    version: "latest"
+  }
+  attr_lists: "common-extension-attrs"
+}
+tags: {  # amp-lightbox
   html_format: AMP4EMAIL
   tag_name: "SCRIPT"
-  spec_name: "SCRIPT[custom-element=amp-lightbox] (AMP4EMAIL/AMP4ADS)"
+  spec_name: "SCRIPT[custom-element=amp-lightbox] (AMP4EMAIL)"
   extension_spec: {
     name: "amp-lightbox"
     version: "0.1"
     version: "latest"
   }
   attr_lists: "common-extension-attrs"
+  deprecation: "amp-lightbox cannot be properly positioned in emails and will soon be invalid in AMP4EMAIL."
+  deprecation_url: "https://github.com/ampproject/amphtml/issues/23170"
 }
 tags: {  # <amp-lightbox>
   html_format: AMP
diff --git a/extensions/amp-script/validator-amp-script.protoascii b/extensions/amp-script/validator-amp-script.protoascii
index 93781374c..59205f343 100644
--- a/extensions/amp-script/validator-amp-script.protoascii
+++ b/extensions/amp-script/validator-amp-script.protoascii
@@ -24,17 +24,50 @@ tags: {
   }
   attr_lists: "common-extension-attrs"
 }
-
+# Temporarily disable until interaction of amp-script and signed exchanges has
+# been security reviewed.
+#tags: {  # <amp-script> (local script)
+#  html_format: AMP
+#  tag_name: "SCRIPT"
+#  spec_name: "amp-script extension local script"
+#  requires_extension: "amp-script"
+#  attrs: {
+#    name: "target"
+#    mandatory: true
+#    value: "amp-script"
+#    dispatch_key: NAME_VALUE_DISPATCH
+#  }
+#  attrs: {
+#    name: "type"
+#    mandatory: true
+#    value_casei: "text/plain"
+#  }
+#  attr_lists: "mandatory-id-attr"
+#  attr_lists: "nonce-attr"
+#  cdata: {
+#    max_bytes: 10000
+#    max_bytes_spec_url: "https://amp.dev/documentation/components/amp-script#faq"
+#    blacklisted_cdata_regex: {
+#      regex: "<!--"
+#      error_message: "html comments"
+#    }
+#  }
+#  spec_url: "https://amp.dev/documentation/components/amp-script"
+#}
 tags: {  # <amp-script>
   html_format: AMP
   tag_name: "AMP-SCRIPT"
   disallowed_ancestor: "AMP-SCRIPT"
   requires_extension: "amp-script"
   requires: "amp-experiment-token"
-  unique: true # TODO(choumx): Remove when global size limit is implemented.
+  attrs: { name: "sandbox" }
+  attrs: {
+    name: "script"
+    mandatory_anyof: "['script', 'src']"
+  }
   attrs: {
     name: "src"
-    mandatory: true
+    mandatory_anyof: "['script', 'src']"
     value_url: {
       protocol: "https"
       allow_relative: false
diff --git a/extensions/amp-soundcloud/validator-amp-soundcloud.protoascii b/extensions/amp-soundcloud/validator-amp-soundcloud.protoascii
index 953ca77c4..84081d8c0 100644
--- a/extensions/amp-soundcloud/validator-amp-soundcloud.protoascii
+++ b/extensions/amp-soundcloud/validator-amp-soundcloud.protoascii
@@ -55,6 +55,12 @@ tags: {  # <amp-soundcloud>
   }
   attr_lists: "extended-amp-global"
   amp_layout: {
+    supported_layouts: FILL
+    supported_layouts: FIXED
     supported_layouts: FIXED_HEIGHT
+    supported_layouts: FLEX_ITEM
+    supported_layouts: INTRINSIC
+    supported_layouts: NODISPLAY
+    supported_layouts: RESPONSIVE
   }
 }
diff --git a/extensions/amp-story/validator-amp-story.protoascii b/extensions/amp-story/validator-amp-story.protoascii
index 4ff1f1e5f..afc738f2f 100644
--- a/extensions/amp-story/validator-amp-story.protoascii
+++ b/extensions/amp-story/validator-amp-story.protoascii
@@ -146,6 +146,11 @@ tags: {  # <amp-story-grid-layer>
     value: "thirds"
     value: "vertical"
   }
+  attrs: {
+    name: "position"
+    value: "landscape-half-left"
+    value: "landscape-half-right"
+  }
   descendant_tag_list: "amp-story-grid-layer-allowed-descendants"
   reference_points: {
     tag_spec_name: "AMP-STORY-GRID-LAYER default"
diff --git a/extensions/amp-twitter/validator-amp-twitter.protoascii b/extensions/amp-twitter/validator-amp-twitter.protoascii
index e1f218e46..02ec95854 100644
--- a/extensions/amp-twitter/validator-amp-twitter.protoascii
+++ b/extensions/amp-twitter/validator-amp-twitter.protoascii
@@ -48,23 +48,11 @@ tags: {  # <amp-twitter>
       also_requires_attr: "data-momentid"
     }
   }
-  attrs: {
-    name: "data-link-color"
-    trigger: {
-      also_requires_attr: "data-tweetid"
-    }
-  }
   attrs: {
     name: "data-momentid"
     mandatory_oneof: "['data-momentid', 'data-timeline-source-type', 'data-tweetid']"
     value_regex: "\\d+"
   }
-  attrs: {
-    name: "data-theme"
-    trigger: {
-      also_requires_attr: "data-tweetid"
-    }
-  }
   attrs: {
     name: "data-timeline-id"
     trigger: {
diff --git a/validator/validator-main.protoascii b/validator/validator-main.protoascii
index 79ad56a11..cec9e6b2b 100644
--- a/validator/validator-main.protoascii
+++ b/validator/validator-main.protoascii
@@ -26,7 +26,7 @@ min_validator_revision_required: 375
 # newer versions of the spec file. This is currently a Google internal
 # mechanism, validator.js does not use this facility. However, any
 # change to this file (validator-main.js) requires updating this revision id.
-spec_file_revision: 896
+spec_file_revision: 920
 
 styles_spec_url: "https://amp.dev/documentation/guides-and-tutorials/develop/style_and_layout/style_pages"
 script_spec_url: "https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#html-tags"
@@ -993,6 +993,24 @@ tags: {
     dispatch_key: NAME_VALUE_DISPATCH
   }
 }
+# AMP metadata, name=amp4ads-vars-*
+# Allows advertisers to pass information from creatives
+# to be included in amp-ad-metadata.
+tags: {
+  html_format: AMP4ADS
+  tag_name: "META"
+  spec_name: "meta name=amp4ads-vars-*"
+  mandatory_parent: "HEAD"
+  attrs: {
+    name: "content"
+    mandatory: true
+  }
+  attrs: {
+    name: "name"
+    mandatory: true
+    value_regex: "amp4ads-vars-.+"
+  }
+}
 # 4.2.6 The style
 # Text contents of the style tag will be validated seperately.
 tags: {  # Special custom 'author' spreadsheet for AMP
@@ -1537,6 +1555,17 @@ tags: {
   tag_name: "ARTICLE"
 }
 # 4.3.3 The section element
+attr_lists: {
+  name: "poool-access-attrs"
+  attrs: {
+    name: "poool-access-preview"
+    requires_extension: "amp-access-poool"
+  }
+  attrs: {
+    name: "poool-access-content"
+    requires_extension: "amp-access-poool"
+  }
+}
 tags: {
   html_format: AMP
   html_format: AMP4ADS
@@ -1544,6 +1573,7 @@ tags: {
   html_format: ACTIONS
   tag_name: "SECTION"
   disallowed_ancestor: "AMP-ACCORDION"
+  attr_lists: "poool-access-attrs"
 }
 # 4.3.4 The nav element
 tags: {
@@ -4123,6 +4153,7 @@ tags {  # <form method=GET ...>
     name: "action-xhr"
     value_url: {
       protocol: "https"
+      allow_relative: false
     }
     blacklisted_value_regex: "__amp_source_origin|"
         "{{|}}"    # Mustache is disallowed in action-xhr.
@@ -6303,6 +6334,8 @@ attr_lists: {
   attrs: { name: "accesskey" }
   attrs: {
     # attribute "class" for transformed is handled within the Validator engine.
+    # If this changes, please be sure to also update:
+    # amp-experiment 1.0 implementation.
     disabled_by: "transformed"
     name: "class"
     blacklisted_value_regex: "(^|\\W)i-amphtml-"
@@ -6810,6 +6843,10 @@ error_specificity {
   code: CSS_EXCESSIVELY_NESTED
   specificity: 119
 }
+error_specificity {
+  code: DOCUMENT_SIZE_LIMIT_EXCEEDED
+  specificity: 120
+}
 # Error formats
 error_formats {
   code: UNKNOWN_CODE
@@ -7270,3 +7307,7 @@ error_formats {
   code: CSS_EXCESSIVELY_NESTED
   format: "CSS excessively nested in tag '%1'."
 }
+error_formats {
+  code: DOCUMENT_SIZE_LIMIT_EXCEEDED
+  format: "Document exceeded %1 bytes limit. Actual size %2 bytes."
+}

Copy link
Collaborator

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@swissspidy swissspidy merged commit e08c72e into develop Aug 12, 2019
@swissspidy swissspidy deleted the update/amphtml-v1907301630320 branch August 12, 2019 11:37
westonruter added a commit that referenced this pull request Aug 12, 2019
…p-bind-syntax

* 'develop' of github.com:ampproject/amp-wp:
  RTLCSS all the things (#2977)
  Fix AMP Story editor compatibility with code editor (#3007)
  Update dependency core-js to v3.2.1 (#3011)
  Update amphtml validator spec to v1907301630320 (#3003)
  Improve handling of unlisted Vimeo videos (#2986)
  Always hide AMP admin menu item and compatibility tool menu ite… (#3005)
  Update dependency dom-scroll-into-view to v2.0.1 (#3008)
  Hide tooltips that should be hidden (#2988)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants