Rework how and whether galleries are rendered as carousels

[westonruter]

Summary

This was co-authored with @pierlon.

Fixes #4774

• Default a gallery shortcode to use carousel only if in legacy Reader mode; otherwise, render shortcode as normal in Standard/Transitional modes. In any mode, allow the presence of the amp-carousel shortcode attribute to override the default behavior.
• Add the carousel toggle to the Gallery block in legacy Reader mode, and default to enabled.
• Eliminate logic which attempts to inject amp-carousel and amp-lightbox attributes into the gallery shortcode block. Remove the toggles from the block settings. Adding galleries via the shortcode block should not need special block settings toggles, as these attributes should be added directly by the author. Galleries normally will only be added via the Gallery block or the Classic block.
• Eliminate injection of data- attributes into the block markup when layout, no-loading, lightbox, or carousel are enabled. Inject these rather via the render_block filter. These are added merely as processing instructions for the sanitizers, and there is no need to duplicate the settings in both the block attributes and the attributes of the block's container element saved to the DB. This will need block deprecation logic added.
• Fix some propTypes for AMP attributes.
• Use rest_sanitize_bool() rather than filter_var().
• If the gallery is to be transformed to amp-carousel and also specifies the lightbox attribute, only the amp-carousel will possess said attribute. This means the gallery would use it's own lightbox; it is not shared with the other galleries and images on the page.
  • Rich (HTML) image captions are now supported.
  • Galleries embedded via shortcodes will default to the large size, if none is specified.

Todo

• Fix Gutenberg block validation errors which occur when carousel or lightbox toggles are flipped for shortcode blocks or gallery blocks.
• Ensure shortcodes and blocks that are not rendered as carousels have good default styling in classic Reader mode templates.
• Update tests.
• Update the above summary.
• Prevent block validation errors from occurring when upgrading to 2.0.

Screenshots of Classic Block

<p>Gallery without any <code>amp-carousel</code> attribute:</p>
<p>[gallery ids="2128,755,757"]</p>
<p>Gallery with <code>amp-carousel=true</code> attribute:</p>
<p>[gallery ids="2128,755,757" amp-carousel=true]</p>
<p>Gallery with <code>amp-carousel=false</code> attribute:</p>
<p>[gallery ids="2128,755,757" amp-carousel=false]</p>

Non-AMP	Reader Mode	Before Transitional 👎	After Transitional 👍

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]

@pierlon Could you finish this one out?

[pierlon]

Sure will do.

[googlebot]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[pierlon]

@googlebot I consent.

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[pierlon]

The PR description needs to be updated, but the PR is ready for review nonetheless.

[github-actions]

Plugin builds for 9d7260f are ready 🛎️!

• Download development build
• Download production build

[westonruter]

I don't understand how this works. The default is changed here, but when is this getting read? It's not part of the below code that does cloneElement().

This seems like the not ideal place to put this code as it is causing side effects. Something makes me nervous, like is this expected to work properly if old unmigrated posts are pasted into the editor? I just tried it and it seemed to work, but why?

Humm, and actually, when in legacy Reader mode and I edit a post that originally had:

<!-- wp:gallery {"ids":[1869,770,760],"ampLightbox":true} -->

It gets migrated as:

<!-- wp:gallery {"ids":[1869,770,760],"ampCarousel":false,"ampLightbox":true} -->

Why is ampCarousel false? The call to select( 'amp/block-editor' ).hasThemeSupport() is returning false so then the default value of the ampCarousel attribute should rather be true, right?

[johnwatkins0]

This isn't taking effect because the save callback containing this line runs some time after settings.attributes is read, so I think some of this should be abstracted out and run in the filter callback rather than in the save function.

[westonruter]

But it does seem to work. What filter callback could be used?

[johnwatkins0]

I am not seeing a good way to do it entirely on the JS side, so I proposed a possible workaround here: #4775 (review)

[pierlon]

Re #4775 (comment):

I don't understand how this works. The default is changed here, but when is this getting read? It's not part of the below code that does cloneElement().

