Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

warnWhenUnsavedChanges does not function properly with disabled inputs #9498

Closed
lvernaillen opened this issue Dec 6, 2023 · 8 comments
Closed

Comments

@lvernaillen
Copy link

lvernaillen commented Dec 6, 2023

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 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 #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 #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:
#9365
react-hook-form/react-hook-form#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)
@slax57 slax57 added the bug label Dec 6, 2023
@slax57
Copy link
Contributor

slax57 commented Dec 6, 2023

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.

@lvernaillen
Copy link
Author

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.

@slax57
Copy link
Contributor

slax57 commented Dec 7, 2023

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
Copy link
Contributor

slax57 commented Dec 14, 2023

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

Can you confirm?

@lvernaillen
Copy link
Author

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
Copy link
Author

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.

@slax57
Copy link
Contributor

slax57 commented Dec 21, 2023

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
Copy link
Contributor

slax57 commented Mar 1, 2024

Fixed by #9656

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants