Improve validation error listing with more context and links to file editors #3505

westonruter · 2019-10-15T05:57:30Z

Summary

Part of #2316, redesigning the compatibility tool.

Include callback file name and line number in source information, with links to the plugin/theme editor and scrolling to the specific line where the function is defined. The URL to open the file in an editor can be filtered (via amp_validation_error_source_file_editor_url_template) to allow a site to request the user's IDE to edit the file, including the use of a amp_validation_error_source_file_path filter to map the file path from a container/VM to the host machine. See example plugin for IntelliJ IDEA for a site running on Lando. ✨
Include hook priority in source information. This is important for writing child themes and extension plugins to override the behavior, as the overriding logic must run at a later priority. Having the hook name alone is not enough.
Add an information section to provide context for JS, CSS, and HTML error types.
Eliminate use of pretty-printed JSON for displaying the sources, in favor of tables.
Add special case for widgets so subclassed WP_Widget::widget method is used as opposed to WP_Widget::display_callback.
Improve styling, including zebra striping for new validation error rows (two shades of red).
Swap order of Error Type and Status columns.
Rename “Details” column to “Context”.
Disambiguates “handle” to indicate whether a handle is for a script or a style.
Incorporate actual plugin and theme names in source information, in addition to slugs. Also include icons.
Use translatable strings for labels in sources, rather than just keys.

Before

After

Informational Messages

JavaScript Error Type

CSS Error Type

HTML Error Type

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).

Also fix VariableAnalysis.CodeAnalysis.VariableAnalysis.UndefinedVariable and keyboard typo

…messages

…#2508

…n editor

…ources

includes/validation/class-amp-validation-error-taxonomy.php

pierlon

Perhaps don't show the colon (:) if there's no value for the attribute.

includes/validation/class-amp-validation-error-taxonomy.php

assets/css/src/amp-validation-error-taxonomy.css