Previously, the ampCarousel attribute was being defaulted to true when using legacy reader mode. So if the ampCarousel attribute was not set on the block, that meant blockAttributes in the save() method will have it set to true, and it would then be appended to the deprecatedProps object, which should not be the case.

So the reasoning behind the change was to set the ampCarousel attribute to always be false, and then update the default value we know that the attribute did not have a truthy value set on the block. As I understand, the save() method is called when the block is to be parsed and serialized, and the parsing of the block is what we want to filter so it can be updated accordingly.

The change I made in 6687931 is indeed hacky and warrants being nervous about; I haven't done much testing with it outside of the bug I was trying to fix. I'll try to see if there is any robust alternative to replicate the fix, but for now I defer to the workaround you provided in #4775 (comment).

[westonruter]

I'm pushing up a commit to introduce a new Service to formalize that. Let's see how it works…

[westonruter]

Please check b2901f0. I tried to explain as best I could the reason for the service, trying to recall the history of these attributes.

[johnwatkins0]

Tested this a bit, and the only issue I'm finding is the one Weston pointed out here: #4775 (comment) It's going to be hard to find the best place on the JS side to set that default conditionally. Perhaps a boolean could be passed from the backend (with wp_add_inline_script) indicating whether legacy reader is active. Then in addAMPAttributes that value could be used to determine ampCarousel's default.

As for the deprecation approach, I kinda like the (albeit hacky) backend approach that Weston proposed earlier. Since the data attributes are originally provided on the backend via a PHP filter -- in a way that Gutenberg isn't designed to be aware of -- it makes sense to me for the backend to take responsibility for cleaning it up. But if the solution implemented here is working, then no need to change it.

[westonruter]

Perhaps a boolean could be passed from the backend (with wp_add_inline_script) indicating whether legacy reader is active. Then in addAMPAttributes that value could be used to determine ampCarousel's default.

I think this is what we already have now with select( 'amp/block-editor' ).hasThemeSupport(), no?

We basically need the reverse of this now-deleted code:

addFilter( 'blocks.getSaveContent.extraProps', 'ampEditorBlocks/addExtraAttributes', addAMPExtraProps );

If not using PHP for this, and if not using deprecated, we'd need some way to hook into the block parsing routine to purge those old attributes from the innerHTML before the block is validated.

[johnwatkins0]

If not using PHP for this, and if not using deprecated, we'd need some way to hook into the block parsing routine to purge those old attributes from the innerHTML before the block is validated.

Unfortunately as far as I know the last filter that runs on post content before it reaches the editor is in the REST handler.

1. This runs immediately after the post is received from the REST API: https://github.com/WordPress/gutenberg/blob/95e4f3f06a449b9f8dd44654b5f9443c465ba354/packages/editor/src/store/actions.js#L41
2. That passes the post content here: https://github.com/WordPress/gutenberg/blob/95e4f3f06a449b9f8dd44654b5f9443c465ba354/packages/blocks/src/api/parser.js#L611 And by the time the content goes through parseImplementation the original content is no longer being used. Unfortunately there isn't a filter in here. One of the 1000 places in Gutenberg where I wish there were a filter/action 😄

[westonruter]

Since the data attributes are originally provided on the backend via a PHP filter -- in a way that Gutenberg isn't designed to be aware of -- it makes sense to me for the backend to take responsibility for cleaning it up.

@johnwatkins0 Actually, the data attributes were not originally added via PHP filter. They were added in JS via the blocks.getSaveContent.extraProps filter. With this PR, the injection via JS is eliminated in favor of injection with PHP via the render_block filter. The problem then is we have these orphaned JS-injected data-* attributes stuck in the post_content. If we had something like blocks.getParsedContent.extraProps to filter to a block's root HTML attributes then we could undo what used to be done in JS, but in lieu of that being available (at least now), perhaps the PHP-based route will make us less nervous?

[westonruter]

The PRs that first introduced storage via the data- attributes are #1026 and #1121.

[westonruter]

To surface the PhpDoc comment from ObsoleteBlockAttributeRemover:

Removes obsolete data-amp-* attributes from block markup in post content.

