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

Upgrading from React 14 to 15 breaks IE11 autofill behavior with controlled inputs #6614

Closed
jimbolla opened this Issue Apr 25, 2016 · 31 comments

Comments

Projects
None yet
10 participants
@jimbolla

jimbolla commented Apr 25, 2016

I have a sign in form with controlled inputs that works fine using React 14. When I upgrade to React 15, autofill behavior in IE11 no longer triggers onChange. Strangely, if I change document mode from "Edge (Default)" to "10" React 15 still behaves as expected.

Reading the changelog for R15, this change sounds like it could be related:

Input events are handled more reliably in IE 10 and IE 11; spurious events no longer fire when using a placeholder.
@jquense in #4051

@gaearon gaearon added the Browser: IE label Apr 25, 2016

@jimbolla

This comment has been minimized.

Show comment
Hide comment
@jimbolla

jimbolla Apr 27, 2016

Here's a gist to repro the bug

Steps:

  1. Download .zip and extract
  2. npm install
  3. node_modules\.bin\webpack
  4. node server.js
  5. Open IE11, goto http://localhost:8081/test.html
  6. Enter any username and password and submit the form. (Notice how the state gets updated while typing.)
  7. Click "Yes" to IE's "Would you like to store your password"
  8. Reload the page ( CTRL+R )
  9. Double-click the username textbox and select the username your entered. Notice the state does NOT get updated as expected.

This is currently blocking us from upgrading to React 15, as most of our users are on IE11 and expect autofill to work.

jimbolla commented Apr 27, 2016

Here's a gist to repro the bug

Steps:

  1. Download .zip and extract
  2. npm install
  3. node_modules\.bin\webpack
  4. node server.js
  5. Open IE11, goto http://localhost:8081/test.html
  6. Enter any username and password and submit the form. (Notice how the state gets updated while typing.)
  7. Click "Yes" to IE's "Would you like to store your password"
  8. Reload the page ( CTRL+R )
  9. Double-click the username textbox and select the username your entered. Notice the state does NOT get updated as expected.

This is currently blocking us from upgrading to React 15, as most of our users are on IE11 and expect autofill to work.

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Apr 27, 2016

Collaborator

I am not sure how the change would affect this. It only keeps changes for the same value from firing more than once. Does IE not fire a native onChange event here? If if does and the value is different it should work. I am wondering if this is still present in master with #5746 which changes the strategy a bit.

Collaborator

jquense commented Apr 27, 2016

I am not sure how the change would affect this. It only keeps changes for the same value from firing more than once. Does IE not fire a native onChange event here? If if does and the value is different it should work. I am wondering if this is still present in master with #5746 which changes the strategy a bit.

@jimbolla

This comment has been minimized.

Show comment
Hide comment
@jimbolla

jimbolla Apr 27, 2016

IE does not fire onChange on autofill. Supposedly, Safari has similar issues. There's some details in #1159

jimbolla commented Apr 27, 2016

IE does not fire onChange on autofill. Supposedly, Safari has similar issues. There's some details in #1159

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Apr 27, 2016

Collaborator

does it fire onInput? how can we know when it happens?

Collaborator

jquense commented Apr 27, 2016

does it fire onInput? how can we know when it happens?

@jimbolla

This comment has been minimized.

Show comment
Hide comment
@jimbolla

jimbolla Apr 27, 2016

I'm not sure. It works in React 0.14.8, but not 15.0.1. So something changed in that release that broke IE. Based on the changelog, #4051 seems to be the likely culprit.

jimbolla commented Apr 27, 2016

I'm not sure. It works in React 0.14.8, but not 15.0.1. So something changed in that release that broke IE. Based on the changelog, #4051 seems to be the likely culprit.

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Apr 27, 2016

Collaborator

seems like the different then is that IE10 fires onChange for autofill whereas IE11 does not :/

Collaborator

jquense commented Apr 27, 2016

seems like the different then is that IE10 fires onChange for autofill whereas IE11 does not :/

@arendjr

This comment has been minimized.

Show comment
Hide comment
@arendjr

arendjr May 11, 2016

@jimfb: Could it be this issue is also related to #6406?

arendjr commented May 11, 2016

@jimfb: Could it be this issue is also related to #6406?

@jimfb

This comment has been minimized.

Show comment
Hide comment
@jimfb

jimfb May 11, 2016

Contributor

@arendjr No, I don't think so. #6406 has nothing to do with events.

Contributor

jimfb commented May 11, 2016

@arendjr No, I don't think so. #6406 has nothing to do with events.

@arendjr

This comment has been minimized.

Show comment
Hide comment
@arendjr

arendjr May 11, 2016

Okay thanks, then I'll try to dig a bit deeper to see if I can track this down.

arendjr commented May 11, 2016

Okay thanks, then I'll try to dig a bit deeper to see if I can track this down.

@arendjr

This comment has been minimized.

Show comment
Hide comment
@arendjr

arendjr May 11, 2016

Okay, can confirm that by reverting #4051 auto-fill issues get fixed for me in IE11.

But that PR was merged to resolve #3826... so what would be the proper way forward here?

arendjr commented May 11, 2016

Okay, can confirm that by reverting #4051 auto-fill issues get fixed for me in IE11.

But that PR was merged to resolve #3826... so what would be the proper way forward here?

@jimfb

This comment has been minimized.

Show comment
Hide comment
@jimfb

jimfb May 11, 2016

Contributor

@arendjr Presumably autofill fires some sort of DOM event. When that event fires with the new value, we should fire an onChange event. So the proper path forward is to figure out which event we should be listening to, and add the logic to ensure an onChange fires.

Contributor

jimfb commented May 11, 2016

@arendjr Presumably autofill fires some sort of DOM event. When that event fires with the new value, we should fire an onChange event. So the proper path forward is to figure out which event we should be listening to, and add the logic to ensure an onChange fires.

@arendjr

This comment has been minimized.

Show comment
Hide comment
@arendjr

arendjr May 17, 2016

@jimfb: Yes, it does, it fires both "input" and "change" events in this case. The problem is, it doesn't fire the "propertychange" event React listens to.

If you want to know more about which events IE fires, see: https://msdn.microsoft.com/en-us/library/dn629640(v=vs.85).aspx
I have confirmed myself also the tables on that page are correct; so for an auto-fill "input" and "change" fire, but "propertychange" doesn't.

The thing is, except for some specific input types, React never listens to the "change" event, and because of #4051, it doesn't listen to the "input" event anymore either for IE. I've tried to dig into how to fix it, but the conclusion I keep coming back to is that #4051 should be reverted, and #3826 should be applied in its stead.

Maybe I should just create a new PR that does exactly that :)

arendjr commented May 17, 2016

@jimfb: Yes, it does, it fires both "input" and "change" events in this case. The problem is, it doesn't fire the "propertychange" event React listens to.

If you want to know more about which events IE fires, see: https://msdn.microsoft.com/en-us/library/dn629640(v=vs.85).aspx
I have confirmed myself also the tables on that page are correct; so for an auto-fill "input" and "change" fire, but "propertychange" doesn't.

The thing is, except for some specific input types, React never listens to the "change" event, and because of #4051, it doesn't listen to the "input" event anymore either for IE. I've tried to dig into how to fix it, but the conclusion I keep coming back to is that #4051 should be reverted, and #3826 should be applied in its stead.

Maybe I should just create a new PR that does exactly that :)

@arendjr

This comment has been minimized.

Show comment
Hide comment
@arendjr

arendjr May 17, 2016

Oh darn, I was testing against the branch "15-stable" (because that's where I'll need it fixed soon), but as soon as I attempted to create a PR against master, I discovered #4051 is already reverted there (maybe not through a regular "git revert", but at least the functional changes are undone) so I'll try more testing with master first...

arendjr commented May 17, 2016

Oh darn, I was testing against the branch "15-stable" (because that's where I'll need it fixed soon), but as soon as I attempted to create a PR against master, I discovered #4051 is already reverted there (maybe not through a regular "git revert", but at least the functional changes are undone) so I'll try more testing with master first...

@arendjr

This comment has been minimized.

Show comment
Hide comment
@arendjr

arendjr May 17, 2016

Tracked the revert, with accompanying fixes, down to #5746.

arendjr commented May 17, 2016

Tracked the revert, with accompanying fixes, down to #5746.

@cyberthom

This comment has been minimized.

Show comment
Hide comment
@cyberthom

cyberthom May 19, 2016

This is not IE or React 15 specific. Safari has the same issue on React 14 when autocompleting input fields. (Note that autofilling e.g. username passwords works fine)

Just to clarify:

  • autofill: The browser populating e.g. username/password form Safari's password manager on page load or after clicking on the input field and selecting the account to use.
  • autocomplete: The browser completing what users started typing (or when pressing the down key and selecting from a list of previously filled in values). I might be wrong but it seems like these values only seem to get saved when the form is submitted using standard POST. Safari does not save and autocomplete values filled in a form which got submitted using AJAX where the standard browser behaviour is canceled with event.preventDefault().

Inspecting two input fields which have autofill username/password saved against them in Safari using monitorEvents($0) shows that the browser fires a 'change' event for each autofilled field as expected.

Inspecting an input field which has autocomplete values stored against it in Safari using monitorEvents($0) shows that not a single event is fired when accepting the autocompleted value which is why React doesn't pick up the value irregardless of the version used.

cyberthom commented May 19, 2016

This is not IE or React 15 specific. Safari has the same issue on React 14 when autocompleting input fields. (Note that autofilling e.g. username passwords works fine)

Just to clarify:

  • autofill: The browser populating e.g. username/password form Safari's password manager on page load or after clicking on the input field and selecting the account to use.
  • autocomplete: The browser completing what users started typing (or when pressing the down key and selecting from a list of previously filled in values). I might be wrong but it seems like these values only seem to get saved when the form is submitted using standard POST. Safari does not save and autocomplete values filled in a form which got submitted using AJAX where the standard browser behaviour is canceled with event.preventDefault().

Inspecting two input fields which have autofill username/password saved against them in Safari using monitorEvents($0) shows that the browser fires a 'change' event for each autofilled field as expected.

Inspecting an input field which has autocomplete values stored against it in Safari using monitorEvents($0) shows that not a single event is fired when accepting the autocompleted value which is why React doesn't pick up the value irregardless of the version used.

@arendjr

This comment has been minimized.

Show comment
Hide comment
@arendjr

arendjr May 19, 2016

@cyberthom Thanks for the clarification. According to your definitions, I was talking about AutoComplete rather than AutoFill, and it appears @jimbolla was as well.

The main difference with what you're describing with Safari however, is that IE does correctly fire a change event on AutoComplete, and with React v14 this was also correctly detected. Unfortunately this is a regression in React v15, because this doesn't get detected anymore.

If it doesn't work either on Safari because Safari doesn't fire any event at all, then I agree there's nothing React can do about that, but I would say that's technically a different issue then.

arendjr commented May 19, 2016

@cyberthom Thanks for the clarification. According to your definitions, I was talking about AutoComplete rather than AutoFill, and it appears @jimbolla was as well.

The main difference with what you're describing with Safari however, is that IE does correctly fire a change event on AutoComplete, and with React v14 this was also correctly detected. Unfortunately this is a regression in React v15, because this doesn't get detected anymore.

If it doesn't work either on Safari because Safari doesn't fire any event at all, then I agree there's nothing React can do about that, but I would say that's technically a different issue then.

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense May 19, 2016

Collaborator

I'm fairly certain this can be fixed with a line of code, that also listens for onChange, in the onInput polyfillsunce it's already deduping by value change...if I send a PR anyone one of y'all want to test it? I don't have access to ie11 today

Collaborator

jquense commented May 19, 2016

I'm fairly certain this can be fixed with a line of code, that also listens for onChange, in the onInput polyfillsunce it's already deduping by value change...if I send a PR anyone one of y'all want to test it? I don't have access to ie11 today

@arendjr

This comment has been minimized.

Show comment
Hide comment
@arendjr

arendjr May 19, 2016

@jquense How would that work given that React doesn't listen to the input events in the IE11 code path?

But maybe I'm just missing something... I'd be happy to test anyway :)

arendjr commented May 19, 2016

@jquense How would that work given that React doesn't listen to the input events in the IE11 code path?

But maybe I'm just missing something... I'd be happy to test anyway :)

@jimbolla

This comment has been minimized.

Show comment
Hide comment
@jimbolla

jimbolla May 28, 2016

What can I do to help resolve this issue?

jimbolla commented May 28, 2016

What can I do to help resolve this issue?

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense May 29, 2016

Collaborator

@jimbolla help figure out why #6806 isn't working :P I have had access to ie11 recently to try myself

Collaborator

jquense commented May 29, 2016

@jimbolla help figure out why #6806 isn't working :P I have had access to ie11 recently to try myself

@jimbolla

This comment has been minimized.

Show comment
Hide comment
@jimbolla

jimbolla May 29, 2016

I've started looking into which events do or don't fire on autofill, but reconciling that with React's source is nontrivial. I'm trying to follow the progress @arendjr has made with identifying specific commits related to the issue.

For anyone that needs access to IE11 for testing purposes, you can download a Windows VM from Microsoft.

jimbolla commented May 29, 2016

I've started looking into which events do or don't fire on autofill, but reconciling that with React's source is nontrivial. I'm trying to follow the progress @arendjr has made with identifying specific commits related to the issue.

For anyone that needs access to IE11 for testing purposes, you can download a Windows VM from Microsoft.

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense May 29, 2016

Collaborator

getting react to listen on specific events in this case should be trivial (provided you know what you are looking at which I admit is not obvious). I don't mind working on it but I won't have access to ie11 still Wednesday or so

Collaborator

jquense commented May 29, 2016

getting react to listen on specific events in this case should be trivial (provided you know what you are looking at which I admit is not obvious). I don't mind working on it but I won't have access to ie11 still Wednesday or so

@jimbolla

This comment has been minimized.

Show comment
Hide comment
@jimbolla

jimbolla Jun 6, 2016

@jquense It seems to work fine for the username, but when the password field goes through getTargetInstForInputEventIE for topChange, the activeElement variable is null. I can see the node in targetInst._nativeNode though.

Looks like currently activeElement is only set when the field receives focus. That's won't work for the password field which never fires the focus event.

IE AutoComplete event handling

jimbolla commented Jun 6, 2016

@jquense It seems to work fine for the username, but when the password field goes through getTargetInstForInputEventIE for topChange, the activeElement variable is null. I can see the node in targetInst._nativeNode though.

Looks like currently activeElement is only set when the field receives focus. That's won't work for the password field which never fires the focus event.

IE AutoComplete event handling

@jimbolla

This comment has been minimized.

Show comment
Hide comment
@jimbolla

jimbolla Jul 2, 2016

Are the changes in master that should fix this included in the new React 15.2.0 build?

jimbolla commented Jul 2, 2016

Are the changes in master that should fix this included in the new React 15.2.0 build?

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Aug 2, 2016

Member

Probably not, sorry. Still need to find and ship a fix.

Member

sophiebits commented Aug 2, 2016

Probably not, sorry. Still need to find and ship a fix.

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Aug 2, 2016

Collaborator

afaict the simplest fixes are: revert my earlier PR or backport my better one :P

I've looking at the existing code and to my mind the current polyfill approach just won't work with ie11 well. Tbh this issue has probably been present for ie8-9 but the lack usage (or features like autofill) means less folks are likely to notice