…alidation-sourcing * 'develop' of github.com:ampproject/amp-wp: (57 commits) Undo over-eager rename of auto_accept_sanitization Rename auto-accept to accept-by-default Rename filter to amp_validation_error_auto_sanitized Fix jumping blocks when changing fonts (#3529) Update dependency browserslist to v4.7.1 (#3551) Make sure that isPageBlock always returns boolean. Fix and improve e2e tests. Remove unnessary check for block. Use shift over access first element of array. Use isPageBlock for cleaner code. Fix minor grammar issues Add docblock for `amp_is_sanitization_auto_accepted` filter Align arrows Update tests to use filter to set sanitization auto acceptance Make AMP_Validation_Manage::is_sanitization_auto_accepted() filterable Remove 'auto_accept_sanitization' option Remove relevant auto-sanitization notices Remove validation handling setting field Add special-case handling for layout=fill when both dimensions are 100% Insert duplicated page as current page. Ensure smooth snap line display (#3514) ...

includes/validation/class-amp-validation-error-taxonomy.php

…hine

westonruter · 2019-10-18T19:19:21Z

The presentation of the sources is much improved yet further with actual labels and theme/plugin names:

westonruter · 2019-10-18T19:27:04Z

Also, now the handle indicates whether it is a Script handle or a Style handle. Previously it was just “handle”, leaving you to figure out from context.

schlessera

The code that renders the pages is a wild mixture of markup and logic, which could really use a refactor, as it is very hard to read through.

I understand we're under time pressure here, but we should mark this as a priority under tech debt at one point.

schlessera · 2019-10-18T20:34:27Z

includes/validation/class-amp-validation-error-taxonomy.php

+								echo wp_kses_post(
+									sprintf(
+										/* translators: 1: script,  2: Documentation URL, 3: Documentation URL, 4: Documentation URL, 5: onclick, 6: Documentation URL, 7: amp-bind, 8: Documentation URL, 9: amp-script */
+										__( 'Arbitrary JavaScript is not allowed in AMP. You cannot use JS %1$s tags unless they are for loading <a href="%2$s">AMP components</a> (which the AMP plugin will add for you automatically). In order for a page to be served as AMP, the invalid JS code must be removed from the page, which is what happens when you <strong>accept</strong> sanitization. Learn more about <a href="%3$s">how AMP works</a>. As an alternative to using custom JS, please consider using a pre-built AMP functionality, including <a href="%4$s">actions and events</a> (as opposed to JS event handler attributes like %5$s) and the <a href="%6$s">%7$s</a> component; you may also add custom JS if encapsulated in the <a href="%8$s">%9$s</a>.', 'amp' ),


I think the translatable strings shouldn't contain markup. Here's some sample code of how I normally deal with links in translatable strings: https://gist.github.com/schlessera/18770dc8d4641dbe9c76

@swissspidy Since this is your handiwork, would you please respond next week?

As with all the translatable strings in the plugin, we're following core's lead and recommendations. Wherever possible we avoid unnecessary or unusual HTML markup. <a> and <strong> tags are quite common in any UI copy, and translators are used to seeing those in strings. translate.wordpress.org even has special handling for them.

While https://gist.github.com/schlessera/18770dc8d4641dbe9c76 looks interesting, it is not a format translators are familiar with.

includes/validation/class-amp-validation-error-taxonomy.php

schlessera · 2019-10-18T20:36:42Z

includes/validation/class-amp-validation-error-taxonomy.php

+							echo wp_kses_post(
+								sprintf(
+									/* translators: 1: Documentation URL, 2: Documentation URL. */
+									__( 'AMP has specific set of allowed elements and attributes that are allowed in valid AMP pages. Learn about the <a href="%1$s">AMP HTML specification</a>. If an element or attribute is not allowed in AMP, it must be removed for the page to <a href="%2$s">cached and be eligible for prerendering</a>.', 'amp' ),


Markup in translation.

Via dfd87bb. @swissspidy Please comment.

schlessera · 2019-10-18T20:48:31Z

includes/validation/class-amp-validation-manager.php

@@ -1265,6 +1265,16 @@ public static function wrap_hook_callbacks( $hook ) {
 					continue;
 				}

+				// Skip considering ourselves.


Only because this logic only runs exclusively during validation, so it's not relevant to validation errors being reported on the frontend.

includes/validation/class-amp-validation-error-taxonomy.php

amedina

The structural and copy changes are very good. Huge improvement on UX from a dev perspective.

westonruter · 2019-10-18T22:16:52Z

The code that renders the pages is a wild mixture of markup and logic, which could really use a refactor, as it is very hard to read through.

I understand we're under time pressure here, but we should mark this as a priority under tech debt at one point.

Agreed 100%. This refactoring of the compatibility tool's codebase needs to be done as part of the 2.0 effort. With redesigning the compatibility tool (#2316) we can have a modern JS-driven UI that doesn't intermix all of the markup and PHP logic together (we can instead intermix markp and JS logic! 😉)

See ampproject/amp-wp#3505

) * Remove dead code after #3505 * Fix matching file paths for child themes

See ampproject/amp-wp#3505

westonruter added 2 commits October 14, 2019 22:10

Include hook priority in source information

41d99af

Include relative file name and line number in source information

977ecd4

googlebot added the cla: yes Signed the Google CLA label Oct 15, 2019

westonruter added 10 commits October 16, 2019 12:36

Add special case for sourcing WP_Widget::display_callback

fb8c82c

Refactor test_decorate_shortcode_and_filter_source

4e0993a

Also fix VariableAnalysis.CodeAnalysis.VariableAnalysis.UndefinedVariable and keyboard typo

Improve display of validation error details, including informational …

e317de2

…messages

Remove confusing brackets from attributes; remove redundant @

8e77f91

Remove obsolete references to removed_unused_css_rules error code; see …

02231bf

…#2508

Fix identifng file for single-file plugins

db22e7e

Improve displaying source information, including links to theme/plugi…

49d1dad

…n editor

Jump to the requested line when opening the file editor

0f0ebcf

Fix CSS linting errors

b99ba62

Ignore WordPress.Security.NonceVerification.Recommended

98bba39

westonruter added this to the v1.3.1 milestone Oct 17, 2019

westonruter marked this pull request as ready for review October 17, 2019 05:22

Skip including AMP_Validation_Manager::add_block_source_comments in s…

04c2f40

…ources

westonruter requested review from samitron7, amedina and cathibosco October 17, 2019 05:32

westonruter added the Size: M label Oct 17, 2019

westonruter changed the title ~~Improve validation sourcing with more context and links to file editors~~ Improve validation error listing with more context and links to file editors Oct 17, 2019

swissspidy reviewed Oct 17, 2019

View reviewed changes

includes/validation/class-amp-validation-error-taxonomy.php Outdated Show resolved Hide resolved

swissspidy reviewed Oct 17, 2019

View reviewed changes

includes/validation/class-amp-validation-error-taxonomy.php Outdated Show resolved Hide resolved

swissspidy self-requested a review October 17, 2019 10:18

swissspidy reviewed Oct 17, 2019

View reviewed changes

includes/validation/class-amp-validation-error-taxonomy.php Outdated Show resolved Hide resolved

swissspidy modified the milestones: v1.3.1, v1.4 Oct 17, 2019

kmyram added the P0 High priority label Oct 17, 2019

pierlon reviewed Oct 17, 2019

View reviewed changes

includes/validation/class-amp-validation-error-taxonomy.php Outdated Show resolved Hide resolved

assets/css/src/amp-validation-error-taxonomy.css Outdated Show resolved Hide resolved

westonruter added 4 commits October 17, 2019 18:57

Fix phpcs linting issues

82a87a6

Swap status column with error type column; rename Details to Context

5b77a64

Allow validation error source file edit link to be filtered

dcb00f7

westonruter commented Oct 18, 2019

View reviewed changes

includes/validation/class-amp-validation-error-taxonomy.php Outdated Show resolved Hide resolved

westonruter requested a review from schlessera October 18, 2019 05:21

westonruter added 4 commits October 17, 2019 22:42

Prevent status dropdown from wrapping after status icon

b4779da

Fix phpcs complaints regarding indentation

7ff9fd7

Introduce file editor URL template and filter for mapping to host mac…

e6cbc52

…hine

Improve display of source information with icons and translated strings

413d41b

Keep track of whether a handle is a script or a style

96590da

schlessera requested changes Oct 18, 2019

View reviewed changes

amedina approved these changes Oct 18, 2019

View reviewed changes

westonruter added 2 commits October 18, 2019 14:43

Prepare CSS informational message for translation

3a6bfe6

Prevent duplicating char findiing; add parens; fix scheme check

1b1e1dc

westonruter merged commit a50ed15 into develop Oct 18, 2019

westonruter deleted the improve/validation-sourcing branch October 18, 2019 22:17

westonruter mentioned this pull request Oct 22, 2019

Further improve compatibility tool design and UX #3599

Merged

3 tasks

westonruter added a commit that referenced this pull request Nov 10, 2019

Remove dead code after #3505

16440c1

westonruter added a commit that referenced this pull request Nov 10, 2019

Remove dead code after #3505

6ff475b

westonruter added a commit to GoogleChromeLabs/wp-origination that referenced this pull request Nov 10, 2019

Improve special-casing the logic for sourcing WP_Widget::widget output

a25a780

See ampproject/amp-wp#3505

westonruter mentioned this pull request Nov 11, 2019

Fix identifying sources for validation errors coming child themes #3708

Merged

3 tasks

westonruter added a commit that referenced this pull request Nov 11, 2019

Fix identifying sources for validation errors coming child themes (#3708

fe2cd4e

) * Remove dead code after #3505 * Fix matching file paths for child themes

westonruter added a commit that referenced this pull request Nov 11, 2019

Fix identifying sources for validation errors coming child themes (#3708

aed2568

) * Remove dead code after #3505 * Fix matching file paths for child themes

brennensmith75 added a commit to brennensmith75/WP-Origination that referenced this pull request Sep 22, 2023

Improve special-casing the logic for sourcing WP_Widget::widget output

23a268d

See ampproject/amp-wp#3505

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve validation error listing with more context and links to file editors #3505

Improve validation error listing with more context and links to file editors #3505

westonruter commented Oct 15, 2019 •

edited

Loading

pierlon left a comment

westonruter commented Oct 18, 2019

westonruter commented Oct 18, 2019

schlessera left a comment

schlessera Oct 18, 2019

westonruter Oct 18, 2019

swissspidy Oct 21, 2019

schlessera Oct 18, 2019

westonruter Oct 18, 2019

schlessera Oct 18, 2019

westonruter Oct 18, 2019

amedina left a comment

westonruter commented Oct 18, 2019

Improve validation error listing with more context and links to file editors #3505

Improve validation error listing with more context and links to file editors #3505

Conversation

westonruter commented Oct 15, 2019 • edited Loading

Summary

Before

After

Informational Messages

JavaScript Error Type

CSS Error Type

HTML Error Type

Checklist

pierlon left a comment

Choose a reason for hiding this comment

westonruter commented Oct 18, 2019

westonruter commented Oct 18, 2019

schlessera left a comment

Choose a reason for hiding this comment

schlessera Oct 18, 2019

Choose a reason for hiding this comment

westonruter Oct 18, 2019

Choose a reason for hiding this comment

swissspidy Oct 21, 2019

Choose a reason for hiding this comment

schlessera Oct 18, 2019

Choose a reason for hiding this comment

westonruter Oct 18, 2019

Choose a reason for hiding this comment

schlessera Oct 18, 2019

Choose a reason for hiding this comment

westonruter Oct 18, 2019

Choose a reason for hiding this comment

amedina left a comment

Choose a reason for hiding this comment

westonruter commented Oct 18, 2019

westonruter commented Oct 15, 2019 •

edited

Loading