Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Media & Text: don't use background-image #64981

Merged
merged 9 commits into from
Sep 4, 2024
Merged

Media & Text: don't use background-image #64981

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
03c82c2
media-text block: don't use background-image
sgomes Sep 2, 2024
d7b91d3
Add PR link to deprecated.js for future reference
sgomes Sep 2, 2024
0c7a9ea
Fix PHP indentation
sgomes Sep 2, 2024
9d10e74
Fix inconsistencies in some fixtures after markup changes
sgomes Sep 2, 2024
a374697
Update PHP tests to match new expected render
sgomes Sep 3, 2024
ac0ea5d
Add back old fixtures as new deprecated fixtures
sgomes Sep 3, 2024
f518c23
Improve deprecation comment wording in styles
sgomes Sep 4, 2024
f11a6b3
Remove 'not contains' checks from PHP tests
sgomes Sep 4, 2024
311f447
Rename new CSS class to is-image-fill-element
sgomes Sep 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
162 changes: 159 additions & 3 deletions packages/block-library/src/media-text/deprecated.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ const v1ToV5ImageFillStyles = ( url, focalPoint ) => {
: {};
};

const v6ImageFillStyles = ( url, focalPoint ) => {
const v6ToV7ImageFillStyles = ( url, focalPoint ) => {
return url
? {
backgroundImage: `url(${ url })`,
Expand Down Expand Up @@ -198,6 +198,20 @@ const v6Attributes = {
},
};

const v7Attributes = {
...v6Attributes,
align: {
type: 'string',
// v7 changed the default for the `align` attribute.
default: 'none',
},
// New attribute.
useFeaturedImage: {
type: 'boolean',
default: false,
},
};

const v4ToV5Supports = {
anchor: true,
align: [ 'wide', 'full' ],
Expand Down Expand Up @@ -237,6 +251,148 @@ const v6Supports = {
},
};

const v7Supports = {
...v6Supports,
__experimentalBorder: {
color: true,
radius: true,
style: true,
width: true,
__experimentalDefaultControls: {
color: true,
radius: true,
style: true,
width: true,
},
},
color: {
gradients: true,
heading: true,
link: true,
__experimentalDefaultControls: {
background: true,
text: true,
},
},
interactivity: {
clientNavigation: true,
},
};

// Version with 'none' as the default alignment.
// See: https://github.com/WordPress/gutenberg/pull/64981
const v7 = {
attributes: v7Attributes,
supports: v7Supports,
usesContext: [ 'postId', 'postType' ],
save( { attributes } ) {
const {
isStackedOnMobile,
mediaAlt,
mediaPosition,
mediaType,
mediaUrl,
mediaWidth,
mediaId,
verticalAlignment,
imageFill,
focalPoint,
linkClass,
href,
linkTarget,
rel,
} = attributes;
const mediaSizeSlug =
attributes.mediaSizeSlug || DEFAULT_MEDIA_SIZE_SLUG;
const newRel = ! rel ? undefined : rel;

const imageClasses = clsx( {
[ `wp-image-${ mediaId }` ]: mediaId && mediaType === 'image',
[ `size-${ mediaSizeSlug }` ]: mediaId && mediaType === 'image',
} );

let image = mediaUrl ? (
<img
src={ mediaUrl }
alt={ mediaAlt }
className={ imageClasses || null }
/>
) : null;

if ( href ) {
image = (
<a
className={ linkClass }
href={ href }
target={ linkTarget }
rel={ newRel }
>
{ image }
</a>
);
}

const mediaTypeRenders = {
image: () => image,
video: () => <video controls src={ mediaUrl } />,
};
const className = clsx( {
'has-media-on-the-right': 'right' === mediaPosition,
'is-stacked-on-mobile': isStackedOnMobile,
[ `is-vertically-aligned-${ verticalAlignment }` ]:
verticalAlignment,
'is-image-fill': imageFill,
} );
const backgroundStyles = imageFill
? v6ToV7ImageFillStyles( mediaUrl, focalPoint )
: {};

let gridTemplateColumns;
if ( mediaWidth !== DEFAULT_MEDIA_WIDTH ) {
gridTemplateColumns =
'right' === mediaPosition
? `auto ${ mediaWidth }%`
: `${ mediaWidth }% auto`;
}
const style = {
gridTemplateColumns,
};

if ( 'right' === mediaPosition ) {
return (
<div { ...useBlockProps.save( { className, style } ) }>
<div
{ ...useInnerBlocksProps.save( {
className: 'wp-block-media-text__content',
} ) }
/>
<figure
className="wp-block-media-text__media"
style={ backgroundStyles }
>
{ ( mediaTypeRenders[ mediaType ] || noop )() }
</figure>
</div>
);
}
return (
<div { ...useBlockProps.save( { className, style } ) }>
<figure
className="wp-block-media-text__media"
style={ backgroundStyles }
>
{ ( mediaTypeRenders[ mediaType ] || noop )() }
</figure>
<div
{ ...useInnerBlocksProps.save( {
className: 'wp-block-media-text__content',
} ) }
/>
</div>
);
},
};

// Version with wide as the default alignment.
// See: https://github.com/WordPress/gutenberg/pull/48404
const v6 = {
Expand Down Expand Up @@ -301,7 +457,7 @@ const v6 = {
'is-image-fill': imageFill,
} );
const backgroundStyles = imageFill
? v6ImageFillStyles( mediaUrl, focalPoint )
? v6ToV7ImageFillStyles( mediaUrl, focalPoint )
: {};

let gridTemplateColumns;
Expand Down Expand Up @@ -902,4 +1058,4 @@ const v1 = {
},
};

export default [ v6, v5, v4, v3, v2, v1 ];
export default [ v7, v6, v5, v4, v3, v2, v1 ];
10 changes: 5 additions & 5 deletions packages/block-library/src/media-text/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,11 +213,11 @@ function MediaTextEdit( {
[ isSelected, mediaId ]
);

const refMediaContainer = useRef();
const refMedia = useRef();
const imperativeFocalPointPreview = ( value ) => {
const { style } = refMediaContainer.current.resizable;
const { style } = refMedia.current;
const { x, y } = value;
style.backgroundPosition = `${ x * 100 }% ${ y * 100 }%`;
style.objectPosition = `${ x * 100 }% ${ y * 100 }%`;
};

const [ temporaryMediaWidth, setTemporaryMediaWidth ] = useState( null );
Expand All @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@sgomes sgomes Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps something that references the new mechanism we're using for these? Here are some ideas:

  • is-image-fill-no-bg
  • is-image-fill-object
  • is-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-fill
  • is-image-cropped

Copy link
Contributor Author

@sgomes sgomes Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} );
const widthString = `${ temporaryMediaWidth || mediaWidth }%`;
const gridTemplateColumns =
Expand Down Expand Up @@ -480,7 +480,7 @@ function MediaTextEdit( {
onSelectMedia={ onSelectMedia }
onWidthChange={ onWidthChange }
commitWidthChange={ commitWidthChange }
ref={ refMediaContainer }
refMedia={ refMedia }
enableResize={ blockEditingMode === 'default' }
toggleUseFeaturedImage={ toggleUseFeaturedImage }
{ ...{
Expand Down
4 changes: 3 additions & 1 deletion packages/block-library/src/media-text/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@
}

.wp-block-media-text.is-image-fill .editor-media-container__resizer,
.wp-block-media-text.is-image-fill .components-placeholder.has-illustration {
.wp-block-media-text.is-image-fill .components-placeholder.has-illustration,
.wp-block-media-text.is-image-fill-v8 .editor-media-container__resizer,
.wp-block-media-text.is-image-fill-v8 .components-placeholder.has-illustration {
// The resizer sets an inline height but for the image fill we set it to full height.
height: 100% !important;
}
Expand Down
11 changes: 11 additions & 0 deletions packages/block-library/src/media-text/image-fill.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
export function imageFillStyles( url, focalPoint ) {
return url
? {
objectPosition: focalPoint
? `${ Math.round( focalPoint.x * 100 ) }% ${ Math.round(
focalPoint.y * 100
) }%`
: `50% 50%`,
}
: {};
}
96 changes: 53 additions & 43 deletions packages/block-library/src/media-text/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,32 @@ function render_block_core_media_text( $attributes, $content ) {
return $content;
}

$has_media_on_right = isset( $attributes['mediaPosition'] ) && 'right' === $attributes['mediaPosition'];
ntsekouras marked this conversation as resolved.
Show resolved Hide resolved
$image_fill = isset( $attributes['imageFill'] ) && $attributes['imageFill'];
$focal_point = isset( $attributes['focalPoint'] ) ? round( $attributes['focalPoint']['x'] * 100 ) . '% ' . round( $attributes['focalPoint']['y'] * 100 ) . '%' : '50% 50%';
$unique_id = 'wp-block-media-text__media-' . wp_unique_id();

$block_tag_processor = new WP_HTML_Tag_Processor( $content );
$block_query = array(
'tag_name' => 'div',
'class_name' => 'wp-block-media-text',
);

while ( $block_tag_processor->next_tag( $block_query ) ) {
if ( $image_fill ) {
// The markup below uses v8+ of the block, so adjust classes accordingly.
$block_tag_processor->remove_class( 'is-image-fill' );
$block_tag_processor->add_class( 'is-image-fill-v8' );
}
}

$content = $block_tag_processor->get_updated_html();

$media_tag_processor = new WP_HTML_Tag_Processor( $content );
$wrapping_figure_query = array(
'tag_name' => 'figure',
'class_name' => 'wp-block-media-text__media',
);
$has_media_on_right = isset( $attributes['mediaPosition'] ) && 'right' === $attributes['mediaPosition'];
$image_fill = isset( $attributes['imageFill'] ) && $attributes['imageFill'];
$focal_point = isset( $attributes['focalPoint'] ) ? round( $attributes['focalPoint']['x'] * 100 ) . '% ' . round( $attributes['focalPoint']['y'] * 100 ) . '%' : '50% 50%';
$unique_id = 'wp-block-media-text__media-' . wp_unique_id();

if ( $has_media_on_right ) {
// Loop through all the figure tags and set a bookmark on the last figure tag.
Expand All @@ -46,59 +63,52 @@ function render_block_core_media_text( $attributes, $content ) {
}
if ( $media_tag_processor->has_bookmark( 'last_figure' ) ) {
$media_tag_processor->seek( 'last_figure' );
if ( $image_fill ) {
$media_tag_processor->set_attribute( 'style', 'background-image:url(' . esc_url( $current_featured_image ) . ');background-position:' . $focal_point . ';' );
} else {
// Insert a unique ID to identify the figure tag.
$media_tag_processor->set_attribute( 'id', $unique_id );
}
// Insert a unique ID to identify the figure tag.
$media_tag_processor->set_attribute( 'id', $unique_id );
}
} else {
if ( $media_tag_processor->next_tag( $wrapping_figure_query ) ) {
if ( $image_fill ) {
$media_tag_processor->set_attribute( 'style', 'background-image:url(' . esc_url( $current_featured_image ) . ');background-position:' . $focal_point . ';' );
} else {
// Insert a unique ID to identify the figure tag.
$media_tag_processor->set_attribute( 'id', $unique_id );
}
// Insert a unique ID to identify the figure tag.
$media_tag_processor->set_attribute( 'id', $unique_id );
}
}

$content = $media_tag_processor->get_updated_html();

// If the image is not set to fill, add the image tag inside the figure tag,
// and update the image attributes in order to display the featured image.
if ( ! $image_fill ) {
$media_size_slug = isset( $attributes['mediaSizeSlug'] ) ? $attributes['mediaSizeSlug'] : 'full';
$image_tag = '<img class="wp-block-media-text__featured_image">';
$content = preg_replace(
'/(<figure\s+id="' . preg_quote( $unique_id, '/' ) . '"\s+class="wp-block-media-text__media"\s*>)/',
'$1' . $image_tag,
$content
);
// Add the image tag inside the figure tag, and update the image attributes
// in order to display the featured image.
$media_size_slug = isset( $attributes['mediaSizeSlug'] ) ? $attributes['mediaSizeSlug'] : 'full';
$image_tag = '<img class="wp-block-media-text__featured_image">';
$content = preg_replace(
'/(<figure\s+id="' . preg_quote( $unique_id, '/' ) . '"\s+class="wp-block-media-text__media"\s*>)/',
'$1' . $image_tag,
$content
);

$image_tag_processor = new WP_HTML_Tag_Processor( $content );
$image_tag_processor = new WP_HTML_Tag_Processor( $content );
if ( $image_tag_processor->next_tag(
array(
'tag_name' => 'figure',
'id' => $unique_id,
)
) ) {
// The ID is only used to ensure that the correct figure tag is selected,
// and can now be removed.
$image_tag_processor->remove_attribute( 'id' );
if ( $image_tag_processor->next_tag(
array(
'tag_name' => 'figure',
'id' => $unique_id,
'tag_name' => 'img',
'class_name' => 'wp-block-media-text__featured_image',
)
) ) {
// The ID is only used to ensure that the correct figure tag is selected,
// and can now be removed.
$image_tag_processor->remove_attribute( 'id' );
if ( $image_tag_processor->next_tag(
array(
'tag_name' => 'img',
'class_name' => 'wp-block-media-text__featured_image',
)
) ) {
$image_tag_processor->set_attribute( 'src', esc_url( $current_featured_image ) );
$image_tag_processor->set_attribute( 'class', 'wp-image-' . get_post_thumbnail_id() . ' size-' . $media_size_slug );
$image_tag_processor->set_attribute( 'alt', trim( strip_tags( get_post_meta( get_post_thumbnail_id(), '_wp_attachment_image_alt', true ) ) ) );

$content = $image_tag_processor->get_updated_html();
$image_tag_processor->set_attribute( 'src', esc_url( $current_featured_image ) );
$image_tag_processor->set_attribute( 'class', 'wp-image-' . get_post_thumbnail_id() . ' size-' . $media_size_slug );
$image_tag_processor->set_attribute( 'alt', trim( strip_tags( get_post_meta( get_post_thumbnail_id(), '_wp_attachment_image_alt', true ) ) ) );
if ( $image_fill ) {
$image_tag_processor->set_attribute( 'style', 'object-position:' . $focal_point . ';' );
}

$content = $image_tag_processor->get_updated_html();
}
}

Expand Down
Loading
Loading