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

Backport input fix #8575

Merged
merged 3 commits into from
May 20, 2017
Merged

Backport input fix #8575

merged 3 commits into from
May 20, 2017

Conversation

jquense
Copy link
Contributor

@jquense 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
Copy link
Contributor Author

jquense commented Dec 14, 2016

CI seems like not my fault :P

@gaearon
Copy link
Collaborator

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
Copy link
Contributor

aweary commented Dec 14, 2016

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

@gaearon
Copy link
Collaborator

gaearon commented Dec 14, 2016

Oops, I didn't realize that.

@jquense
Copy link
Contributor Author

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
Copy link
Contributor Author

jquense commented Dec 29, 2016

@aweary
Copy link
Contributor

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
Copy link
Contributor Author

jquense commented Dec 29, 2016

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

@gaearon
Copy link
Collaborator

gaearon commented Jan 24, 2017

@sebmarkbage You wanted to have another look at this?

@aweary
Copy link
Contributor

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
Copy link
Collaborator

gaearon commented Jan 31, 2017

Does this work with Fiber too?

@aweary
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Contributor

aweary commented Jan 31, 2017

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

@gaearon
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

gaearon commented Jan 31, 2017

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

@sebmarkbage
Copy link
Collaborator

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?

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

Successfully merging this pull request may close these issues.