Skip to content

Update allowed tags/attributes from spec in amphtml 1910161528000#3619

Merged
westonruter merged 7 commits intodevelopfrom
update/amphtml-v1910161528000
Oct 27, 2019
Merged

Update allowed tags/attributes from spec in amphtml 1910161528000#3619
westonruter merged 7 commits intodevelopfrom
update/amphtml-v1910161528000

Conversation

@westonruter
Copy link
Copy Markdown
Member

@westonruter westonruter commented Oct 24, 2019

Previously #3239.

  • 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

  • Support inline script for amp-script, eliminating requirement that external scripts be used. Take note of the required meta[name=amp-script-src] tag, but the plugin will automatically merge multiple instances of the meta tag in the head into the one required. A new amp_generate_script_hash() function is provided to aid with generating the sha384 script hashes. See demonstration plugin for usage. ✨
    image
  • Add to list of bindable attributes for amp-audio.
  • Add smoothing attribute to amp-orientation-observer.
  • Add max-age attribute to amp-script.
  • Add amp-experiment story extension .json script. Allow script in amp-story-grid-layer.

Details

(
    PREV_VERSION=1910071803120;
    THIS_VERSION=1910161528000; 
    git checkout $THIS_VERSION;
    git diff $PREV_VERSION...$THIS_VERSION -w -- $( git ls-files | grep '.protoascii' );
    git checkout - > /dev/null
)
Compare 1910071803120..1910161528000
diff --git a/extensions/amp-accordion/validator-amp-accordion.protoascii b/extensions/amp-accordion/validator-amp-accordion.protoascii
index 66eea7306..c646ff1a8 100644
--- a/extensions/amp-accordion/validator-amp-accordion.protoascii
+++ b/extensions/amp-accordion/validator-amp-accordion.protoascii
@@ -16,7 +16,6 @@
 tags: {  # amp-accordion
   html_format: AMP
   html_format: AMP4ADS
-  html_format: AMP4EMAIL
   html_format: ACTIONS
   tag_name: "SCRIPT"
   extension_spec: {
@@ -28,6 +27,19 @@ tags: {  # amp-accordion
   }
   attr_lists: "common-extension-attrs"
 }
+tags: {  # amp-accordion
+  html_format: AMP4EMAIL
+  tag_name: "SCRIPT"
+  spec_name: "SCRIPT[custom-element=amp-accordion] (AMP4EMAIL)"
+  extension_spec: {
+    name: "amp-accordion"
+    # AMP4EMAIL doesn't allow version: "latest".
+    version: "0.1"
+    requires_usage: GRANDFATHERED
+    deprecated_allow_duplicates: true
+  }
+  attr_lists: "common-extension-attrs"
+}
 tags: {  # <amp-accordion>
   html_format: AMP
   html_format: AMP4ADS
diff --git a/extensions/amp-anim/validator-amp-anim.protoascii b/extensions/amp-anim/validator-amp-anim.protoascii
index bdbdfb320..56cbfae06 100644
--- a/extensions/amp-anim/validator-amp-anim.protoascii
+++ b/extensions/amp-anim/validator-amp-anim.protoascii
@@ -34,8 +34,8 @@ tags: {  # amp-anim
   spec_name: "amp-anim extension .js script (AMP4EMAIL)"
   extension_spec: {
     name: "amp-anim"
+    # AMP4EMAIL doesn't allow version: "latest".
     version: "0.1"
-    version: "latest"
   }
   attr_lists: "common-extension-attrs"
 }
diff --git a/extensions/amp-audio/validator-amp-audio.protoascii b/extensions/amp-audio/validator-amp-audio.protoascii
index 424c2fe84..8592a2f64 100644
--- a/extensions/amp-audio/validator-amp-audio.protoascii
+++ b/extensions/amp-audio/validator-amp-audio.protoascii
@@ -43,6 +43,14 @@ attr_lists: { # amp-audio attributes that are common to both AMP and A4A format.
     }
     blacklisted_value_regex: "__amp_source_origin"
   }
+  # <amp-bind>
+  attrs: { name: "[album]" }
+  attrs: { name: "[artist]" }
+  attrs: { name: "[artwork]" }
+  attrs: { name: "[controlslist]" }
+  attrs: { name: "[loop]" }
+  attrs: { name: "[src]" }
+  attrs: { name: "[title]" }
 }
 tags: {  # <amp-audio> for AMP (autoplay attribute allowed)
   html_format: AMP
diff --git a/extensions/amp-bind/validator-amp-bind.protoascii b/extensions/amp-bind/validator-amp-bind.protoascii
index 9dfa91387..67b4208d1 100644
--- a/extensions/amp-bind/validator-amp-bind.protoascii
+++ b/extensions/amp-bind/validator-amp-bind.protoascii
@@ -16,7 +16,6 @@
 tags: {  # amp-bind
   html_format: AMP
   html_format: AMP4ADS
-  html_format: AMP4EMAIL
   html_format: ACTIONS
   tag_name: "SCRIPT"
   extension_spec: {
@@ -30,6 +29,21 @@ tags: {  # amp-bind
   }
   attr_lists: "common-extension-attrs"
 }
+tags: {  # amp-bind
+  html_format: AMP4EMAIL
+  tag_name: "SCRIPT"
+  spec_name: "SCRIPT[custom-element=amp-bind] (AMP4EMAIL)"
+  extension_spec: {
+    name: "amp-bind"
+    # AMP4EMAIL doesn't allow version: "latest"
+    version: "0.1"
+    # amp-bind has no associated tag which indicates usage of the extension.
+    # TODO(gregable): Implement a mechanism to associate attributes with
+    # extension usage and then set this to GRANDFATHERED or ERROR.
+    requires_usage: NONE
+  }
+  attr_lists: "common-extension-attrs"
+}
 tags: {  # <amp-state> (json)
   html_format: AMP
   html_format: AMP4ADS
diff --git a/extensions/amp-carousel/validator-amp-carousel.protoascii b/extensions/amp-carousel/validator-amp-carousel.protoascii
index b29a9b5e5..11b1e55ab 100644
--- a/extensions/amp-carousel/validator-amp-carousel.protoascii
+++ b/extensions/amp-carousel/validator-amp-carousel.protoascii
@@ -35,6 +35,7 @@ tags: {  # amp-carousel for AMP4EMAIL
   spec_name: "SCRIPT[custom-element=amp-carousel] (AMP4EMAIL)"
   extension_spec: {
     name: "amp-carousel"
+    # AMP4EMAIL doesn't allow version: "latest".
     version: "0.1"
   }
   attr_lists: "common-extension-attrs"
diff --git a/extensions/amp-fit-text/validator-amp-fit-text.protoascii b/extensions/amp-fit-text/validator-amp-fit-text.protoascii
index e84f723a0..c29376537 100644
--- a/extensions/amp-fit-text/validator-amp-fit-text.protoascii
+++ b/extensions/amp-fit-text/validator-amp-fit-text.protoascii
@@ -29,6 +29,22 @@ tags: {  # amp-fit-text
   }
   attr_lists: "common-extension-attrs"
 }
+tags: {  # amp-fit-text
+  html_format: AMP
+  html_format: AMP4ADS
+  html_format: AMP4EMAIL
+  html_format: ACTIONS
+  tag_name: "SCRIPT"
+  spec_name: "SCRIPT[custom-element=amp-fit-text] (AMP4EMAIL)"
+  extension_spec: {
+    name: "amp-fit-text"
+    # AMP4EMAIL doesn't allow version: "latest".
+    version: "0.1"
+    requires_usage: GRANDFATHERED
+    deprecated_allow_duplicates: true
+  }
+  attr_lists: "common-extension-attrs"
+}
 tags: {  # <amp-fit-text>
   html_format: AMP
   html_format: AMP4ADS
diff --git a/extensions/amp-form/validator-amp-form.protoascii b/extensions/amp-form/validator-amp-form.protoascii
index 74fc7e60d..e06e18b78 100644
--- a/extensions/amp-form/validator-amp-form.protoascii
+++ b/extensions/amp-form/validator-amp-form.protoascii
@@ -18,7 +18,6 @@ tags: {  # amp-form
   # under section "4.10 Forms"
   html_format: AMP
   html_format: AMP4ADS
-  html_format: AMP4EMAIL
   html_format: ACTIONS
   tag_name: "SCRIPT"
   extension_spec: {
@@ -30,3 +29,16 @@ tags: {  # amp-form
   }
   attr_lists: "common-extension-attrs"
 }
+tags: {  # amp-form
+  html_format: AMP4EMAIL
+  tag_name: "SCRIPT"
+  spec_name: "SCRIPT[custom-element=amp-form] (AMP4EMAIL)"
+  extension_spec: {
+    name: "amp-form"
+    # AMP4EMAIL doesn't allow version: "latest".
+    version: "0.1"
+    deprecated_allow_duplicates: true
+    requires_usage: GRANDFATHERED
+  }
+  attr_lists: "common-extension-attrs"
+}
diff --git a/extensions/amp-list/validator-amp-list.protoascii b/extensions/amp-list/validator-amp-list.protoascii
index f41c4446b..267713a0f 100644
--- a/extensions/amp-list/validator-amp-list.protoascii
+++ b/extensions/amp-list/validator-amp-list.protoascii
@@ -31,8 +31,8 @@ tags: {  # amp-list
   spec_name: "SCRIPT[custom-element=amp-list] (AMP4EMAIL)"
   extension_spec: {
     name: "amp-list"
+    # AMP4EMAIL doesn't allow version: "latest".
     version: "0.1"
-    version: "latest"
   }
   attr_lists: "common-extension-attrs"
 }
@@ -212,6 +212,10 @@ tags: {  # <amp-list>
     value: "no"
     value: "refresh"
   }
+  attrs: {
+    name: "diffable"
+    value: ""
+  }
   attrs: { name: "items" }
   attrs: { name: "max-items" }
   attrs: { name: "single-item" }
@@ -262,6 +266,10 @@ tags: {  # <amp-list>
     name: "crossorigin"
     value: "amp-viewer-auth-token-via-post"
   }
+  attrs: {
+    name: "diffable"
+    value: ""
+  }
   attrs: { name: "items" }
   attrs: { name: "max-items" }
   attrs: {
diff --git a/extensions/amp-mustache/validator-amp-mustache.protoascii b/extensions/amp-mustache/validator-amp-mustache.protoascii
index dc2acc1e5..8f310f62d 100644
--- a/extensions/amp-mustache/validator-amp-mustache.protoascii
+++ b/extensions/amp-mustache/validator-amp-mustache.protoascii
@@ -36,10 +36,9 @@ tags: {  # amp-mustache
 }
 tags: {  # amp-mustache
   html_format: AMP4ADS
-  html_format: AMP4EMAIL
   html_format: ACTIONS
   tag_name: "SCRIPT"
-  spec_name: "SCRIPT[custom-element=amp-mustache] (AMP4ADS/AMP4EMAIL)"
+  spec_name: "SCRIPT[custom-template=amp-mustache] (AMP4ADS/ACTIONS)"
   extension_spec: {
     name: "amp-mustache"
     extension_type: CUSTOM_TEMPLATE
@@ -50,7 +49,21 @@ tags: {  # amp-mustache
   }
   attr_lists: "common-extension-attrs"
 }
-# The script element.
+tags: {  # amp-mustache
+  html_format: AMP4EMAIL
+  tag_name: "SCRIPT"
+  spec_name: "SCRIPT[custom-template=amp-mustache] (AMP4EMAIL)"
+  extension_spec: {
+    name: "amp-mustache"
+    extension_type: CUSTOM_TEMPLATE
+    # AMP4EMAIL doesn't allow version: "latest".
+    version: "0.1"
+    version: "0.2"
+    deprecated_version: "0.1"
+  }
+  attr_lists: "common-extension-attrs"
+}
+# Templates using script[type=text/plain].
 tags: {
   html_format: AMP
   html_format: AMP4ADS
diff --git a/extensions/amp-orientation-observer/validator-amp-orientation-observer.protoascii b/extensions/amp-orientation-observer/validator-amp-orientation-observer.protoascii
index 3c4579bc4..ff0ffcd0a 100644
--- a/extensions/amp-orientation-observer/validator-amp-orientation-observer.protoascii
+++ b/extensions/amp-orientation-observer/validator-amp-orientation-observer.protoascii
@@ -49,6 +49,10 @@ tags: {  # <amp-orientation-observer>
     # Values such as: "100 100", "0 100", etc..
     value_regex: "(\\d+)\\s{1}(\\d+)"
   }
+  attrs: {
+    name: "smoothing"
+    value_regex: "[0-9]+|"
+  }
   amp_layout {
     supported_layouts: NODISPLAY
   }
diff --git a/extensions/amp-script/validator-amp-script.protoascii b/extensions/amp-script/validator-amp-script.protoascii
index bf05ad10c..c0f6d5c69 100644
--- a/extensions/amp-script/validator-amp-script.protoascii
+++ b/extensions/amp-script/validator-amp-script.protoascii
@@ -24,45 +24,176 @@ 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> (local script)
+  html_format: AMP
+  tag_name: "SCRIPT"
+  spec_name: "amp-script extension local script"
+  requires_extension: "amp-script"
+  attrs: {
+    name: "id"
+    add_value_to_set: AMP_SCRIPT_IDS
+    mandatory: true
+    blacklisted_value_regex: "(^|\\s)("  # Values are space separated
+        "__amp_\\S*|"
+        "__count__|"
+        "__defineGetter__|"
+        "__defineSetter__|"
+        "__lookupGetter__|"
+        "__lookupSetter__|"
+        "__noSuchMethod__|"
+        "__parent__|"
+        "__proto__|"
+        "__AMP_\\S*|"
+        "\\$p|"
+        "\\$proxy|"
+        "acceptCharset|"
+        "addEventListener|"
+        "appendChild|"
+        "assignedSlot|"
+        "attachShadow|"
+        "AMP|"
+        "baseURI|"
+        "checkValidity|"
+        "childElementCount|"
+        "childNodes|"
+        "classList|"
+        "className|"
+        "clientHeight|"
+        "clientLeft|"
+        "clientTop|"
+        "clientWidth|"
+        "compareDocumentPosition|"
+        "computedName|"
+        "computedRole|"
+        "contentEditable|"
+        "createShadowRoot|"
+        "enqueAction|"
+        "firstChild|"
+        "firstElementChild|"
+        "getAnimations|"
+        "getAttribute|"
+        "getAttributeNS|"
+        "getAttributeNode|"
+        "getAttributeNodeNS|"
+        "getBoundingClientRect|"
+        "getClientRects|"
+        "getDestinationInsertionPoints|"
+        "getElementsByClassName|"
+        "getElementsByTagName|"
+        "getElementsByTagNameNS|"
+        "getRootNode|"
+        "hasAttribute|"
+        "hasAttributeNS|"
+        "hasAttributes|"
+        "hasChildNodes|"
+        "hasPointerCapture|"
+        "i-amphtml-\\S*|"
+        "innerHTML|"
+        "innerText|"
+        "inputMode|"
+        "insertAdjacentElement|"
+        "insertAdjacentHTML|"
+        "insertAdjacentText|"
+        "isContentEditable|"
+        "isDefaultNamespace|"
+        "isEqualNode|"
+        "isSameNode|"
+        "lastChild|"
+        "lastElementChild|"
+        "lookupNamespaceURI|"
+        "namespaceURI|"
+        "nextElementSibling|"
+        "nextSibling|"
+        "nodeName|"
+        "nodeType|"
+        "nodeValue|"
+        "offsetHeight|"
+        "offsetLeft|"
+        "offsetParent|"
+        "offsetTop|"
+        "offsetWidth|"
+        "outerHTML|"
+        "outerText|"
+        "ownerDocument|"
+        "parentElement|"
+        "parentNode|"
+        "previousElementSibling|"
+        "previousSibling|"
+        "querySelector|"
+        "querySelectorAll|"
+        "releasePointerCapture|"
+        "removeAttribute|"
+        "removeAttributeNS|"
+        "removeAttributeNode|"
+        "removeChild|"
+        "removeEventListener|"
+        "replaceChild|"
+        "reportValidity|"
+        "requestPointerLock|"
+        "scrollHeight|"
+        "scrollIntoView|"
+        "scrollIntoViewIfNeeded|"
+        "scrollLeft|"
+        "scrollWidth|"
+        "setAttribute|"
+        "setAttributeNS|"
+        "setAttributeNode|"
+        "setAttributeNodeNS|"
+        "setPointerCapture|"
+        "shadowRoot|"
+        "styleMap|"
+        "tabIndex|"
+        "tagName|"
+        "textContent|"
+        "toString|"
+        "valueOf|"
+        "(webkit|ms|moz|o)dropzone|"
+        "(webkit|moz|ms|o)MatchesSelector|"
+        "(webkit|moz|ms|o)RequestFullScreen|"
+        "(webkit|moz|ms|o)RequestFullscreen"
+        ")(\\s|$)"
+  }
+  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"
+  attrs: {
+    name: "max-age"
+    value_regex: "[0-9]+"
+    trigger: {
+      # max-age only applies to inline scripts.
+      also_requires_attr: "script"
+    }
+  }
   attrs: { name: "sandbox" }
   attrs: {
     name: "script"
     mandatory_oneof: "['script', 'src']"
+    value_oneof_set: AMP_SCRIPT_IDS
   }
   attrs: {
     name: "src"
diff --git a/extensions/amp-selector/validator-amp-selector.protoascii b/extensions/amp-selector/validator-amp-selector.protoascii
index bb387392d..82d7e6b2b 100644
--- a/extensions/amp-selector/validator-amp-selector.protoascii
+++ b/extensions/amp-selector/validator-amp-selector.protoascii
@@ -32,8 +32,8 @@ tags: {
   spec_name: "SCRIPT[custom-element=amp-selector] (AMP4EMAIL)"
   extension_spec: {
     name: "amp-selector"
+    # AMP4EMAIL doesn't allow version: "latest".
     version: "0.1"
-    version: "latest"
     requires_usage: GRANDFATHERED
   }
   attr_lists: "common-extension-attrs"
diff --git a/extensions/amp-sidebar/validator-amp-sidebar.protoascii b/extensions/amp-sidebar/validator-amp-sidebar.protoascii
index c2f39dc37..b4e5f7605 100644
--- a/extensions/amp-sidebar/validator-amp-sidebar.protoascii
+++ b/extensions/amp-sidebar/validator-amp-sidebar.protoascii
@@ -32,8 +32,8 @@ tags: {  # amp-sidebar
   tag_name: "SCRIPT"
   extension_spec: {
     name: "amp-sidebar"
+    # AMP4EMAIL doesn't allow version: "latest".
     version: "0.1"
-    version: "latest"
   }
   attr_lists: "common-extension-attrs"
 }
diff --git a/extensions/amp-story/validator-amp-story.protoascii b/extensions/amp-story/validator-amp-story.protoascii
index d798ffdac..94c57912f 100644
--- a/extensions/amp-story/validator-amp-story.protoascii
+++ b/extensions/amp-story/validator-amp-story.protoascii
@@ -442,6 +442,29 @@ tags: {  # <amp-story-cta-layer>
     tag_spec_name: "AMP-STORY-CTA-LAYER animate-in"
   }
 }
+tags: {  # amp-experiment (json)
+  html_format: AMP
+  html_format: ACTIONS
+  tag_name: "SCRIPT"
+  spec_name: "amp-experiment story extension .json script"
+  mandatory_parent: "AMP-EXPERIMENT"
+  attrs: {
+    name: "type"
+    mandatory: true
+    value_casei: "application/json"
+    dispatch_key: NAME_VALUE_PARENT_DISPATCH
+  }
+  attr_lists: "nonce-attr"
+  cdata: {
+    max_bytes: 15000
+    max_bytes_spec_url: "https://amp.dev/documentation/components/amp-experiment#configuration"
+    blacklisted_cdata_regex: {
+      regex: "<!--"
+      error_message: "html comments"
+    }
+  }
+  spec_url: "https://amp.dev/documentation/components/amp-experiment"
+}
 tags: {
   html_format: AMP
   tag_name: "$REFERENCE_POINT"
@@ -704,6 +727,7 @@ descendant_tag_list: {
   tag: "RUBY"
   tag: "S"
   tag: "SAMP"
+  tag: "SCRIPT"
   tag: "SECTION"
   tag: "SMALL"
   tag: "SOLIDCOLOR"
diff --git a/extensions/amp-timeago/validator-amp-timeago.protoascii b/extensions/amp-timeago/validator-amp-timeago.protoascii
index 6568d6a4e..f80feff41 100644
--- a/extensions/amp-timeago/validator-amp-timeago.protoascii
+++ b/extensions/amp-timeago/validator-amp-timeago.protoascii
@@ -16,7 +16,6 @@
 
 tags: {  # amp-timeago
   html_format: AMP
-  html_format: AMP4EMAIL
   html_format: ACTIONS
   tag_name: "SCRIPT"
   extension_spec: {
@@ -26,6 +25,17 @@ tags: {  # amp-timeago
   }
   attr_lists: "common-extension-attrs"
 }
+tags: {  # amp-timeago
+  html_format: AMP4EMAIL
+  tag_name: "SCRIPT"
+  spec_name: "SCRIPT[custom-element=amp-timeago] (AMP4EMAIL)"
+  extension_spec: {
+    name: "amp-timeago"
+    # AMP4EMAIL doesn't allow version: "latest".
+    version: "0.1"
+  }
+  attr_lists: "common-extension-attrs"
+}
 tags: {  # <amp-timeago>
   html_format: AMP
   html_format: AMP4EMAIL
diff --git a/extensions/amp-youtube/validator-amp-youtube.protoascii b/extensions/amp-youtube/validator-amp-youtube.protoascii
index 5d3f90bc5..b87d55b5f 100644
--- a/extensions/amp-youtube/validator-amp-youtube.protoascii
+++ b/extensions/amp-youtube/validator-amp-youtube.protoascii
@@ -16,7 +16,6 @@
 
 tags: {  # amp-youtube
   html_format: AMP
-  html_format: AMP4ADS
   html_format: ACTIONS
   tag_name: "SCRIPT"
   extension_spec: {
@@ -30,7 +29,6 @@ tags: {  # amp-youtube
 }
 tags: {  # <amp-youtube>
   html_format: AMP
-  html_format: AMP4ADS
   html_format: ACTIONS
   tag_name: "AMP-YOUTUBE"
   requires_extension: "amp-youtube"
diff --git a/validator/validator-main.protoascii b/validator/validator-main.protoascii
index aaa099d82..f9b75ec6e 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: 957
+spec_file_revision: 961
 
 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"
@@ -5379,7 +5379,7 @@ attr_lists: {
     name: "id"
     mandatory: true
     # When updating this blacklisted_value_regex, also update the entry for
-    # "id" in $GLOBAL_ATTRS.
+    # "id" in $GLOBAL_ATTRS and "id" in "amp-script extension local script".
     blacklisted_value_regex: "(^|\\s)("  # Values are space separated
         "__amp_\\S*|"
         "__count__|"

@westonruter westonruter added this to the v1.4 milestone Oct 24, 2019
@googlebot googlebot added the cla: yes Signed the Google CLA label Oct 24, 2019
@westonruter westonruter force-pushed the update/amphtml-v1910161528000 branch from 9d0abc1 to d0b13f1 Compare October 25, 2019 00:40
@westonruter westonruter marked this pull request as ready for review October 25, 2019 00:51
@amedina amedina self-requested a review October 26, 2019 04:24
Copy link
Copy Markdown
Member

@amedina amedina left a comment

Choose a reason for hiding this comment

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

Ship it.

// Merge meta amp-script-src elements.
$first_meta_amp_script_src = array_shift( $meta_amp_script_srcs );
foreach ( $meta_amp_script_srcs as $meta_amp_script_src ) {
$first_meta_amp_script_src->setAttribute(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Seems wasteful to constantly rewrite this attribute (although I ignore how much processing happens in the DOM abtraction). Would make more sense to collect and merge as a variable, and then write (i.e. setAttribute()) only once.

Also, storing the first element, and then iterating over the rest is a rather arbitrary logic. Using array_reduce() would be conceptually clearer.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, I think calling setAttribute multiple times may not be a concern like it would be in the browser, since there are no layout calculations being performed. Nevertheless, minimizing writes and redundant string concatenations seems beneficial. To that end, I think actually array_reduce() isn't the best fit here because it would do more concatenation operations than required, while all we need to do is push onto a single copy of an array. At the same time, we also need to remove the elements from the DOM, and that doesn't seem super clean to do in an array_reduce().

Please take a look at 03d587a.

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.

4 participants