Use an ordered property list for inputs, fix some Chrome number input issues #7474

nhunzaker · 2016-08-11T20:11:04Z

Discussed in #7428. I'm finally to a point where I feel good enough about this code and I thought I'd make it easier to comment on it.

This is a reimplementation of #7359 that additionally eliminates much of the work done inside of ReactDOMInput to handle quirks associated with defaultValue, value, defaultChecked, and checked.

It does this by ensuring a specific order for property assignment on the input element. This is implemented within DOMProperty and HTMLDOMPropertyConfig, which may allow for other elements to eventually take advantage of it (should they need it).

Performance changes

Since they do less work, controlled inputs are slightly faster, while non-ordered HTML tags have no observable performance regression (in my ability to test, anyway).

Fixes from the original PR

This is a major deviation from my solution in #7359, but it includes the same input fixes. These are enumerated in #7253 (comment). Basically, since the mutation method for value first checks to see if the property is loosely the same, it eliminates many edge cases with controlled number inputs, like:

Controlled and uncontrolled numbers won't drop off decimal places when backspacing, like converting 3. to 3
Controlled numbers that use parseFloat or parseInt won't "expand" values, like magically replace 3e1 with 30.

Other fixes

Some fixes are no longer necessary inside of ReactDOMInput:

Assigning type and step before value for IE11
Properly ordering defaultValue/value for inputs to ensure date inputs display properly on Android and iOS.
Ensure defaultChecked does not influence checked in Chrome 50.x

This is a big change. I don't expect it to be merged easily. If there is any additional documentation I can provide, or benchmarks you would like to see, I am happy to produce them.

nhunzaker · 2016-08-11T20:37:02Z

It would appear that a couple of server-side rendering tests fail:

● ReactDOMInput › it should update `defaultValue` for uncontrolled input
  - Expected '1' to be '0'.
        at Object.eval (src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js:80:24)
● ReactDOMInput › it should update `defaultValue` for uncontrolled date/time input
  - Expected '2000-01-01' to be '1980-01-01'.
        at Object.eval (src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js:93:24)
● ReactDOMInput › it should take `defaultValue` when changing to uncontrolled input
  - Expected '1' to be '0'.
        at Object.eval (src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js:108:24)

I apologize for not catching this, there's a specific server-side rendered test that I added the noMarkup option for:

https://github.com/facebook/react/blob/master/src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js#L112-L118

However these other tests relate to updates, which I was not aware was possible for server side rendering.

Is this the correct server-side rendered behavior for these tests?

Either way, it looks like this test path isn't activated when running npm test. How hard would it be to execute both test paths on environments other than CI?

gaearon · 2016-08-13T15:33:49Z

Either way, it looks like this test path isn't activated when running npm test. How hard would it be to execute both test paths on environments other than CI?

It’s not hard, just a bit long and currently requires changing the source code (yea, not ideal).
For now you can flip useCreateElement in ReactDOMFlags to test server rendering.

nhunzaker · 2016-08-13T15:35:58Z

@gaearon Cool. I'll make that a part of my flow.

As for the other question. I'm simply wrong. The tests caught a bug.

If markup is server-rendered, I removed React's chance chance to directly assign value to the input before it updated. defaultValue never got a chance to detach.

I've pushed a fix. Passes locally. We'll see what Travis says :)

aweary · 2016-08-16T04:04:36Z

src/renderers/dom/shared/ReactDOMComponent.js

    for (propKey in nextProps) {
-      var nextProp = nextProps[propKey];
-      var lastProp =
+      if (ordered != null && ordered.indexOf(propKey) >= 0) {


How does this perform for an <input /> that may have numerous props, compared to the current implementation? An indexOf check for every prop in nextProps seems potentially expensive.

I'll run a benchmark. In an earlier version of this PR I stored the properties in an array and an object to eliminate indexOf calls, but I removed it because it added extra parts and primitive benchmarks of indexOf weren't compelling (especially with the general performance improvement by eliminating work in ReactDOMInput).

So let's find out!

Formalized my original benchmark and put it on the internet:
https://github.com/nhunzaker/react-ordered-props-bench

I'm happy to accept any profiling tips here. I'm a heavy user of React and benefit from accuracy. I want to get this right.

For your specific question, I added a bunch of properties (custom attributes, if that matters) to some controlled inputs:

http://natehunzaker.com/react-ordered-props-bench/heavy-props.html CONTROL 1628 (+= 2.299877893626147) EXPERIMENT 1750 (+= 1.870748777520246) Fastest is EXPERIMENT

Though the noise makes it hard to say it's faster, I do not think it is slower.

Unfortunately, I noticed that there seems to be a performance regression with uncontrolled inputs:

http://natehunzaker.com/react-ordered-props-bench/uncontrolled.html CONTROL 5307 (+= 3.5737538615198474) EXPERIMENT 4475 (+= 2.2288121746511043) Fastest is CONTROL

Unclear why I didn't catch this before. I'm also not sure specifically why there's a regression. This PR includes fixes to "decontrol" uncontrolled inputs (so that they don't reassign themselves and screw up number inputs). I do not know yet if the performance degradation relates to that, or the ordered properties approach generally. I'm looking in to that now.

One other interesting secondary finding is that the _handleChange binding always gets applied to uncontrolled inputs, even if there is no change handler:
https://github.com/facebook/react/blob/master/src/renderers/dom/client/wrappers/ReactDOMInput.js#L79

Best I can tell this is waste. Is there a good reason for _handleChange to fire on uncontrolled inputs? If not, we could kill some overhead here.

kaushik94 · 2016-09-15T12:18:56Z

Hi, we use controlled inputs in a lot of our forms and face this issue. @nhunzaker thanks a ton for taking the pain of trying to solve this. May I know the current state of this PR, I would like to be of some help if needed.

nhunzaker · 2016-09-15T13:37:03Z

Hey @kaushik94 Check out #7359. This PR is an alternative fix (that also eliminates the need for a lot of other input fixes), but #7359 has helped me to identify that it doesn't address every concern raised by the React team (specifically setting the value attribute as a controlled input updates).

There's a pretty long conversation there, but I believe I've documented every approach I have tried within the comments. The gist of it is, I've solved every use case except the following:

Controlled inputs should (according to the team) assign a value attribute as a controlled input updates. This is to avoid issues with form.reset() and browser extensions that rely on the value attribute. Essentially, the thought is that all state for the input should live in React.

To be honest, I'm sort of tapped out at this point and waiting some feedback from the team (see #7359 (comment)). I'm still putting in time as I can to explore solutions, but I would love fresh thoughts.

In the short term, I think something has to be done to at least make number inputs useable. I'll submit another PR shortly that solves these issues for uncontrolled inputs in a non-breaking way. No reason to block progress while we sort out what to do about controlled inputs.

syranide · 2016-09-15T13:41:43Z

src/renderers/dom/shared/HTMLDOMPropertyConfig.js

@@ -209,6 +229,54 @@ var HTMLDOMPropertyConfig = {
  },
  DOMPropertyNames: {
  },
+  DOMMutationMethods: {
+    value: function(node, next) {


value is shared by <progress> (and some other tag?), so this seems like a no-go. A progress with null value must remove the attribute.

Ya. Took care of that in #7359.

Hmm.. This PR is creating some extra noise for the input issue. Should I close this for now and resubmit after that settles?

Ah heck, I'll just bring it over while I'm thinking about it. That way things are at least up to date. Thanks for the prod!

Added, and I brought over the <progress> test case from #7359 too. Though it re-introduces the input[type="number"] decimal place loss issue. Either way both PRs are synced.

I don't feel awesome about this value mutation method, but with all of the mutations in one place, it's way easier to reason about the [type="number"] issues on this branch. Curious if I'll get any inspiration shifting back here.

…date fix

Server rendered markup never has `value` assigned as a property, so it never detaches from defaultValue. There must be a postMount hook on ReactDOMInput to assign value.

…on method

nhunzaker · 2016-11-19T17:10:36Z

I'm going to close this out to cut down on clutter and avoid confusion with #7359.

nhunzaker changed the title ~~Use an ordered property list for inputs, fix number input backspace issues in Chrome and invalid input. Eliminate need for input assignment fixes.~~ Use an ordered property list for inputs, fix some Chrome number input issues Aug 11, 2016

ghost added GH Review: review-needed CLA Signed labels Aug 11, 2016

nhunzaker mentioned this pull request Aug 11, 2016

Proposal for an ordered attribute white list, and new property injection types #7428

Closed

nhunzaker force-pushed the nh-ordered-props branch from f182a2f to bce5dff Compare August 12, 2016 22:34

ghost added the CLA Signed label Aug 12, 2016

facebook-github-bot added the CLA Signed label Aug 13, 2016

nhunzaker mentioned this pull request Aug 13, 2016

Controlled input with type=email produces console warnings with Chrome 52 #7487

Closed

nhunzaker force-pushed the nh-ordered-props branch from 30fa07c to f5e5324 Compare August 16, 2016 02:19

ghost added the CLA Signed label Aug 16, 2016

aweary reviewed Aug 16, 2016
View reviewed changes

nhunzaker force-pushed the nh-ordered-props branch from f5e5324 to f58f614 Compare August 17, 2016 02:00

ghost added the CLA Signed label Aug 17, 2016

nhunzaker force-pushed the nh-ordered-props branch from f58f614 to ae22ecf Compare August 30, 2016 13:33

ghost added the CLA Signed label Aug 30, 2016

ghost added the CLA Signed label Sep 15, 2016

syranide reviewed Sep 15, 2016

View reviewed changes

nhunzaker force-pushed the nh-ordered-props branch from ae22ecf to 8e70fa2 Compare September 16, 2016 01:33

ghost added the CLA Signed label Sep 16, 2016

nhunzaker added 5 commits September 28, 2016 20:48

Store properties as a list

eaf986b

Remove mutation methods

5e92612

Add property assignment test for similar value.

f836654

No need to update value/checked on updateWrapper or have iOS/Android …

2f5050e

…date fix

Switch back to object config. Add priority field

5a8c422

nhunzaker added 14 commits September 28, 2016 20:48

No need to add forced update branch

7bfcb06

Do not remove mutation method. Keep change small

2032fc8

Use mutation method for value. Add test coverage for all states

762fca3

defaultChecked chrome bug can be handled as mutation method

6238eec

Faster enumeration of ordered props

3739426

Only order props for inputs

e8c77cd

Revert null assignment condition removal

4d54faa

No need to prioritize defaultValue and defaultChecked

0495c8f

Eliminate some white space. Add comment about order property.

1dfbd8c

Place min/max ahead of value input prop order list

cc54aa4

There must be a post mount wrapper

0501c39

Server rendered markup never has `value` assigned as a property, so it never detaches from defaultValue. There must be a postMount hook on ReactDOMInput to assign value.

Re-eliminate the postmount wrapper with a special defaultValue mutati…

d39d919

…on method

Be sure to update value attributes

5d8c8ef

Add test case for progress elements

4e5ac47

nhunzaker force-pushed the nh-ordered-props branch from 8e70fa2 to 4e5ac47 Compare September 29, 2016 00:48

facebook-github-bot added the CLA Signed label Sep 29, 2016

nhunzaker closed this Nov 19, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use an ordered property list for inputs, fix some Chrome number input issues #7474

Use an ordered property list for inputs, fix some Chrome number input issues #7474

nhunzaker commented Aug 11, 2016

nhunzaker commented Aug 11, 2016

gaearon commented Aug 13, 2016

nhunzaker commented Aug 13, 2016 •

edited

Loading

aweary Aug 16, 2016

nhunzaker Aug 16, 2016 •

edited

Loading

nhunzaker Aug 17, 2016

kaushik94 commented Sep 15, 2016

nhunzaker commented Sep 15, 2016

syranide Sep 15, 2016

nhunzaker Sep 15, 2016

nhunzaker Sep 15, 2016

nhunzaker Sep 16, 2016

nhunzaker commented Nov 19, 2016

Use an ordered property list for inputs, fix some Chrome number input issues #7474

Use an ordered property list for inputs, fix some Chrome number input issues #7474

Conversation

nhunzaker commented Aug 11, 2016

Performance changes

Fixes from the original PR

Other fixes

nhunzaker commented Aug 11, 2016

gaearon commented Aug 13, 2016

nhunzaker commented Aug 13, 2016 • edited Loading

aweary Aug 16, 2016

Choose a reason for hiding this comment

nhunzaker Aug 16, 2016 • edited Loading

Choose a reason for hiding this comment

nhunzaker Aug 17, 2016

Choose a reason for hiding this comment

kaushik94 commented Sep 15, 2016

nhunzaker commented Sep 15, 2016

syranide Sep 15, 2016

Choose a reason for hiding this comment

nhunzaker Sep 15, 2016

Choose a reason for hiding this comment

nhunzaker Sep 15, 2016

Choose a reason for hiding this comment

nhunzaker Sep 16, 2016

Choose a reason for hiding this comment

nhunzaker commented Nov 19, 2016

nhunzaker commented Aug 13, 2016 •

edited

Loading

nhunzaker Aug 16, 2016 •

edited

Loading