react-admin icon indicating copy to clipboard operation
react-admin copied to clipboard

warnWhenUnsavedChanges does not function properly with disabled inputs

Open lvernaillen opened this issue 1 year ago • 7 comments

What you were expecting:

  • When you open an edit page containing a disabled input for any resource and then leave that page without making any changes, you should not be prompted with an alert for unsaved changes.
  • Being able to uniformly set a react-admin input component to disabled without this incorrect warnWhenUnsavedChanges behavior.

What happened instead:

  • When you open an edit page containing a disabled input and then leave that page without making any modifications, an alert for unsaved changes is shown.
  • Some react-admin input components can use disabled, others should use InputProps workaround and other don't have a clear workaround.

Related code:

  • stackblitz showing that disabled AutoCompleteInput exposes the warnWhenUnsavedChanges bug https://stackblitz.com/edit/github-ny4hdu?file=src%2Fposts%2FPostEdit.tsx,src%2Fposts%2FPostList.tsx,src%2Fposts%2FPostShow.tsx
    • Go to the post list.
    • Select the first post "Fusce massa lorem, pulvinar a posuere ut, accumsan ac nisi" which has id 13.
    • Go to the Miscellaneous tab
    • click on the Posts icon in the menu on the left
    • Notice the warning of unsaved changes although you did not change anything
  • stackblitz showing that using InputProps is not working to disable an AutoCompleteInput https://stackblitz.com/edit/github-6a8auj?file=src%2Fposts%2FPostEdit.tsx
    • Go to the post list.
    • Select the first post "Fusce massa lorem, pulvinar a posuere ut, accumsan ac nisi" which has id 13.
    • Go to the Miscellaneous tab
    • Notice the first tag AutoCompleteInput is not disabled although it has the InputProps set
    • Notice the second tag AutoCompleteInput looks as if enabled, but is not (because readonly)
    • Notice the views TextInput behave correctly with the InputProps workaround
    • Notice the pictures ArrayInput readonly but still able to add / remove items

image

Other information: We have this situation a lot. Our edit pages disable certain inputs when the logged on user has no permission to change them. But now they get a confusing warning about unsaved changes when the form was untouched.

I already mentioned this in the closed related issue https://github.com/marmelab/react-admin/issues/9378. There a workaround is described for the TextInput component by using the InputProps to set disabled instead of directly using the disabled prop. However that only works for a TextInput. Other inputs expose that same bug without having the workaround.

  • AutoCompleteInput: has this bug, has InputProps, but the workaround does not work
  • AutoCompleteArrayIput and ArrayInput: have this bug, have no InputProps so the workaround does not work
  • BooleanInput: does not have this bug, so disabled can be used
  • other inputs not tested

So some components do not expose the bug, others expose the bug but have a workaround using InputProps and again others have no clear workaround with props. For those without prop workaround we might wrap them in a div and set the div disabled while adding some styling (pointer-events, color, MuiInputBase-root) to simulate a disabled input. So it's a bit messy currently. When something needs to be disabled we need different kinds of solutions per input.

In https://github.com/marmelab/react-admin/issues/9378 @slax57 suggested using readonly. However that does not work on an ArrayInput for example, you can still change it. For an AutoCompleteInput the readonly prop results in a component that looks as if you can change it, but actually you can't. Users think something is broken without the visual feedback of being greyed out, which the disabled prop does give. So readonly is not really an alternative.

Related issues: https://github.com/marmelab/react-admin/issues/9365 https://github.com/react-hook-form/react-hook-form/issues/11055

Environment

  • React-admin version: 4.16.2
  • Last version that did not exhibit the issue (which I checked): 4.8.4
  • React version: 18.2.0
  • Browser: Google Chrome Version 119.0.6045.202 (Official Build) (64-bit)

lvernaillen avatar Dec 06 '23 16:12 lvernaillen

Thanks for the detailed report.

As you saw, this issue finds its root in a decision made by the react-hook-form team, to which we are bound to and left with few room for maneuver.

At the moment I'm not sure what solutions we can offer for this problem. But, since we don't have a fix nor a workaround for all cases, I think we should consider this as a bug.

Any help or suggestion are appreciated.

slax57 avatar Dec 06 '23 16:12 slax57

I understand that it is RHF that changed the logic to set disabled fields to undefined, changing the original value and making the form dirty.

Is there a possibility that:

  • react-admin's hook useWarnWhenUnsavedChanges would ignore the fields on the form that are disabled when isDirty is calculated so that the warning only appears for dirty non-disabled fields?
  • OR react-admin's Form components reset the value of the disabled fields after registering them, so that it does not trigger RHF isDirty?

That way, we can still use the regular disabled prop like before, without incorrectly triggering warnWhenUnsavedChanges.

lvernaillen avatar Dec 06 '23 17:12 lvernaillen

These are good suggestions, thanks!

I'd also add the following one:

  • Fix (and enhance) the support for readOnly on all inputs.

Indeed, semantically, for this use case where we want the input to be immutable but still submitted, disabled is not the correct attribute to use, whereas readOnly is.

slax57 avatar Dec 07 '23 08:12 slax57

@lvernaillen It seems to me that #9527 actually allows to apply the workaround I suggested here, which fixes this issue.

Can you confirm?

slax57 avatar Dec 14 '23 15:12 slax57

I'm afraid that doesn't work. It disables the input, but it also removes the AutoCompleteInput behavior. It becomes a regular text field. No dropdown anymore and no autocomplete.

An AutoCompleteInput that needs to be disabled in some cases will have this: TextFieldProps={{ InputProps: { disabled: someConditionHere} }} But it won't render an autocomplete input in case the "someConditionHere" is not met.

I created a new stackblitz with react-admin 4.16.3 containing #9527. https://stackblitz.com/edit/github-6a8auj-necluw?file=package-lock.json,src%2Fposts%2FPostEdit.tsx

image

lvernaillen avatar Dec 21 '23 10:12 lvernaillen

Same for AutoCompleteArrayIput. That component now has a TextFieldProps, but it does not show the values that were selected anymore and renders no removes the autocomplete behavior in case disabled = false.

For ArrayInput there is no solution. We currently wrap it in another div, disable it and set some styling to make it look disabled.

lvernaillen avatar Dec 21 '23 10:12 lvernaillen

Thanks @lvernaillen for the feedback. You are correct about AutoCompleteInput, and actually it looks like we have the same issue with MUI's Autocomplete directly. But I agree this is not a satisfactory solution. We'll keep working on a better solution.

slax57 avatar Dec 21 '23 14:12 slax57

Fixed by https://github.com/marmelab/react-admin/pull/9656

slax57 avatar Mar 01 '24 14:03 slax57