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.

Sign up for GitHub

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

Jump to bottom

Tidy up validation error details #3721

Merged
merged 7 commits into from Nov 12, 2019
Merged

Tidy up validation error details #3721

merged 7 commits into from Nov 12, 2019

Conversation

westonruter
Copy link
Member

Summary

Given a Custom HTML block that contains this style element:

<style amp-keyframes>
@-webkit-keyframes NAME-YOUR-ANIMATION {
  0%   { height: 0px !important; }
  100% { height: 100px !important; }
}
</style>

There should be 4 validation errors (two for !important and two for illegal height), but there are three:

image

Also, the presentation of these validation errors is not ideal:

  1. Translated error codes are not used.
  2. The CSS property name is not mentioned in the summary.
  3. The property name and value are not presented with a translated title in the details and they do not appear together.
  4. Expanding the text content for a style (or script) element does not present the contents in a pre element, leading to the content appearing minified.
  5. Multiple illegal_css_important errors get conflated because they lack property context (hence the 3 instead of 4 errors).
  6. A blatant bug in the source logic causes some conditions to never be reached (e.g. post type info).

This PR fixes those problems.

Before

wordpressdev lndo site_core-dev_src_wp-admin_post php_post=2374 action=edit (2)

After

wordpressdev lndo site_core-dev_src_wp-admin_post php_post=2374 action=edit amp_urls_tested=1 amp_remaining_errors=0

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@westonruter westonruter added this to the v1.4.1 milestone Nov 12, 2019
@westonruter westonruter added this to Ready for Review in Ongoing Nov 12, 2019
@googlebot googlebot added the cla: yes Signed the Google CLA label Nov 12, 2019
kienstra
kienstra approved these changes Nov 12, 2019
View reviewed changes
Copy link
Contributor

@kienstra kienstra left a comment

Choose a reason for hiding this comment

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

Approved

Hi @westonruter,
This looks good, pending your comment about the condition.

Like you mentioned, when using your Custom HTML snippet, the error codes look good:

error-codes-now

Also, the validated URL page looks good. It doesn't have the 'Unknown error' code anymore:

unknown-error-shown

includes/validation/class-amp-validation-error-taxonomy.php
case 'stylesheet_file_missing':
return __( 'Missing stylesheet file', 'amp' );
case 'illegal_css_important':
return __( 'Illegal CSS !important property', 'amp' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, it's good to have translated error messages.

@westonruter westonruter merged commit e798258 into develop Nov 12, 2019
@westonruter westonruter deleted the fix/validation-error-screen-issues branch November 12, 2019 22:06
@westonruter
Copy link
Member Author

Merged in spite of unrelated E2E test failures: https://travis-ci.org/ampproject/amp-wp/jobs/611080755#L923-L954

westonruter added a commit that referenced this pull request Nov 12, 2019
@westonruter
Tidy up validation error details (#3721)
3c2acd7 
* Improve display of validation errors for invalid CSS properties

* Present style/script text content in pre tag

* Translate additional error codes from style sanitizer

* Fix blatent logic typo which caused some sourcing info to not display

* Include property for illegal_css_important errors

* Prevent array-to-string conversion when CSS property has unexpected value

* Remove duplicated condition
@westonruter westonruter moved this from Ready for Review to Ready for QA in Ongoing Nov 13, 2019
westonruter added a commit that referenced this pull request Nov 13, 2019
@westonruter
Merge branch 'develop' of github.com:ampproject/amp-wp into 3489-remo…
7d1bce5 
…ve-duplicate-amp-scripts

* 'develop' of github.com:ampproject/amp-wp: (66 commits)
  Improve display of validation errors for scripts (#3722)
  Conditionally run E2E tests (#3723)
  Tidy up validation error details (#3721)
  Add missing space after sentence (#3720)
  Default to the homepage instead of fetching the first AMP compatible post to customize (#3715)
  Include text content of style element in validation error (#3717)
  Fix summarizing error sources both parent theme and child theme (#3709)
  Exclude WordPress.PHP.DisallowShortTernary phpcs sniff
  Fix phpcs issues with date() and current_time()
  Exclude Generic.Arrays.DisallowShortArraySyntax from WordPress-Core
  Update dependency wp-coding-standards/wpcs to v2.2.0
  Improve specificity of JS doc
  Fix identifying sources for validation errors coming child themes (#3708)
  Fix failing E2E tests (#3707)
  Remove amp_validate query var from Validated URL 'View' row action
  Re-factor get_html_attribute_pattern as match_element_attributes
  Quote variables added to regex pattern
  Replace incorrect usage of esc_url() with esc_url_raw()
  Remove empty alt attributes
  Add object-fit=contain to amp-youtube placeholder image
  ...
westonruter added a commit that referenced this pull request Nov 14, 2019
@westonruter
Merge tag '1.4.1'
3133665 
* tag '1.4.1': (26 commits)
  Bump 1.4.1
  Update screenshots for 1.4.1
  Fix expected image name after upstream change (#3749)
  Use length property instead of count() method on DOMNodeList (#3727)
  Improve display of validation errors for scripts (#3722)
  Conditionally run E2E tests (#3723)
  Tidy up validation error details (#3721)
  Bump 1.4.1-RC1
  Default to the homepage instead of fetching the first AMP compatible post to customize (#3715)
  Add missing space after sentence (#3720)
  Include text content of style element in validation error (#3717)
  Use bitwise operator.
  Check if element is not in top toolbar.
  Fix user select for meta date and author
  Allow right click for meta blocks
  Fix summarizing error sources both parent theme and child theme (#3709)
  Fix identifying sources for validation errors coming child themes (#3708)
  Fix failing E2E tests (#3707)
  Remove amp_validate query var from Validated URL 'View' row action (#3706)
  Escape instances of unescapeed output in AMP settings screen code (#3703)
  ...
@csossi
Copy link

csossi commented Nov 19, 2019

Verified in QA:
image
image

@csossi csossi moved this from Ready for QA to Done in Ongoing Nov 19, 2019
@csossi csossi added the QA passed Has passed QA and is done label Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kienstra kienstra kienstra approved these changes

@pierlon pierlon Awaiting requested review from pierlon

Assignees
No one assigned
Labels
cla: yes Signed the Google CLA QA passed Has passed QA and is done
Projects
Ongoing
  
Done
Milestone
v1.4.1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@westonruter @csossi @kienstra @googlebot