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

[TextField] Fix first character composition issue #6456

Merged
merged 2 commits into from
Apr 1, 2017

Conversation

keifuji
Copy link

@keifuji keifuji commented Mar 29, 2017

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".

Tiny change. It seems to me that, when the component is conrolled,
'value' needs to be checked only in componentWillReceiveProps,
and event.target.value in handleInputChange has nothing to do with
hasValue state.

At least, this PR can fix #3394 issue which, still remaining despite #5540,
breaks 'first' character composition of CJK or other input methods
when used with, for example, redux-form.

Closes #3394.

@muibot muibot added PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Mar 29, 2017
Tiny change. It seems to me that, when the component is conrolled,
'value' needs to be checked only in componentWillReceiveProps,
and event.target.value in handleInputChange has nothing to do with
hasValue state.

At least, this PR can fix mui#3394 issue which, still remaining despite mui#5540,
breaks 'first' character composition of CJK or other input methods
when used with, for example, redux-form.
@keifuji keifuji force-pushed the fix_textfield_rerender_bug branch from 8fb8c09 to ddc1925 Compare March 29, 2017 14:56
@oliviertassinari oliviertassinari added component: text field This is the name of the generic UI component, not the React module! and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Mar 30, 2017
@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 30, 2017

@keifuji The idea behind that change looks good to me. Would you mind adding a test in case it's introducing a regression and we need to do further work? Thanks for the fix!

@oliviertassinari oliviertassinari changed the title [TextField]Fix first character composition issue #3394 [TextField] Fix first character composition issue Mar 30, 2017
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Mar 30, 2017
Tests for uncontrolled component's hasValue state regarding event.target.value.
Tests for controlled component's hasValue state reagarding value prop.
@keifuji
Copy link
Author

keifuji commented Mar 31, 2017

@oliviertassinari I added some tests to check hasValue state regarding event and value prop.
(I'm not familiar with enzyme, so it might be odd.)

Anyway, I found the problem has the same origin as inconsistency between state.hasValue and props.value. In fact, the question is not re-rendering but pre-rendering before it receives next props. Until then, the state rest true even though props.value is still invalid.

These are logs from TextField wrapped by reduxForm:

  1. handleInputChange -----------------------------------
    states and props: {this.props.value: "", hasValue: false}
    reduxForm.onChange {firstName: "a"}
    ** render TextField ** // unnecessary rendering triggered by setState
  2. componentWillReceiveProps ---------------------------
    states and props: {this.props.value: "", hasValue: true} // <==== Inconsistency!
    componentWillReceiveProps {nextProps: Object, value: "a"}
    ** render TextField ** // reasonable rendering
    (callback) setState in componentWillReceiveProps: {this.props.value: "a", isValid: true, hasValue: true}
    (callback) setState in handleInputChange: {this.props.value: "a", isValid: true, hasValue: true}

So I included additional tests for controlled component, which will be able to prevent the issue from reappearing, and necessarily fails in the current version.

@oliviertassinari oliviertassinari added PR: Review Accepted and removed PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Mar 31, 2017
@mbrookes mbrookes merged commit 5d3ffe7 into mui:master Apr 1, 2017
@mbrookes
Copy link
Member

mbrookes commented Apr 1, 2017

@keifuji Thanks!

@keifuji keifuji deleted the fix_textfield_rerender_bug branch April 2, 2017 03:47
170102 pushed a commit to 170102/material-ui that referenced this pull request Apr 7, 2017
* [TextField]Fix first character composition issue mui#3394

Tiny change. It seems to me that, when the component is conrolled,
'value' needs to be checked only in componentWillReceiveProps,
and event.target.value in handleInputChange has nothing to do with
hasValue state.

At least, this PR can fix mui#3394 issue which, still remaining despite mui#5540,
breaks 'first' character composition of CJK or other input methods
when used with, for example, redux-form.

* Add tests of hasValue

Tests for uncontrolled component's hasValue state regarding event.target.value.
Tests for controlled component's hasValue state reagarding value prop.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: text field This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants