mui-x icon indicating copy to clipboard operation
mui-x copied to clipboard

[pickers] Reset the value of the input field using controlled value

Open satwinder8294 opened this issue 2 years ago β€’ 16 comments

Steps to reproduce πŸ•Ή

Link to live example:

Steps: 1. 2. 3.

Current behavior 😯

I am using <DesktopDatePicker /> in my React + MUI application... My calendar date selections are working fine as expected. But the user is allowed to type invalid dates in the input field e.g 31st of November or 13th month of the year. if user types an invalid value in the input field then instead of updating the state value my code reset it to previous value by using the inputRefs.

It changes the value to previous one but when i open the calendar it again change the value(in input field) to invalid date.

Can you help how to handle this ?

Expected behavior πŸ€”

Expectation is that if the date is invalid, set it back to valid one programmatically both in calendar and the input field.

Context πŸ”¦

as a work around, i have used key attribute and re-render on each invalid date. I would like to get a proper solution.

Your environment 🌎

npx @mui/envinfo
  Don't forget to mention which browser you used.
  Output from `npx @mui/envinfo` goes here.

Order ID or Support key πŸ’³ (optional)

No response

satwinder8294 avatar Sep 21 '23 16:09 satwinder8294

Thank you for creating this issue. πŸ™ A reproduction example would have been great to better understand your problem, but I think I get the idea and see the problem. The problem is that currently there is no way to tell the picker to use the bound value, because of this: https://github.com/mui/mui-x/blob/master/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts#L313

A possible solution could be to add an optional ref equality check:

diff --git a/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts b/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts
index 6cb85178f..7a9b504c9 100644
--- a/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts
+++ b/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts
@@ -310,7 +310,8 @@ export const usePickerValue = <
   if (
     inValue !== undefined &&
     (dateState.lastControlledValue === undefined ||
-      !valueManager.areValuesEqual(utils, dateState.lastControlledValue, inValue))
+      !valueManager.areValuesEqual(utils, dateState.lastControlledValue, inValue) ||
+      dateState.lastControlledValue !== inValue)
   ) {
     const isUpdateComingFromPicker = valueManager.areValuesEqual(utils, dateState.draft, inValue);

This would allow re-setting the picker value when the bound value reference changes, i.e. by doing setValue(currentValue => dayjs(currentValue)).

Are there any problems that I don't see with this or better suggestions? πŸ€”

LukasTy avatar Sep 22 '23 08:09 LukasTy

Thank you for creating this issue. πŸ™ A reproduction example would have been great to better understand your problem, but I think I get the idea and see the problem. The problem is that currently there is no way to tell the picker to use the bound value, because of this: https://github.com/mui/mui-x/blob/master/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts#L313

A possible solution could be to add an optional ref equality check:

diff --git a/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts b/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts
index 6cb85178f..7a9b504c9 100644
--- a/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts
+++ b/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts
@@ -310,7 +310,8 @@ export const usePickerValue = <
   if (
     inValue !== undefined &&
     (dateState.lastControlledValue === undefined ||
-      !valueManager.areValuesEqual(utils, dateState.lastControlledValue, inValue))
+      !valueManager.areValuesEqual(utils, dateState.lastControlledValue, inValue) ||
+      dateState.lastControlledValue !== inValue)
   ) {
     const isUpdateComingFromPicker = valueManager.areValuesEqual(utils, dateState.draft, inValue);

This would allow re-setting the picker value when the bound value reference changes, i.e. by doing setValue(currentValue => dayjs(currentValue)).

Are there any problems that I don't see with this or better suggestions? πŸ€”

hi @LukasTy , Thank you for quick response. I have already tried to set the state like this but that didn't help. Do I need to make any other change as well except this setState ?

If you want me to make a change in some package file, that wont help, because before each deployment on server, it install the npm modules again,, so my change will be lost

satwinder8294 avatar Sep 22 '23 12:09 satwinder8294

Do I need to make any other change as well except this setState ?

If you want me to make a change in some package file, that wont help, because before each deployment on server, it install the npm modules again,, so my change will be lost

My question was more open-ended and targeted to any other team member who would take a look at this issue. I've moved the issue into the needs grooming state so that we can address it and come up with the best solution for this case. πŸ˜‰ In the meantime, if you really need this functionality and want to "battle-test" it, you can use https://www.npmjs.com/package/patch-package to apply my mentioned change (diff) to your installation. Just confirming, that It will also run on CI. πŸ˜‰

LukasTy avatar Sep 22 '23 12:09 LukasTy

Do I need to make any other change as well except this setState ? If you want me to make a change in some package file, that wont help, because before each deployment on server, it install the npm modules again,, so my change will be lost

My question was more open-ended and targeted to any other team member who would take a look at this issue. I've moved the issue into the needs grooming state so that we can address it and come up with the best solution for this case. πŸ˜‰ In the meantime, if you really need this functionality and want to "battle-test" it, you can use https://www.npmjs.com/package/patch-package to apply my mentioned change (diff) to your installation. Just confirming, that It will also run on CI. πŸ˜‰

Thank you. Really amazed by your quick responses. I am sorry, but I don't know how this patch thing work and I am afraid of sending it to the production. Is there any way that we can stop invalid dates in the textbox itself ? I mean not allow user to enter invalid numbers such as 13th month or 31st of November etc. ? Then we won't even need to reset it through refs.

satwinder8294 avatar Sep 23 '23 09:09 satwinder8294

Is there any way that we can stop invalid dates in the textbox itself ? I mean not allow user to enter invalid numbers such as 13th month or 31st of November etc. ? Then we won't even need to reset it through refs.

The initial version of fields did work like that, but it was changed with https://github.com/mui/mui-x/issues/7934 The issue is that the dependendant sections is not something that is very accessibility friendly and on top of that, it's not how the native html date input works, that's why we decided not to re-invent a wheel on this regard.

LukasTy avatar Sep 25 '23 12:09 LukasTy

Thank you for creating this issue. πŸ™ A reproduction example would have been great to better understand your problem, but I think I get the idea and see the problem. The problem is that currently there is no way to tell the picker to use the bound value, because of this: https://github.com/mui/mui-x/blob/master/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts#L313 A possible solution could be to add an optional ref equality check:

diff --git a/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts b/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts
index 6cb85178f..7a9b504c9 100644
--- a/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts
+++ b/packages/x-date-pickers/src/internals/hooks/usePicker/usePickerValue.ts
@@ -310,7 +310,8 @@ export const usePickerValue = <
   if (
     inValue !== undefined &&
     (dateState.lastControlledValue === undefined ||
-      !valueManager.areValuesEqual(utils, dateState.lastControlledValue, inValue))
+      !valueManager.areValuesEqual(utils, dateState.lastControlledValue, inValue) ||
+      dateState.lastControlledValue !== inValue)
   ) {
     const isUpdateComingFromPicker = valueManager.areValuesEqual(utils, dateState.draft, inValue);

This would allow re-setting the picker value when the bound value reference changes, i.e. by doing setValue(currentValue => dayjs(currentValue)). Are there any problems that I don't see with this or better suggestions? πŸ€”

hi @LukasTy , Thank you for quick response. I have already tried to set the state like this but that didn't help. Do I need to make any other change as well except this setState ?

If you want me to make a change in some package file, that wont help, because before each deployment on server, it install the npm modules again,, so my change will be lost

okay, is there any plan to merge this fix as well ?

satwinder8294 avatar Sep 26 '23 05:09 satwinder8294

okay, is there any plan to merge this fix as well ?

I'll try creating a PR this week. πŸ‘Œ

LukasTy avatar Sep 26 '23 06:09 LukasTy

okay, is there any plan to merge this fix as well ?

I'll try creating a PR this week. πŸ‘Œ

Thank you very much. Will keep an eye :)

