Media & Text: don't use background-image #64981
Conversation
sgomes
added
[Type] Enhancement
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +617 B (+0.03%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
sgomes
force-pushed
the
update/media-text-block-always-use-img
branch
4 times, most recently
from
c6d83a8 to
5a50960
Compare
sgomes
force-pushed
the
update/media-text-block-always-use-img
branch
from
5a50960 to
9d10e74
Compare
|
Rebased on trunk to see if that gets around the native build errors. |
|
Issue for this - #52789 |
|
Looking into the failing tests. Looks like this is another set of tests I wasn't aware of, that aren't part of Edit: PHP tests updated in a374697. Everything is now green. |
There was a problem hiding this comment.
Thanks for the PR! It took me some time to look and will still more reviews.
I've left some comments and in my testing I saw some differences in the front end. I had
block with fill:
- with featured image
- one simple image
On the front end the featured image one got the classes but not the styles and the other one didn't get the classes change(the v8 one).
test/integration/fixtures/blocks/core__media-text__image-fill-no-focal-point-selected.html
Show resolved
Hide resolved
| @@ -243,7 +243,7 @@ function MediaTextEdit( { | |||
| 'is-selected': isSelected, | |||
| 'is-stacked-on-mobile': isStackedOnMobile, | |||
| [ `is-vertically-aligned-${ verticalAlignment }` ]: verticalAlignment, | |||
| 'is-image-fill': imageFill, | |||
| 'is-image-fill-v8': imageFill, | |||
There was a problem hiding this comment.
I'm not sure we can avoid the new class name in order to keep back compat for older versions. @Mamaduka do you see another way around for this? 🤔
If we can't avoid it let's find a better name without the version number.
There was a problem hiding this comment.
Perhaps something that references the new mechanism we're using for these? Here are some ideas:
is-image-fill-no-bgis-image-fill-objectis-image-fill-element
Alternatively, we could take the opportunity to align more with the wording in the UI ("Crop image to fill"), so:
is-image-crop-to-fillis-image-cropped
There was a problem hiding this comment.
As for why I opted for a new classname in the first place, let me explain in detail.
The issue is that we need new styles because of the markup changes, and these styles are incompatible with the previous ones because of the nature of these changes (changing display behaviour for the <img> element).
We want posts with the old markup to keep working with new releases, without requiring any user intervention, because it's unacceptable to break them.
This leaves us with three options, as I understand:
Replace the classname
The idea here is to create a new classname for the "Crop image to fill" option, to replace the previous one. We keep the old styles under the old name as well, to ensure backward compatibility.
Markup example:
<div class="wp-block-media-text is-image-fill-v8">
This is the option I went with in this PR.
Add another classname
The idea here is to keep the old classname for the "Crop image to fill" option, but add a new one to indicate the new markup is being used. We keep the old styles as-is under the old name, to ensure backward compatibility; the new styles override them.
Markup example:
<div class="wp-block-media-text is-image-fill is-v8">
I opted against this approach, since it would have meant additional CSS to return the properties changed in is-image-fill to their default values, and it didn't seem to come with significant benefits that I could think of at the time.
There is one that didn't occur to me, however. Adding a new class would mean that we wouldn't need to remove the old classname when rendering the block dynamically, which would mean slightly less confusing PHP code.
Upgrade the markup
Another option would be to treat the block as dynamic, and rewrite its content in PHP if the "Crop image to fill" option is enabled. This would ensure the new markup is always used, removing the need to preserve backwards compatibility with styles. This means we could drop the old styles, and simply have the same classname with the new styles.
Currently, this block is only dynamic when using a featured image. I opted against this approach because it would have meant further usage of dynamic blocks, which can result in increased server processing times.
Other approaches?
Are there any other approaches I'm not aware of that we should be considering?
There was a problem hiding this comment.
Unless we get feedback for a possible different approach, the new class name seems fine to me. Maybe the name could be something like is-image-fill-element you suggested, but this is the easiest change we can do later, before merging.
Thank you for taking a look, @ntsekouras! 🙏 Is there any chance you could share the markup you used in your testing? I'm unable to reproduce your findings; upgrades seem to consistently work well for me in all my tests. From your description it sounds like perhaps the PHP code got updated when you switched branches, but the JS / CSS did not get built? |
There was a problem hiding this comment.
From your description it sounds like perhaps the PHP code got updated when you switched branches, but the JS / CSS did not get built?
That was a false alarm because for some reason the updated css files wouldn't load on front end, as they did in backend.
This looks good to me to land! Thank you Sérgio! Let's just update the new class name and 🚢 with a green CI.
Co-authored-by: Nik Tsekouras <[email protected]>
|
Thank you so much for the review, @ntsekouras! 🙌 I've addressed every comment, renamed the new class, and everything is looking nice and green, so I'm going to go ahead and merge it! 👍 |
fabiankaegy
added
the
Needs Dev Note
|
Hey @sgomes 👋 Would you be able to help write a dev note for this for the 6.7 release? We are planning to have this as part of a larger Miscellaneous Editor Updates note. We are hoping to get all drafts in by October 13th to leave some time for reviews before the RC1. All Dev Notes get tracked in #65784 so feel free to leave a note there or ping me directly :) Please let us know if you can assist with that. Thanks in advance :) |
|
Hey @fabiankaegy! 👋 Sure, happy to help! Do you happen to know of any dev notes for similar situations; i.e., updating a block such that a new deprecation was needed? I've never written a dev note before, so it would be great to have something to base it off of! |
|
Hey @sgomes 👋 here is an example of something similar that happened with the cover block a good while back: https://make.wordpress.org/core/2021/02/24/changes-to-block-editor-components-and-blocks/ there are also some links to guides for writing dev notes in the tracking issue :) |
|
Thank you so much, @fabiankaegy! 🙌 I'll add the note asap 👍 |
What?
The Media & Text block includes an
<img>element as part of its markup. However, when the "Crop image to fill" option is enabled, it uses a background image instead for the actual render (the<img>getting hidden with styles).This PR drops the background image, instead using the
<img>exclusively. While the markup/styles will be different, blocks should render the same visually.There are no changes to the UI.
Why?
Background images are always fetched eagerly by the browser, which invalidates the lazy loading that WordPress applies to content images where adequate. It also invalidates other performance improvements, such as being able to choose from multiple image sizes (only the original is provided in
background-image).By making this change, we ensure that Media & Cover blocks don't introduce performance issues when the "Crop image to fill" option is enabled.
How?
The PR moves to using an
object-fillandobject-positionapproach on the<img>element, instead of usingbackground-imageandbackground-positionon the parent.For backwards-compatibility reasons, it switches to a new classname for the "Crop image to fill" option, namely
is-image-fill-element, so that older markup can still render correctly.As part of this, a number of changes were needed:
I also took the opportunity to clean things up a bit by moving
imageFillStyles()out ofmedia-container.jsand into its own module. This avoids the need to re-export it in native, when it's not used there at all.Testing Instructions
Initial setup
Testing backwards compatibility and PHP rendering
background-imageinline styles.Testing the editor