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

Bento: Validate Facebook components #35395

Merged
merged 14 commits into from Aug 4, 2021
Expand Up @@ -17,6 +17,8 @@
tags: { # amp-facebook-comments
html_format: AMP
tag_name: "SCRIPT"
satisfies: "amp facebook 0.1"
excludes: "amp facebook 1.0"
extension_spec: {
name: "amp-facebook-comments"
version: "0.1"
Expand Down
Expand Up @@ -17,6 +17,8 @@
tags: { # amp-facebook-like
html_format: AMP
tag_name: "SCRIPT"
satisfies: "amp facebook 0.1"
excludes: "amp facebook 1.0"
extension_spec: {
name: "amp-facebook-like"
version: "0.1"
Expand Down
Expand Up @@ -17,6 +17,8 @@
tags: { # amp-facebook-page
html_format: AMP
tag_name: "SCRIPT"
satisfies: "amp facebook 0.1"
excludes: "amp facebook 1.0"
extension_spec: {
name: "amp-facebook-page"
version: "0.1"
Expand Down
@@ -0,0 +1,46 @@
<!--
Copyright 2021 The AMP HTML Authors. All Rights Reserved.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS-IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the license.
-->
<!--
Test Description:
Tests support for the amp-facebook-comments tag.
-->
<!doctype html>
<html ⚡ lang="en">
<head>
<meta charset="utf-8">
<link rel="canonical" href="./regular-html-version.html">
<meta name="viewport" content="width=device-width,minimum-scale=1">
<style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
<script async src="https://cdn.ampproject.org/v0.js"></script>
<script async custom-element="amp-facebook" src="https://cdn.ampproject.org/v0/amp-facebook-1.0.js"></script>
</head>
<body>
<!-- Example of a valid amp-facebook (embedded post) -->
<amp-facebook
width=486
height=657
layout="responsive"
data-href="https://www.facebook.com/zuck/posts/10102593740125791">
</amp-facebook>
<!-- Example of a valid amp-facebook-comments -->
<amp-facebook-comments
width=486
height=657
layout="responsive"
data-href="http://www.directlyrics.com/adele-25-complete-album-lyrics-news.html">
</amp-facebook-comments>
</body>
</html>
@@ -0,0 +1,49 @@
FAIL
| <!--
| Copyright 2021 The AMP HTML Authors. All Rights Reserved.
|
| Licensed under the Apache License, Version 2.0 (the "License");
| you may not use this file except in compliance with the License.
| You may obtain a copy of the License at
|
| http://www.apache.org/licenses/LICENSE-2.0
|
| Unless required by applicable law or agreed to in writing, software
| distributed under the License is distributed on an "AS-IS" BASIS,
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
| See the License for the specific language governing permissions and
| limitations under the license.
| -->
| <!--
| Test Description:
| Tests support for the amp-facebook-comments tag.
| -->
| <!doctype html>
| <html ⚡ lang="en">
| <head>
| <meta charset="utf-8">
| <link rel="canonical" href="./regular-html-version.html">
| <meta name="viewport" content="width=device-width,minimum-scale=1">
| <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
| <script async src="https://cdn.ampproject.org/v0.js"></script>
| <script async custom-element="amp-facebook" src="https://cdn.ampproject.org/v0/amp-facebook-1.0.js"></script>
| </head>
| <body>
| <!-- Example of a valid amp-facebook (embedded post) -->
| <amp-facebook
| width=486
| height=657
| layout="responsive"
| data-href="https://www.facebook.com/zuck/posts/10102593740125791">
| </amp-facebook>
| <!-- Example of a valid amp-facebook-comments -->
Copy link
Member

Choose a reason for hiding this comment

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

Do you intend for this to be valid? You may want to add the extension script in that case.

Same question for a few of the other tests below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this example, amp-facebook-comments, amp-facebook-like, and amp-facebook-page are all registered by the one amp-facebook script included. As long as amp-facebook-1.0 is being used, all four components should have valid use. Is that possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reference, this is what I was having trouble expressing in this PR:

  • The script inclusion is not exclusive after applying satisfies/excludes, as can be seen with the passing cases that should be failing in extensions/amp-facebook/1.0/test/validator-amp-facebook-exclusive.html
  • Use of the amp-facebook-* components is not valid with the amp-facebook-1.0.js script alone -- it appears to be running into the requires_extension line, i.e.:

    I assume there's not such a thing as requires_extension_oneof like we have with other properties? Example test file that should pass but fails: extensions/amp-facebook/1.0/test/validator-amp-facebook-page.out

Copy link
Member

@Gregable Gregable Jul 30, 2021

Choose a reason for hiding this comment

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

I think I see now. I'm not 100% sure this'll work without running the tests, but can you define two amp-facebook-comments tagspecs, one requiring each extension but otherwise identical?

To reduce duplication a tiny bit, you can use a named attr_list with all of the attrs in it which you then include in both tagspecs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this recommendation! It looks like your recommendation, plus giving the 1.0 script a spec_name and requiring by that, did the trick for the second bullet point above (allowing amp-facebook-* to be used when importing amp-facebook:1.0). I added more tests to verify also that importing 0.1 does not work, but it helped me to notice that the secondary tag spec obscures the validation message for amp-facebook-*, which might normally recommend importing the script corresponding to the tag in use. Instead the invalid message shows:

The tag 'amp-facebook 1.0' is missing or incorrect, but required by 'amp-facebook-page'.

Copy link
Member

Choose a reason for hiding this comment

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

You'll need to give the different tagspecs spec_names now that there are more than one tag_spec for each of the tag names like amp-facebook-comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this recommendation! Adding spec_name now suffices for the other bullet point above (exclusion between tags).

As for the "obscured message", the more I think about it, the more I settle on the perspective that it is WAI. In that case things are all set for review. :) Thank you again for all your patience and wisdom here 🙏

| <amp-facebook-comments
>> ^~~~~~~~~
amp-facebook/1.0/test/validator-amp-facebook-comments.html:39:2 The tag 'amp-facebook-comments' requires including the 'amp-facebook-comments' extension JavaScript. (see https://amp.dev/documentation/components/amp-facebook-comments)
| width=486
| height=657
| layout="responsive"
| data-href="http://www.directlyrics.com/adele-25-complete-album-lyrics-news.html">
| </amp-facebook-comments>
| </body>
| </html>
@@ -0,0 +1,64 @@
<!--
Copyright 2021 The AMP HTML Authors. All Rights Reserved.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS-IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the license.
-->
<!--
Test Description:
Tests support for the amp-facebook tag.
-->
<!doctype html>
<html ⚡ lang="en">
<head>
<meta charset="utf-8">
<link rel="canonical" href="./regular-html-version.html">
<meta name="viewport" content="width=device-width,minimum-scale=1">
<style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
<script async src="https://cdn.ampproject.org/v0.js"></script>
<script async custom-element="amp-facebook" src="https://cdn.ampproject.org/v0/amp-facebook-1.0.js"></script>
<!-- Invalid: Cannot have both amp-facebook 1.0 and any amp-facebook-* 0.1 -->
<script async custom-element="amp-facebook-comments" src="https://cdn.ampproject.org/v0/amp-facebook-comments-0.1.js"></script>
<!-- Invalid: Cannot have both amp-facebook 1.0 and any amp-facebook-* 0.1 -->
<script async custom-element="amp-facebook-like" src="https://cdn.ampproject.org/v0/amp-facebook-like-0.1.js"></script>
<!-- Invalid: Cannot have both amp-facebook 1.0 and any amp-facebook-* 0.1 -->
<script async custom-element="amp-facebook-page" src="https://cdn.ampproject.org/v0/amp-facebook-page-0.1.js"></script>
</head>
<body>
<amp-facebook-like
width=486
height=657
layout="responsive"
data-href="https://www.facebook.com/zuck/posts/10102593740125791">
</amp-facebook-like>
<amp-facebook-comments
width=486
height=657
layout="responsive"
data-href="https://www.facebook.com/zuck/posts/10102593740125791">
</amp-facebook-comments>
<amp-facebook-page
width=486
height=657
layout="responsive"
data-href="https://www.facebook.com/zuck/posts/10102593740125791">
</amp-facebook-page>
<amp-facebook
width=552
height=200
layout="responsive"
data-embed-as="comment"
data-include-parent="true"
data-href="https://www.facebook.com/zuck/posts/10102735452532991?comment_id=717538375088203">
</amp-facebook>
</body>
</html>
@@ -0,0 +1,65 @@
PASS
| <!--
| Copyright 2021 The AMP HTML Authors. All Rights Reserved.
|
| Licensed under the Apache License, Version 2.0 (the "License");
| you may not use this file except in compliance with the License.
| You may obtain a copy of the License at
|
| http://www.apache.org/licenses/LICENSE-2.0
|
| Unless required by applicable law or agreed to in writing, software
| distributed under the License is distributed on an "AS-IS" BASIS,
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
| See the License for the specific language governing permissions and
| limitations under the license.
| -->
| <!--
| Test Description:
| Tests support for the amp-facebook tag.
| -->
| <!doctype html>
| <html ⚡ lang="en">
| <head>
| <meta charset="utf-8">
| <link rel="canonical" href="./regular-html-version.html">
| <meta name="viewport" content="width=device-width,minimum-scale=1">
| <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
| <script async src="https://cdn.ampproject.org/v0.js"></script>
| <script async custom-element="amp-facebook" src="https://cdn.ampproject.org/v0/amp-facebook-1.0.js"></script>
| <!-- Invalid: Cannot have both amp-facebook 1.0 and any amp-facebook-* 0.1 -->
| <script async custom-element="amp-facebook-comments" src="https://cdn.ampproject.org/v0/amp-facebook-comments-0.1.js"></script>
| <!-- Invalid: Cannot have both amp-facebook 1.0 and any amp-facebook-* 0.1 -->
| <script async custom-element="amp-facebook-like" src="https://cdn.ampproject.org/v0/amp-facebook-like-0.1.js"></script>
| <!-- Invalid: Cannot have both amp-facebook 1.0 and any amp-facebook-* 0.1 -->
| <script async custom-element="amp-facebook-page" src="https://cdn.ampproject.org/v0/amp-facebook-page-0.1.js"></script>
| </head>
| <body>
| <amp-facebook-like
| width=486
| height=657
| layout="responsive"
| data-href="https://www.facebook.com/zuck/posts/10102593740125791">
| </amp-facebook-like>
| <amp-facebook-comments
| width=486
| height=657
| layout="responsive"
| data-href="https://www.facebook.com/zuck/posts/10102593740125791">
| </amp-facebook-comments>
| <amp-facebook-page
| width=486
| height=657
| layout="responsive"
| data-href="https://www.facebook.com/zuck/posts/10102593740125791">
| </amp-facebook-page>
| <amp-facebook
| width=552
| height=200
| layout="responsive"
| data-embed-as="comment"
| data-include-parent="true"
| data-href="https://www.facebook.com/zuck/posts/10102735452532991?comment_id=717538375088203">
| </amp-facebook>
| </body>
| </html>
48 changes: 48 additions & 0 deletions extensions/amp-facebook/1.0/test/validator-amp-facebook-like.html
@@ -0,0 +1,48 @@
<!--
Copyright 2021 The AMP HTML Authors. All Rights Reserved.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS-IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the license.
-->
<!--
Test Description:
Tests support for the amp-facebook-like tag.
-->
<!doctype html>
<html ⚡ lang="en">
<head>
<meta charset="utf-8">
<link rel="canonical" href="./regular-html-version.html">
<meta name="viewport" content="width=device-width,minimum-scale=1">
<style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
<script async src="https://cdn.ampproject.org/v0.js"></script>
<script async custom-element="amp-facebook" src="https://cdn.ampproject.org/v0/amp-facebook-1.0.js"></script>
</head>
<body>
<!-- Example of a valid amp-facebook (embedded post) -->
<amp-facebook
width=486
height=657
layout="responsive"
data-href="https://www.facebook.com/zuck/posts/10102593740125791">
</amp-facebook>
<!-- Example of a valid amp-facebook-like -->
<amp-facebook-like width=225 height=80
layout="fixed"
title="Like Button"
data-layout="standard"
data-size="large"
data-show-faces="true"
data-href="https://www.facebook.com/testesmegadivertidos/">
</amp-facebook-like>
</body>
</html>
51 changes: 51 additions & 0 deletions extensions/amp-facebook/1.0/test/validator-amp-facebook-like.out
@@ -0,0 +1,51 @@
FAIL
| <!--
| Copyright 2021 The AMP HTML Authors. All Rights Reserved.
|
| Licensed under the Apache License, Version 2.0 (the "License");
| you may not use this file except in compliance with the License.
| You may obtain a copy of the License at
|
| http://www.apache.org/licenses/LICENSE-2.0
|
| Unless required by applicable law or agreed to in writing, software
| distributed under the License is distributed on an "AS-IS" BASIS,
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
| See the License for the specific language governing permissions and
| limitations under the license.
| -->
| <!--
| Test Description:
| Tests support for the amp-facebook-like tag.
| -->
| <!doctype html>
| <html ⚡ lang="en">
| <head>
| <meta charset="utf-8">
| <link rel="canonical" href="./regular-html-version.html">
| <meta name="viewport" content="width=device-width,minimum-scale=1">
| <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
| <script async src="https://cdn.ampproject.org/v0.js"></script>
| <script async custom-element="amp-facebook" src="https://cdn.ampproject.org/v0/amp-facebook-1.0.js"></script>
| </head>
| <body>
| <!-- Example of a valid amp-facebook (embedded post) -->
| <amp-facebook
| width=486
| height=657
| layout="responsive"
| data-href="https://www.facebook.com/zuck/posts/10102593740125791">
| </amp-facebook>
| <!-- Example of a valid amp-facebook-like -->
| <amp-facebook-like width=225 height=80
>> ^~~~~~~~~
amp-facebook/1.0/test/validator-amp-facebook-like.html:39:4 The tag 'amp-facebook-like' requires including the 'amp-facebook-like' extension JavaScript. (see https://amp.dev/documentation/components/amp-facebook-like)
| layout="fixed"
| title="Like Button"
| data-layout="standard"
| data-size="large"
| data-show-faces="true"
| data-href="https://www.facebook.com/testesmegadivertidos/">
| </amp-facebook-like>
| </body>
| </html>