onClick firing for disabled buttons in v15.4.0

[RoccoC]

With the latest 15.4.0 ReactJS release, onClick is now firing for disabled button elements which have child HTML elements:

https://jsfiddle.net/rcataldo/69z2wepo/62589/

In the previous ReactJS version, the onClick handler would not be called.

Note that onClick is not called if I remove the <span> from the button.

[gaearon]

cc @spicyj @nhunzaker

[nhunzaker]

Is the target element the span or button? I can take a deeper look later tonight, but my first hunch is that we need to look at the way SimpleEventPlugin filters out disabled elements.

SimpleEventPlugin and bails out if the target element is disabled, but I don't know that we account for cases where the target element lives inside of a disabled element.

<button disabled>
    <span onClick />    <------ Click!
</button>

So if the target is the span, I wonder if it's skipping past our disabled element filter, leading to a two phased dispatch (bubbling and capturing) that bubbles up from the span.

The result would be an event dispatch on both the button and the span.

Again, purely speculative, but maybe it can help out @spicyj until I can look at it in a few hours.

[RoccoC]

The target is the button element:

<button disabled onClick={() => { alert("Click!"); }}>
  <span>My Button</span>
</button>

The same but without the span works as expected and does not dispatch the event on the button:

<button disabled onClick={() => { alert("Click!"); }}>
  My Button
</button>

[nhunzaker]

@RoccoC 👍 React manipulates the target field as it bubbles events up the component tree. I wonder if we can performantly crawl up the tree to see if an element is disabled.

[RoccoC]

I see the function shouldPreventMouseEvent in SimpleEventPlugin which seems to handle whether a mouse event should be prevented based on component instance's tag and disabled prop, but the SimpleEventPlugin's extractEvents function is passing the event target (in this case the span) to shouldPreventMouseEvent, not the actual dispatch target to which the listener is assigned (e.g. the button).

Adding this to the end of extractEvents fixes this, but there is probably a cleaner way:

/* SimpleEventPlugin.js */
// extractEvents function snip
if (shouldPreventMouseEvent(event._dispatchInstances)) {
  return null;
}
return event;

[aweary]

traverseTwoPhase ends up calling accumulateDirectionalDispatches for every element in the target element's parent tree, eventually finding the disabled button's listener thanks to listenerAtPhase. That listener is then later executed.

At no point does any step check to see if the instance for the listener is disabled. Maybe we can do that early on in accumulateDirectionalDispatches?

var listener = listenerAtPhase(inst, event, phase);
  if (listener && shouldDispatchListener(inst, event, phase)) {
    // only redefine `_dispatchInstances` and _dispatchListener` if it should actually execute
  }

[aweary]

Adding this to the end of extractEvents fixes this, but there is probably a cleaner way:

@RoccoC one issue there is _dispatchInstances and _dispatchListeners can be arrays, and there may be cases where some listeners should be dispatched and some shouldn't. So we'd have to introduce more logic to shouldPreventMouseEvent and make it polymorphic as a result, which isn't ideal.

I think it would be better if we could prevent the listener from being registered with the event in the first place.

[nhunzaker]

I think it would be better if we could prevent the listener from being registered with the event in the first place.

@aweary aye. Happy to dig into this unless someone has already taken it. Here's a test case:

nhunzaker/react@master...nhunzaker:disabled-button-children

[nhunzaker]

Following @aweary's thoughts, I first looked into avoiding the subscription all together. However, it turns out, there are some additional things to figure out there.

When React updates a component, it adds/removes event listeners based on whether or not they have changed. This is problematic for disabling events. Changing the disabled prop is a separate operation from attaching/removing listeners. If we prevent disabled elements from attaching mouse events, we need to make sure they attached if an element stops being disabled.

The simplest solution does appear to be just making SimpleEventPlugin smarter by modifying shouldPreventMouseEvent to be:

function shouldPreventMouseEvent(inst) {
  if (inst) {
    var focus = inst;

    while (focus) {
      if (focus._currentElement && focus._currentElement.props.disabled) {
        return isInteractive(focus._tag);
      }

      focus = focus._hostParent;
    }
  }

  return false;
}

Basically bubble up the tree and see if the child is within a disabled parent. If so, return true/false if that element is interactive or not.

---

Side note: it looks like there is a substantial difference in the event implementation between master and 15.x. We might need to implement this different in both versions, but I think the solution could be the same in principle.

[nhunzaker]

I sent out this PR for discussion:
#8329

[zargold]

Not sure if this was closed or not but I'm still experiencing the issue: (will check different versions):
Codepen Example. Specifically, although I used button I guess:
// .babelrc

{
  "presets": ["es2015", "react", "stage-1"],
}

causes my <button></button> to render as an <a></a> and as that are disabled even in 15.4.2 still fire onClick even if disabled.

[monkindey]

Hi, @zargold, I saw your sample, just the a tag trigger but the a tag have not the disabled property. Element a for more reading.

[zargold]

Yes but in my ES6 code I do not have an <a></a> I have:

        <button
          className='button' disabled={true}
          onClick={(e) => addNewStory(e, 'greenStories')}
        ><span className='fa fa-plus-circle'></span>Add Story to Top
        </button>

but when rendered it turns into:

<a disabled>
<span className='fa fa-plus-circle'></span>Add Story to Top
</a>

Which did not work

[monkindey]

Sorry, I can not follow you. I saw your ES6 code in the Codepen

<a disabled={this.state.shouldDisable} onClick={(e) => funcCheck(e)}>
    <span>Stuff in button</span>
</a>

it's obviously have the A tag in the React render function.

Did you put the wrong code in the Codepen ?

[zargold]

Ok thanks You were correct monkindey. This was actually an issue my babel... and after reinstalling babel and switching from <a> to <button> it worked fully so yes this could definitely be considered closed. Thank you for your help.

[gaearon]

This has since been fixed.

[ClemDelp]

@gaearon It looks like the bug is back since react@16.0.0

[sophiebits]

If you have a case where this doesn't work properly, please open a new issue.

[bryanltobing]

not sure if this is related.

In react when we set button props disabled, the onClick event will not fire.
When the user enables the button from devtool/inspect-element by removing the disabled attribute from html. the onClick still will not fire.
I have tried this and that's work and this is my expected behavior. because I want to prevent users from clicking by role access to the button.

I'm just still not certain that this is correct.