VideoPress: Prevent jetpack/videopress block removing align attribute from core/video block.

[yansern]

This PR fixes core/video block losing align attribute when editor is reloaded.

This is happening because jetpack/videopress block is recreating the core/video's block attributes (when it should inherit them). The align attribute was introduced in core/video's block which was not recreated by jetpack/videopress's hook.

Changes proposed in this Pull Request:

Inherit core/video's block attributes instead of recreating them.

Before: https://www.screencast.com/t/eGCIAAci4Q1l
After: https://www.screencast.com/t/fitMKygi

More debugging context here: Automattic/wp-calypso#43252

Does this pull request change what data or activity we track or use?

No

Testing instructions:

• Open up the editor to create a new post.
• Insert some paragraphs of content.
• In the middle of the paragraph, insert a Video block.
• Align the video block to the left or right.
• Save the blog post.
• Refresh the editor page.
• The video block should still be aligned to the left in the editor.

Proposed changelog entry for your changes:

• Fix video alignment not respected in the block editor.

Fixes #16158

[matticbot]

Caution: This PR has changes that must be merged to WordPress.com
Hello yansern! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D47173-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

[jetpackbot]

	Warnings
⚠️	The PR is missing at least one [Status] label. Suggestions: [Status] In Progress, [Status] Needs Review
⚠️	The Privacy section is missing for this PR. Please specify whether this PR includes any changes to data or privacy.

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-16627

Generated by 🚫 dangerJS against fb5a369

[ockham]

Could do

Suggested change

	...settings.attributes,
	...attributes,

since attributes is being destructured on R66.

[ockham]

This is unfortunately currently breaking the block for me (even without setting alignment): After saving and reloading, I get an 'Invalid block' message:

(The same doesn't happen on master.)

This is the block HTML that's saved on this branch:

<!-- wp:video {"id":329,"guid":"cKxRo2fW"} -->
<figure class="wp-block-video wp-block-embed is-type-video is-provider-videopress"><div class="wp-block-embed__wrapper">
https://videopress.com/v/cKxRo2fW?preloadContent=metadata
</div></figure>
<!-- /wp:video -->

Note that the src attribute is missing from the block.

Compare that to the block HTML on master:

<!-- wp:video {"guid":"cKxRo2fW","id":329,"src":"https://videos.files.wordpress.com/cKxRo2fW/can-search-for-a-blog-name-can-see-and-select-a-free-wordpress-com-blog-address-in-results-2019-03-11t12-50-20_hd.mp4"} -->
<figure class="wp-block-video wp-block-embed is-type-video is-provider-videopress"><div class="wp-block-embed__wrapper">
https://videopress.com/v/cKxRo2fW?preloadContent=metadata
</div></figure>
<!-- /wp:video -->

[ockham]

I think I see the issue: The core/video block extracts it src attribute from the wrapped <video /> HTML element: https://github.com/WordPress/gutenberg/blob/6cce6e0fdbb27d496370d48305913523e3251ea1/packages/block-library/src/video/block.json#L53-L55

This works, since the core/video block HTML looks like this:

<!-- wp:video {"id":322} -->
<figure class="wp-block-video"><video controls src="http://deadbeef.ngrok.io/wp-content/uploads/2020/05/can-publish-and-view-content-2019-03-11T12-50-16.mpg"></video></figure>
<!-- /wp:video -->

However, the VideoPress block HTML doesn't have a <video /> element:

<!-- wp:video {"id":329,"guid":"cKxRo2fW"} -->
<figure class="wp-block-video wp-block-embed is-type-video is-provider-videopress"><div class="wp-block-embed__wrapper">
https://videopress.com/v/cKxRo2fW?preloadContent=metadata
</div></figure>
<!-- /wp:video -->

[ockham]

This is true for most of the core/video block's attributes. I think that this means that we'll have to continue to replace them manually (with 'normal' attributes that are stored as part of the block, rather than inferred from one of its wrapped elements).

[ockham]

Some additional notes:

The core/video block sets the align supports field to true, rather than adding the attribute explicitly: https://github.com/WordPress/gutenberg/blob/6cce6e0fdbb27d496370d48305913523e3251ea1/packages/block-library/src/video/block.json#L66

Handling of that field is implemented in https://github.com/WordPress/gutenberg/blob/6cce6e0fdbb27d496370d48305913523e3251ea1/packages/block-editor/src/hooks/align.js, mostly through a number of filters that

1. add the align attribute to the block
2. inject the align{left|right|center} class name into the saved HTML.

Since the VideoPress block inherits the supports field from the core/video block, this raises the question: Why do we need to add the align attribute explicitly to make this work?

I think the answer is that we're attaching the VideoPress wrapper to the same filter that Gutenberg's addAttribute filter uses to automatically inject the align attribute -- they're probably just called in the wrong order. So we might be able to fix this problem by assigning a different priority to ours -- arguably, our block-level modifications should run before the more generic modifiers that insert extra class names.

[ockham]

That seems to work. I'll file a PR 😊

[yansern]

Superceded by #16651