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

Investigate validation errors that are not sanitized in WordPress.org themes #4060

Closed
westonruter opened this issue Jan 10, 2020 · 7 comments
Assignees
Labels
Bug Something isn't working Validation
Milestone

Comments

@westonruter
Copy link
Member

westonruter commented Jan 10, 2020

Bug Description

I'm doing an analysis of how the entire directory of themes on WordPress.org fair in terms of AMP-compatibility. For each theme (which doesn't go over the 50KB limit), I've obtained the validation results for the initial Hello World post, using the current 1.5-alpha state of the plugin.

The themes which the AMP plugin thinks is serving as valid AMP but actually is not, are:

Theme Validation Error
brovy The mandatory tag 'meta name=viewport' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#required-markup)
catch-wedding CSS syntax error in tag 'style amp-custom' - invalid declaration.
catch-wheels CSS syntax error in tag 'style amp-custom' - invalid declaration.
curiosity-lite CSS syntax error in tag 'style amp-custom' - end of stylesheet encountered in prelude of a qualified rule.
curiosity-lite CSS syntax error in tag 'style amp-custom' - end of stylesheet encountered in prelude of a qualified rule.
curiosity-lite CSS syntax error in tag 'style amp-custom' - invalid declaration.
foodoholic CSS syntax error in tag 'style amp-custom' - invalid declaration.
hamza-lite The mandatory tag 'meta name=viewport' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#required-markup)
intuitive CSS syntax error in tag 'style amp-custom' - invalid declaration.
keep-calm-and-e-comm The mandatory tag 'meta name=viewport' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#required-markup)
lakshmi-lite CSS syntax error in tag 'style amp-custom' - end of stylesheet encountered in prelude of a qualified rule.
lakshmi-lite CSS syntax error in tag 'style amp-custom' - end of stylesheet encountered in prelude of a qualified rule.
lakshmi-lite CSS syntax error in tag 'style amp-custom' - invalid declaration.
namaste-lite CSS syntax error in tag 'style amp-custom' - end of stylesheet encountered in prelude of a qualified rule.
namaste-lite CSS syntax error in tag 'style amp-custom' - end of stylesheet encountered in prelude of a qualified rule.
namaste-lite CSS syntax error in tag 'style amp-custom' - end of stylesheet encountered in prelude of a qualified rule.
namaste-lite CSS syntax error in tag 'style amp-custom' - end of stylesheet encountered in prelude of a qualified rule.
namaste-lite CSS syntax error in tag 'style amp-custom' - end of stylesheet encountered in prelude of a qualified rule.
nullpoint CSS syntax error in tag 'style amp-custom' - end of stylesheet encountered in prelude of a qualified rule.
nullpoint CSS syntax error in tag 'style amp-custom' - end of stylesheet encountered in prelude of a qualified rule.
organic-lite CSS syntax error in tag 'style amp-custom' - invalid declaration.
organic-lite CSS syntax error in tag 'style amp-custom' - invalid declaration.
palette CSS syntax error in tag 'style amp-custom' - end of stylesheet encountered in prelude of a qualified rule.
palette CSS syntax error in tag 'style amp-custom' - end of stylesheet encountered in prelude of a qualified rule.
personal-trainer CSS syntax error in tag 'style amp-custom' - invalid declaration.
postmag The property 'width' in attribute 'content' in tag 'meta name=viewport' is set to ' device-width', which is invalid. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#required-markup)
robot CSS syntax error in tag 'style amp-custom' - end of stylesheet encountered in prelude of a qualified rule.
robot CSS syntax error in tag 'style amp-custom' - end of stylesheet encountered in prelude of a qualified rule.
ultimate-showcase The mandatory tag 'meta name=viewport' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#required-markup)
zincy-lite The mandatory tag 'meta name=viewport' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#required-markup)

The frequency is as follows:

Count Error
15 CSS syntax error in tag 'style amp-custom' - end of stylesheet encountered in prelude of a qualified rule.
9 CSS syntax error in tag 'style amp-custom' - invalid declaration.
5 The mandatory tag 'meta name=viewport' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#required-markup)
1 The property 'width' in attribute 'content' in tag 'meta name=viewport' is set to ' device-width', which is invalid. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#required-markup)

