VideoPress Block: Retain alignment support #16651

ockham · 2020-07-30T16:30:24Z

The VideoPress block wraps around the core/video block. 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

The VideoPress block inherits the supports field from the core/video block, but somehow in the process, the alignment support is lost -- that is issue #16158.

It turns out that handling of that align supports 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

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

The reason for alignment support not carrying over to the VideoPress block 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 just called in the wrong order.

Fixes #16158
Supersedes #16627

props @yansern for the original PR #16627 and underlying research (Automattic/wp-calypso#43252).

Changes proposed in this Pull Request:

This PR fixes the issue by assigning a higher priority to the VideoPress block's blocks.registerBlockType filter -- arguably, our block-level modifications should run before the more generic modifiers that insert extra class names. Furthermore, this solution will also cover other supports fields that use the same mechanism (e.g. anchor) without requiring adding those attributes explicitly in the VideoPress block.

Jetpack product discussion

N/A

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.
Publish the blog post.
Refresh the editor page.
Verify that there is no block validation error.
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 VideoPress block alignment not respected in the block editor.

jetpackbot · 2020-07-30T16:47:21Z

	Warnings
⚠️	The PR is missing at least one `[Status]` label. Suggestions: `[Status] In Progress`, `[Status] Needs Review`

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-16651

Generated by 🚫 dangerJS against 1dd70fd

ockham · 2020-07-30T18:34:11Z

Not sure why the wpcom job (Fusion) is failing, the files seem to be in sync. Maybe it's some newline-at-EOF problem 🙄
I'm inclined to force-set it to Green and create the WP.com diff manually.

yansern

This solution is much more elegant! I've tested it on my sandbox and it is working fine!

matticbot · 2020-07-31T10:33:29Z

Caution: This PR has changes that must be merged to WordPress.com
Hello ockham! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D47361-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

jeherve

Tests well for me.

ockham · 2020-08-03T11:09:03Z

Thanks Jan and Jeremy!

ockham · 2020-08-03T11:09:41Z

~~Going to file a WP.com counterpart patch. I'll drop a link here for reference.~~

@jeherve pointed me to the Fusion comment (thanks!): #16651 (comment) -- it did produce a WP.com patch after he forced it 🙂

jeherve · 2020-08-03T12:18:25Z

r211514-wpcom

