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 allowed tags/attributes from spec in amphtml 1908162134430 #3084

Merged
merged 13 commits into from Aug 28, 2019

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Aug 23, 2019

Previously: #3003.

  • Run ./bin/amphtml-update.sh
  • Examine diff for changelog
  • Update spec generator as needed based on spec format changes.
  • Modify validating sanitizer based on changes to spec, if needed.
  • Add tests for key changes

Changelog

  • ✨ Introduce support for the new AMP dev mode (Slightly looser validation in dev-mode amphtml#20974). When the html element has the data-ampdevmode attribute, prevent validating any element that also has the data-ampdevmode attribute. This will ultimately be used to prevent removing the admin bar per Ignore admin bar for AMP validation purposes #1921 (comment). See full writeup: Integrating with AMP Dev Mode in WordPress.
  • Recognize intrinsicsize attribute on img for specifying width/height dimensions, but then omit intrinsicsize from amp-img since handled by component. Pass through importance, intrinsicsize, and loading attributes to amp-img > noscript > img but omit from converted amp-img since also supported by component (largely).
  • ✨ Remove restriction that amp-sidebar be a direct child of the body element.
  • 🖌 Allow canvas inside of amp-script.
  • Add support for validating CDATA max_bytes constraint. Note this was previously being validated for stylesheets in the style sanitizer, but now with amp-script allowing inline scripts (soon) it is important to support this in the validating sanitizer.
  • Add meta name=amp-script-src tag spec for amp-script which are sourced either cross-origin or inline.
  • Force amp-carousel on version 0.1 since 0.2 depends on an experimental amp-base-carousel.
  • Add slide attribute to amp-carousel (apparently only relevant to 0.2).
  • Eliminate restriction that amp-autocomplete be inside of a form.
  • Add template attribute to amp-date-display.
  • Add AMP-LIST DIV [fetch-error] tag spec.
  • Add amp-video to amp-story-page-attachment.
  • Disallow empty rel attribute on link elements.
  • Add width and height attributes to symbol SVG element.

Details

(
    PREV_VERSION=1907301630320;
    THIS_VERSION=1908162134430; 
    git diff $PREV_VERSION...$THIS_VERSION -w -- $( git ls-files $THIS_VERSION -- . | grep '.protoascii' )
)
Compare 1907301630320...1908162134430
diff --git a/extensions/amp-autocomplete/validator-amp-autocomplete.protoascii b/extensions/amp-autocomplete/validator-amp-autocomplete.protoascii
index 6c0485078..03ec7cfb0 100644
--- a/extensions/amp-autocomplete/validator-amp-autocomplete.protoascii
+++ b/extensions/amp-autocomplete/validator-amp-autocomplete.protoascii
@@ -29,7 +29,6 @@ tags: {  # <amp-autocomplete>
   tag_name: "AMP-AUTOCOMPLETE"
   spec_name: "amp-autocomplete"
   requires_extension: "amp-autocomplete"
-  mandatory_ancestor: "FORM"
   attrs: {
     name: "filter"
     value_casei: "custom"
diff --git a/extensions/amp-carousel/validator-amp-carousel.protoascii b/extensions/amp-carousel/validator-amp-carousel.protoascii
index 1b1f31e9c..1984908e9 100644
--- a/extensions/amp-carousel/validator-amp-carousel.protoascii
+++ b/extensions/amp-carousel/validator-amp-carousel.protoascii
@@ -23,6 +23,7 @@ tags: {  # amp-carousel
   extension_spec: {
     name: "amp-carousel"
     version: "0.1"
+    version: "0.2"
     version: "latest"
     requires_usage: GRANDFATHERED
     deprecated_allow_duplicates: true
@@ -53,6 +54,10 @@ attr_lists: {
   attrs: {
     name: "loop"
     value: ""
+  }
+    attrs: {
+    name: "slide"
+    value_regex: "[0-9]+"
   }
   # <amp-bind>
   attrs: { name: "[slide]" }
@@ -111,6 +116,7 @@ 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"
@@ -169,6 +175,7 @@ tags: {  # <amp-carousel lightbox 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-display/validator-amp-date-display.protoascii b/extensions/amp-date-display/validator-amp-date-display.protoascii
index 2391ad711..e7e1550bb 100644
--- a/extensions/amp-date-display/validator-amp-date-display.protoascii
+++ b/extensions/amp-date-display/validator-amp-date-display.protoascii
@@ -42,6 +42,7 @@ tags: {  # <amp-date-display>
     value_regex: "-?\\d+"
   }
   attrs: { name: "locale" }
+  attrs: { name: "template" }
   attrs: {
     name: "timestamp-ms"
     mandatory_oneof: "['datetime', 'timestamp-ms', 'timestamp-seconds']"
diff --git a/extensions/amp-date-picker/validator-amp-date-picker.protoascii b/extensions/amp-date-picker/validator-amp-date-picker.protoascii
index 0e96a1eba..07633ddae 100644
--- a/extensions/amp-date-picker/validator-amp-date-picker.protoascii
+++ b/extensions/amp-date-picker/validator-amp-date-picker.protoascii
@@ -62,6 +62,7 @@ tags: {
     name: "mode"
     mandatory: true
     value_casei: "overlay"
+    dispatch_key: NAME_VALUE_DISPATCH
   }
   attrs: {
     name: "type"
@@ -115,6 +116,7 @@ tags: {
     name: "mode"
     mandatory: true
     value_casei: "overlay"
+    dispatch_key: NAME_VALUE_DISPATCH
   }
   attrs: {
     name: "type"
diff --git a/extensions/amp-list/validator-amp-list.protoascii b/extensions/amp-list/validator-amp-list.protoascii
index f27d16bcb..b5f35a026 100644
--- a/extensions/amp-list/validator-amp-list.protoascii
+++ b/extensions/amp-list/validator-amp-list.protoascii
@@ -118,10 +118,24 @@ tags: {  # <amp-list> with mandatory src and/or [src] attr
     supported_layouts: RESPONSIVE
   }
 }
+tags: {
+  html_format: AMP
+  html_format: AMP4ADS
+  html_format: AMP4EMAIL
+  html_format: ACTIONS
+  tag_name: "DIV"
+  spec_name: "AMP-LIST DIV [fetch-error]"
+  mandatory_ancestor: "AMP-LIST"
+  attrs: { name: "align" }
+  attrs: {
+    name: "fetch-error"
+    mandatory: true
+  }
+}
 
 tags: { # amp-list-load-more
   html_format: AMP
-  html_format: EXPERIMENTAL
+  html_format: ACTIONS
   tag_name: "AMP-LIST-LOAD-MORE"
   requires_extension: "amp-list"
   mandatory_parent: "AMP-LIST"
@@ -150,7 +164,7 @@ tags: { # amp-list-load-more
 # The button element variant allowed in amp-list-load-more.
 tags: {
   html_format: AMP
-  html_format: EXPERIMENTAL
+  html_format: ACTIONS
   requires_extension: "amp-list"
   tag_name: "BUTTON"
   spec_name: "amp-list-load-more button[load-more-clickable]"
@@ -187,6 +201,12 @@ tags: {  # <amp-list>
   disallowed_ancestor: "AMP-LIST"
   disallowed_ancestor: "AMP-STATE"
   disallowed_ancestor: "TEMPLATE"
+  attrs: {
+    name: "binding"
+    value: "always"
+    value: "no"
+    value: "refresh"
+  }
   attrs: { name: "items" }
   attrs: { name: "max-items" }
   attrs: { name: "single-item" }
diff --git a/extensions/amp-script/validator-amp-script.protoascii b/extensions/amp-script/validator-amp-script.protoascii
index 59205f343..7e7c62f01 100644
--- a/extensions/amp-script/validator-amp-script.protoascii
+++ b/extensions/amp-script/validator-amp-script.protoascii
@@ -59,7 +59,6 @@ tags: {  # <amp-script>
   tag_name: "AMP-SCRIPT"
   disallowed_ancestor: "AMP-SCRIPT"
   requires_extension: "amp-script"
-  requires: "amp-experiment-token"
   attrs: { name: "sandbox" }
   attrs: {
     name: "script"
@@ -85,3 +84,13 @@ tags: {  # <amp-script>
     supported_layouts: RESPONSIVE
   }
 }
+# HTML5, 4.8.11 The canvas element.
+tags: {
+  html_format: AMP
+  tag_name: "CANVAS"
+  attrs: { name: "height" }
+  attrs: { name: "width" }
+  mandatory_ancestor: "AMP-SCRIPT"
+  requires_extension: "amp-script"
+  spec_url: "https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml/#html-tags"
+}
diff --git a/extensions/amp-sidebar/validator-amp-sidebar.protoascii b/extensions/amp-sidebar/validator-amp-sidebar.protoascii
index eb8d6bec6..c2f39dc37 100644
--- a/extensions/amp-sidebar/validator-amp-sidebar.protoascii
+++ b/extensions/amp-sidebar/validator-amp-sidebar.protoascii
@@ -44,7 +44,6 @@ tags: {  # <amp-sidebar> not in amp-story
   tag_name: "AMP-SIDEBAR"
   # There is an alternate spec for amp-sidebar in amp-story.
   disallowed_ancestor: "AMP-STORY"
-  mandatory_parent: "BODY"
   requires_extension: "amp-sidebar"
   attrs: {
     name: "side"
diff --git a/extensions/amp-video/validator-amp-video.protoascii b/extensions/amp-video/validator-amp-video.protoascii
index e16cf5242..9c41b799c 100644
--- a/extensions/amp-video/validator-amp-video.protoascii
+++ b/extensions/amp-video/validator-amp-video.protoascii
@@ -132,6 +132,29 @@ tags: {  # <amp-video> not in amp-story.
   }
 }
 
+tags: {  # <amp-video> in amp-story-page-attachment (same rules as regular AMPHTML).
+  html_format: AMP
+  html_format: AMP4ADS
+  html_format: ACTIONS
+  tag_name: "AMP-VIDEO"
+  spec_name: "amp-story >> amp-story-page-attachment >> amp-video"
+  mandatory_ancestor: "AMP-STORY-PAGE-ATTACHMENT"
+  also_requires_tag_warning: "amp-video extension .js script"
+  attrs: { name: "poster" }
+  attr_lists: "extended-amp-global"
+  attr_lists: "amp-video-common"
+  attr_lists: "lightboxable-elements"
+  spec_url: "https://amp.dev/documentation/components/amp-video"
+  amp_layout: {
+    supported_layouts: FILL
+    supported_layouts: FIXED
+    supported_layouts: FIXED_HEIGHT
+    supported_layouts: FLEX_ITEM
+    supported_layouts: NODISPLAY
+    supported_layouts: RESPONSIVE
+  }
+}
+
 tags: {  # <amp-video> in amp-story
   html_format: AMP
   html_format: AMP4ADS
@@ -144,6 +167,17 @@ tags: {  # <amp-video> in amp-story
     value: ""
     mandatory: true
   }
+  attrs: {
+    name: "controls"
+    value: ""
+    deprecation: "- no replacement"
+    deprecation_url: "https://github.com/ampproject/amphtml/issues/23798"
+  }
+  attrs: {
+    name: "[controls]"
+    deprecation: "- no replacement"
+    deprecation_url: "https://github.com/ampproject/amphtml/issues/23798"
+  }
   attrs: {
     name: "poster"
     mandatory: true
diff --git a/validator/validator-main.protoascii b/validator/validator-main.protoascii
index cec9e6b2b..aa86c144e 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: 920
+spec_file_revision: 935
 
 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"
@@ -222,7 +222,7 @@ tags: {
         "preload|"  # Handled separately below, has specific requirements.
         "serviceworker|"
         "stylesheet|"  # Handled separately below, has specific requirements.
-        "subresource|"
+        "subresource"
         ")(\\s|$)"
     # It is worth noting that user-authored tags for dns-prefectch, preconnect,
     # prefetch, and prerender will be disabled by the transformations applied
@@ -574,7 +574,9 @@ tags: {
     value_casei: "amp-experiment-token"
     dispatch_key: NAME_VALUE_DISPATCH
   }
-  satisfies: "amp-experiment-token"
+  # Disabled because there are no active origin experiments; uncomment when
+  # there is at least one corresponding `requires` clause.
+  # satisfies: "amp-experiment-token"
 }
 # AMP metadata, name=amp-link-variable-allowed-origin
 # https://github.com/ampproject/amphtml/issues/8132
@@ -659,6 +661,23 @@ tags: {
     dispatch_key: NAME_VALUE_DISPATCH
   }
 }
+# AMP metadata, name=amp-script-src
+tags: {
+  html_format: AMP
+  tag_name: "META"
+  spec_name: "meta name=amp-script-src"
+  mandatory_parent: "HEAD"
+  attrs: {
+    name: "content"
+    mandatory: true
+  }
+  attrs: {
+    name: "name"
+    mandatory: true
+    value_casei: "amp-script-src"
+    dispatch_key: NAME_VALUE_DISPATCH
+  }
+}
 # AMP4ADS metadata, name=amp4ads-id
 # https://github.com/ampproject/amphtml/issues/7730
 tags: {
@@ -1888,7 +1907,7 @@ tags: {
         "prerender|"
         "serviceworker|"
         "stylesheet|"  # Only allowed for link tags, specific req's for AMP.
-        "subresource|"
+        "subresource"
         ")(\\s|$)"
   }
   attrs: {
@@ -2270,6 +2289,9 @@ tags: {
     }
     blacklisted_value_regex: "__amp_source_origin"
   }
+  attrs: { name: "importance" }  # Not yet part of the html spec
+  attrs: { name: "intrinsicsize" }  # Not yet part of the html spec
+  attrs: { name: "loading" }  # Not yet part of the html spec
   attrs: {
     name: "src"
     alternative_names: "srcset"
@@ -3068,12 +3090,16 @@ attr_lists: {
     css_declaration: { name: "text-decoration-color" }
     css_declaration: { name: "text-decoration-line" }
     css_declaration: { name: "text-decoration-style" }
+    css_declaration: { name: "text-fill-color" }
     css_declaration: { name: "text-indent" }
     css_declaration: { name: "text-justify" }
     css_declaration: { name: "text-orientation" }
     css_declaration: { name: "text-overflow" }
     css_declaration: { name: "text-rendering" } # SVG
     css_declaration: { name: "text-shadow" }
+    css_declaration: { name: "text-stroke" }
+    css_declaration: { name: "text-stroke-color" }
+    css_declaration: { name: "text-stroke-width" }
     css_declaration: { name: "text-transform" }
     css_declaration: { name: "text-underline-position" }
     css_declaration: { name: "top" }
@@ -3687,8 +3713,10 @@ tags: {
   tag_name: "SYMBOL"
   mandatory_ancestor: "SVG"
   attrs: { name: "externalresourcesrequired" }
+  attrs: { name: "height" }
   attrs: { name: "preserveaspectratio" }
   attrs: { name: "viewbox" }
+  attrs: { name: "width" }
   attr_lists: "svg-core-attributes"
   attr_lists: "svg-presentation-attributes"
   attr_lists: "svg-style-attr"
@@ -3839,6 +3867,9 @@ tags: {
 # 4.8 Links
 # Links are a concept, rather than a tag. See tagspecs for "LINK", "A", etc.
 
+# 4.8.11 The canvas element
+# See extensions/amp-mustache/validator-amp-script.protoascii
+
 # 4.9 Tabular data
 # 4.9.1 The table element
 tags: {
@@ -4426,6 +4457,7 @@ tags: {
 tags: {
   html_format: AMP
   html_format: AMP4ADS
+  html_format: AMP4EMAIL
   html_format: ACTIONS
   tag_name: "INPUT"
   attrs: {
@@ -4442,11 +4474,12 @@ tags: {
         "password|"
         ")(\\s|$)"
   }
+  attrs: {
+    disabled_by: "amp4email"
+    name: "[type]"
+  }
   attr_lists: "input-common-attr"
   attr_lists: "name-attr"
-  # Note that this amp-bind attribute is not available for the AMP4Email format
-  # thus we add this manually for the formats that should.
-  attrs: { name: "[type]" }
   spec_url: "https://amp.dev/documentation/components/amp-form"
 }
 tags: {
@@ -4464,6 +4497,10 @@ tags: {
     mandatory: true
     dispatch_key: NAME_VALUE_DISPATCH
   }
+  attrs: {
+    disabled_by: "amp4email"
+    name: "[type]"
+  }
   attr_lists: "input-common-attr"
   attr_lists: "name-attr"
   mandatory_ancestor: "FORM [method=POST]"
@@ -4479,27 +4516,13 @@ tags: {
     mandatory: true
     dispatch_key: NAME_VALUE_DISPATCH
   }
-  attr_lists: "input-common-attr"
-  attr_lists: "name-attr"
-  mandatory_ancestor: "FORM [method=POST]"
-  spec_url: "https://amp.dev/documentation/components/amp-form"
-}
-# amp-bind usage of [type] is not allowed in AMP4EMAIL
-tags: {
-  html_format: AMP4EMAIL
-  tag_name: "INPUT"
-  spec_name: "INPUT (AMP4EMAIL)"
   attrs: {
-    name: "type"
-    blacklisted_value_regex: "(^|\\s)("  # Values are space separated.
-        "button|"
-        "file|"
-        "image|"
-        "password|"
-        ")(\\s|$)"
+    disabled_by: "amp4email"
+    name: "[type]"
   }
   attr_lists: "input-common-attr"
   attr_lists: "name-attr"
+  mandatory_ancestor: "FORM [method=POST]"
   spec_url: "https://amp.dev/documentation/components/amp-form"
 }
 # 4.10.6 The button element
@@ -6299,11 +6322,15 @@ attr_lists: {
     css_declaration: { name: "text-decoration-color" }
     css_declaration: { name: "text-decoration-line" }
     css_declaration: { name: "text-decoration-style" }
+    css_declaration: { name: "text-fill-color" }
     css_declaration: { name: "text-indent" }
     css_declaration: { name: "text-justify" }
     css_declaration: { name: "text-orientation" }
     css_declaration: { name: "text-overflow" }
     css_declaration: { name: "text-shadow" }
+    css_declaration: { name: "text-stroke" }
+    css_declaration: { name: "text-stroke-color" }
+    css_declaration: { name: "text-stroke-width" }
     css_declaration: { name: "text-transform" }
     css_declaration: { name: "text-underline-position" }
     css_declaration: { name: "top" }
@@ -6847,6 +6874,14 @@ error_specificity {
   code: DOCUMENT_SIZE_LIMIT_EXCEEDED
   specificity: 120
 }
+error_specificity {
+  code: DEV_MODE_ONLY
+  # This should always trump any other error. It is asserting
+  # that the developer intends for this tag to be an error but
+  # that no additional errors should be reported for this tag.
+  specificity: 1000
+}
+
 # Error formats
 error_formats {
   code: UNKNOWN_CODE
@@ -7311,3 +7346,8 @@ error_formats {
   code: DOCUMENT_SIZE_LIMIT_EXCEEDED
   format: "Document exceeded %1 bytes limit. Actual size %2 bytes."
 }
+error_formats {
+  code: DEV_MODE_ONLY
+  format: "Tag 'html' marked with attribute 'ampdevmode'. Validator "
+          "will suppress errors regarding any other tag with this attribute."
+}

@westonruter westonruter added this to the v1.2.2 milestone Aug 23, 2019
@googlebot googlebot added the cla: yes Signed the Google CLA label Aug 23, 2019
@@ -4796,7 +4812,6 @@ class AMP_Allowed_Tags_Generated {
'disallowed_ancestor' => array(
'amp-story',
),
'mandatory_parent' => 'body',
Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting. No longer does amp-sidebar have to be directly a child of the body element. Now it is merely recommended:

<amp-sidebar> is recommended to be be a direct child of the <body> to preserve a logical DOM order (for accessibility) as well as to avoid altering its behavior by a container element. Note that having an ancestor of amp-sidebar with a set z-index may cause the sidebar to appear below other elements (such as headers), breaking its functionality.

@westonruter westonruter mentioned this pull request Aug 23, 2019
12 tasks
@@ -12197,6 +12374,7 @@ class AMP_Allowed_Tags_Generated {
'requires_usage' => 2,
'version' => array(
'0.1',
'0.2',
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 change is suppressed by c93cb0b.

includes/amp-helper-functions.php Outdated Show resolved Hide resolved
includes/sanitizers/class-amp-base-sanitizer.php Outdated Show resolved Hide resolved
includes/sanitizers/class-amp-base-sanitizer.php Outdated Show resolved Hide resolved
includes/sanitizers/class-amp-img-sanitizer.php Outdated Show resolved Hide resolved
includes/sanitizers/class-amp-img-sanitizer.php Outdated Show resolved Hide resolved
tests/php/test-class-amp-base-sanitizer.php Outdated Show resolved Hide resolved
tests/php/test-tag-and-attribute-sanitizer.php Outdated Show resolved Hide resolved
@@ -13,7 +13,7 @@
*/
class AMP_Allowed_Tags_Generated {

private static $spec_file_revision = 920;
private static $spec_file_revision = 935;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where does this come from? It does not seem to be the spec version this PR refers to ( 1908162134430 ).

This is probably part of the data that is being retrieved when generating this file, but a comment as to where this is mirrored might be useful.

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 comes from the AMP validator spec: https://github.com/ampproject/amphtml/blob/dad372c5c339b07302effbea75c28eae99a7a54e/validator/validator-main.protoascii#L17-L29

As you can see, it's not actually even used anywhere. We could remove it and the $minimum_validator_revision_required variable, as they are private.

@@ -1857,6 +1856,9 @@ class AMP_Allowed_Tags_Generated {
'',
),
),
'slide' => array(
Copy link
Collaborator

Choose a reason for hiding this comment

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

The generator should probably be adapted to use short array syntax as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

True, although we currently ignore the whole file from PHPCS:

<exclude-pattern>includes/sanitizers/class-amp-allowed-tags-generated.php</exclude-pattern>

Also, this may be made irrelevant with #2769, depending on how we decide to store the spec.

@westonruter westonruter removed this from the v1.2.2 milestone Aug 27, 2019
@westonruter westonruter added this to the v1.3 milestone Aug 27, 2019
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 Sanitizers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants