Do not extract mouse events for children of disabled parents

[nhunzaker]

Potential fix for #8308. React (my fault) only sifts out mouse events for disabled elements that are the direct target of dispatch. If an element is the child of a disabled parent, the event incorrectly dispatches because SimpleEventPlugin doesn't see a disabled prop. For example:

<button onClick={...} disabled>
    <span />    <------ Click!
</button>

Incorrectly fires the onClick handler

Original test case (From that issue) here:
https://jsfiddle.net/rcataldo/69z2wepo/62589/

[sophiebits]

Thanks for sending this in. I think this makes sense though we will need to change it for fiber soon. If this cherry-picks cleanly from master, can you send the PR to master instead and then we'll pick it over? Travis is also failing but I'm not sure why. I restarted it.

cc @sebmarkbage who might want it written a different way

[nhunzaker]

@spicyj I just added one more test to be sure we also cover the case when a child is the element with the mouse handler prop.

Master's event listening is definitely different, but I think we may be able to use the same technique. I'll give it a shot.

[sophiebits]

I don't think it's valid to nest interactive elements so we should be able to bail out and stop climbing once we encounter an element where isInteractive returns true.

[sebmarkbage]

Ideally we should do this during traverseTwoPhase. We're already walking this. Conceptually events traverse from the root and down to the target and back up. Any time a parent should be able to intercept.

We could make traverseTwoPhase return a boolean and if it returns false we abort the capture phase and return back up the bubble from that point on.

[aweary]

If an element is nested within an interactive element that is disabled, and has its own event handler, it looks like it still triggers the event handler.

https://jsfiddle.net/outdxpb9/

<button disabled>
  <span onclick="window.alert('clicked')">
    Click me
  </span>
</button>

Correct me if I'm wrong, but won't this change prevent the click handler on the span from being triggered? I think this is behavior we should likely support since some people use it to register click events on disabled inputs.

Ideally we should do this during traverseTwoPhase. We're already walking this. Conceptually events traverse from the root and down to the target and back up. Any time a parent should be able to intercept.

@sebmarkbage I think that's close to what I suggested doing here, right?

[nhunzaker]

@aweary ah got it. Very interesting. I'll update the second test to match that.

[nhunzaker]

There's an extra wrench thrown in here: fieldset. Interactive elements inherit the disabled prop from fieldsets, but el.disabled doesn't actually report true:

http://codepen.io/nhunzaker/pen/XNpVJv

<fieldset disabled onclick="alert('fieldset')">
  <label onclick="alert('label')">Clickable Label</label>
  <button onclick="alert('button')">
    Click Me
  </button>
</fieldset>

In the example above, the label will tricker a click event, but the button and fieldset will not. I think the rules here are:

1. Clicks that originate on disabled interactive elements are nullified
2. Clicks targeting a non-interactive element within a disabled interactive element bubble/capture, but do not include disabled elements
3. Interactive elements that live inside of another interactive element inherit their disabled state

[nhunzaker]

Okay. I've added this logic to traverseTwoPhase:

1. Events originating from a disabled interactive element are ignored outright
2. Interactive elements within the two-phase traversal that are disabled, or within a disabled interactive element, are removed from the list of nodes targetable for event dispatch.

I think I'm on the right track, and all tests pass, but I don't think this is a valid solution yet. I have a couple of outstanding questions:

1. Is traverseTwoPhase the right place to put DOM specific behavior? It seems like the new ReactTreeTraversal is used across more than just the DOM.
2. This has been generalized to all events, which is probably bad (I think). I'm unclear if disabled interactive elements actually prevent dispatch of event types other than mouse events. What is the best way to get access to the event type within tree traversal?

[nhunzaker]

Neat! I was able to fix the Fiber test errors (though I don't understand Fiber very well, and it probably needs 3 shrewed 👀).

I noticed that SimulateNative was double-dispatching events, so I fixed that too (which I could PR against master if you want). Basically getting the parent instance was fixed in ReactTreeTraversal but it never made it to ReactEventListener.

So cool! Even closer to Fiber being ready!

[sebmarkbage]

This is not sufficient because we can use ReactDOM and ReactDOMFiber at the same time when the flag is not on.

[nhunzaker]

Hmm. ReactTreeTraversal uses this check:

https://github.com/facebook/react/blob/master/src/renderers/shared/shared/ReactTreeTraversal.js#L20

  if (typeof inst.tag === 'number') {

Is that the preferred route? Would it be worth making that a more established convention?

[nhunzaker]

Sorry... tunnel vision. There's also the condition above it:

https://github.com/facebook/react/blob/master/src/renderers/shared/shared/ReactTreeTraversal.js#L17

  if (inst._hostParent !== undefined) {
    return inst._hostParent;
  }

Which could be subbed out for inst._currentElement in this case.

[mieszko4]

👍 for @nhunzaker

[mattfysh]

Has anyone noticed behaviour differs per browser, e.g. Chrome triggers browser events on children of disabled elements, Firefox does not. Is this pull request more of a cross-browser fix?

[jddxf]

@mattfysh Yes, browsers behave differently.Not sure about the expected behavior in React.Here are my test results without React.

Cases:

1. click on a disabled element
2. click on a descendant of a disabled element

Behaviors:

1. don't dispatch
2. dispatch but skip the disabled element

	IE	Firefox	Chrome	Opera	Edge
C1	B2	B1	B1	B1	B1
C2	B2	B1	B2	B2	B2

Note that the event target in IE is always the disabled element.

[gaearon]

Closing as stale.