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

Backport input fix #8575

Merged
merged 3 commits into from May 20, 2017

Conversation

Projects
None yet
@jquense
Collaborator

jquense commented Dec 14, 2016

cc @aweary

fixes #7211 fixes #6822 fixes #6614

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

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Dec 14, 2016

Collaborator

CI seems like not my fault :P

Collaborator

jquense commented Dec 14, 2016

CI seems like not my fault :P

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Dec 14, 2016

Member

If you added new tests please run ./scripts/fiber/record-tests and commit the result.
It’s how we keep track of Fiber regressions.

Member

gaearon commented Dec 14, 2016

If you added new tests please run ./scripts/fiber/record-tests and commit the result.
It’s how we keep track of Fiber regressions.

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Dec 14, 2016

Member

@gaearon these tests already exists in master, this PR is open against the 15-dev branch.

Member

aweary commented Dec 14, 2016

@gaearon these tests already exists in master, this PR is open against the 15-dev branch.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Dec 14, 2016

Member

Oops, I didn't realize that.

Member

gaearon commented Dec 14, 2016

Oops, I didn't realize that.

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Dec 29, 2016

Collaborator

ping, anything I can do here? I'm not sure what the issue with CI is but it doesn't seem related.

Collaborator

jquense commented Dec 29, 2016

ping, anything I can do here? I'm not sure what the issue with CI is but it doesn't seem related.

@jquense

This comment has been minimized.

Show comment
Hide comment
@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Dec 29, 2016

Member

Sorry for the delay @jquense, I haven't had too much free time unfortunately. I reviewed it and it looks 👍 to me, but haven't actually pulled it down and tested how it might affect #3926 yet

Member

aweary commented Dec 29, 2016

Sorry for the delay @jquense, I haven't had too much free time unfortunately. I reviewed it and it looks 👍 to me, but haven't actually pulled it down and tested how it might affect #3926 yet

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Dec 29, 2016

Collaborator

no worries, just wanted to make sure it wasn't blocked by me :)

Collaborator

jquense commented Dec 29, 2016

no worries, just wanted to make sure it wasn't blocked by me :)

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 24, 2017

Member

@sebmarkbage You wanted to have another look at this?

Member

gaearon commented Jan 24, 2017

@sebmarkbage You wanted to have another look at this?

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Jan 31, 2017

Member

@jquense Alright, I finally got a chance to verify behavior locally in regards to #3926. Without any changes it doesn't make the issue any worse (still fires before composition event finishes). But I cherry picked ae54338 from #8438 and after minimal changes (making sure nativeEvent was getting passed correctly at a few places) it seems to work. onChange doesn't fire until the composition event ends.

I tested with the native Pinyin keyboard provided by OS X (10.12.2).

I'll continue to look at it and verify that there are no regressions, but I think this looks good 👍

@gaearon @sebmarkbage if either of you want to give it a second review let me know and I can hold off on moving forward with this.

Member

aweary commented Jan 31, 2017

@jquense Alright, I finally got a chance to verify behavior locally in regards to #3926. Without any changes it doesn't make the issue any worse (still fires before composition event finishes). But I cherry picked ae54338 from #8438 and after minimal changes (making sure nativeEvent was getting passed correctly at a few places) it seems to work. onChange doesn't fire until the composition event ends.

I tested with the native Pinyin keyboard provided by OS X (10.12.2).

I'll continue to look at it and verify that there are no regressions, but I think this looks good 👍

@gaearon @sebmarkbage if either of you want to give it a second review let me know and I can hold off on moving forward with this.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 31, 2017

Member

Does this work with Fiber too?

Member

gaearon commented Jan 31, 2017

Does this work with Fiber too?

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Jan 31, 2017

Member

I'm not super familiar with how Fiber interfaces with the existing event system, but I can switch on the flag and test it out.

Member

aweary commented Jan 31, 2017

I'm not super familiar with how Fiber interfaces with the existing event system, but I can switch on the flag and test it out.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 31, 2017

Member

You can see ReactDOMComponent being forked as ReactDOMFiberComponent, the rest of the code is mostly reused. Let me know if you have any specific questions. Thanks!

We’d need Fiber parity to ship.

Member

gaearon commented Jan 31, 2017

You can see ReactDOMComponent being forked as ReactDOMFiberComponent, the rest of the code is mostly reused. Let me know if you have any specific questions. Thanks!

We’d need Fiber parity to ship.

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Jan 31, 2017

Member

This change is already in master too btw, so you may have already been testing with it.

Member

aweary commented Jan 31, 2017

This change is already in master too btw, so you may have already been testing with it.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 31, 2017

Member

Oooh. I didn’t realize this was against 15-dev, sorry.
Then Fiber parity doesn’t matter that much I guess.

In fact it’s impossible because we haven’t cherry-picked Fiber PRs there at all.

Member

gaearon commented Jan 31, 2017

Oooh. I didn’t realize this was against 15-dev, sorry.
Then Fiber parity doesn’t matter that much I guess.

In fact it’s impossible because we haven’t cherry-picked Fiber PRs there at all.

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Jan 31, 2017

Collaborator

the meat of this is already in and functions with v16, so it should be ok?