satwinder8294 avatar Sep 26 '23 15:09 satwinder8294

@satwinder8294 @LukasTy we would like to pick this up

gitstart avatar Oct 09 '23 09:10 gitstart

@gitstart Thank you for being interested in contributing to the Pickers codebase! πŸ™ Do you have a rough idea for how difficult this problem would be? We feel that this could be a pretty tricky problem to tackle. On top of that, it's the most integral behavioral part of the components. If you are not afraid of such challenge, feel free to explore a solution! πŸ‘ But there also are other issues that are more clear and also pretty important. Have you considered https://github.com/mui/mui-x/issues/9956 or https://github.com/mui/mui-x/issues/7633, which is potentially a bit more tricky given our v7 plans. πŸ™ˆ

LukasTy avatar Oct 09 '23 14:10 LukasTy

Thanks for getting back to us @LukasTy, we are willing to work on as many of these issues as you like.

gitstart avatar Oct 09 '23 14:10 gitstart

Another case to test and cover when fixing this: #10727.

LukasTy avatar Oct 20 '23 06:10 LukasTy

I just hit the same problem. I want to limit the range used to 3 years, so I want to programatically control the value where I basically force the end date to be a maximum of 3 years from the start date.

Any idea when we can expect that fixed? also, may I suggest that if it's difficult to fix now, you put a note in the doc saying that there is currently no "controlled" version of the component? I needed quite a bit of time to figure out that I was doing things right and that there was a bug in the library

Thanks

gbataille avatar Jun 27 '24 05:06 gbataille

@gbataille given your use case, a maxDate validation reliant on the selected start date comes to mind. Have you tried such validation? πŸ€”

Forcibly replacing the changed value with the one you need sounds like a bit strange UX.

I think that relying on form validation and preventing form submission with validation errors present is a better UX for both regular users as well as visually impaired relying on a11y features.

As for the issue, thank you for bumping it, we put it in the To explore board, which has lower priority than the To do board. πŸ™ˆ I've adjusted this. πŸ‘Œ

LukasTy avatar Jun 27 '24 08:06 LukasTy

I quite agree for the UX side of what I'm doing (I mean that as you say it's not awesome).

I did not think to change the maxDate dynamically. I guess it gives me something interesting indeed, based on which I could avoid going further. I'll investigate what's possible. Thanks for the pointer.

gbataille avatar Jun 27 '24 09:06 gbataille

We agreed to work on aligning the controlled behavior as close to the regular <input type="date" /> as possible. This would meanβ€”not changing the value in the input if onChange does not update the value when editing in both the field as well as the Picker view (i.e. calendar).

LukasTy avatar Aug 06 '24 09:08 LukasTy

This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue. Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

[!NOTE] @satwinder8294 How did we do? Your experience with our support team matters to us. If you have a moment, please share your thoughts in this short Support Satisfaction survey.

github-actions[bot] avatar Nov 20 '24 09:11 github-actions[bot]