Dimensions: Fix aspect ratio and scale controls not reflecting external updates

packages/block-editor/CHANGELOG.md

@@ -53,6 +53,7 @@
 
 ### Bug Fixes
 
+-   `DimensionsTool`: Reflect aspect ratio and scale values that are updated from outside the component, such as by undo or `updateBlockAttributes`. The scale control no longer displays a stale value, and an aspect ratio that is written differently to its preset, e.g. `1/1` rather than `1`, is displayed as that preset instead of as "Original" ([#80747](https://github.com/WordPress/gutenberg/pull/80747)).
 -   Gate the HEIC canvas conversion fallback on `window.__clientSideMediaProcessing` instead of the redundant `window.__heicUploadSupport` flag, fixing client-side HEIC conversion in Safari on core WordPress installs ([#80452](https://github.com/WordPress/gutenberg/pull/80452)).
 -   `URLInput`: Request suggestions for a value the field is mounted with, instead of waiting for the input to be focused, and stop requesting initial suggestions on mount when `disableSuggestions` is set ([#80721](https://github.com/WordPress/gutenberg/pull/80721)).
 -   The multi-selection inspector card and the spoken selection announcement now disclose the total number of blocks a selection contains when the selected blocks have nested content, e.g. "2 blocks selected, 4 including nested blocks." ([#80745](https://github.com/WordPress/gutenberg/pull/80745)).

packages/block-editor/src/components/dimensions-tool/aspect-ratio-tool.js

@@ -2,6 +2,7 @@ import { SelectControl } from '@wordpress/components';
 import { __, _x } from '@wordpress/i18n';
 import { useSettings } from '../use-settings';
 import { InheritanceToolsPanelItem } from '../global-styles/inheritance';
+import { findAspectRatioOption } from './utils';
 
 /**
  * @typedef {import('@wordpress/components/build-types/select-control/types').SelectControlProps} SelectControlProps
@@ -38,9 +39,6 @@ export default function AspectRatioTool( {
 	isInherited,
 	hasLocalOverride,
 } ) {
-	// Match the CSS default so if the value is used directly in CSS it will look correct in the control.
-	const displayValue = value ?? 'auto';
-
 	const [ defaultRatios, themeRatios, showDefaultRatios ] = useSettings(
 		'dimensions.aspectRatios.default',
 		'dimensions.aspectRatios.theme',
@@ -75,6 +73,20 @@ export default function AspectRatioTool( {
 		},
 	];
 
+	const resolvedOptions = options ?? aspectRatioOptions;
+
+	// The value is free-form CSS, so it doesn't necessarily match an option as
+	// written, e.g. '1/1' and '1' describe the same ratio. Resolve it to the
+	// option that represents it, falling back to 'custom' so that a ratio
+	// without a matching preset isn't displayed as one of the presets. Values
+	// that are empty match the CSS default, so if the value is used directly in
+	// CSS it will look correct in the control.
+	const displayValue =
+		value === undefined || value === null || value === 'auto'
+			? 'auto'
+			: findAspectRatioOption( value, resolvedOptions )?.value ??
+			  'custom';
+
 	return (
 		<InheritanceToolsPanelItem
 			className={ className }
@@ -91,7 +103,7 @@ export default function AspectRatioTool( {
 			<SelectControl
 				label={ __( 'Aspect ratio' ) }
 				value={ displayValue }
-				options={ options ?? aspectRatioOptions }
+				options={ resolvedOptions }
 				onChange={ onChange }
 			/>
 		</InheritanceToolsPanelItem>

packages/block-editor/src/components/dimensions-tool/index.js

@@ -1,4 +1,4 @@
-import { useState } from '@wordpress/element';
+import { useEffect, useRef } from '@wordpress/element';
 import AspectRatioTool from './aspect-ratio-tool';
 import ScaleTool from './scale-tool';
 import WidthHeightTool from './width-height-tool';
@@ -68,13 +68,33 @@ function DimensionsTool( {
 			: value.aspectRatio;
 	const scale = value.scale === undefined ? null : value.scale;
 
-	// Keep track of state internally, so when the value is cleared by means
-	// other than directly editing that field, it's easier to restore the
-	// previous value.
-	const [ lastScale, setLastScale ] = useState( scale );
-	const [ lastAspectRatio, setLastAspectRatio ] = useState( aspectRatio );
 	const hasCustomAspectRatio = !! ( width && height );
 
+	// Keep track of the previous values, so when a value is cleared by means
+	// other than directly editing that field, it's easier to restore it. These
+	// are only ever read while handling a change, so they're kept in refs and
+	// synced from the value to stay correct when it is updated externally, e.g.
+	// by undo or by another client.
+	const lastScaleRef = useRef( scale );
+	const lastAspectRatioRef = useRef( aspectRatio );
+
+	useEffect( () => {
+		// Scale is cleared alongside the aspect ratio, so only remember the
+		// values it is actually set to.
+		if ( scale ) {
+			lastScaleRef.current = scale;
+		}
+	}, [ scale ] );
+
+	useEffect( () => {
+		// A custom aspect ratio, i.e. both a width and a height, clears the
+		// aspect ratio value. Keep the remembered one so it can be restored
+		// once the custom aspect ratio is removed.
+		if ( ! hasCustomAspectRatio ) {
+			lastAspectRatioRef.current = aspectRatio;
+		}
+	}, [ aspectRatio, hasCustomAspectRatio ] );
+
 	// 'custom' is not a valid value for CSS aspect-ratio, but it is used in the
 	// dropdown to indicate that setting both the width and height is the same
 	// as a custom aspect ratio.
@@ -97,8 +117,6 @@ function DimensionsTool( {
 						nextAspectRatio =
 							nextAspectRatio === 'auto' ? null : nextAspectRatio;
 
-						setLastAspectRatio( nextAspectRatio );
-
 						// Update aspectRatio.
 						if ( ! nextAspectRatio ) {
 							delete nextValue.aspectRatio;
@@ -109,11 +127,9 @@ function DimensionsTool( {
 						// Auto-update scale.
 						if ( ! nextAspectRatio ) {
 							delete nextValue.scale;
-						} else if ( lastScale ) {
-							nextValue.scale = lastScale;
 						} else {
-							nextValue.scale = defaultScale;
-							setLastScale( defaultScale );
+							nextValue.scale =
+								lastScaleRef.current ?? defaultScale;
 						}
 
 						// Auto-update width and height.
@@ -154,8 +170,8 @@ function DimensionsTool( {
 						// Auto-update aspectRatio.
 						if ( nextWidth && nextHeight ) {
 							delete nextValue.aspectRatio;
-						} else if ( lastAspectRatio ) {
-							nextValue.aspectRatio = lastAspectRatio;
+						} else if ( lastAspectRatioRef.current ) {
+							nextValue.aspectRatio = lastAspectRatioRef.current;
 						} else {
 							// No setting defaultAspectRatio here, because
 							// aspectRatio is optional in this scenario,
@@ -164,15 +180,13 @@ function DimensionsTool( {
 
 						// Auto-update scale.
 						if (
-							! lastAspectRatio &&
+							! lastAspectRatioRef.current &&
 							!! nextWidth !== !! nextHeight
 						) {
 							delete nextValue.scale;
-						} else if ( lastScale ) {
-							nextValue.scale = lastScale;
 						} else {
-							nextValue.scale = defaultScale;
-							setLastScale( defaultScale );
+							nextValue.scale =
+								lastScaleRef.current ?? defaultScale;
 						}
 
 						onChange( nextValue );
@@ -184,12 +198,10 @@ function DimensionsTool( {
 					panelId={ panelId }
 					options={ scaleOptions }
 					defaultValue={ defaultScale }
-					value={ lastScale }
+					value={ scale }
 					onChange={ ( nextScale ) => {
 						const nextValue = { ...value };
 
-						setLastScale( nextScale );
-
 						// Update scale.
 						if ( ! nextScale ) {
 							delete nextValue.scale;

packages/block-editor/src/components/dimensions-tool/test/index.js

@@ -1,14 +1,17 @@
-import { render, screen } from '@testing-library/react';
+import { act, render, screen } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
 import { __experimentalToolsPanel as ToolsPanel } from '@wordpress/components';
-import { useState } from '@wordpress/element';
+import { useEffect, useState } from '@wordpress/element';
 import DimensionsTool from '../';
 
 const EMPTY_OBJECT = {};
 const ASPECT_RATIO_OPTIONS = [
 	{ label: 'Original', value: 'auto' },
 	{ label: '16/9', value: '16/9' },
 	{ label: '4/3', value: '4/3' },
+	// Matches the core preset for a square, which is written as a single
+	// number rather than a ratio.
+	{ label: 'Square', value: '1' },
 	{
 		label: 'Custom',
 		value: 'custom',
@@ -41,6 +44,30 @@ function Example( { initialValue, onChange, ...props } ) {
 	);
 }
 
+// Holds the value like `Example`, but also lets a test update it from the
+// outside, the way `updateBlockAttributes`, undo, or another client would.
+function ExternallyUpdatedExample( { initialValue, onChange, updateRef } ) {
+	const [ value, setValue ] = useState( initialValue );
+	useEffect( () => {
+		updateRef.current = setValue;
+	}, [ updateRef ] );
+	return (
+		<ToolsPanel label="Dimensions" panelId="panel-id" resetAll={ () => {} }>
+			<DimensionsTool
+				panelId="panel-id"
+				onChange={ ( nextValue ) => {
+					setValue( nextValue );
+					onChange( nextValue );
+				} }
+				defaultScale="cover"
+				defaultAspectRatio="auto"
+				aspectRatioOptions={ ASPECT_RATIO_OPTIONS }
+				value={ value }
+			/>
+		</ToolsPanel>
+	);
+}
+
 function ControlledExample( { value, onChange, ...props } ) {
 	return (
 		<ToolsPanel
@@ -102,6 +129,143 @@ describe( 'DimensionsTool', () => {
 			);
 			expect( aspectRatioSelect ).toHaveValue( 'auto' );
 		} );
+
+		it( 'displays an aspect ratio that is written differently to the option with the same ratio', () => {
+			const onChange = jest.fn();
+			const { rerender } = render(
+				<ControlledExample
+					value={ { aspectRatio: '1/1' } }
+					onChange={ onChange }
+				/>
+			);
+			const aspectRatioSelect = screen.getByRole( 'combobox', {
+				name: 'Aspect ratio',
+			} );
+
+			expect( aspectRatioSelect ).toHaveValue( '1' );
+
+			rerender(
+				<ControlledExample
+					value={ { aspectRatio: '16 / 9' } }
+					onChange={ onChange }
+				/>
+			);
+			expect( aspectRatioSelect ).toHaveValue( '16/9' );
+		} );
+
+		it( 'displays an aspect ratio without a matching option as custom', () => {
+			const onChange = jest.fn();
+			render(
+				<ControlledExample
+					value={ { aspectRatio: '7/5' } }
+					onChange={ onChange }
+				/>
+			);
+
+			expect(
+				screen.getByRole( 'combobox', { name: 'Aspect ratio' } )
+			).toHaveValue( 'custom' );
+		} );
+
+		it( 'updates the scale control when the value prop changes', () => {
+			const onChange = jest.fn();
+			const { rerender } = render(
+				<ControlledExample
+					value={ { aspectRatio: '16/9', scale: 'cover' } }
+					onChange={ onChange }
+				/>
+			);
+
+			expect(
+				screen.getByRole( 'radio', { name: 'Cover' } )
+			).toBeChecked();
+
+			rerender(
+				<ControlledExample
+					value={ { aspectRatio: '16/9', scale: 'contain' } }
+					onChange={ onChange }
+				/>
+			);
+
+			expect(
+				screen.getByRole( 'radio', { name: 'Contain' } )
+			).toBeChecked();
+		} );
+	} );
+
+	describe( 'external updates', () => {
+		it( 'restores the scale that was set externally', async () => {
+			const user = userEvent.setup();
+			const onChange = jest.fn();
+			const updateRef = { current: null };
+
+			render(
+				<ExternallyUpdatedExample
+					initialValue={ { aspectRatio: '16/9', scale: 'cover' } }
+					onChange={ onChange }
+					updateRef={ updateRef }
+				/>
+			);
+
+			// Update the scale from outside the component.
+			await act( async () =>
+				updateRef.current( { aspectRatio: '16/9', scale: 'contain' } )
+			);
+
+			// Clearing and setting the aspect ratio again restores the scale.
+			await user.selectOptions(
+				screen.getByRole( 'combobox', { name: 'Aspect ratio' } ),
+				'auto'
+			);
+			await user.selectOptions(
+				screen.getByRole( 'combobox', { name: 'Aspect ratio' } ),
+				'4/3'
+			);
+
+			expect( onChange ).toHaveBeenLastCalledWith( {
+				aspectRatio: '4/3',
+				scale: 'contain',
+			} );
+		} );
+
+		it( 'restores the aspect ratio that was set externally', async () => {
+			const user = userEvent.setup();
+			const onChange = jest.fn();
+			const updateRef = { current: null };
+
+			render(
+				<ExternallyUpdatedExample
+					initialValue={ { aspectRatio: '16/9', scale: 'cover' } }
+					onChange={ onChange }
+					updateRef={ updateRef }
+				/>
+			);
+
+			// Update the aspect ratio from outside the component.
+			await act( async () =>
+				updateRef.current( { aspectRatio: '4/3', scale: 'cover' } )
+			);
+
+			// Setting both a width and a height replaces the aspect ratio with
+			// a custom one, and clearing the width restores the previous one.
+			await user.type(
+				screen.getByRole( 'spinbutton', { name: 'Width' } ),
+				'100'
+			);
+			await user.type(
+				screen.getByRole( 'spinbutton', { name: 'Height' } ),
+				'100'
+			);
+			await user.clear(
+				screen.getByRole( 'spinbutton', { name: 'Width' } )
+			);
+
+			expect( onChange ).toHaveBeenLastCalledWith( {
+				aspectRatio: '4/3',
+				scale: 'cover',
+				height: '100px',
+			} );
+		} );
 	} );
 
 	describe( 'updating aspectRatio', () => {