Add AMP validation checking for Gutenberg blocks #1019

westonruter · 2018-03-13T17:26:56Z

Add source comments around each Gutenberg block to track validation errors.
Pass validation errors back in REST API response when Gutenberg saves a post. (Discovery needed.)
Show validation status in editor after saving post.

An invalid script inside of an HTML block which is inside of a Column block has sources that look like this:

westonruter · 2018-03-13T17:28:30Z

@kienstra feel free to pick this up from me.

kienstra · 2018-03-13T17:46:25Z

Thanks, @westonruter! It'll be great to work on this PR. If it's alright, tomorrow is probably the earliest I could get to it. I'm working on #1018 now.

westonruter · 2018-03-13T23:06:52Z

There is a bug with how Mustache replacements are being done which is corrupting the JSON in the block attributes. I'm working on a fix for that. Not a blocker for work being done on this.

…ssues Remove requirement that amp_debug query var have a value

…y target template elements

westonruter · 2018-03-14T15:48:29Z

Rebased to include test page generated from #1018. Former head was 4a10ee4.

westonruter · 2018-03-14T16:11:51Z

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

+			if ( ! empty( $block['innerBlocks'] ) ) {
+				$rendered_inner_blocks = self::render_blocks( $block['innerBlocks'] );
+
+				// Inject rendered inner blocks before closing tag of outer block (or at end of string if no closing). @todo Confirm approach here.


I've raised this in WordPress #core-editor:

https://wordpress.slack.com/archives/C02QB2JS7/p1521043684000458

westonruter · 2018-03-14T16:29:16Z

Let's not merge this as part of 0.7 since nested blocks are still in flux.

Commit ba9365 removed this, as it wasn't needed for 0.7. This restores the logic for the REST API, but not the wp-cron logic. See PR #971.

The POST to the REST API in the JS file doesn't work yet. And this doesn't apply a requirement in PR #1019: 'Pass validation errors back in REST API response when Gutenberg saves a post.' But this is a prototype of a way to display notices for each block, async. The UX would be like that in the previous PR #902. As Weston mentioned earlier, edit() is synchronous. And REST requests to validate content could delay it. So add a component for 'edit.' Display a notice, based on the state of isValidAMP. The REST API request will update the state.

westonruter · 2018-03-19T20:29:48Z

@kienstra Here's an idea: what if we piggy-backed on the autosave functionality to incorporate the validation results? This could avoid the need to trigger an update when changing each block. We would be piggy-backing on the autosave interval.

Upon saving draft we'll want to do the loopback request to get the full validation results anyway and display them in a notification. (Also, a block may enqueue scripts which would need to be caught but process_markup does not currently do that since it does not do wp_head or wp_footer. ) The results after this save we'll want to add to the blocks. So in addition to there being a validation message at the top, there should also be the specific validation errors added to their respective individual blocks inside so that users know which ones specifically are problematic.

In short, I think perhaps this REST API endpoint should not be used, but rather a new REST field should be added to the existing post/page endpoints. When a POST/PUT request is made to create/update a post, this can trigger the same loopback request we're doing during save_post now, and then the array of validation errors could be returned as the contents of the field for the client to then display in the notification error and with each block. This could be done both during autosave and regular save requests.

kienstra · 2018-03-19T20:41:39Z

Validation On Saving Post

Hi @westonruter,
Thanks, that's a good idea to make a request to get the validation results on autosaving the post. The block edit() functions can fire a few times a second, so it was going to be hard to hook into those or componentDidMount().

I'll look at how we might display individual block notices when the page autosaves. So far, it looks setting the state of the AMPNotice component from outside the block might be hard. But there's probably another approach.

westonruter · 2018-03-19T20:48:43Z

So far, it looks setting the state of the AMPNotice component from outside the block might be hard. But there's probably another approach.

I think that it shouldn't actually be part of the state then in this case. Instead the AMP validation results should be stored in the store that then sends the data down as props. So then you just update the store with the validation results, and then the edit function for a block can determine (somehow) which validation errors are relevant to itself and then display them.

I'm guessing this is what you'll want to use: https://github.com/WordPress/gutenberg/tree/master/data

kienstra · 2018-03-19T20:54:00Z

Thanks

Hi @westonruter,
Thanks, that looks like a better approach.

The approach changed, and I'm going to add a response field to existing endpoints.

@todo

@todo: implement the output callback. Thanks to Weston's suggestion, This will enable displaying notices in blocks.

@todo

In new field 'amp_validation_errors'. @todo: we might not always want to output this. It's mainly useful in the Gutenberg editor.

westonruter · 2018-03-20T17:47:56Z

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

+	 * @return array|null $validation_data Validation data if it's available, or null.
+	 */
+	public static function rest_field_amp_validation( $post_data, $field_name ) {
+		$validation_post = self::get_validation_status_post( $post_data['link'] );


Excellent re-use of this function.

One key thing is that instead of $post_data['link'] it should do amp_get_permalink( $post_data['id'] ) so that it can work in paired mode.

Idea: If this post is empty or the post type is not viewable, should we take the post content and try to validate it? In other words, this:

https://github.com/Automattic/amp-wp/blob/0bd1d58451d7a78b125e8eac2f1b7d02ebfb124a/includes/utils/class-amp-validation-utils.php#L549-L560

Good idea, @westonruter. This commit abstracts that snippet above into get_existing_validation_errors(), and reuses it here. If there are no existing errors, it validates the front-end.

The only issue is that this delays the response if it validates several posts.

westonruter · 2018-03-20T17:50:28Z

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

+	 */
+	public static function add_rest_api_fields() {
+		register_rest_field(
+			array( 'post', 'page' ),


When ! amp_is_canonical() then this should be the REST object types for all post types that have amp post type support. Otherwise, it should be added for all post types which have the editor post type support.

Thanks, @westonruter. This commit implements that, and updates the tests.

westonruter · 2018-03-20T17:51:54Z

assets/js/amp-block-validation.js

+			let block = wp.blocks.unregisterBlockType( blockType );
+			let OriginalBlockEdit = block.edit;
+
+			block.edit = class AMPNotice extends wp.element.Component {


ES6 classes aren't going to work in IE11.

Thanks, @westonruter. This commit removes the ES6 class.

westonruter · 2018-03-20T17:52:31Z

assets/js/amp-block-validation.js

+
+	return module;
+
+} )( window.jQuery );


We should eliminate jQuery as being a dependency for this script. Extending Gutenberg should be jQuery-free.

Thanks, @westonruter. This commit removes the jQuery dependency.

As Weston mentioned, we can't use ES6 classes in IE11. The script is probably going to change drastically, in how it displays notices for validation errors. So remove almost all of the existing logic.

Per Weston's suggestion, if this is Native AMP (canonical), it will be all posts with 'editor' support. Otherwise, all REST objects that have 'amp' support.

westonruter · 2018-04-12T06:47:43Z

~~This is currently blocked by WordPress/gutenberg#6134~~

@gziolo

Props @gziolo

westonruter · 2018-04-12T15:29:14Z

~~Autosaves just got improved as well, so this needs to be accounted for: WordPress/gutenberg#4156~~

…dd/block-validation

westonruter · 2018-04-13T19:27:09Z

@kienstra this is now unblocked and ready for review/merge. The latest state of master in Gutenberg works with this PR (so first do git pull; npm install; npm run build for that plugin).

There will be more work to do on it of course, but the initial Gutenberg block validation seems important to get merged to start testing and using.

The easiest way to test this is to add a Custom HTML block and add a script tag to it. Upon saving you should see the notification at the top and with the block. Upon removal of the script and re-saving you should see the notice with the block disappear. There may be non-block validation errors (e.g. regarding enqueued CSS) which persist at the top and this is expected. You can ignore this warning in the console because it is regarding a problem with the Custom HTML block:

Warning: Failed prop type: You provided a value prop to a form field without an onChange handler. This will render a read-only field. If the field should be mutable use defaultValue. Otherwise, set either onChange or readOnly.

kienstra · 2018-04-16T23:33:01Z

Approved

Hi @westonruter,
Great job with the Gutenberg validation here. It displays errors where expected:

It's nice how the notice at the top of the editor says "But none are directly due to content here."

And as expected, there was no block validation error on the page created by the WP-CLI script create-gutenberg-test-post.php.

kienstra · 2018-04-16T22:03:44Z

assets/js/amp-block-validation.js

+
+			module.store = module.registerStore();
+
+			wp.data.subscribe( module.handleValidationErrorsStateChange );


Good idea to use wp.data.subscribe().

kienstra · 2018-04-16T22:10:59Z

assets/js/amp-block-validation.js

+				// Clear out block validation errors in case the block sand errors cannot be aligned.
+				wp.data.dispatch( module.storeName ).updateBlocksValidationErrors( {} );
+
+				noticeMessage += ' ' + wp.i18n._n(


Nice use of _n() to make the message clearer.

kienstra · 2018-04-16T22:20:31Z

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

+			class_exists( 'WP_Block_Type_Registry' )
+		);
+		if ( $is_gutenberg_active ) {
+			add_filter( 'the_content', array( __CLASS__, 'add_block_source_comments' ), $do_blocks_priority - 1 );


Good idea to run this at the priority of $do_blocks_priority - 1.

kienstra · 2018-04-16T22:52:28Z

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

+	 * @return array|null $validation_data Validation data if it's available, or null.
+	 */
+	public static function get_amp_validity_rest_field( $post_data, $field_name, $request ) {
+		unset( $field_name );


This doesn't look to do any harm, but is there a reason to unset() this? Maybe to clarify that it's not used in the function.

It's to prevent PhpStorm from flagging an unused function argument.

Thanks, that makes sense.

kienstra · 2018-06-05T21:20:29Z

Request For Testing

Hi @csossi,
Could you please test this?

Go to https://native-ampconfdemo.pantheonsite.io/
Create a new post
Add a Custom HTML block
Put this in the block:

<script>doThis();</script>
<button onclick="doSomething();">
	Click Me
</button>

Publish the post
Expected: there's a notice above the block, and at the top of the editor:

7. Expected: clicking "Review issues" in the top notice leads to a page "Invalid AMP Page (URL)"

csossi · 2018-06-08T14:08:45Z

verified in QA

kienstra · 2018-09-05T03:46:50Z

Request For Regression Testing

Hi @csossi,
Thanks for testing this. Could you please retest this to ensure there wasn't a regression, using the test steps above?

Also, could you please test for edge cases by creating several blocks, moving and duplicating blocks, and other combinations?

Here's the staging site:
https://paired-ampconfdemo.pantheonsite.io/

csossi · 2018-09-30T16:16:45Z

verified in QA

westonruter requested a review from kienstra March 13, 2018 17:26

westonruter mentioned this pull request Mar 13, 2018

[Gutenberg] Integrate validator with Gutenberg blocks #1009

Closed

1 task

This was referenced Mar 14, 2018

Prevent schema.org duplicates #992

Merged

Conduct a Discovery to determine support for Gutenberg blocks. #845

Closed

westonruter added 2 commits March 14, 2018 08:47

Add source comments around each Gutenberg block to track validation i…

8413457

…ssues Remove requirement that amp_debug query var have a value

Defer mustache tag replacements to right before serialization and onl…

07aeee2

…y target template elements

westonruter force-pushed the add/block-validation branch from 4a10ee4 to 07aeee2 Compare March 14, 2018 15:47

westonruter commented Mar 14, 2018

View reviewed changes

Eliminate needless use of PEG parser for adding block source comments

01691e5

kienstra self-assigned this Mar 18, 2018

Ryan Kienstra added 2 commits March 18, 2018 02:19

Revert commit that removed REST API logic.

27e8bec

Commit ba9365 removed this, as it wasn't needed for 0.7. This restores the logic for the REST API, but not the wp-cron logic. See PR #971.

Ryan Kienstra added 3 commits March 19, 2018 18:57

Rever commit 27e8b, which added REST API endpoint.

8218919

The approach changed, and I'm going to add a response field to existing endpoints.

Add amp_validation field to REST API response.

9c3568d

@todo: implement the output callback. Thanks to Weston's suggestion, This will enable displaying notices in blocks.

Output validation errors in REST API response.

ab82e8d

In new field 'amp_validation_errors'. @todo: we might not always want to output this. It's mainly useful in the Gutenberg editor.

westonruter commented Mar 20, 2018

View reviewed changes

Ryan Kienstra added 2 commits March 20, 2018 16:33

Remove jQuery dependency and ES6 class.

ea1c37a

As Weston mentioned, we can't use ES6 classes in IE11. The script is probably going to change drastically, in how it displays notices for validation errors. So remove almost all of the existing logic.

Change which post types have the added field.

96297fc

Per Weston's suggestion, if this is Native AMP (canonical), it will be all posts with 'editor' support. Otherwise, all REST objects that have 'amp' support.

Add link to validation error details in Gutenberg notice

ca6c96c

westonruter force-pushed the add/block-validation branch from ca6c96c to 8372dbd Compare April 12, 2018 07:04

Use wp.element.Fragment instead of wrangling arrays with key props

7a60509

Props @gziolo

westonruter force-pushed the add/block-validation branch from 8372dbd to 7a60509 Compare April 12, 2018 07:11

westonruter added this to the v1.0 milestone Apr 12, 2018

Merge branch 'develop' of https://github.com/Automattic/amp-wp into a…

5bf12da

…dd/block-validation

westonruter changed the title ~~[WIP] Add AMP validation checking for Gutenberg blocks~~ Add AMP validation checking for Gutenberg blocks Apr 13, 2018

kienstra approved these changes Apr 16, 2018

View reviewed changes

kienstra assigned westonruter and unassigned kienstra Apr 16, 2018

westonruter merged commit c15a368 into develop Apr 16, 2018

westonruter deleted the add/block-validation branch April 16, 2018 23:58

westonruter moved this from In progress to Ready for QA in v1.0 Apr 17, 2018

kevincoleman added v1.0-beta1 Release labels May 31, 2018

kienstra assigned csossi Jun 5, 2018

csossi moved this from Ready for QA to Ready for Merging in v1.0 Jun 8, 2018

kienstra moved this from Ready for Merging to Ready for QA in v1.0 Sep 5, 2018

csossi moved this from Ready for QA to Ready for Merging in v1.0 Sep 30, 2018

csossi removed their assignment Sep 30, 2018

kienstra moved this from Ready for Merging to In Production in v1.0 Dec 11, 2018

swissspidy mentioned this pull request Feb 21, 2019

AMP Stories: Use Gutenberg ESLint config #1894

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AMP validation checking for Gutenberg blocks #1019

Add AMP validation checking for Gutenberg blocks #1019

westonruter commented Mar 13, 2018 •

edited

Loading

westonruter commented Mar 13, 2018

kienstra commented Mar 13, 2018

westonruter commented Mar 13, 2018

westonruter commented Mar 14, 2018

westonruter Mar 14, 2018

westonruter commented Mar 14, 2018

westonruter commented Mar 19, 2018

kienstra commented Mar 19, 2018 •

edited

Loading

westonruter commented Mar 19, 2018

kienstra commented Mar 19, 2018

westonruter Mar 20, 2018

kienstra Mar 21, 2018

westonruter Mar 20, 2018

kienstra Mar 21, 2018

westonruter Mar 20, 2018

kienstra Mar 21, 2018

westonruter Mar 20, 2018

kienstra Mar 21, 2018

westonruter commented Apr 12, 2018 •

edited

Loading

westonruter commented Apr 12, 2018 •

edited

Loading

westonruter commented Apr 13, 2018

kienstra commented Apr 16, 2018 •

edited

Loading

kienstra Apr 16, 2018

kienstra Apr 16, 2018

kienstra Apr 16, 2018

kienstra Apr 16, 2018

westonruter Apr 16, 2018

kienstra Apr 16, 2018

kienstra commented Jun 5, 2018

csossi commented Jun 8, 2018

kienstra commented Sep 5, 2018

csossi commented Sep 30, 2018


		module.store = module.registerStore();

		wp.data.subscribe( module.handleValidationErrorsStateChange );

Add AMP validation checking for Gutenberg blocks #1019

Add AMP validation checking for Gutenberg blocks #1019

Conversation

westonruter commented Mar 13, 2018 • edited Loading

westonruter commented Mar 13, 2018

kienstra commented Mar 13, 2018

westonruter commented Mar 13, 2018

westonruter commented Mar 14, 2018

Choose a reason for hiding this comment

westonruter commented Mar 14, 2018

westonruter commented Mar 19, 2018

kienstra commented Mar 19, 2018 • edited Loading

westonruter commented Mar 19, 2018

kienstra commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Apr 12, 2018 • edited Loading

westonruter commented Apr 12, 2018 • edited Loading

westonruter commented Apr 13, 2018

kienstra commented Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra commented Jun 5, 2018

csossi commented Jun 8, 2018

kienstra commented Sep 5, 2018

csossi commented Sep 30, 2018

westonruter commented Mar 13, 2018 •

edited

Loading

kienstra commented Mar 19, 2018 •

edited

Loading

westonruter commented Apr 12, 2018 •

edited

Loading

westonruter commented Apr 12, 2018 •

edited

Loading

kienstra commented Apr 16, 2018 •

edited

Loading