Remove the div wrapper from the aligned image blocks #38657
Conversation
youknowriad
added
[Type] Enhancement
youknowriad
requested review from
Mamaduka,
scruffian,
jasmussen,
MaggieCabrera,
carolinan,
jorgefilipecosta and
a team
youknowriad
requested review from
ajitbohra,
spacedmonkey and
TimothyBJacobs
as code owners
| * @return string Filtered block content. | ||
| */ | ||
| function gutenberg_restore_image_outer_container( $block_content, $block ) { | ||
| $image_with_align = '/(^\s*<figure\b[^>]*)wp-block-image\s([^"]*((alignleft)|(alignright)|(aligncenter))(\s|")[^>]*>((.|\S|\s)*)<\/figure>)/U'; |
There was a problem hiding this comment.
nitpick (non-blocker): We can probably return earlier if the block isn't an image or doesn't have theme.json.
if (
'core/image' !== $block['blockName'] ||
WP_Theme_JSON_Resolver::theme_has_support()
) {
return $block_content;
}P.S. Sorry, I'm also not an expert here.
There was a problem hiding this comment.
yeah, that's what it does already but since this is just an assignment, I think it's fine. (the variable is also used for the early return)
There was a problem hiding this comment.
Notes from reviewing the regular expression:
-
Word boundary needed before "wp-block-image", otherwise you would match
<figure class="not-wp-block-image […]. Fix:\bwp-block-image. -
The RE doesn't target the
classattribute specifically; shouldn't it? Right now I can match<figure not-class=".... -
No parens needed around each string:
(alignleft|alignright|aligncenter) -
Many of the remaining groups don't need capturing: we only care about
$match[1]and$match[2], so the above example could become(?:alignleft|alignright:aligncenter), and same for(?:\s|"). -
((.|\S|\s)*)is amusing!\S|\smeans "I want to match something as well as its opposite", i.e. "everything", which is the same as.. So, unless I missed something, the outer group could be.* -
(\s|"): I'm not sure what purpose this serves. Oh, maybe it's used as a word boundary after the alignment class? Right, and now I notice that there is missing word boundary at the start of the alignment class. You'll see that right now you're able to (incorrectly) matchzzzzalignleft. So let's use proper word boundaries on both sides. Fix:\b(?:alignleft|alignright|aligncenter)\b. -
(I don't think I'd ever seen the U (ungreedy) flag in use, that's cool)
Based on all of the above except for the fact that we don't target the class attribute, the RE could be:
/(^\s*<figure\b[^>]*)\bwp-block-image\b([^"]*\b(?:alignleft|alignright|aligncenter)\b[^>]*>.*<\/figure>)/U
I've tested this in the venerable regexr.com, but not with live content, so take my review accordingly. :)
There was a problem hiding this comment.
For the first remark, it could also be "wp-block-image, is " a word boundary?
There was a problem hiding this comment.
2- It could target class to be 100% accurate but I didn't think it was mandatory (like group blocks).
5- ((.|\S|\s)) is amusing! \S|\s means "I want to match something as well as its opposite", i.e. "everything", which is the same as .. So, unless I missed something, the outer group could be .
Indeed, but for some reason in the site I was using . was not catching everything (I think related to multiline)
6- (\s|") this one targets a space or end of attribute (if it's the last class)
There was a problem hiding this comment.
For the first remark, it could also be "wp-block-image, is " a word boundary?
That should work just as well, if you're sure that wp-block-image is always the first class.
Indeed, but for some reason in the site I was using . was not catching everything (I think related to multiline)
Hm, interesting. Take my advice with caution, then. :) But make sure the trick is really necessary.
|
Size Change: +9 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
|
From a first glance this works really well, with editor and frontend looking right:
The containing div remains in the editor, and is likely responsible for the margin inconsistency. This is also likely what #38613 will fix as a part 2. But on the frontend, things are looking beautiful: In trunk, things look largely the same in the editor, but a big difference can be seen in TT2, where this patch fixes an issue without the theme having to do anything at all. Here's trunk TT2 frontend: Now there's an argument to be made that containing floats in the main column is desirable. But that's still possible with the div-less markup, and for block themes it's arguably more important that editor matches frontend, which this PR addresses. |
Yes, that's the role of the other PR #38613 which hopefully we can get land right after we merge this one (if we merge this one) |
|
This looks great with emptytheme: In twentytwentytwo things are different between the back and front end, but I think that's because of alignment styles in the theme. |
There was a problem hiding this comment.
✅ Markup is updated correctly.
✅ Markup restoration for Classic Themes works as well.
✅ No extra div on the front-end for themes with theme.json.
I agree with Riad that more testing would be needed to land this change.
We should probably post a dev note for this change once both PRs are merged.
If that's the case (and this PR gets merged), would someone mind opening a Trac issue to ensure this gets followed up on in Twenty Twenty-Two? |
|
I'm not sure I see anything different between backend and frontend in 2022 (aside the zeroed margins which is fixed in the linked PR), @scruffian do you have some example content to test with? |
|
@youknowriad, I used this example by @MaggieCabrera for testing - https://gist.github.com/MaggieCabrera/799d09c819616354a1b8eb3605f4fc69. |
|
@Mamaduka In that markup I don't see any image aligned left/right or center, meaning the current PR shouldn't have any impact there. |
|
I changed alignment for a few images, just for this PR 😄 |
I've been doing some more testing and I can't replicate the issue now. I think I must have had a bad build before. |
youknowriad
force-pushed
the
simplify/image-block-alignment
branch
from
cbb99e8
to
f625c68
Compare
|
Ok I'm considering merging this PR, let me know if you have any reservations. |
|
We know from past experience that in the cases where we can keep the markup the same, there are many benefits to be had. Especially for base ingredients like an image, there's all the more value to the simplicity this can bring. At this moment, block themes offer us a way to make the change happen. Merging it sooner can help us unearth side effects. |
youknowriad
force-pushed
the
simplify/image-block-alignment
branch
from
f625c68
to
bc851aa
Compare
|
@youknowriad Does this allow us to remove the extra wrapper from the editor as well? As far as I remember, this was specifically there for image block compatibility. gutenberg/packages/block-editor/src/components/block-list/block.js Lines 122 to 135 in cab8cc5 |
The comment about the main column, that should still be possible to achieve with the wrapping |
|
|
||
| $updated_content = preg_replace_callback( | ||
| $image_with_align, | ||
| static function( $matches ) { |
There was a problem hiding this comment.
What's the effect of the static keyword here? I thought static anonymous function declarations only mattered in the context of a class (docs).
There was a problem hiding this comment.
I don't know it was copied from the "group" implementation but If I'm not wrong most of these static were introduced in a patch to support php 8 or something.
| return $updated_content; | ||
| } | ||
|
|
||
| add_filter( 'render_block', 'gutenberg_restore_image_outer_container', 10, 2 ); |
There was a problem hiding this comment.
Was there a reason for not using render_block_core/image as the filter?
There was a problem hiding this comment.
No particular reason, I guess my reasoning is that this change is tied to the "layout support" so I put here similarly to the previous one we have for group blocks. (same use-case)
There was a problem hiding this comment.
Or maybe you mean why not use the render_block_core/image filter name? If that's the case, I just didn't know that these block specific filters exist.
There was a problem hiding this comment.
Yep, sorry for not being clear here. The filter was introduced in 5.7, see https://core.trac.wordpress.org/changeset/50123. It basically allows you to skip the 'core/image' !== $block['blockName'] check and only runs if the block is actually used on a page.
There was a problem hiding this comment.
Nice 👍 We should probably use it for both image and group filters.
There was a problem hiding this comment.
Opened a follow-up here #38657
Related #38613
For historic reasons, the image block had a div wrapper around the block when it was left/right or center aligned (Frontend only). This has a number of issues:
In this PR, I'm updating the image block to remove that extra div and make the block work like any other block. That said, and since this block has been like that for a long time, I'm dynamically restoring that div in the frontend for classic themes (themes without theme.json and layout support). The idea is that if you don't use the layout, you're providing your own alignment styles, so we should limit the impact with your theme's existing styles for backward compatibility.
Testing instructions
This is a somewhat impactful change that we need to check properly:
I'd appreciate a lot of help testing this PR cause it can be impactful.