If the event marshaling bit here (the code on top of #5746) is a problem for fiber we can remove it. It is convenient but not really needed except to prevent a small but technically breaking change in tests.

Collaborator

jquense commented Jan 31, 2017

the meat of this is already in and functions with v16, so it should be ok?

If the event marshaling bit here (the code on top of #5746) is a problem for fiber we can remove it. It is convenient but not really needed except to prevent a small but technically breaking change in tests.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 31, 2017

Member

I wonder why CI is failing. Could you rebase? Might be the old version of 15 branch.

Member

gaearon commented Jan 31, 2017

I wonder why CI is failing. Could you rebase? Might be the old version of 15 branch.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Jan 31, 2017

Member

Why is it important to have the getter/setter on the .value/.checked properties? Couldn't we just compare it to React's internal state of the prop?

Is it that you want to protect against manual mutation of the value? What scenario does that happen that is common enough to warrant this?

My concern is that the input value tracker code is:

  • Rather big and we'll want to address file size by minimizing code of little used features.
  • Complex so it's kind of difficult to follow along and be sure that everything is covered.
  • There are attach/detach phases which is not great for Fiber because we explicitly don't have a clean up phase for DOM nodes in most cases and instead rely on GC.
  • It can probably be slow to do all that object property descriptor stuff on critical paths like initialization.

I wonder if we could sacrifice one of the edge cases for a smaller solution that only compares to the internal state of the React prop?

Member

sebmarkbage commented Jan 31, 2017

Why is it important to have the getter/setter on the .value/.checked properties? Couldn't we just compare it to React's internal state of the prop?

Is it that you want to protect against manual mutation of the value? What scenario does that happen that is common enough to warrant this?

My concern is that the input value tracker code is:

  • Rather big and we'll want to address file size by minimizing code of little used features.
  • Complex so it's kind of difficult to follow along and be sure that everything is covered.
  • There are attach/detach phases which is not great for Fiber because we explicitly don't have a clean up phase for DOM nodes in most cases and instead rely on GC.
  • It can probably be slow to do all that object property descriptor stuff on critical paths like initialization.

I wonder if we could sacrifice one of the edge cases for a smaller solution that only compares to the internal state of the React prop?

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage May 18, 2017

Member

We're going to merge this into the release of 15.6. We really should've done this way early. I apologize for that. We didn't fully understand the consequences but I think I have a good grasp of it now.

In the future we will likely stop supporting the use case for a polyfilled onChange on uncontrolled components and with use of ref mutation. See #9657 for more detail. This would allow us to get rid of the value tracking approach in a future version. However, to preserve the existing APIs and fix the relevant issues in 15.x we'll need this approach as identified.

Member

sebmarkbage commented May 18, 2017

We're going to merge this into the release of 15.6. We really should've done this way early. I apologize for that. We didn't fully understand the consequences but I think I have a good grasp of it now.

In the future we will likely stop supporting the use case for a polyfilled onChange on uncontrolled components and with use of ref mutation. See #9657 for more detail. This would allow us to get rid of the value tracking approach in a future version. However, to preserve the existing APIs and fix the relevant issues in 15.x we'll need this approach as identified.

@flarnie

This comment has been minimized.

Show comment
Hide comment
@flarnie

flarnie May 20, 2017

Contributor

Just tried rebasing this onto 15.6 and the only conflicts seem related to the 'prettier' syntax changes we made on 15.6.

I'm not sure what the CI failure here is. Going to try and get this merged into 15.6. Thanks everyone for your patience on this PR. :)

Contributor

flarnie commented May 20, 2017

Just tried rebasing this onto 15.6 and the only conflicts seem related to the 'prettier' syntax changes we made on 15.6.

I'm not sure what the CI failure here is. Going to try and get this merged into 15.6. Thanks everyone for your patience on this PR. :)

@flarnie flarnie changed the base branch from 15-dev to 15.6-dev May 20, 2017

@flarnie

This comment has been minimized.

Show comment
Hide comment
@flarnie

flarnie May 20, 2017

Contributor

Makes sense that 'prettier' is failing, since this was originally made against a branch where prettier was failing. I'll make a separate commit to run 'prettier' after merging this.

Contributor

flarnie commented May 20, 2017

Makes sense that 'prettier' is failing, since this was originally made against a branch where prettier was failing. I'll make a separate commit to run 'prettier' after merging this.

@flarnie flarnie merged commit 68347c9 into facebook:15.6-dev May 20, 2017

1 check failed

ci/circleci Your tests failed on CircleCI
Details

flarnie added a commit to flarnie/react that referenced this pull request May 21, 2017

Add #8575 to changelog for 15.6
Just two more items before we will have an RC ready. :)

flarnie added a commit that referenced this pull request May 23, 2017

Add #8575 to changelog for 15.6 (#9740)
Just two more items before we will have an RC ready. :)

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

Remove tests which use dependency that is missing in 15.6
**what is the change?:**
Removed some tests which were added in facebook@e29871e

**why make this change?:**
These tests use `inputValueTracking`, which was only added on the master branch and has not yet been added to the 15.* branch.
[There is a PR out](facebook#8575) to add this and related bug fixes to the 15.* branch, but there are some unresolved issues there. We plan to follow up on this after releasing 15.6.
For now, we are removing these to unblock releasing 15.6.

**test plan:**
`npm run test`

**issue:**
facebook#9410

flarnie added a commit to flarnie/react that referenced this pull request 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)

hannesj added a commit to HSLdevcom/digitransit-ui that referenced this pull request Jun 13, 2017

enumerable: descriptor.enumerable,
configurable: true,
get: function() {
return descriptor.get.call(this);

This comment has been minimized.

@gaearon

gaearon Jun 14, 2017

Member

This is reported to crash iOS Safari: #9956

@gaearon

gaearon Jun 14, 2017

Member

This is reported to crash iOS Safari: #9956

@slorber slorber referenced this pull request Jul 10, 2017

Open

IE 11 not supported #43

@renovate renovate bot referenced this pull request Feb 2, 2018

Open

Update dependency react to v16 #29

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