RangeControl: disable reset button consistently

[ciampo]

What?

Fixes #63061

Disable the reset button in RangeControl when the current value of the component is already equal to the "reset value".

Why?

Leaving the reset button enabled in this scenario can cause confusion in the end user and make the UX worse.

How?

By tweaking the disabled logic for the reset button, making sure that value is compared to the correct "reset" value in every scenario.

I also updated unit tests in the process.

Testing Instructions

In Storybook, set allowReset to true and play around the RangeControl component and the initialPosition and resetFallbackValue props. Make sure that:

• if resetFallbackValue is provided, the value resets to resetFallbackValue;
• otherwise, if initialPosition is provided, the value resets to initilaPosition;
• otherwise, the slider resets to the value half way between min and max, and the number input clears up.

In any case, the reset button should become disabled when resetting the RangeControl component.

[ciampo]

@mirka pinging you for some early feedback.

I took a look at the component, specifically about how it behaves when clicking the reset button. This is what I gathered:

• when resetFallbackValue is provided:
  • we call setValue(resetFallbackValue), but if the component is controlled, the external value still prevails;
  • onChange() is called with resetFallbackValue, which causes the parent component to update the value prop, which causes the internal value to update to resetFallbackValue;
• when initialPosition is provided:
  • we call setValue(null), but if the component is controlled, the external value still prevails;
  • onChange() is called with undefined, which causes the parent component to update the value prop, which causes the internal value to update to fallback to initialPosition;
• when neither resetFallbackValue nor initialPosition are provided:
  • we call setValue(null), but if the component is controlled, the external value still prevails;
  • onChange() is called with undefined, which causes the parent component to update the value prop, but since there is not initialPosition to fallback to, the internal value stays null;

I extracted the computeResetValue function which kind of codifies the expected reset value logic. But if I applied it to the handleOnReset function, it would cause some changes to how the component works (especially to the arguments of onChange).

My gut feeling is to keep this PR's scope more limited and focus on disabling the reset button correctly.

Separately, IMO we should consider re-thinking the controlled/uncontrolled logic entirely, so that onChange and the reset functionality behave in a more "standard" fashion (from a quick test, the reset button doesn't seem to work at all in uncontrolled mode). If we think that the changes are breaking enough, we may even want to consider an ariakit rewrite in the process (and consider the plan highlighted in #40507)

[mirka]

Great investigation, thank you. It's a good look into the kind of things we'll be dealing with for #57004.

My gut feeling is to keep this PR's scope more limited and focus on disabling the reset button correctly.

Definitely. And also, I just noticed that allowReset may be a candidate for deprecation. It doesn't look like it's used anywhere in the codebase anymore, as well as the one on BoxControl. Makes sense, because we've delegated all reset functionality to the ToolsPanel.

[ciampo]

Cool, i'll keep the scope of this PR limited to fixing the reset button disabled behavior.

I just noticed that allowReset may be a candidate for deprecation.

True, we could also open a follow-up PR to deprecated allowReset and resetFallbackValue, although it's probably not a high priority.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ciampo <[email protected]>
Co-authored-by: mirka <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[mirka]

The changes make sense, and tests well 👍