The themes are:

  1. brovy
  2. catch-wedding
  3. catch-wheels
  4. curiosity-lite
  5. foodoholic
  6. hamza-lite
  7. intuitive
  8. keep-calm-and-e-comm
  9. lakshmi-lite
  10. namaste-lite
  11. nullpoint
  12. organic-lite
  13. palette
  14. personal-trainer
  15. postmag
  16. robot
  17. ultimate-showcase
  18. zincy-lite

Expected Behaviour

When the AMP plugin intends to serve a valid AMP page, no validation errors should be encountered by the AMP validator.

Steps to reproduce

  1. Enable AMP Standard template mode.
  2. Activate one of the above themes.
  3. Navigate to the Hello World post.
  4. Look for validation errors reported by the AMP validator.

Screenshots

Additional context

  • WordPress version:
  • Plugin version:
  • Gutenberg plugin version (if applicable):
  • AMP plugin template mode:
  • PHP version:
  • OS:
  • Browser: [e.g. chrome, safari]
  • Device: [e.g. iPhone6]

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Issues #4070, #4113, #4179 and #4197 were created to resolve the errors above and they should be resolved before this issue can be closed.

Implementation brief

QA testing instructions

Demo

Changelog entry

@westonruter westonruter added Bug Something isn't working Validation labels Jan 10, 2020
@westonruter westonruter added this to the v1.5 milestone Jan 10, 2020
@westonruter
Copy link
Member Author

For the themes that have excessive CSS, there is just one that has unexpected validation errors: corpoz. There appears to be a problem with the output buffering because the first content is <title> followed later by <!DOCTYPE html>.

@westonruter
Copy link
Member Author

CSS syntax error in tag 'style amp-custom' - invalid declaration.

These seem to all be instances of #4113, except for curiosity-lite and lakshmi-lite.

In organic-lite it is due to properties like 4z-index:1;.

@pierlon pierlon self-assigned this Jan 16, 2020
@westonruter
Copy link
Member Author

westonruter commented Jan 17, 2020

brovy The mandatory tag 'meta name=viewport' is missing or incorrect. (see https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#required-markup)

I suspect this one will be resolved by #4070. The meta viewport added by the theme is:

<meta name="viewport" content="width=device-width, initial-scale=1, user-scalabe=no">

So the invalid properties are causing the meta tag to be removed.

@pierlon
Copy link
Contributor

pierlon commented Jan 30, 2020

An overview of the work being done to resolve the validation errors reported above:

Theme Reported Issue
brovy #4070
catch-wedding #4113
catch-wheels #4113
curiosity-lite #4197
foodoholic #4113
hamza-lite #4070
intuitive #4113
keep-calm-and-e-comm #4070
lakshmi-lite #4197
namaste-lite #4197
nullpoint #4197
organic-lite #4113
palette #4197
personal-trainer #4113
postmag #4070, #4179 and #4197
robot #4197
ultimate-showcase #4070
zincy-lite #4070

@pierlon
Copy link
Contributor

pierlon commented Feb 19, 2020

Now that the sub-issues (#4070, #4113, #4179, #4197) created from this discovery are resolved, I'll review the themes in question once more to ensure the reported validation errors for each no longer occurs.

@pierlon
Copy link
Contributor

pierlon commented Feb 20, 2020

All reported validation errors for each theme are fixed 🎉.

For the themes that have excessive CSS, there is just one that has unexpected validation errors: corpoz. There appears to be a problem with the output buffering because the first content is <title> followed later by .

Ah I missed that one. I'll open a separate issue for that, but this issue can be closed since the validation errors in the description are resolved.

@westonruter
Copy link
Member Author

Ah I missed that one. I'll open a separate issue for that, but this issue can be closed since the validation errors in the description are resolved.

Sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Validation
Projects
None yet
Development

No branches or pull requests

3 participants