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

Validator incorrectly identifying required extensions for amp-carousel #18091

Closed
garanj opened this issue Sep 17, 2018 · 1 comment
Closed

Validator incorrectly identifying required extensions for amp-carousel #18091

garanj opened this issue Sep 17, 2018 · 1 comment
Assignees
Labels
P3: When Possible Type: Bug Validator: Type: Error Messages Issue with the quality of the error messages that the validator produces WG: caching
Milestone

Comments

@garanj
Copy link

garanj commented Sep 17, 2018

What's the issue?

AMP HTML containing <amp-carousel> without the accompanying component <script> element is wrongly identified by the validator as being the lightbox mode version, as opposed to the ordinary version, and requesting components beyond just amp-carousel be included, namely amp-lightbox-gallery:

$ amphtml-validator example.html
example.html:18:2 The mandatory attribute 'lightbox' is missing in tag 'AMP-CAROUSEL [lightbox]'. (see https://www.ampproject.org/docs/reference/components/amp-carousel)
example.html:18:2 The tag 'AMP-CAROUSEL [lightbox]' requires including the 'amp-carousel' extension JavaScript. (see https://www.ampproject.org/docs/reference/components/amp-carousel)
example.html:18:2 The tag 'AMP-CAROUSEL [lightbox]' requires including the 'amp-lightbox-gallery' extension JavaScript. (see https://www.ampproject.org/docs/reference/components/amp-carousel)

Note that including the amp-carousel extension <script> is enough to make validation pass, in spite of the above messages.

How do we reproduce the issue?

  1. Copy HTML from http://jsbin.com/tuvibewiko/edit?html,output
  2. Run this HTML through amphtml-validator.

What browsers are affected?

At least: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.92 Safari/537.36

Which AMP version is affected?

amphtml-validator cli tool.

@garanj garanj changed the title Validator incorrectly identifying required extensions Validator incorrectly identifying required extensions for amp-carousel Sep 17, 2018
@aghassemi aghassemi added this to the Pending Triage milestone Sep 17, 2018
@Gregable
Copy link
Member

Thanks for the report. The validator is selecting the more specific set of errors it could report in this case, but that's (usually) not the author's intent in this case.

@Gregable Gregable modified the milestones: Pending Triage, Backlog Bugs Sep 17, 2018
@Gregable Gregable added WG: caching Validator: Type: Error Messages Issue with the quality of the error messages that the validator produces labels Jun 24, 2019
Gregable pushed a commit that referenced this issue Jul 2, 2019
* cl/255021843 Add a test file illustrating the issue in #18091

* cl/255463519 Revision bump for #22679

* cl/256058522 Revision bump for #23043

* cl/256067928 Revision bump for #23135

* cl/256199406 Revision bump for #23147
@Gregable Gregable closed this as completed Aug 8, 2019
thekorn pushed a commit to edelight/amphtml that referenced this issue Sep 11, 2019
* cl/255021843 Add a test file illustrating the issue in ampproject#18091

* cl/255463519 Revision bump for ampproject#22679

* cl/256058522 Revision bump for ampproject#23043

* cl/256067928 Revision bump for ampproject#23135

* cl/256199406 Revision bump for ampproject#23147
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3: When Possible Type: Bug Validator: Type: Error Messages Issue with the quality of the error messages that the validator produces WG: caching
Projects
None yet
Development

No branches or pull requests

5 participants