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

Image block: unwrap img element in editor #68666

Merged
merged 3 commits into from
Feb 28, 2025
Merged

Image block: unwrap img element in editor #68666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
84185b7
Remove wrapper around Image block’s img
stokesman Jan 3, 2025
bb1e466
Render only used dimension control and monopolize its conditional
stokesman Jan 14, 2025
b4356d2
Account for potential top offset of img in figure
stokesman Jan 14, 2025
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
1 change: 1 addition & 0 deletions packages/block-library/src/image/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ export const LINK_DESTINATION_CUSTOM = 'custom';
export const NEW_TAB_REL = [ 'noreferrer', 'noopener' ];
export const ALLOWED_MEDIA_TYPES = [ 'image' ];
export const MEDIA_ID_NO_FEATURED_IMAGE_SET = 0;
export const SIZED_LAYOUTS = [ 'flex', 'grid' ];
13 changes: 0 additions & 13 deletions packages/block-library/src/image/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,19 +48,6 @@ figure.wp-block-image:not(.wp-block) {
}
}

// This is necessary for the editor resize handles to accurately work on a non-floated, non-resized, small image.
.wp-block-image .components-resizable-box__container {
// Using "display: table" because:
// - it visually hides empty white space in between elements
// - it allows the element to be as wide as its contents (instead of 100% width, as it would be with `display: block`)
display: table;
Comment on lines -51 to -56
Copy link
Contributor Author

@stokesman stokesman Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm adding some commentary here since it was asked about

Currently the comment and style are outdated because display: table is overridden by inline styles. For reference, according to #44965 (comment) it was added to hide some empty space below the image (that made the selected block outline look wrong) while also avoiding a regression of broken center alignments. In trunk, it’s overridden with display: block from inline styles that were added in #51545 and I couldn’t find discussion on that particular change but it seems to have been okay and did not regress anything.

In regards to this PR, this selector and rule applied to the wrapper around the img and now there is no wrapper. Since there is no wrapper it’s worth considering the the display of the img itself and that is not specified. From my testing everything around this works. That is, resizing "non-floated, non-resized small images" works fine. Better than on trunk since the resize handles are initially out of place on small images (less than the container’s content width) there. Also:

  • center aligned images work as expected
  • there is no empty space below the image between the selected block outline
Some markup that can be used to test such images. 
<!-- wp:group {"style":{"spacing":{"padding":{"top":"var:preset|spacing|20","bottom":"var:preset|spacing|20","left":"var:preset|spacing|20","right":"var:preset|spacing|20"}},"color":{"background":"#f3d5f1"}},"fontSize":"xx-large","layout":{"type":"constrained"}} -->
<div class="wp-block-group has-background has-xx-large-font-size" style="background-color:#f3d5f1;padding-top:var(--wp--preset--spacing--20);padding-right:var(--wp--preset--spacing--20);padding-bottom:var(--wp--preset--spacing--20);padding-left:var(--wp--preset--spacing--20)"><!-- wp:image {"width":"32px","sizeSlug":"large","linkDestination":"none","style":{"spacing":{"margin":{"left":"var:preset|spacing|20","right":"0"}}}} -->
<figure class="wp-block-image size-large is-resized" style="margin-right:0;margin-left:var(--wp--preset--spacing--20)"><img src="https://placehold.co/10x10/crimson/white?text=%23" alt="" style="width:32px"/></figure>
<!-- /wp:image --></div>
<!-- /wp:group -->

<!-- wp:group {"style":{"spacing":{"padding":{"top":"var:preset|spacing|20","bottom":"var:preset|spacing|40","left":"var:preset|spacing|20","right":"var:preset|spacing|20"}},"typography":{"letterSpacing":"21px"},"color":{"background":"#f3d5f1"}},"fontSize":"xx-large","layout":{"type":"constrained","justifyContent":"center"}} -->
<div class="wp-block-group has-background has-xx-large-font-size" style="background-color:#f3d5f1;padding-top:var(--wp--preset--spacing--20);padding-right:var(--wp--preset--spacing--20);padding-bottom:var(--wp--preset--spacing--40);padding-left:var(--wp--preset--spacing--20);letter-spacing:21px"><!-- wp:image {"width":"32px","height":"auto","sizeSlug":"large","linkDestination":"none","align":"left","style":{"spacing":{"margin":{"left":"var:preset|spacing|20","right":"var:preset|spacing|20"}}}} -->
<figure class="wp-block-image alignleft size-large is-resized" style="margin-right:var(--wp--preset--spacing--20);margin-left:var(--wp--preset--spacing--20)"><img src="https://placehold.co/10x10/crimson/white?text=%23" alt="" style="width:32px;height:auto" title="fub"/></figure>
<!-- /wp:image -->

<!-- wp:paragraph -->
<p>SOMETHING</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->

img {
display: block;
width: inherit;
height: inherit;
}
}

