Use the virtual target for change events to avoid restoring controlled state on the real target

[sebmarkbage]

Fixes #10420

The problem is that we try to restore the controlled value after the oninput event. So we've restored it to its previous value by the time we fire onChange. So the controlling component doesn't have a chance to update it.

This ensures that we "restore controlled state" on the wrapper instead of the real target - which is a noop.

[nhunzaker]

🙈. I wonder who did this.

[sebmarkbage]

I think this could be related to: 0b220d0

@jquense Do you have an idea why that would be? Input events no longer fire onChange on <select />?

[sebmarkbage]

@jquense This fix seems to fix it but I'm not sure if that breaks something else. 2348742

The fixture test seems to work, but the fixture selector itself seems broken. :) So it does break something.

[jquense]

arg this is was beyond weird. yeah I originally had #10238 do this but ran it the issue you are seeing on the fixture select (#10238 (comment))

I am honestly not sure why it's happening. The value on the input ends up being empty by the time the onInput runs even tho it was correct for the onChange (or maybe its the other way around). I didn't spend to much time trying to figure it out since it seemed fine to keep the old behavior.

Let me poke at it...

[jquense]

Maybe the problem is we don't have corresponding setup calls in https://github.com/sebmarkbage/react/blob/2348742f0baccca745b2f76a2720706a56eafce1/src/renderers/dom/fiber/ReactDOMFiberComponent.js#L544 ?

[sebmarkbage]

So one issue is that targetInst here can be the surrounding <div /> instead of the <select />. https://github.com/sebmarkbage/react/blob/2348742f0baccca745b2f76a2720706a56eafce1/src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js#L140

That's because extractEvents gets called first for the inner subtree, and then later for the outer subtree. In the inner subtree, the deepest target instance is the div.

[jquense]

With both events it only seems to break in the case where you change locations onChange. What I see is:

• onInput fires with the correct value on event.target (the select)
• Header.handleFixtureChange fires and sets window.location
• onChange fires with the previous value on event.target (still the select, (WAT?))

At no point though does the select.value descriptor setter get called, so something isn't inadvertently resetting the value in React. it really seems like like the event is firing with the wrong value. This also happens in FF though so it is maybe not some undefined behavior

[sebmarkbage]

I think the issue that makes the behavior different in the nested case is that this returns the wrong thing and a change event gets fired on the surrounding "div". https://github.com/facebook/react/blob/master/src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js#L172

Because targetNode is the "div" and then it tries to do a "restore". But the "restore" works against the native event target so it restores the value.

[sebmarkbage]

This ensures that they're consistent for the nested vs not case: 43e4619

However, this only works because the happen to ignore the restore event properly. The fact that this worked in the normal case seems super fragile to me. Is there another nesting where this breaks? So I'm not super happy with this solution. :/

[sebmarkbage]

I think I'm going to merge this because this solves this particular issue.

I think we've discovered other issues here. If there is a change listener, higher up in the tree, then it will get invoked as a "change" event. I don't know what kind of consequences that will have.

[jquense]

That's because extractEvents gets called first for the inner subtree, and then later for the outer subtree. In the inner subtree, the deepest target instance is the div.

ah that is tricky :/ is the underlying problem that the additional change event fires? I was seeing extractEvents called for the select first, then the div. The extractEvents shouldn't fire at all for the div right?

[sebmarkbage]

If someone uses dispatchEvent to dispatch a "change" event on something arbitrary that the browser wouldn't. Such as a custom-element. Should that be picked up by a React onChange listener above it? ¯\_(ツ)_/¯

[sebmarkbage]

I think the issue behavior change is that this is now always true. https://github.com/sebmarkbage/react/blob/2348742f0baccca745b2f76a2720706a56eafce1/src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js#L165

It used to be that we sometimes would fall into the case where this was false for unknown targets.

[sebmarkbage]

Hm. I'm going to change my patch a bit. I don't think we want to pass the virtual target as the event target in the event object. We don't do that elsewhere.

With regard to when to fire this. I think we probably don't want to bubble the event on any blur which I think it what will happen now, right?

[sebmarkbage]

There's something off about the way we do bubbling and how we create custom events. We extract events once per subtree so that we can do the bubbling within that subtree. That means that we have to recreate the same event for each subtree. Ideally using the nativeEventTarget as the basis for all decisions so it's consistent. However, we can't do that right now because the change event plugin's extractEvents function is not idempotent because updateValueIfChanged changes the value when observed.

I'm tempted to leave it semi-broken as is now so we don't break it more and then revisit semantics in 17.

The workaround to get sane semantics is to use Portals for now.