[Image Block]: Fix block validation errors when clearing height/width

[stacimc]

Description

Issue #30131

When you change the 'width' or 'height' fields on an image block, it attempts to parse the new value as an Integer before passing it to setAttributes. This causes block invalidation errors if you empty out the field and save the post (because of the resulting NaN attributes).

This PR just checks for the empty string before attempting to parse and sets the attribute to null instead.

Note: This has the side effect of making it impossible to "clear out" the input, because the ImageSizeControl falls back to displaying the default image height/width when the custom height/width attributes are null. The intention is that the input is always displaying the actual values used. If this is instead deemed confusing when the user tries to clear the input, we could alternatively only fallback to the default image dimensions when first rendering the form.

How has this been tested?

1. Create a new post and insert an Image block. Select or upload an image.
2. Save and verify the width and height are set to "auto" by inspecting the element in the console.
3. In the Inspector Controls, set a custom height/width and save. Verify that the image updates visually and that the height and width are set correctly by inspecting the element in the console.
4. In the Inspector Controls, delete the custom height/width. The input should update to read the default dimension. Save the post and verify that there are no validation errors, and that the inline height and width are once again "auto".

Screenshots

Types of changes

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• I've tested my changes with keyboard and screen readers.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

[aaronrobertshaw]

Thanks for addressing this issue 👍

I can confirm this does prevent the error in console. There is one small issue/nitpick with the solution which I'll detail below.

This has the side effect of making it impossible to "clear out" the input

Given the fact that if the error wasn't been thrown this would be the behaviour of the control. I think it's fine to proceed. It could be addressed in a separate PR/issue if it's a problem.

This PR just checks for the empty string before attempting to parse and sets the attribute to null instead.

This should probably set the attributes to undefined instead, as I think that more accurately reflects the attribute's lack of a value. It is also more consistent with how other block attributes are cleared.

Using this PR, clearing the image width field, then switching to the code editor I get a "width": null attribute.

Tweaking the change to use undefined instead I get no null attribute and therefore cleaner saved block attribute code.

[stacimc]

This should probably set the attributes to undefined instead

💯 Completely agree, done!

[glendaviesnz]

This tested well for me, but I wonder if the fact that you can't clear the input will confuse some users. Most people would probably highlight and overtype, but if backspacing to clear it is a bit of an odd UX:

If we instead only worry about reverting back to the defaults when the form is reloaded with:

onChange={ ( value ) =>
    onChange( {
	width:
	    value === ''
		? value
		: parseInt( value, 10 ),
    } )
}

it fixes the NaN validation issue and also allows the user to clear the form. I don't have a strong opinion either way though.

[stacimc]

If we instead only worry about reverting back to the defaults when the form is reloaded with:

onChange={ ( value ) =>
    onChange( {
	width:
	    value === ''
		? value
		: parseInt( value, 10 ),
    } )
}

it fixes the NaN validation issue and also allows the user to clear the form. I don't have a strong opinion either way though.

Yeah, the UI is definitely odd. My concern here is that if a user deletes the value, in the editor the image will shrink to nothing/disappear, which does not reflect the actual state of the block on the frontend. So a user might delete the width (appearing to shrink the image until it disappears), save the post, and then be surprised when the image is clearly visible at its default width in the frontend (and in the editor when the page is refreshed).

That may not be a realistic scenario, but it will also result in setting the attributes to an empty string rather than undefined when the value is removed.

We could separate the TextControl value from the actual attribute. Something like:

const [ widthValue, setWidthValue ] = useState( width ?? imageWidth ?? '' );

...
// and then in the TextControl:
...
value={ widthValue }
onChange={ ( value ) => {
    setWidthValue( value );
    onChange( {
        width: 
           value === ''
               ? undefined
               : parseInt( value, 10 ),
    } );
} }

I'm not sure how I feel about that code 😄 It's definitely less clean-looking but would give us the default only on first load, fix the NaN, and set the attribute to undefined as expected when the input is cleared.

[glendaviesnz]

I'm not sure how I feel about that code

it is a bit more verbose, but I think it gives a better UX. The only issue I found with this approach is that it does not set the values when you first insert an image, from media library, etc. as all the dimensions values initially come through as undefined.

I was able to fix this with :

	const [ widthValue, setWidthValue ] = useState( width ?? imageWidth ?? '' );
	const [ heightValue, setHeightValue ] = useState(
		height ?? imageHeight ?? ''
	);

	useEffect( () => {
		if ( width === undefined && imageWidth !== undefined ) {
			setWidthValue( imageWidth );
		}
		if ( height === undefined && imageHeight !== undefined ) {
			setHeightValue( imageHeight );
		}
	}, [ imageWidth, imageHeight ] );

Which is even more verbose, but potentially worth it for the nicer UX.

[glendaviesnz]

Another option would be to put this dimension handling in a custom hook, which would simplify the code in the main component, and make it easier to unit test the dimension handling.

[aaronrobertshaw]

The UX around the dimensions controls is much better after the refactoring to the useDimensionHandler hook. Nice work 👍

I think we need to take the hook one step further. Prior to this PR the Reset button would reset the dimension fields to the default image width and height. The preset width buttons in the segmented control also updated the dimension fields.

If you move the updateDimensions function from the main component into the hook and add it to the hook's returned object. We can then also set the currentWidth and currentHeight state in updateDimensions. Then all that is needed is to add updateDimensions when destructuring the hook result.

	function updateDimensions( nextWidth, nextHeight ) {
		return () => {
			setCurrentWidth( nextWidth ?? defaultWidth );
			setCurrentHeight( nextHeight ?? defaultHeight );
			onChange( { width: nextWidth, height: nextHeight } );
		};
	}

After making the above changes everything worked very smoothly for me. I'm looking forward to see this one land ✨

[stacimc]

Thanks @aaronrobertshaw! It's been updated to fix the presets and tested with preset sizes ("Medium" "Large" etc), percentage widths, and the Reset.

I've added some tests for the hook as well. In my opinion it's very heavy-handed considering the hook is only used in this component, and I've had to do a lot of contrived setup to get around not adding a hook testing library. Curious for yours and others' opinions about whether it's worth keeping something like this.

[aaronrobertshaw]

Thanks for the updates @stacimc 👍

I've added some tests for the hook as well. In my opinion it's very heavy-handed considering the hook is only used in this component, and I've had to do a lot of contrived setup to get around not adding a hook testing library.

Adding tests around this sounds good.

My suggestion would be to perhaps change the approach to test the image size control itself rather than the dimension handler hook. The hook is used solely by this component and should be covered by any component tests. It might also reduce the contrived setup issue you mentioned.

Future updates are more likely to be made to the component rather than the hook. Having tests at that level can help others when those updates are made.

[stacimc]

My suggestion would be to perhaps change the approach to test the image size control itself rather than the dimension handler hook.

Agreed. I don't think pulling the tests out to the level of the hook ended up simplifying the unit tests (maybe even the opposite). That the hook is not intended to be used by any other component is reason enough to move the tests up a level IMO. Appreciate the confidence check, I will move things around.

[aaronrobertshaw]

This is looking good and working well 🚢

• Programmatic tests all pass
• Clearing the field works free from any errors
• No longer encounter block validation errors after save
• Editing the height and width fields work as expected
• Image preset size dropdown correctly updates the fields
• Image percentage size presets update fields appropriately
• Reset button correctly resets the field and attribute values

Tested this for the image, latest posts and media & text blocks, worked well for all of them.

I'm happy to merge this. Great job @stacimc!