Fix event handling when input name='nodeName'

[jimfb]

Fix event handling when input name='nodeName'. Fixes #6284

cc @zpao

[zpao]

Let's figure out the solution first and then fix the whole class of issues, whack-a-mole isn't fun (there's another method in that file which needs to be fixed anyway).

[jimfb]

@zpao Just fixed the other method (I assume that's the one you were referring to), also switched to the instanceof solution you preferred. Tested it here: http://jsfiddle.net/r0espr50/

I think whack-a-mole is a little unavoidable in this case. Anywhere we assume that any attribute of an element exists (or rather, contains the expected value), it could break if the element happens to be a form. We just need to find them and squash them.

[facebook-github-bot]

@jimfb updated the pull request.

[zpao]

There are still a bunch of other uses of nodeName used in the exact same pattern that aren't covered here (just grep for it). Again though, we should make sure that's a good solution to the problem and not opening us up to other problems (eg, do you have any insight into the perf tradeoff, are there other things we should look out for?)

[jimfb]

There are still a bunch of other uses of nodeName used in the exact same pattern that aren't covered here (just grep for it).

Yeah, nodeName is mostly used for unit tests. It's only a problem when we're potentially going to run up against a form element (or window, with Ids). If you spot another one that you think will have a similar issue, feel free to flag it. At least this makes things incrementally better.

Again though, we should make sure that's a good solution to the problem and not opening us up to other problems (eg, do you have any insight into the perf tradeoff, are there other things we should look out for?)

instanceof is faster than doing a typeof check (http://jsperf.com/wgerherherh/2), so this seems fine from a perf perspective. It's a pretty straight forward diff, so I think we should just push it forward. If we happen to discover that it's bad, we can always revert, but I think it's fine. If you have an idea of an alternative you would prefer, just say so.

[zpao]

If you spot another one that you think will have a similar issue, feel free to flag it.

Might not have the same issue, but we should just use the same pattern. No point doing the same thing different ways when we know 1 has an issue and there are supposedly no tradeoffs.

react/src/renderers/dom/client/ReactInputSelection.js

Lines 33 to 36 in c528732

	var nodeName = elem && elem.nodeName && elem.nodeName.toLowerCase();
	return nodeName && (
	(nodeName === 'input' && elem.type === 'text') ||
	nodeName === 'textarea' ||

(another case in that file).

Also

react/src/renderers/dom/shared/ReactDOMContainerInfo.js

Line 25 in c528732

_tag: node ? node.nodeName.toLowerCase() : null,

which we have to do differently since we want the tag name. We can probably just check we have a string and not care too much in this specific case. That was trivial to repro: http://jsfiddle.net/nf8tr6k9/

[jimfb]

No point doing the same thing different ways when we know 1 has an issue and there are supposedly no tradeoffs.

To be clear: doing an instanceof check is slightly more expensive than doing a strcmp check. It's just much cheaper than doing a typeof.

Might not have the same issue, but we should just use the same pattern

We should avoid adding an instanceof check if we don't have a reason to believe we need it.

We can probably just check we have a string and not care too much in this specific case.

Not necessarily the right thing to do here. The point was that it's more expensive to first check that we have a string. We could probably add a devmode warning there if we wanted to. But in the 99% case, that container is going to be a div or similar; people are unlikely to mount into a form component, and if they do, they are further unlikely to give their fields a conflicting name.

The reason it made sense in the cases that I fixed is because we are traversing the DOM and if any of the nodes are forms with this problem, we fatal. In ReactDOMContainerInfo.js, we aren't traversing (so lower probability of being hit) and we aren't likely to be mounting into a form node that suffers from having it's attributes expando'd away, so the value add is asymptotically close to zero.

[facebook-github-bot]

@jimfb updated the pull request.

[zpao]

👍

[gaearon]

Aren’t instanceof checks with window going to give false negatives in iframes?

[gaearon]

Here’s a repro case for when the new version reports false for inputs inside iframes whereas the original version reported true: https://jsfiddle.net/h0L5vLkw/1/

[facebook-github-bot]

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired.

Before we can review or merge your code, we need you to email cla@fb.com with your details so we can update your status.

[aweary]

@gaearon #7936 is meant to address selection handling with iframes. If instanceof would break with iframes, would it make more sense to get #7936 in then address the issue with those changes merged?

[sebmarkbage]

Changes requested above.

[nhunzaker]

@jimfb Are you game for making the requested changes? Otherwise, I'm happy to pick this up.

[nhunzaker]

Closing this out, rebased and upstreamed in #10076. Thanks @jimfb for all of your hard work!