Don't cache purged nodes that are focused on unmount

[pletcher]

Fixes #2988.

Unmounting was causing the blur event to fire, which was in turn causing the blurred node to be added to ReactMount.nodeCache.

[sophiebits]

Why does adding a transaction prevent the blur event from firing?

[pletcher]

From ReactReconcileTransaction:

/**
 * Suppresses events (blur/focus) that could be inadvertently dispatched due to
 * high level DOM manipulations (like temporarily removing a text input from the
 * DOM).
 */
var EVENT_SUPPRESSION = {
  /**
   * @return {boolean} The enabled status of `ReactBrowserEventEmitter` before
   * the reconciliation.
   */
  initialize: function() {
    var currentlyEnabled = ReactBrowserEventEmitter.isEnabled();
    ReactBrowserEventEmitter.setEnabled(false);
    return currentlyEnabled;
  },

  /**
   * @param {boolean} previouslyEnabled Enabled status of
   *   `ReactBrowserEventEmitter` before the reconciliation occured. `close`
   *   restores the previous value.
   */
  close: function(previouslyEnabled) {
    ReactBrowserEventEmitter.setEnabled(previouslyEnabled);
  }
};

It's not that it prevents the blur event from firing -- that's a bad description on my part -- it just suspends React's listening to it.

I'll fix that comment, my bad.

[syranide]

Not really familiar with this part of React, but it seems to me that it would make more sense to just use ReactBrowserEventEmitter.setEnabled directly, which is all we want really, rather than rely on transactions which could have other side-effects/overheads down the line.

cc @zpao ... ?

[pletcher]

@syranide, that was my impulse too; I went with the full-blown transaction because it didn't look like ReactBrowserEventEmitter.setEnabled was being used on its own outside of ReactReconcileTransaction.EVENT_SUPPRESSION and because the whole shebang was used for mounting (although I can see where mounting and unmounting aren't inverses of each other, especially after hunting down this bug).

The "blur-on-destroy" thing feels like a special case, though, so if you all think it'd be all right, I'm happy to swap in

var previouslyEnabled = ReactBrowserEventEmitter.isEnabled();
ReactBrowserEventEmitter.setEnabled(false);
ReactMount.unmountComponentFromNode(component, container);
ReactBrowserEventEmitter.setEnabled(previouslyEnabled);

which also seems to work.

I'd also love to get a test added for this, but I couldn't see a clean way to get at nodeCache. I guess we could spy on ReactBrowserEventEmitter.{isEnabled | setEnabled}, but that feels sort of secondary to the actual issue.

[pletcher]

Just keeping this PR up-to-date with the latest changes. It looks like unmounting nodes are still being cached on blur:

http://quick.as/q6mlu7yxq

And fixed with the changes proposed:

http://quick.as/yb0lizo3x

[sophiebits]

I guess this is related to #3790. I'm not in love with this PR because it feels like a bit of a monkey patch to me instead of fixing the true issue that unmounted components can receive events.

[pletcher]

That's fair. On the other hand it feels like a tradeoff between fighting the browser and ignoring what the browser is trying to tell a component while it's unmounting.

In this case, the blur event is expected to fire with the input as its target -- that's cool for the DOM but not so much for React. The component is still mounted when it receives the blur event (since the blur fires as a result of it unmounting) and gets added back to the cache. If I'm not missing something (which is definitely possible), this while loop:

// ReactMount.js#331-333
while (container.lastChild) {
  container.removeChild(container.lastChild);
}

is removing (in the test case) a div containing an input. The browser fires a blur when it removes the input, which gets dispatched and triggers ReactMount.getID for the still-mounted input, which adds the node to the cache before it's finally unmounted (but the reference to it remains in the cache).

I'll keep looking for a better way to prevent React from hitting the unmounting node with the event. I know you all are busy, and I don't mean to bug you with monkey patches. :)

[sebmarkbage]

Is this fixed now? I know we tried to avoid firing events on unmounted components. Regardless we need the full fix.

[sophiebits]

#4983 should fix the rest.