Disabled: preserve input values when toggling the isDisabled prop

[torounit]

What?

There is a bug in the Disabled component where changing isDisabled resets the form value. This has been fixed.

Why?

#42708 (review)

How?

Testing Instructions

1. npm run storybook:dev
2. Go to localhost:50240?path=/story/components-disabled--default
3. Change isDisabled prop to false in controls.
4. Change text box or select.
5. Change isDisabled prop to true in controls.

Screenshots or screencast

2022-08-23.18.17.36.mov

[ciampo]

Code changes are looking good!

Interesting fix, I guess that using a div instead of a styled.div helps React's reconciliation and doesn't re-render the children input elements, throwing away their values (cc @mirka )

Could you also add a unit test, please?

Thank you!

[torounit]

@ciampo Certainly a unit test is needed. I will add it.

[torounit]

If an instance previously obtained by screen.getByRole( 'textbox' ) is used, rerender is not reflected, so a new instance is obtained.

[torounit]

@ciampo Thanks for reviewing! Sorry for the delay in responding.

I have committed the fixed version.

[ciampo]

LGTM 🚀

I've got one more suggestion to make this fix less hacky:

Let's make the fix more explicit by refactoring that `div` inside a separate component

diff --git a/packages/components/src/disabled/index.tsx b/packages/components/src/disabled/index.tsx
index bcdbf4ba48..dcaef3e173 100644
--- a/packages/components/src/disabled/index.tsx
+++ b/packages/components/src/disabled/index.tsx
@@ -1,8 +1,13 @@
+/**
+ * External dependencies
+ */
+import type { TdHTMLAttributes, HTMLProps } from 'react';
+
 /**
  * WordPress dependencies
  */
 import { useDisabled } from '@wordpress/compose';
-import { createContext } from '@wordpress/element';
+import { createContext, forwardRef } from '@wordpress/element';
 
 /**
  * Internal dependencies
@@ -15,6 +20,15 @@ import { useCx } from '../utils';
 const Context = createContext< boolean >( false );
 const { Consumer, Provider } = Context;
 
+// Extracting this ContentWrapper component in order to make it more explicit
+// the same 'ContentWrapper' component is needed so that React can reconcile
+// the dom correctly when switching between disabled/non-disabled (instead
+// of thrashing the previous DOM and therefore losing the form fields values).
+const ContentWrapper = forwardRef<
+	HTMLDivElement,
+	HTMLProps< HTMLDivElement >
+>( ( props, ref ) => <div { ...props } ref={ ref } /> );
+
 /**
  * `Disabled` is a component which disables descendant tabbable elements and prevents pointer interaction.
  *
@@ -56,14 +70,14 @@ function Disabled( {
 	if ( ! isDisabled ) {
 		return (
 			<Provider value={ false }>
-				<div>{ children }</div>
+				<ContentWrapper>{ children }</ContentWrapper>
 			</Provider>
 		);
 	}
 
 	return (
 		<Provider value={ true }>
-			<div
+			<ContentWrapper
 				ref={ ref }
 				className={ cx(
 					disabledStyles,
@@ -73,7 +87,7 @@ function Disabled( {
 				{ ...props }
 			>
 				{ children }
-			</div>
+			</ContentWrapper>
 		</Provider>
 	);
 }