…ic/jetpack into add/jetpack-lazy-images-package * 'add/jetpack-lazy-images-package' of github.com:Automattic/jetpack: (40 commits) Lodash: Revert to previous version (#16735) New class to handle async XML-RPC requests (#16674) Social Previews: Sidebar design updates (#16725) Update E2E locator for classic connection flow (#16708) Woo Services: update to use existing Jetpack plugin install tools (#16672) Admin Page: avoid blank dashboard when notice is not a string (#16721) Admin Page: update string still using old i18n format (#16722) Social Previews: Add sidebar UI (#16633) Fix recurring payments block disconnecting (sometimes) when existing article is reopened in block editor. (#16640) Connection Register: Add current user email to connection register request (#16712) Update versions to start 8.9 Release cycle (#16706) Donations block: Make currency and amounts editable (#16593) Update dependency @automattic/calypso-color-schemes to v2 Error Notice: removing HTML code, adjusting maximum width. (#16690) Update dependency swiper to v6 (#16587) Site Scan: Short-circuit update_threats_link() if Admin Bar is not present (#16677) Update vulnerable NPM packages (#16659) E2E Tests: Add Jetpack updater test (#16437) check for subdir site before rendering Ads.txt section (#16671) VideoPress Block: Retain alignment support (#16651) ...

The VideoPress block wraps around the `core/video` block. 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 The VideoPress block [inherits the `supports` field](https://github.com/Automattic/jetpack/blob/2ec6da0446c15fd0c87ac1c799c4075063db5598/extensions/blocks/videopress/editor.js#L158-L161) from the `core/video` block, but somehow in the process, the alignment support is lost -- that is issue #16158. It turns out that handling of that `align` `supports` 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](https://github.com/WordPress/gutenberg/blob/6cce6e0fdbb27d496370d48305913523e3251ea1/packages/block-editor/src/hooks/align.js#L228-L232) the [`align` attribute](https://github.com/WordPress/gutenberg/blob/6cce6e0fdbb27d496370d48305913523e3251ea1/packages/block-editor/src/hooks/align.js#L80-L102) to the block 2. [inject](https://github.com/WordPress/gutenberg/blob/6cce6e0fdbb27d496370d48305913523e3251ea1/packages/block-editor/src/hooks/align.js#L243-L247) the [`align{left|right|center}` class name into the saved HTML](https://github.com/WordPress/gutenberg/blob/6cce6e0fdbb27d496370d48305913523e3251ea1/packages/block-editor/src/hooks/align.js#L200-L226). The reason for alignment support not carrying over to the VideoPress block is that we're [attaching](https://github.com/Automattic/jetpack/blob/2ec6da0446c15fd0c87ac1c799c4075063db5598/extensions/blocks/videopress/editor.js#L199) the VideoPress wrapper to the same filter that Gutenberg's [`addAttribute`]((https://github.com/WordPress/gutenberg/blob/6cce6e0fdbb27d496370d48305913523e3251ea1/packages/block-editor/src/hooks/align.js#L228-L232)) filter uses to automatically inject the `align` attribute -- they're just called in the wrong order.

VideoPress Block: Retain alignment support

1dd70fd

ockham added the [Feature] VideoPress A feature to help you upload and insert videos on your site. label Jul 30, 2020

ockham self-assigned this Jul 30, 2020

matticbot added Touches WP.com Files labels Jul 30, 2020

ockham marked this pull request as ready for review July 30, 2020 17:09

ockham requested review from a team and yansern July 30, 2020 17:09

yansern approved these changes Jul 31, 2020

View reviewed changes

yansern mentioned this pull request Jul 31, 2020

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

Closed

jeherve added the [Status] Ready to Merge Go ahead, you can push that green button! label Jul 31, 2020

jeherve approved these changes Jul 31, 2020

View reviewed changes

ockham merged commit 222c0cd into master Aug 3, 2020

ockham deleted the add/align-attribute-to-videopress-block branch August 3, 2020 11:09

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Aug 3, 2020

github-actions bot added this to the 8.8 milestone Aug 3, 2020

jeherve modified the milestones: 8.8, 8.9 Aug 3, 2020

ockham mentioned this pull request Aug 4, 2020

Editor: Saved Changes in Alignment for Video Blocks are Reverted Automattic/wp-calypso#43252

Closed

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Aug 25, 2020

jeherve added a commit that referenced this pull request Aug 25, 2020

Changelog: add #16651

4a83fb6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

VideoPress Block: Retain alignment support #16651

VideoPress Block: Retain alignment support #16651

Uh oh!

ockham commented Jul 30, 2020 •

edited

Loading

Uh oh!

jetpackbot commented Jul 30, 2020

Uh oh!

ockham commented Jul 30, 2020

Uh oh!

yansern left a comment

Uh oh!

matticbot commented Jul 31, 2020

Uh oh!

jeherve left a comment

Uh oh!

ockham commented Aug 3, 2020

Uh oh!

ockham commented Aug 3, 2020 •

edited

Loading

Uh oh!

jeherve commented Aug 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

VideoPress Block: Retain alignment support #16651

VideoPress Block: Retain alignment support #16651

Uh oh!

Conversation

ockham commented Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Jetpack product discussion

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

Testing instructions:

Proposed changelog entry for your changes:

Uh oh!

jetpackbot commented Jul 30, 2020

Uh oh!

ockham commented Jul 30, 2020

Uh oh!

yansern left a comment

Choose a reason for hiding this comment

Uh oh!

matticbot commented Jul 31, 2020

Uh oh!

jeherve left a comment

Choose a reason for hiding this comment

Uh oh!

ockham commented Aug 3, 2020

Uh oh!

ockham commented Aug 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeherve commented Aug 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ockham commented Jul 30, 2020 •

edited

Loading

ockham commented Aug 3, 2020 •

edited

Loading