.block-editor-block-list__block[data-type="core/image"] .block-editor-block-toolbar .block-editor-url-input__button-modal {
position: absolute;
left: 0;
Expand Down
237 changes: 115 additions & 122 deletions packages/block-library/src/image/image.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@ import {
DropdownMenu,
Popover,
} from '@wordpress/components';
import { useViewportMatch } from '@wordpress/compose';
import {
useMergeRefs,
useResizeObserver,
useViewportMatch,
} from '@wordpress/compose';
import { useSelect, useDispatch } from '@wordpress/data';
import {
BlockControls,
Expand All @@ -34,7 +38,7 @@ import {
privateApis as blockEditorPrivateApis,
BlockSettingsMenuControls,
} from '@wordpress/block-editor';
import { useEffect, useMemo, useState, useRef } from '@wordpress/element';
import { useCallback, useEffect, useMemo, useState } from '@wordpress/element';
import { __, _x, sprintf, isRTL } from '@wordpress/i18n';
import { getFilename } from '@wordpress/url';
import { getBlockBindingsSource, switchToBlockType } from '@wordpress/blocks';
Expand All @@ -54,7 +58,7 @@ import { Caption } from '../utils/caption';
* Module constants
*/
import { useToolsPanelDropdownMenuProps } from '../utils/hooks';
import { MIN_SIZE, ALLOWED_MEDIA_TYPES } from './constants';
import { MIN_SIZE, ALLOWED_MEDIA_TYPES, SIZED_LAYOUTS } from './constants';
import { evalAspectRatio } from './utils';

const { DimensionsTool, ResolutionTool } = unlock( blockEditorPrivateApis );
Expand Down Expand Up @@ -280,12 +284,22 @@ export default function Image( {
lightbox,
metadata,
} = attributes;

// The only supported unit is px, so we can parseInt to strip the px here.
const numericWidth = width ? parseInt( width, 10 ) : undefined;
const numericHeight = height ? parseInt( height, 10 ) : undefined;
Comment on lines -284 to -286
Copy link
Contributor Author

@stokesman stokesman Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

These became unneeded with the introduction of pixelSize that stays in sync with the displayed size of the img.


const imageRef = useRef();
const [ imageElement, setImageElement ] = useState();
Comment on lines -288 to +287
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switching to state instead of a ref for the img. I suppose this isn’t strictly necessary but because the value is read during render putting it in state follows the guidance of not reading refs in render.

const [ resizeDelta, setResizeDelta ] = useState( null );
const [ pixelSize, setPixelSize ] = useState( {} );
const [ offsetTop, setOffsetTop ] = useState( 0 );
const setResizeObserved = useResizeObserver( ( [ entry ] ) => {
if ( ! resizeDelta ) {
const [ box ] = entry.borderBoxSize;
setPixelSize( { width: box.inlineSize, height: box.blockSize } );
}
// This is usually 0 unless the image height is less than the line-height.
setOffsetTop( entry.target.offsetTop );
} );
const effectResizeableBoxPlacement = useCallback( () => {
setOffsetTop( imageElement?.offsetTop ?? 0 );
}, [ imageElement ] );
const setRefs = useMergeRefs( [ setImageElement, setResizeObserved ] );
const { allowResize = true } = context;
const { getBlock, getSettings } = useSelect( blockEditorStore );

Expand Down Expand Up @@ -369,34 +383,18 @@ export default function Image( {
.catch( () => {} );
}, [ id, url, isSingleSelected, externalBlob ] );

// Get naturalWidth and naturalHeight from image ref, and fall back to loaded natural
// Get naturalWidth and naturalHeight from image, and fall back to loaded natural
// width and height. This resolves an issue in Safari where the loaded natural
// width and height is otherwise lost when switching between alignments.
// See: https://github.com/WordPress/gutenberg/pull/37210.
const { naturalWidth, naturalHeight } = useMemo( () => {
return {
naturalWidth:
imageRef.current?.naturalWidth ||
loadedNaturalWidth ||
undefined,
imageElement?.naturalWidth || loadedNaturalWidth || undefined,
naturalHeight:
imageRef.current?.naturalHeight ||
loadedNaturalHeight ||
undefined,
imageElement?.naturalHeight || loadedNaturalHeight || undefined,
};
}, [
loadedNaturalWidth,
loadedNaturalHeight,
imageRef.current?.complete,
] );

function onResizeStart() {
toggleSelection( false );
}

function onResizeStop() {
toggleSelection( true );
}
Comment on lines -393 to -399
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved into the bodies of the handlers at the actual ResizableBox JSX.

}, [ loadedNaturalWidth, loadedNaturalHeight, imageElement?.complete ] );

function onImageError() {
setHasImageErrored( true );
Expand Down Expand Up @@ -541,49 +539,49 @@ export default function Image( {

const dropdownMenuProps = useToolsPanelDropdownMenuProps();

const dimensionsControl = (
<DimensionsTool
value={ { width, height, scale, aspectRatio } }
onChange={ ( {
width: newWidth,
height: newHeight,
scale: newScale,
aspectRatio: newAspectRatio,
} ) => {
// Rebuilding the object forces setting `undefined`
// for values that are removed since setAttributes
// doesn't do anything with keys that aren't set.
setAttributes( {
// CSS includes `height: auto`, but we need
// `width: auto` to fix the aspect ratio when
// only height is set due to the width and
// height attributes set via the server.
width: ! newWidth && newHeight ? 'auto' : newWidth,
const dimensionsControl =
isResizable &&
( SIZED_LAYOUTS.includes( parentLayoutType ) ? (
Copy link
Contributor Author

@stokesman stokesman Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is extraneous but it bugged me that the condition for which dimension controls would be visible was repeated in a couple places and both versions of the controls were being invoked regardless. If it helps to see it was done atomically in 6b28210.

<DimensionsTool
value={ { aspectRatio } }
onChange={ ( { aspectRatio: newAspectRatio } ) => {
setAttributes( {
aspectRatio: newAspectRatio,
scale: 'cover',
} );
} }
defaultAspectRatio="auto"
tools={ [ 'aspectRatio' ] }
/>
) : (
<DimensionsTool
value={ { width, height, scale, aspectRatio } }
onChange={ ( {
width: newWidth,
height: newHeight,
scale: newScale,
aspectRatio: newAspectRatio,
} );
} }
defaultScale="cover"
defaultAspectRatio="auto"
scaleOptions={ scaleOptions }
unitsOptions={ dimensionsUnitsOptions }
/>
);

const aspectRatioControl = (
<DimensionsTool
value={ { aspectRatio } }
onChange={ ( { aspectRatio: newAspectRatio } ) => {
setAttributes( {
aspectRatio: newAspectRatio,
scale: 'cover',
} );
} }
defaultAspectRatio="auto"
tools={ [ 'aspectRatio' ] }
/>
);
} ) => {
// Rebuilding the object forces setting `undefined`
// for values that are removed since setAttributes
// doesn't do anything with keys that aren't set.
setAttributes( {
// CSS includes `height: auto`, but we need
// `width: auto` to fix the aspect ratio when
// only height is set due to the width and
// height attributes set via the server.
width: ! newWidth && newHeight ? 'auto' : newWidth,
height: newHeight,
scale: newScale,
aspectRatio: newAspectRatio,
} );
} }
defaultScale="cover"
defaultAspectRatio="auto"
scaleOptions={ scaleOptions }
unitsOptions={ dimensionsUnitsOptions }
/>
) );

const resetAll = () => {
setAttributes( {
Expand All @@ -603,10 +601,7 @@ export default function Image( {
resetAll={ resetAll }
dropdownMenuProps={ dropdownMenuProps }
>
{ isResizable &&
( parentLayoutType === 'grid'
? aspectRatioControl
: dimensionsControl ) }
{ dimensionsControl }
</ToolsPanel>
</InspectorControls>
);
Expand Down Expand Up @@ -835,10 +830,7 @@ export default function Image( {
/>
</ToolsPanelItem>
) }
{ isResizable &&
( parentLayoutType === 'grid'
? aspectRatioControl
: dimensionsControl ) }
{ dimensionsControl }
{ !! imageSizeOptions.length && (
<ResolutionTool
value={ sizeSlug }
Expand Down Expand Up @@ -926,17 +918,19 @@ export default function Image( {
alt={ defaultedAlt }
onError={ onImageError }
onLoad={ onImageLoad }
ref={ imageRef }
ref={ setRefs }
className={ borderProps.className }
width={ naturalWidth }
height={ naturalHeight }
Comment on lines +923 to +924
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding these img attributes is congruent with the front end and I'm pretty sure helped in some layout context though I forget the specifics.

style={ {
width:
( width && height ) || aspectRatio
? '100%'
: undefined,
height:
( width && height ) || aspectRatio
? '100%'
: undefined,
aspectRatio,
...( resizeDelta
? {
width: pixelSize.width + resizeDelta.width,
height:
pixelSize.height + resizeDelta.height,
}
: { width, height } ),
objectFit: scale,
...borderProps.style,
...shadowProps.style,
Expand All @@ -953,8 +947,7 @@ export default function Image( {
<ImageEditor
id={ id }
url={ url }
width={ numericWidth }
height={ numericHeight }
{ ...pixelSize }
naturalHeight={ naturalHeight }
naturalWidth={ naturalWidth }
onSaveImage={ ( imageAttributes ) =>
Expand All @@ -967,26 +960,21 @@ export default function Image( {
/>
</ImageWrapper>
);
} else if ( ! isResizable || parentLayoutType === 'grid' ) {
img = (
<div style={ { width, height, aspectRatio } }>
<ImageWrapper href={ href }>{ img }</ImageWrapper>
</div>
);
} else {
img = <ImageWrapper href={ href }>{ img }</ImageWrapper>;
}

let resizableBox;
if (
isResizable &&
isSingleSelected &&
! isEditingImage &&
! SIZED_LAYOUTS.includes( parentLayoutType )
) {
const numericRatio = aspectRatio && evalAspectRatio( aspectRatio );
const customRatio = numericWidth / numericHeight;
const customRatio = pixelSize.width / pixelSize.height;
const naturalRatio = naturalWidth / naturalHeight;
const ratio = numericRatio || customRatio || naturalRatio || 1;
const currentWidth =
! numericWidth && numericHeight
? numericHeight * ratio
: numericWidth;
const currentHeight =
! numericHeight && numericWidth
? numericWidth / ratio
: numericHeight;

const minWidth =
naturalWidth < naturalHeight ? MIN_SIZE : MIN_SIZE * ratio;
const minHeight =
Expand Down Expand Up @@ -1032,21 +1020,17 @@ export default function Image( {
}
}
/* eslint-enable no-lonely-if */
img = (
resizableBox = (
<ResizableBox
ref={ effectResizeableBoxPlacement }
style={ {
display: 'block',
objectFit: scale,
aspectRatio:
! width && ! height && aspectRatio
? aspectRatio
: undefined,
} }
size={ {
width: currentWidth ?? 'auto',
height: currentHeight ?? 'auto',
position: 'absolute',
// To match the vertical-align: bottom of the img (from style.scss)
// syncs the top with the img. This matters when the img height is
// less than the line-height.
inset: `${ offsetTop }px 0 0 0`,
} }
showHandle={ isSingleSelected }
size={ pixelSize }
minWidth={ minWidth }
maxWidth={ maxResizeWidth }
minHeight={ minHeight }
Expand All @@ -1058,9 +1042,19 @@ export default function Image( {
bottom: true,
left: showLeftHandle,
} }
onResizeStart={ onResizeStart }
onResizeStop={ ( event, direction, elt ) => {
onResizeStop();
onResizeStart={ () => {
toggleSelection( false );
} }
onResize={ ( event, direction, elt, delta ) => {
setResizeDelta( delta );
} }
onResizeStop={ ( event, direction, elt, delta ) => {
toggleSelection( true );
setResizeDelta( null );
setPixelSize( ( current ) => ( {
width: current.width + delta.width,
height: current.height + delta.height,
} ) );

// Clear hardcoded width if the resized width is close to the max-content width.
if (
Expand Down Expand Up @@ -1091,9 +1085,7 @@ export default function Image( {
} );
} }
resizeRatio={ align === 'center' ? 2 : 1 }
>
<ImageWrapper href={ href }>{ img }</ImageWrapper>
</ResizableBox>
/>
);
}

Expand Down Expand Up @@ -1139,6 +1131,7 @@ export default function Image( {
{ controls }
{ featuredImageControl }
{ img }
{ resizableBox }

<Caption
attributes={ attributes }
Expand Down
Loading