Address issues where ID of nodeName causes internal errors in React

[nhunzaker]

This is a continuation of #6311. More or less, using nodeName as an ID on an input causes references to the nodeName prototype property to be overriden.

I've rebased @jimfb's original PR and added some test coverage. I've also browser tested this against IE11, Firefox, Safari, and Chrome.

I would like to test down to IE9, however it looks like I need the Set polyfill on our DOM fixtures. Is there a preferred method of adding the Set polyfill?

[sebmarkbage]

This doesn't work when these nodes are rendered into another iframe. It's not a super common use case but a bigger discussion if we should stop supporting that.

[nhunzaker]

Woah:

iframe.contentWindow.window.HTMLInputElement instanceof HTMLInputElement // false

Today I learned....

[nhunzaker]

Seems like we could at least get some test coverage for this. I'll dig into that too. Though, if we don't have any coverage for it presently, I wonder if it's just a can of worms in JSDOM.

[nhunzaker]

How much slower would it be to do something like:

getInstanceForNode(elem)._tag === 'input'

That gets us around working with the DOM.

[sebmarkbage]

Let's chat about the possibility of dropping multiple iframes support. I think the instanceof check can probably often be faster when available.

I'm not as concerned about that use case as I am about supporting React itself being in a different realm than the one being rendered into. That's still trivially supported even with instanceof checks.

[nhunzaker]

Cool, I'd be interested in knowing more about that use case. Time to dig into realms.

For discussion's sake, another option might be to perform a check like:

function elementIsType (element: HTMLElement, type: string): boolean {
  return  element instanceof element.ownerDocument.defaultView[type]
}

Which is interesting, but it doesn't really get around the underling problem: the id attribute can arbitrarily screw up any specific DOM API access. This just recreates the same issue with ownerDocument instead.

I wonder if this is only isolated to forms and window.

[sebmarkbage]

This only fixes the problem for nodeName, but as you noted, this happens for any property name. Including appendChild. If someone uses the appendChild name, we can't add new children to the form neither. So this general problem still goes unsolved.

I think in theory we could extract these methods from the prototype chain. Like this:

var nodeNameGetter = Object.getOwnPropertyDescriptor(
  Node.prototype,
  'nodeName'
).get;
console.log(nodeNameGetter.call(element));

I'm not sure it's worth the performance overhead and file size to deal with this kind of defensive coding.

[sebmarkbage]

Another proposal: Throw if any of the known names are used as a form name?

Warning would be nice but I'm also concerned about the potential attack surface of dynamic values.

[nhunzaker]

I think the nodeName getter will work. Seems fast too:
https://esbench.com/bench/598b95c499634800a0348d6e

I've almost got this setup, I just hit a snag because the original test case doesn't hit the code path with the new ChangeEvent updates. Be back shortly :).

[nhunzaker]

@sebmarkbage this is the only downside I see to calling the nodeName getter. Occasionally this value is the window. That's because the target instance falls back to the window when it can't find a target instance:

https://github.com/facebook/react/blob/master/src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js#L149

I can't help but wonder if this could be document.body instead of window.

[nhunzaker]

A good example of this is if you throw a breakpoint in this function and click anywhere on the page other than the target input.

[nhunzaker]

Sorry. To clarify: calling nodeNameGetter on window raises an exception.

[aweary]

The reason this is even an issue in the first place is because elements with IDs are registered as properties on window, so when shouldUseChangeEvent is called with window we get a DOM element instead of a string for nodeName

Why don't we just type check the nodeName property? If its not a string, we just return false

[nhunzaker]

@aweary That might work. The only other element that exhibits this behavior, that I can find, is inputs within forms:

https://codepen.io/nhunzaker/pen/qXjOGy?editors=1111

The toLowerCase call is what makes this break because nodeName returns a Node instead of a string. Maybe we could just get away with directly accessing nodeName, which would fail because typeof Node !== 'string' or even just Node !== 'SELECT' && ....

Beyond that, if we access nodeName directly, are there any other edge cases that producing a nodeName getter protects us from?

[aweary]

Specifically, we need to know if there's any situation where this can occur on a select or input element, since those are the only elements where this check might return true. If there isn't, returning false for non-strings should be safe.

[gaearon]

Stale.