Collaborator

jquense commented Aug 2, 2016

afaict the simplest fixes are: revert my earlier PR or backport my better one :P

I've looking at the existing code and to my mind the current polyfill approach just won't work with ie11 well. Tbh this issue has probably been present for ie8-9 but the lack usage (or features like autofill) means less folks are likely to notice

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Aug 2, 2016

Collaborator

and fwiw the changes in master could be back ported without breaking changes if folks wanted to (as I've noted on other such issues). @jimfb can feel free to ping me about it if anyone is interested in that approach

Collaborator

jquense commented Aug 2, 2016

and fwiw the changes in master could be back ported without breaking changes if folks wanted to (as I've noted on other such issues). @jimfb can feel free to ping me about it if anyone is interested in that approach

@brobertsUPS

This comment has been minimized.

Show comment
Hide comment
@brobertsUPS

brobertsUPS Dec 1, 2016

Are there any updates on when this might be fixed?

brobertsUPS commented Dec 1, 2016

Are there any updates on when this might be fixed?

jquense added a commit to jquense/react that referenced this issue Dec 14, 2016

Allow simulated native events to propagate
fixes #7211 fixes #6822 fixes #6614

we should make sure it doesn't break #3926 any worse (or works with #8438)

This was referenced Dec 14, 2016

@simonvizzini

This comment has been minimized.

Show comment
Hide comment
@simonvizzini

simonvizzini Mar 30, 2017

Any updates on this issue? I just implemented some controlled inputs/forms and this was instantly the first defect QA has reported 😞 Is there maybe a workaround I could implement in our code? Unfortunately, IE10+ is the main browser our customers use and without proper autocomplete support I might have to revert all React based forms to something else, creating some kind of frankenstein monster (React components rendering non-React forms)

simonvizzini commented Mar 30, 2017

Any updates on this issue? I just implemented some controlled inputs/forms and this was instantly the first defect QA has reported 😞 Is there maybe a workaround I could implement in our code? Unfortunately, IE10+ is the main browser our customers use and without proper autocomplete support I might have to revert all React based forms to something else, creating some kind of frankenstein monster (React components rendering non-React forms)

@brobertsUPS

This comment has been minimized.

Show comment
Hide comment
@brobertsUPS

brobertsUPS Mar 30, 2017

@simonvizzini I ran into a problem when running selenium tests where the test would type into the search box, but the onChange event was not triggered in React. If this is the same problem your QA is experiencing, my hack was to add an onKeyUp listener to the input so the selenium test could correctly insert text into it.

brobertsUPS commented Mar 30, 2017

@simonvizzini I ran into a problem when running selenium tests where the test would type into the search box, but the onChange event was not triggered in React. If this is the same problem your QA is experiencing, my hack was to add an onKeyUp listener to the input so the selenium test could correctly insert text into it.

flarnie added a commit that referenced this issue May 20, 2017

Backport input fix (#8575)
* Only fire input value change events when the value changes (#5746)

* Allow simulated native events to propagate

fixes #7211 fixes #6822 fixes #6614

we should make sure it doesn't break #3926 any worse (or works with #8438)

flarnie added a commit to flarnie/react that referenced this issue Jun 7, 2017

Backport input fix (#8575)
* Only fire input value change events when the value changes (#5746)

* Allow simulated native events to propagate

fixes #7211 fixes #6822 fixes #6614

we should make sure it doesn't break #3926 any worse (or works with #8438)
@flarnie

This comment has been minimized.

Show comment
Hide comment
@flarnie

flarnie Jun 13, 2017

Contributor

This should be fixed in v15.6.0, which was just released today. Please reopen or open a new issue referencing this one if it still is an issue.

Contributor

flarnie commented Jun 13, 2017

This should be fixed in v15.6.0, which was just released today. Please reopen or open a new issue referencing this one if it still is an issue.

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