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 spec to dc6cd22a52 #6530

Merged
merged 7 commits into from
Aug 12, 2021
Merged

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Aug 11, 2021

Previously #6436.

  • Run ./bin/amphtml-update.sh (lando ssh -c 'bash ./bin/amphtml-update.sh vendor/amphtml').
  • Examine diff for changelog.
  • Examine upstream diff to ensure nothing was missed.
  • 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

Details

(
    PREV_VERSION=ed0504b;
    THIS_VERSION=dc6cd22a52;
    git checkout $THIS_VERSION;
    git diff $PREV_VERSION...$THIS_VERSION -w -- $( git ls-files | grep '.protoascii' );
    git checkout - > /dev/null
)
diff --git a/extensions/amp-base-carousel/validator-amp-base-carousel.protoascii b/extensions/amp-base-carousel/validator-amp-base-carousel.protoascii
index 6c80f07f78..0e6529ac48 100644
--- a/extensions/amp-base-carousel/validator-amp-base-carousel.protoascii
+++ b/extensions/amp-base-carousel/validator-amp-base-carousel.protoascii
@@ -70,7 +70,7 @@ attr_lists: {
   attrs: {
     name: "loop"
     # Media query / boolean pairs
-    value_regex: "([^,]+\\s+(true|false),\\s*)*(true|false)"
+    value_regex: "([^,]+\\s+(true|false),\\s*)*(true|false|^$)"
   }
   attrs: {
     name: "mixed-length"
diff --git a/extensions/amp-brightcove/validator-amp-brightcove.protoascii b/extensions/amp-brightcove/validator-amp-brightcove.protoascii
index e38295a94a..225f4f9813 100644
--- a/extensions/amp-brightcove/validator-amp-brightcove.protoascii
+++ b/extensions/amp-brightcove/validator-amp-brightcove.protoascii
@@ -14,11 +14,26 @@
 # limitations under the license.
 #
 
-tags: {  # amp-brightcove
+tags: {  # amp-brightcove 1.0
   html_format: AMP
   tag_name: "SCRIPT"
+  satisfies: "amp-brightcove 1.0"
+  excludes: "amp-brightcove 0.1"
   extension_spec: {
     name: "amp-brightcove"
+    version_name: "v1.0"
+    version: "1.0"
+  }
+  attr_lists: "common-extension-attrs"
+}
+tags: {  # amp-brightcove 0.1 and latest
+  html_format: AMP
+  tag_name: "SCRIPT"
+  satisfies: "amp-brightcove 0.1"
+  excludes: "amp-brightcove 1.0"
+  extension_spec: {
+    name: "amp-brightcove"
+    version_name: "v0.1"
     version: "0.1"
     version: "latest"
     requires_usage: EXEMPTED
diff --git a/extensions/amp-facebook-comments/validator-amp-facebook-comments.protoascii b/extensions/amp-facebook-comments/validator-amp-facebook-comments.protoascii
index d38cee1698..2e98c02721 100644
--- a/extensions/amp-facebook-comments/validator-amp-facebook-comments.protoascii
+++ b/extensions/amp-facebook-comments/validator-amp-facebook-comments.protoascii
@@ -17,6 +17,8 @@
 tags: {  # amp-facebook-comments
   html_format: AMP
   tag_name: "SCRIPT"
+  satisfies: "amp-facebook-comments 0.1"
+  excludes: "amp-facebook 1.0"
   extension_spec: {
     name: "amp-facebook-comments"
     version: "0.1"
@@ -28,11 +30,9 @@ tags: {  # <amp-facebook-comments>
   html_format: AMP
   tag_name: "AMP-FACEBOOK-COMMENTS"
   requires_extension: "amp-facebook-comments"
-  # data-* is generally allowed, but it's not generally mandatory.
-  attrs: {
-    name: "data-href"
-    mandatory: true
-  }
+  requires: "amp-facebook-comments 0.1"
+  # The amp-facebook attr_list is defined in the amp-facebook protoascii
+  attr_lists: "amp-facebook"
   attr_lists: "extended-amp-global"
   amp_layout: {
     supported_layouts: FILL
diff --git a/extensions/amp-facebook-like/validator-amp-facebook-like.protoascii b/extensions/amp-facebook-like/validator-amp-facebook-like.protoascii
index eee4f29d7b..214109856b 100644
--- a/extensions/amp-facebook-like/validator-amp-facebook-like.protoascii
+++ b/extensions/amp-facebook-like/validator-amp-facebook-like.protoascii
@@ -17,6 +17,8 @@
 tags: {  # amp-facebook-like
   html_format: AMP
   tag_name: "SCRIPT"
+  satisfies: "amp-facebook-like 0.1"
+  excludes: "amp-facebook 1.0"
   extension_spec: {
     name: "amp-facebook-like"
     version: "0.1"
@@ -28,16 +30,9 @@ tags: {  # <amp-facebook-like>
   html_format: AMP
   tag_name: "AMP-FACEBOOK-LIKE"
   requires_extension: "amp-facebook-like"
-  # data-* is generally allowed, but it's not generally mandatory.
-  attrs: {
-    name: "data-href"
-    mandatory: true
-    value_url: {
-      protocol: "http"
-      protocol: "https"
-      allow_relative: false
-    }
-  }
+  requires: "amp-facebook-like 0.1"
+  # The amp-facebook-strict attr_list is defined in the amp-facebook protoascii
+  attr_lists: "amp-facebook-strict"
   attr_lists: "extended-amp-global"
   amp_layout: {
     supported_layouts: FILL
diff --git a/extensions/amp-facebook-page/validator-amp-facebook-page.protoascii b/extensions/amp-facebook-page/validator-amp-facebook-page.protoascii
index 046b4e4659..70719c57ba 100644
--- a/extensions/amp-facebook-page/validator-amp-facebook-page.protoascii
+++ b/extensions/amp-facebook-page/validator-amp-facebook-page.protoascii
@@ -17,6 +17,8 @@
 tags: {  # amp-facebook-page
   html_format: AMP
   tag_name: "SCRIPT"
+  satisfies: "amp-facebook-page 0.1"
+  excludes: "amp-facebook 1.0"
   extension_spec: {
     name: "amp-facebook-page"
     version: "0.1"
@@ -28,16 +30,9 @@ tags: {  # <amp-facebook-page>
   html_format: AMP
   tag_name: "AMP-FACEBOOK-PAGE"
   requires_extension: "amp-facebook-page"
-  # data-* is generally allowed, but it's not generally mandatory.
-  attrs: {
-    name: "data-href"
-    mandatory: true
-    value_url: {
-      protocol: "http"
-      protocol: "https"
-      allow_relative: false
-    }
-  }
+  requires: "amp-facebook-page 0.1"
+  # The amp-facebook-strict attr_list is defined in the amp-facebook protoascii
+  attr_lists: "amp-facebook-strict"
   attr_lists: "extended-amp-global"
   amp_layout: {
     supported_layouts: FILL
diff --git a/extensions/amp-facebook/validator-amp-facebook.protoascii b/extensions/amp-facebook/validator-amp-facebook.protoascii
index 100c4e8ada..526fcdaa5a 100644
--- a/extensions/amp-facebook/validator-amp-facebook.protoascii
+++ b/extensions/amp-facebook/validator-amp-facebook.protoascii
@@ -14,11 +14,33 @@
 # limitations under the license.
 #
 
-tags: {  # amp-facebook
+
+tags: {  # amp-facebook 1.0
+  html_format: AMP
+  tag_name: "SCRIPT"
+  # The 1.0 amp-facebook script registers all amp-facebook-* components.
+  # See https://github.com/ampproject/amphtml/pull/35395 for context.
+  spec_name: "amp-facebook 1.0"
+  satisfies: "amp-facebook 1.0"
+  excludes: "amp-facebook 0.1"
+  excludes: "amp-facebook-comments 0.1"
+  excludes: "amp-facebook-like 0.1"
+  excludes: "amp-facebook-page 0.1"
+  extension_spec: {
+    name: "amp-facebook"
+    version_name: "v1.0"
+    version: "1.0"
+  }
+  attr_lists: "common-extension-attrs"
+}
+tags: {  # amp-facebook 0.1 and latest
   html_format: AMP
   tag_name: "SCRIPT"
+  satisfies: "amp-facebook 0.1"
+  excludes: "amp-facebook 1.0"
   extension_spec: {
     name: "amp-facebook"
+    version_name: "v0.1"
     version: "0.1"
     version: "latest"
     requires_usage: EXEMPTED
@@ -30,11 +52,24 @@ tags: {  # <amp-facebook>
   html_format: AMP
   tag_name: "AMP-FACEBOOK"
   requires_extension: "amp-facebook"
-  # data-* is generally allowed, but it's not generally mandatory.
-  attrs: {
-    name: "data-href"
-    mandatory: true
+  attr_lists: "amp-facebook"
+  attr_lists: "extended-amp-global"
+  amp_layout: {
+    supported_layouts: FILL
+    supported_layouts: FIXED
+    supported_layouts: FIXED_HEIGHT
+    supported_layouts: FLEX_ITEM
+    supported_layouts: NODISPLAY
+    supported_layouts: RESPONSIVE
   }
+}
+tags: {  # <amp-facebook-comments> via amp-facebook script
+  html_format: AMP
+  spec_name: "amp-facebook-comments 1.0"
+  tag_name: "AMP-FACEBOOK-COMMENTS"
+  requires_extension: "amp-facebook"
+  requires: "amp-facebook 1.0"
+  attr_lists: "amp-facebook"
   attr_lists: "extended-amp-global"
   amp_layout: {
     supported_layouts: FILL
@@ -45,3 +80,58 @@ tags: {  # <amp-facebook>
     supported_layouts: RESPONSIVE
   }
 }
+tags: {  # <amp-facebook-like> via amp-facebook script
+  html_format: AMP
+  spec_name: "amp-facebook-like 1.0"
+  tag_name: "AMP-FACEBOOK-LIKE"
+  requires_extension: "amp-facebook"
+  requires: "amp-facebook 1.0"
+  attr_lists: "amp-facebook-strict"
+  attr_lists: "extended-amp-global"
+  amp_layout: {
+    supported_layouts: FILL
+    supported_layouts: FIXED
+    supported_layouts: FIXED_HEIGHT
+    supported_layouts: FLEX_ITEM
+    supported_layouts: NODISPLAY
+    supported_layouts: RESPONSIVE
+  }
+}
+tags: {  # <amp-facebook-page> via amp-facebook script
+  html_format: AMP
+  spec_name: "amp-facebook-page 1.0"
+  tag_name: "AMP-FACEBOOK-PAGE"
+  requires_extension: "amp-facebook"
+  requires: "amp-facebook 1.0"
+  attr_lists: "amp-facebook-strict"
+  attr_lists: "extended-amp-global"
+  amp_layout: {
+    supported_layouts: FILL
+    supported_layouts: FIXED
+    supported_layouts: FIXED_HEIGHT
+    supported_layouts: FLEX_ITEM
+    supported_layouts: NODISPLAY
+    supported_layouts: RESPONSIVE
+  }
+}
+attr_lists: {
+  name: "amp-facebook"
+  # data-* is generally allowed, but it's not generally mandatory.
+  attrs: {
+    name: "data-href"
+    mandatory: true
+  }
+}
+attr_lists: {
+  name: "amp-facebook-strict"
+  # data-* is generally allowed, but it's not generally mandatory.
+  attrs: {
+    name: "data-href"
+    mandatory: true
+    value_url: {
+      protocol: "http"
+      protocol: "https"
+      allow_relative: false
+    }
+  }
+}
diff --git a/extensions/amp-inline-gallery/validator-amp-inline-gallery.protoascii b/extensions/amp-inline-gallery/validator-amp-inline-gallery.protoascii
index 088a3f8fe9..ac921ea96b 100644
--- a/extensions/amp-inline-gallery/validator-amp-inline-gallery.protoascii
+++ b/extensions/amp-inline-gallery/validator-amp-inline-gallery.protoascii
@@ -101,6 +101,7 @@ tags: {  # <amp-inline-gallery-thumbnails>
     name: "loop"
     value: "true"
     value: "false"
+    value: ""
   }
   attr_lists: "extended-amp-global"
   spec_url: "https://amp.dev/documentation/components/amp-inline-gallery/"
diff --git a/extensions/amp-story/validator-amp-story.protoascii b/extensions/amp-story/validator-amp-story.protoascii
index c71289deb3..ccfd025ecb 100644
--- a/extensions/amp-story/validator-amp-story.protoascii
+++ b/extensions/amp-story/validator-amp-story.protoascii
@@ -716,6 +716,7 @@ descendant_tag_list: {
   tag: "AMP-LIST"
   tag: "AMP-LIVE-LIST"
   tag: "AMP-PIXEL"
+  tag: "AMP-RENDER"
   tag: "AMP-STATE"
   tag: "AMP-STORY-360"
   tag: "AMP-STORY-AUTO-ANALYTICS"
@@ -1090,6 +1091,7 @@ descendant_tag_list {
   tag: "AMP-POWR-PLAYER"
   tag: "AMP-REACH-PLAYER"
   tag: "AMP-REDDIT"
+  tag: "AMP-RENDER"
   tag: "AMP-RIDDLE-QUIZ"
   tag: "AMP-SOUNDCLOUD"
   tag: "AMP-SPRINGBOARD-PLAYER"
@@ -1201,6 +1203,7 @@ descendant_tag_list {
   tag: "TABLE"
   tag: "TBODY"
   tag: "TD"
+  tag: "TEMPLATE"
   tag: "TEXT"
   tag: "TEXTPATH"
   tag: "TFOOT"
diff --git a/extensions/amp-stream-gallery/validator-amp-stream-gallery.protoascii b/extensions/amp-stream-gallery/validator-amp-stream-gallery.protoascii
index 543f541a62..1284bff7fa 100644
--- a/extensions/amp-stream-gallery/validator-amp-stream-gallery.protoascii
+++ b/extensions/amp-stream-gallery/validator-amp-stream-gallery.protoascii
@@ -60,7 +60,7 @@ attr_lists: {
   attrs: {
     name: "loop"
     # Media query / boolean pairs
-    value_regex: "([^,]+\\s+(true|false),\\s*)*(true|false)"
+    value_regex: "([^,]+\\s+(true|false),\\s*)*(true|false|^$)"
   }
   attrs: {
     name: "min-visible-count"
diff --git a/validator/validator-main.protoascii b/validator/validator-main.protoascii
index 485c435ce6..2fa66f8224 100644
--- a/validator/validator-main.protoascii
+++ b/validator/validator-main.protoascii
@@ -100,14 +100,6 @@ tags: {  # HTML tag for non-transformed AMP.
   tag_name: "HTML"
   mandatory: true
   mandatory_parent: "!DOCTYPE"
-  attrs: {
-    # This attribute should always be invalid. See https://github.com/ampproject/amphtml/pull/27174
-    # To align with HTML spec, custom attributes should be prefixed with `data-`.
-    # Because `data-*` attributes are always valid AMP, the following rules explicitly disallow usage.
-    name: "data-amp-autocomplete-opt-in"
-    value: "false"
-    disallowed_value_regex: "false"
-  }
   unique: true
   spec_url: "https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml/#required-markup"
 }

@westonruter westonruter added this to the v2.1.4 milestone Aug 11, 2021
Comment on lines 3142 to 3172
array(
'attr_spec_list' => array(
'data-href' => array(
'mandatory' => true,
),
'media' => array(),
'noloading' => array(
'value' => array(
'',
),
),
),
'tag_spec' => array(
'amp_layout' => array(
'supported_layouts' => array(
6,
2,
3,
7,
1,
4,
),
),
'requires_extension' => array(
'amp-facebook',
),
'spec_name' => 'amp-facebook-comments 1.0',
),
),
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 is problematic for our existing auto-extension importing logic, since the two versions look the same but one requires amp-facebook-comments and the other requires amp-facebook.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to add a new bento boolean flag to each tag_spec that requires_extension so we can easily differentiate between Bento and non-Bento specs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or rather, requires_extension should be a mapping from component to the version(s). In this way, we could supply to components and versions that we are wanting and it can automatically omit tag specs that don't have a matching version.

@westonruter westonruter changed the base branch from develop to add/bento August 11, 2021 21:10
@westonruter westonruter changed the base branch from add/bento to develop August 11, 2021 21:10
@westonruter westonruter marked this pull request as ready for review August 12, 2021 04:23
@westonruter westonruter requested a review from pierlon August 12, 2021 04:23
@github-actions
Copy link
Contributor

github-actions bot commented Aug 12, 2021

Plugin builds for 69b99b0 are ready 🛎️!

@westonruter westonruter force-pushed the update/amphtml-spec-20210811 branch from 6e282a3 to 7406336 Compare August 12, 2021 05:36
if (
isset( $rule_spec['tag_spec']['bento'] )
&&
$this->args['prefer_bento'] !== $rule_spec['tag_spec']['bento']
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is temporary, but just a note that this will load the bento and non-bento versions once the prefer_bento argument is true.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will it? How so?

Copy link
Member Author

Choose a reason for hiding this comment

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

When prefer_bento is true, then it will exclude the Bento versions because where bento => false.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah OK I missed the bento => false, this is good then 👍.

tests/php/test-tag-and-attribute-sanitizer.php Outdated Show resolved Hide resolved
Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
@westonruter westonruter merged commit 97e68cb into develop Aug 12, 2021
@westonruter westonruter deleted the update/amphtml-spec-20210811 branch August 12, 2021 05:58
westonruter added a commit that referenced this pull request Aug 12, 2021
Co-authored-by: Pierre Gordon <pierregordon@protonmail.com>
westonruter added a commit that referenced this pull request Aug 12, 2021
…-2.1

[2.1] Update amphtml spec to dc6cd22a52 (#6530)
@westonruter westonruter self-assigned this Aug 30, 2021
@westonruter
Copy link
Member Author

QA Passed

I added a Custom HTML block with the following markup:

<amp-base-carousel layout="responsive" height="1" width="2" auto-advance="true" auto-advance-interval="500" loop>
  <div>1</div>
  <div>2</div>
  <div>3</div>
</amp-base-carousel>

Notice boolean loop attribute without a value.

Before this was a validation error but after this is now no longer flagged as an issue (and the carousel loops as expected).

Screen Before After
Frontend image image
Validated URL image image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants