MaskedTextField doesn't update with a controlled value

ArrayKnight commented 5 years ago

Environment Information

Package version(s): 6.161.0

Please provide a reproduction of the bug in a codepen:

https://codepen.io/anon/pen/LaoWzG

Actual behavior:

Value is not properly updated when being externally controlled.

Expected behavior:

Value is updated

Priorities and help requested:

Are you willing to submit a PR to fix? Yes

Requested priority: Blocking

Products/sites affected: (if applicable) MS DNA Storage

Area of issue:

https://github.com/OfficeDev/office-ui-fabric-react/blob/master/packages/office-ui-fabric-react/src/components/TextField/MaskedTextField/MaskedTextField.tsx#L92

Requested fix:

Currently:

if (newProps.mask !== this.props.mask) {
  this._maskCharData = parseMask(newProps.mask, newProps.maskFormat);
  this.state = {
    displayValue: getMaskDisplay(newProps.mask, this._maskCharData, newProps.maskChar)
  };
}

Update to:

if (newProps.mask !== this.props.mask || newProps.value !== this.props.value) {
  this._maskCharData = parseMask(newProps.mask, newProps.maskFormat);

  newProps.value && this.setValue(newProps.value);

  this.setState({
    displayValue: getMaskDisplay(newProps.mask, this._maskCharData, newProps.maskChar)
  });
}

Vitalius1 commented 5 years ago

@ArrayKnight thanks for submitting the issue and looks like you have a solution for it ready. You mentioned that you are willing to submit a fix. Thanks for this too 😄 . Let us know if you need any help with submitting the PR 👍 . We will be happy to review it when you have it put together.

@lambertwang FYI

ArrayKnight commented 5 years ago

@Vitalius1 What's the possiblity/timeline of your team being able to make this update? I'm willing to do the PR, but would like to stay focused on my project if possible.

Vitalius1 commented 5 years ago

We can certainly have a fix quite soon. We will need to test the change first and make sure we have a unit test in place to prevent regression. Will work on a PR soon and tag you when ready.

dzearing commented 5 years ago

@Vitalius1 you got this? LMK if you need assist

Vitalius1 commented 5 years ago

@dzearing I wouldn't mind some help as I am working on a HoverCard issue. The author provided the solution but it needs to be tested. Also, I wanted to tackle #7580 within the same PR but have no Windows environment to test 😄.

dzearing commented 5 years ago

@Vitalius1 no sweat :)

dzearing commented 5 years ago

ah you got it already, thank you @Vitalius1 !

msft-github-bot commented 5 years ago

:tada:This issue was addressed in #8529, which has now been successfully released as office-ui-fabric-react@v6.164.0.:tada:

Handy links:

ArrayKnight commented 5 years ago

@dzearing @Vitalius1 Thank you for the quick turn around

microsoft / fluentui