LineHeightControl: Custom spin buttons adjust from zero instead of base default

[aaronrobertshaw]

Related:

• UX feedback on working with the site editor #48443
• LineHeightControl: Unexpected decrements and adjustments of zero values #48768

Description

The custom spin buttons for the LineHeightControl do not increment or decrement from the default 1.5 represented in the control's placeholder value. Instead they increment/decrement from 0.

This is a regression from the behaviour prior to when the custom spin buttons were added to the NumberControl in #45333.

Expected Behaviour

The spin buttons work as they did prior to #45333 i.e. when the field is initially unset and displaying the 1.5 placeholder value, clicking either custom spin button should adjust that placeholder value not zero.

Step-by-step reproduction instructions

1. Edit a post, select a paragraph, and switch to the styles tab in the Block Inspector
2. Toggle on the line-height option from the Typography panel's menu
3. Click the + button and note that the field now has 0.1 as its value instead of 1.6.
4. Reset the line height control via the Typography panel's menu
5. Toggle it back on and this time click the - button. Note the value in the field is now 0.

Screenshots, screen recording, code snippet

Actual	Expected
Screen.Recording.2023-03-06.at.5.13.20.pm.mp4	Screen.Recording.2023-03-06.at.5.15.04.pm.mp4

Environment info

• WP 6.1
• Gutenberg trunk

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

[aaronrobertshaw]

@ciampo and @mirka it looks like we have a small regression here after the implementation of the NumberControl custom spin buttons in #45333.

I'm not sure of the best approach to solving this. Would it be possible to make the custom spin buttons in NumberControl trigger one of the InputControl actions so that everything continues to pass through the state reducers and we pick up the old behaviour again?

Alternatively, as a hacky quick fix option that works, we could wrap the LineHeightControl's onChange so it can pass the value through the same adjustNextValue function used by the state reducer. I don't feel this is a great option as I could see other consumers of NumberControl using the custom spin buttons and wanting to add their own custom state reducer.

Any suggestions?

Quick fix diff

diff --git a/packages/block-editor/src/components/line-height-control/index.js b/packages/block-editor/src/components/line-height-control/index.js
index 670d6fac37..61e0c4a3f3 100644
--- a/packages/block-editor/src/components/line-height-control/index.js
+++ b/packages/block-editor/src/components/line-height-control/index.js
@@ -84,6 +84,15 @@ const LineHeightControl = ( {
 		? undefined
 		: { marginBottom: 24 };
 
+	const handleOnChange = ( nextValue, { event } ) => {
+		if ( event.type === 'click' ) {
+			onChange( adjustNextValue( nextValue, false ) );
+			return;
+		}
+
+		onChange( nextValue );
+	};
+
 	return (
 		<div
 			className="block-editor-line-height-control"
@@ -93,7 +102,7 @@ const LineHeightControl = ( {
 				{ ...otherProps }
 				__unstableInputWidth={ __unstableInputWidth }
 				__unstableStateReducer={ stateReducer }
-				onChange={ onChange }
+				onChange={ handleOnChange }
 				label={ __( 'Line height' ) }
 				placeholder={ BASE_DEFAULT_VALUE }
 				step={ STEP }

[aaronrobertshaw]

There are also some other quirks/bugs with the LineHeightControl currently that I've detailed in a separate issue (#48768) as they are less significant. That said, it might be worth considering them when assessing potential solutions.

[mirka]

As we discussed in the original PR thread, I do think that the cleanest solution within the current implementation is to move the spin buttons down into the InputControl component and hook it up to the reducer system. This LineHeightControl regression (and potentially in other similar consumers) is another good reason to do that.

@stokesman has floated the idea of an overhaul to the reducer system, but barring that I think it's probably worth moving the spin buttons down.

(Not that I'm opposed to your quick fix if it's urgent.)

[richtabor]

Can't tell you how many times I've been snagged here. I'd like to get a quick fix in, then circle back to clean up.

[aaronrobertshaw]

move the spin buttons down into the InputControl component and hook it up to the reducer system

I'm not clear on how we would best achieve this. I'm guessing we'd need to lift the state reducers from InputField to the root InputControl so they can then be shared between InputField and InputBase, which would need to render the custom spin buttons.

The amount of refactoring there, makes it seems to me like we should just refactor the component to the hook/component pattern as @stokesman proposed.

Can't tell you how many times I've been snagged here. I'd like to get a quick fix in, then circle back to clean up.

Given the frequency with which this bug catches people out, I have put up a PR to apply the quick fix at the LineHeightControl level. That should buy us some time to address the root issue in a more thorough manner.