These HTML attributes serve as processing instructions to control how the sanitizers handle converting HTML to AMP.
For each HTML attribute there is also a block attribute, so if there is a data-amp-carousel HTML attribute then there
is also an ampCarousel block attribute. The block attributes were originally mirrored onto the HTML attributes because
the render_block filter was not available in Gutenberg (or WordPress Core) when this was first implemented; now that
this filter is available, there is no need to duplicate/mirror the attributes, and so they are injected into the
root HTML element via AMP_Core_Block_Handler::filter_rendered_block(). In hindsight, instead of having the data
mirrored between block attributes and HTML attributes, the block attributes should have perhaps used an attribute
as the block attribute source. Then again, that may have complicated things yet further to migrate away from using
these data attributes. A key reason for why these HTML data-* attributes are bad is that they cause block validation
errors. If someone creates a Gallery block and enables a carousel, then if they go and deactivate the AMP plugin,
this block will then show as having block validation errors. If, however, we restrict the block attributes to only
be in the block comment, then no block validation errors occur. Also, since the render_block filter is now
available, the reason for storing these block attributes as data-amp-* HTML attributes in post_content is now obsolete.

[westonruter]

With the current state of the PR, here's what I see before/after in legacy Reader mode. No block validation errors and no regressions in rendering (even bugfix with gallery caption now appearing).

Screen	Before	After
Editor
Frontend

And here is the difference if I switch to Transitional mode:

Screen	Before	After
Editor
Frontend

Importantly, note that the galleries are now shown as being not carousels in Transitional mode. This is intentional because in legacy Reader mode, no Carousel toggle was available (as it was forced to be on). But now in Transitional mode, the Carousel defaults to false. Compare an unmigrated Gallery block in these three states:

Legacy Reader on Develop	Legacy Reader on Branch	Transitional on Branch

Notice how in develop there is no toggle even presented for Carousel. In this branch, legacy Reader mode at least allows it to be toggled but it defaults to enabled, but then in Transitional the toggle defaults to being disabled.

This seems to be working as expected/desired.

[pierlon]

I notice in legacy Reader mode that if you have a post with:

<!-- wp:gallery {"ids":[752,586],"ampCarousel":true} -->

it's migrated as:

<!-- wp:gallery {"ids":[752,586]} -->

The ampCarousel attribute should still be set, but due to the block attribute defaulting to true, that does not seem to be the case.

[pierlon]

@westonruter Otherwise from that, the Service has been working as intended during my testing so far.

[westonruter]

The ampCarousel attribute should still be set, but due to the block attribute defaulting to true, that does not seem to be the case.

Yeah, this threw me a bit as well. However, I think it is expected. It's because the default value of the attribute is true and since it is the same as the default, it gets omitted from the serialized block attributes. This is the logic in question:

https://github.com/WordPress/gutenberg/blob/b9de73ddb58b6b88faa4846d3f8294c815c9d4df/packages/blocks/src/api/serializer.js#L193-L199

So I think this is OK.

The only area where I see it being a problem is if you keep switching back and forth between legacy Reader mode and a non-legacy mode, which should not be normal.

[johnwatkins0]

Actually, the data attributes were not originally added via PHP filter.

Ah, I see now. Yeah, in the absence of a JS filter to modify a block's raw HTML on the way into the editor, I do think handling in the REST endpoint is the most solid approach. The deprecation approach was working, but it does make me a little nervous to apply deprecation logic to a block we don't control.

[westonruter]

If nothing else, through this process we've had a good education to some block internals!

[pierlon]

What a journey! Much has been learnt from working on this PR and it will definitely help in the future. I'm comfortable with how the obsolete attributes are removed from the post so it's a 👍 from me.

[westonruter]

Commit 6eb20db prevents propagating the block attributes as HTML data attributes for shortcodes, preventing this from happening:

There's no need to do such propagation since the shortcode attributes area already present and we're not stripping them out like we are the data attributes for block root elements.

[westonruter]

Merging in spite of unrelated TikTok oEmbed external HTTP request failure.