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

Type of input field is not correctly updated in Safari #7144

Closed
yihangho opened this issue Jun 29, 2016 · 5 comments · Fixed by #7333
Closed

Type of input field is not correctly updated in Safari #7144

yihangho opened this issue Jun 29, 2016 · 5 comments · Fixed by #7333

Comments

@yihangho
Copy link

Do you want to request a feature or report a bug?

This is a bug report.

What is the current behavior?

In Safari, the type of input field is not updated properly. Consider the example linked below. It is a React component that display a button and an input field. The type of input field will be toggled between file or text by clicking the button. The change of input type is not being reflected.

I believe this is an issue with Safari. Setting the type property in Safari does not affect what is being rendered:

var a = document.createElement("input");
a.type = "text";
document.body.appendChild(a); // A text input is shown
a.type = "file"; // The input remains a text input in Safari.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://jsfiddle.net or similar (template: https://jsfiddle.net/reactjs/69z2wepo/).

https://jsfiddle.net/x1xtbjt0/2/

What is the expected behavior?

The input type should be changed, as in Chrome and Firefox.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

React: 15.1.0
ReactDOM: 15.1.0
Browser: Safari 9.1.1, Chrome 51.0.2704.103, Firefox 47.0
OS: OS X 10.11.5

@syranide
Copy link
Contributor

This affects other browsers too, notably IE8 (and more?) which will actually throw an error if you change the type #2242. IMHO changing type should be equal to changing component type and remount entirely, changing the type but keeping "state" doesn't really make sense. I would even assume that whenever you see this it means you are missing a key somewhere.

@aweary
Copy link
Contributor

aweary commented Jun 29, 2016

The one use case I've run across is toggling an input between "text" and "password" for workflows that let the user see the password they're entering. This requires the state to persist between the type change, but this does seem to work in Safari and other browsers here. It's just switching between a text input and file input that's not working which, as @syranide mentioned, seems like a step where persisting state doesn't make a lot of sense.

@yihangho
Copy link
Author

@syranide, @aweary - Thanks for your comments!

The example I gave is a contrived one. It is the smallest example I can come up with to demonstrate the problem. Indeed, why would anyone wants to toggle between a text and file input? (Well, one practical use that I can think of is to let the user choose between uploading a file or giving a link to that file.) The problem I encounter in a real application is a little more realistic than that (it is described at the end of this comment if you are curious).

Anyway, I do think React should do something about this. I think React should either (I'm not sure what's React's policy on cross-browser compatibility, so some of these might be irrelevant):

  • Handle this case correctly on its own. (Detects that it's running on affected browsers (e.g., Safari and IE) and actually remount a new DOM instead of updating the type property when the diff says that the type property should be updated to a "problematic" value.)
  • Opt to not fix this. At the very least, the docs should be updated to warn the user about this issue and how the user can avoid this issue (e.g., by setting the key attribute). Better still, a warning can be logged during runtime.

Ideally, I think React should go with the first option. Since the really nice thing about React is that whatever my render method returns, it will be rendered as such, and as a user, I don't really care what are the mutations done to achieve that. Having said that, I totally understand if option 2 is chosen (since this is really a browser bug IMO).

Aside (what my real application is doing when I encountered this problem):

The component I am building is a form, but there are multiple variations of the same form (each variation differs from another by perhaps one or two fields). Hence, the component has several methods in this form:

renderFieldName() {
  return (
    <div className="ui field">
      <label>Field Name</label>
      <input type="text" />
    </div>
  );
}

To render each variation, there are several methods in this form:

renderVariationA() {
  return (
    <form className="ui form">
      { this.renderFieldName1() }
      { this.renderFieldName2() }
      { /* and so on */ }
    </form>
  );
}

Finally, the render method is simply a big if-else statement (in this case, the values of variationX are derived from some state property, and that particular property can be modified after the user interact with some of the fields, but that is not very important in this context):

render() {
  if (variationA) {
    return this.renderVariationA();
  } else if (variationB) {
    return this.renderVariationB();
  } /* and so on */
}

The problem comes when we change between two variations that have a file input and text input at the same position. For e.g., changing from variation A to B will be problematic (notice text field B and file field D are in the same position):

renderVariationA() {
  return (
    <form className="ui form">
      { this.renderTextFieldA() }
      { this.renderTextFieldB() }
    </form>
  );
},
renderVariationB() {
  return (
    <form className="ui form">
      { this.renderTextFieldC() }
      { this.renderFileFieldD() }
    </form>
  );

@zpao
Copy link
Member

zpao commented Jun 30, 2016

#6441 has another variation of this problem with an "easy" workaround (assign a key so we know when you swap types and forcing a node removal and new node insertion)

@yihangho
Copy link
Author

yihangho commented Jul 1, 2016

@zpao Yup, someone from the IRC channel suggested that too!

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

Successfully merging a pull request may close this issue.

4 participants