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

Dynamic form text, textarea and number fields clear values on blur after another field has been edited. #1923

Open
gchamplin opened this issue Dec 13, 2024 · 7 comments

Comments

@gchamplin
Copy link

gchamplin commented Dec 13, 2024

Category

[A dynamic form for a list with text, textarea, and number field inputs. loses values in those fields if you click or tab into the field and then click or tab out. This happens after you have edited another field. Or sometimes w/o even editing another field. This happens with no field overrides or other changes from the OOTB dynamic form. ] Bug

Version

I just updated to 3.20.0 and it's still happening

Please specify what version of the library you are using: [ 3.20.0 ]

Expected / Desired Behavior / Question

Expected behavior is for fields with values to retain those values on blur._

Observed Behavior

Observed behavior is text, textarea, and number fields losing value on blur.

Steps to Reproduce

Create a FormCustomizer and Dynamic form for an existing list with text, textarea, and number fields, and other fields as desired.
gulp serve the edit form and click in and out of a text field. It may retain its value, however, next edit another field and then click or tab into any text, textarea, or number field and then click or tab out of the field. The behavior we're seeing is the field losing focus will then also lose its value. Saving the form will update the input field as empty in the list.

Copy link

Thank you for submitting your first issue to this project.

@gchamplin
Copy link
Author

gchamplin commented Dec 19, 2024

I did some more testing and have found the issue also occurs when you open a debug (or production) edit form then:

  • Edit a field OR Save OR open a modal window
  • Click in a field showing a value from the list item
  • Click or tab out of the field
  • Field clears
  • Note: the debug form does not leave the form on Save

Additionally if you click in a field with a value and edit it, it does NOT clear, but if you enter and leave without editing the field it DOES clear.

@gchamplin
Copy link
Author

gchamplin commented Dec 19, 2024

I found where the failure is happening. I get this error in the DynamicForm.js file on leaving the field without editing the original value:
Uncaught (in promise) TypeError: Cannot read properties of null (reading 'toString')
image

@gchamplin
Copy link
Author

Have you been able to check this issue? Our workaround is to override all the fields with this problem with fluent-ui-react components, which do not have the problem.

@t0mgerman
Copy link

@gchamplin we've encountered this issue as well, and I think we know which line(s) are responsible.

It looks like this entire componentDidUpdate implementation in DynamicField.tsx is unnecessary:
https://github.com/pnp/sp-dev-fx-controls-react/blob/master/src/controls/dynamicForm/dynamicField/DynamicField.tsx#L39

It is resetting the changedValue set in state to an empty string.

The onBlur method also seems to be doing an unnecessary state change, and even if the condition isn't triggered to make that change happen there, componentDidUpdate will have done so before it calls this.props.onChanged.

If we remove the componentDidUpdate method and change:

private onBlur = (): void => {
    if (this.state.changedValue === null && this.props.defaultValue === "") {
      this.setState({ changedValue: "" });
    }
    this.props.onChanged(this.props.columnInternalName, this.state.changedValue, true);
  }

to

private onBlur = (): void => {
    // If we have a changedValue, or the user has cleared out a field
    // note: might need to amend this for complex values, array values, taxonomy values etc
    if (this.state.changedValue || this.state.changedValue === "") {
      // Notify DynamicForm and fire its onChange event
      this.props.onChanged(this.props.columnInternalName, this.state.changedValue, true);
    }
  }

This seems to fix the issue. I need to do more testing to confirm that doesn't break anything else.

As for why those things are in there in the first place, I have to speculate really:

Prior to a slight refactor that I contributed a couple of years ago, there were various props and state variables being used in DynamicForm and DynamicField - in some cases the value for defaultValue was being assigned to the value prop of field components, which made them controlled components, and I think some of this checking and state changing was a way to make all of that work. After slight refactoring, these were simplified down to defaultValue (the default value from the column settings), value (the value currently assigned to the item) and changedValue (the value if the field is changed in the form). It looks that simplification made these lines unnecessary but I hadn't caught it.

In compiled code (in node_modules, DynamicField.js) you might be able to make the changes above if you look for _this.onBlur = function () {, and DynamicField.prototype.componentDidUpdate

If I get my dev branch up to date and test the above, I'll submit a PR.

@Rothrock42
Copy link

@t0mgerman thank you for taking a look at this. Making the two changes you suggested in DynamicField.js seems to solve the issue. We haven't seen any ill effects from the change; at least with a casual glance. We'll post back if we notice anything hinky.

Thank you so much.

@gchamplin
Copy link
Author

@t0mgerman I want to second Rothrock's comments above, thanks so much for your help, so far so good.

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

No branches or pull requests

3 participants