New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mouseenter fires on disabled inputs whereas mouseleave does not #4251

Open
jquense opened this Issue Jun 29, 2015 · 30 comments

Comments

Projects
None yet
@jquense
Copy link
Collaborator

jquense commented Jun 29, 2015

There is an asymmetry to EnterLeave event plugin. Since mouseenter is created from the relativeTarget of the mouseout event it fires even though the target is disabled. Since the mouseleave is the inverse, i.e requires that the disabled element fire a mouseout, it doesn't fire a mouseleave for the disabled element.

I am pretty sure the correct behavior here is that neither event should fire if its target is disabled, since this mirrors mouseout. No idea if none-chrome browsers have the same behavior for which mouse events fire on disabled elements.

Additional caveat I just realized, React is probably also not firing mousenter events in the case where the mouse leaves a disabled element into a non disabled element

@jquense

This comment has been minimized.

Copy link
Collaborator Author

jquense commented Jun 29, 2015

So here is an initial attempt at a fix but I can't figure out how to properly use EventPropagators here. Is there a way I am missing to listen for child events (i.e mouseout/over) but also return an event that doesn't bubble itself?

the below only listens for mouseout/over on the element that has the callback attached :/

var EventConstants = require("./EventConstants");
var EventPropagators = require("./EventPropagators");
var SyntheticMouseEvent = require("./SyntheticMouseEvent");
var containsNode = require("./containsNode");

var ReactMount = require("./ReactMount");
var keyOf = require("./keyOf");

var topLevelTypes = EventConstants.topLevelTypes;
var getFirstReactDOM = ReactMount.getFirstReactDOM;

var eventTypes = {
  mouseEnter: {

    registrationName: keyOf({ onMouseEnter: null }),
    dependencies: [topLevelTypes.topMouseOut, topLevelTypes.topMouseOver]
  },
  mouseLeave: {
    registrationName: keyOf({ onMouseLeave: null }),
    dependencies: [topLevelTypes.topMouseOut, topLevelTypes.topMouseOver]
  }
};

var extractedEvents = [null, null];

var EnterLeaveEventPlugin = {

  eventTypes: eventTypes,

  /**
   * For almost every interaction we care about, there will be both a top-level
   * `mouseover` and `mouseout` event that occurs. Only use `mouseout` so that
   * we do not extract duplicate events. However, moving the mouse into the
   * browser from outside will not fire a `mouseout` event. In this case, we use
   * the `mouseover` top-level event.
   *
   * @param {string} topLevelType Record from `EventConstants`.
   * @param {DOMEventTarget} topLevelTarget The listening component root node.
   * @param {string} topLevelTargetID ID of `topLevelTarget`.
   * @param {object} nativeEvent Native browser event.
   * @return {*} An accumulation of synthetic events.
   * @see {EventPluginHub.extractEvents}
   */
  extractEvents: function (topLevelType, topLevelTarget, topLevelTargetID, nativeEvent) {

    if (topLevelType !== topLevelTypes.topMouseOut && topLevelType !== topLevelTypes.topMouseOver) {
      // Must not be a mouse in or mouse out - ignoring.
      return null;
    }

    var win;
    if (topLevelTarget.window === topLevelTarget) {
      // `topLevelTarget` is probably a window object.
      win = topLevelTarget;
    } else {
      // TODO: Figure out why `ownerDocument` is sometimes undefined in IE8.
      var doc = topLevelTarget.ownerDocument;
      if (doc) {
        win = doc.defaultView || doc.parentWindow;
      } else {
        win = window;
      }
    }

    var eventType;

    var target = getFirstReactDOM(nativeEvent.target) || win;
    var related = getFirstReactDOM(nativeEvent.relatedTarget || nativeEvent.toElement);

    //console.log('hii!!')
    if (!related || related !== target && !containsNode(target, related)) {

      related = related || win;

      if (topLevelType === topLevelTypes.topMouseOut) {
        eventType = 'mouseLeave';
      } else {
        eventType = 'mouseEnter';
      }

      var event = SyntheticMouseEvent.getPooled(eventTypes[eventType], topLevelTargetID, nativeEvent);

      event.type = eventType.toLowerCase();
      //event.target = target;
      event.relatedTarget = related;

      // this isn't right!~!!
      EventPropagators.accumulateDirectDispatches(event);

      return event;
    }

    return null;
  }

};

module.exports = EnterLeaveEventPlugin;
@jimfb

This comment has been minimized.

Copy link
Contributor

jimfb commented Jul 6, 2015

@rogchap

This comment has been minimized.

Copy link

rogchap commented Aug 18, 2015

I am pretty sure the correct behavior here is that neither event should fire if its target is disabled.

An example of when you would want mouseleave to fire on a disabled element is if you disabled the button onclick.

Think of a payment button: mouseenter is called changing the hover state {hover:true} then onclick sets state to disabled {disabled:true}, meanwhile, user moves mouse away from button and the button state is changed back to enabled {disabled:false}. The button now is in a state that is incorrect as it currently has the state {hover:true}.

@jquense

This comment has been minimized.

Copy link
Collaborator Author

jquense commented Dec 17, 2015

Ping @jimfb @sebmarkbage @syranide

Got bit by this again, I'd be happy to PR something, but the current mouseleave/enter code seems really deeply integrated (has its own event propagator?). My current stumbling block is just the below:

Is there a way I am missing to listen for child events (i.e mouseout/over) but also return an event that doesn't bubble itself?

The problem is that I need to emit an event that does not fire when the dependent DOM events (out/leave) fire. tests seem using other examples seem either to emit for every child event, or not respond to bubbled child events at all.

@jimfb

This comment has been minimized.

Copy link
Contributor

jimfb commented Dec 17, 2015

adding @spicyj.

@syranide and @spicyj: either of you know of a good example where we normalize in this way? Can either of you provide some tips here?

@sophiebits

This comment has been minimized.

Copy link
Collaborator

sophiebits commented Dec 18, 2015

No, I'm not sure. I would be inclined to think that both mouseenter and mouseleave should fire. It's a little weird to me that click doesn't but I can kind of justify that one in my mind.

@jquense

This comment has been minimized.

Copy link
Collaborator Author

jquense commented Dec 18, 2015

@spicyj I tend to agree with you, the problem though is that both mouseout and mouseover do not fire on disabled elements. I am not sure why...all spec stuff I've seen suggests that just click events shouldn't, but browsers seem to go with "all mouse events". In that context it might more consistent to also not allow mouse enter/leave as well. But to be honest its all a moot discussion unless there is an implementation that doesn't rely on either mouseout or mouseover, which may be possible but not common it seems.

@andykog

This comment has been minimized.

Copy link

andykog commented Dec 29, 2015

There is also a problem for elements, containing disabled element.
onMouseEnter works, onMouseLeave doesn't.

<div onMouseEnter={e => console.log("ok")}
     onMouseLeave={e => alert("doesn't work")}
>
    <button disabled={true} style={{ width: "100%" }}>Test</button>
</div>

Native mouseleave event works as expected in the same situation.

@chicoxyzzy

This comment has been minimized.

Copy link
Contributor

chicoxyzzy commented Mar 7, 2016

A have same issue. I expect to get onMouseLeave event on disabled button but in doesn't work

@chicoxyzzy

This comment has been minimized.

Copy link
Contributor

chicoxyzzy commented Mar 7, 2016

More on this: it works proper in Firefox but doesn't work in Chrome and Safari

@attilaaronnagy

This comment has been minimized.

Copy link

attilaaronnagy commented May 23, 2016

After react 15, onClick on disabled elements does not fire. That makes me a ton of trouble. Should I open an other issue?

@jquense

This comment has been minimized.

Copy link
Collaborator Author

jquense commented May 23, 2016

@attilaaronnagy fairly certain it's not a React issue, but a browser one.

@attilaaronnagy

This comment has been minimized.

Copy link

attilaaronnagy commented May 23, 2016

I'm using the latest chrome on the latest mac and with react 0.14.4 this worked perfectly... So I don't think so. Just create an <input type="text" onClick={this.whatever} disabled />, and the onClick won't fire.

@sophiebits

This comment has been minimized.

Copy link
Collaborator

sophiebits commented May 23, 2016

@attilaaronnagy I believe this is expected and matches the HTML/DOM spec. @gaearon Did we miss this in the changelog?

@attilaaronnagy

This comment has been minimized.

Copy link

attilaaronnagy commented May 23, 2016

@spicyj and if I need an input, select etc. that is disabled, but has an onClick event what can I do now? (we made a graphical html editor, and I would like to select the components with onClick, but when the edit mode active I would like to disable the component so I can't 'use' it can just 'select' it)

@attilaaronnagy

This comment has been minimized.

Copy link

attilaaronnagy commented May 31, 2016

@spicyj even if I try this:

this.refs.textbox.addEventListener( 'click', clickEvent, true ); <input ref="textbox" />

it's still not working. I don't know what kind of magic you have guys cooked up, but when it's disabled, you are blocking every possible thing no matter whatever I do. I'm thinking about puting a global click event listener on the body and backward calculating which one of the components has been clicked from the coordinates, but that is absurd... Common...

@sophiebits

This comment has been minimized.

Copy link
Collaborator

sophiebits commented May 31, 2016

@attilaaronnagy Browsers don't support click events on inputs. We don't do anything that would affect how addEventListener works.

@attilaaronnagy

This comment has been minimized.

Copy link

attilaaronnagy commented May 31, 2016

@spicyj if the element is not disabled: this code works and the "normal" onClick is also working on an input.

I just tried with the same browser in a jsbin, with the older react (0.13) and it's working with the disabled input also. So if the only difference is the react version number... but whatever I gave up.

@sophiebits

This comment has been minimized.

Copy link
Collaborator

sophiebits commented May 31, 2016

@attilaaronnagy This jsbin using React 0.13.3 doesn't work for me:

http://react.jsbin.com/qesulefepu/edit?html,js,output

Let me know if you're seeing otherwise.

@attilaaronnagy

This comment has been minimized.

@attilaaronnagy

This comment has been minimized.

Copy link

attilaaronnagy commented May 31, 2016

@sophiebits

This comment has been minimized.

Copy link
Collaborator

sophiebits commented May 31, 2016

https://jsfiddle.net/qfLzkz5x/ doesn't have a disabled input; if you disable it then you see the behavior I posted. That is the browser's doing, not React's.

If you want to capture the onClick event on a disabled input, you have to put a wrapper node around it (or listen to it at the top level, if you prefer). This matches the standard DOM behavior. I personally think this behavior is surprising and not desirable, but we find it valuable to match the DOM spec for this so we're planning to leave it this way.

@CoryDanielson

This comment has been minimized.

Copy link

CoryDanielson commented Jul 27, 2016

Any updates on this onMouseLeave issue? I'm having the same issue with almost identical code to what @andykog posted.

https://jsfiddle.net/qfLzkz5x/1/

As a workaround, I've updated my component to watch for the native mouseleave event on the parent of the disabled element, which seems to work, but fires more than expected. My workaround is using code similar to this:

https://jsfiddle.net/qfLzkz5x/8/

Edit: Same code with the events bound directly to the disabled button. The native mouseleave event does not work in this case, either. (on Chrome and FF)

https://jsfiddle.net/qfLzkz5x/6/

@andykog

This comment has been minimized.

Copy link

andykog commented Dec 14, 2016

There is also a css workaroud:

button[disabled] { pointer-events: none; }

Updated @CoryDanielson's fiddle: https://jsfiddle.net/Sl1v3r/sLsut3cy/

@A11oW

This comment has been minimized.

Copy link

A11oW commented Feb 17, 2017

aduth added a commit to WordPress/gutenberg that referenced this issue Aug 21, 2017

Components: Use mouseenter for tooltip toggle
Mouse events behave unreliably in React for disabled elements, firing on mouseenter but not mouseleave. Further, the default behavior for disabled elements in some browsers is to ignore mouse events. Don't bother trying to to handle them.

See: facebook/react#4251
@everdimension

This comment has been minimized.

Copy link
Contributor

everdimension commented Oct 4, 2017

Any updates?

There still seems to be an inconsistency with the browser behavior.

Here's the demonstration which shows the differences between adding the mouseenter / mouseleave listeners with react and with plain js:
https://jsfiddle.net/everdimension/Lkgapb3t/

In chrome (61.0.3163.100):
React triggers a mouseenter event on the disabled button when it shouldn't.
React doesn't trigger a mouseleave event on the parent node when it should.

In safari (11.0):
React triggers a mouseenter event on the disabled button when it shouldn't.

In firefox (55.0.3):
behavior is consistent with the expected browser behavior

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Oct 4, 2017

If there were updates they would be on this issue 😉
Would you like to look into why this happens?

@everdimension

This comment has been minimized.

Copy link
Contributor

everdimension commented Oct 4, 2017

Yeah, well, finding out why this happens is not the hardest part.

Basically react doesn't listen to mouseleave / mouseenter events at all (probably because they do not bubble) and instead listens only to the mouseout event (and in one special case to the mouseover, too) on the document.
And uses the reverse logic (the element you moved "out" from is sent a synthetic mouseleave event and the element you're entering (e.relatedTarget) is sent a synthetic mouseenter event).

Mouseout event is typically fired when you move the cursor from any element to any element (or out of the viewport).

It also fires when you move the cursor into a disabled button, this butting becoming the e.relatedTarget of the native event. That's why react consistently sends the mouseenter event when you enter a disabled button, even if the browser doesn't consider it a mouseenter.

But when you leave the disabled button, no browser except firefox sends a mouseout event. So react doesn't send it and we're left with this asymmetry.

The next subtle difference is that if you wrap a disabled button with another element, chrome still doesn't fire any mouseout events, but safari does fire a mouseout event for the wrapper element.


possible solutions?

  1. When we move the cursor away from disabled button, all browsers fire a mouseover event. The disabled button becomes the e.relatedTarget again in this case, and again using the "reverse" logic we can consistently send the appropriate mouseleave event. Therefore obtaining consistent (and probably expected) behavior in all browsers.

    React already uses the mouseover event for one special case — when mouse enters the viewport. One more special case for disabled form elements will be the least invasive solution.

    But it has the following cons:

    • (–) You have detect the user agent and do this only where needed to avoid sending duplicate events.
    • (–) You have to keep a list of node elements which have this problem when they are disabled. In chrome, for example, disabled textarea doesn't suffer from this problem and according to mdn, some input types don't, either.
    • (–) In chrome (and probably IE), we will still miss the mouseleave event on a parent element if it has the same size as the button, because chrome doesn't send a mouseover event for it.
  2. We should attach mouseenter / mouseleave listeners to the node directly. I don't know if this is possible with current Event System, but I think I remember there were talks about this concerning the scroll events, because delegating scroll events to the document can sometimes actually be worse for performance.
    I personally think this solution would be the best one — it would directly reflect native browser behavior.


In any case, this is basically a bug and produces inconsistency with the browser behavior.
May be at least let's decide which path to take in fixing it?

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Oct 4, 2017

Thanks for the analysis! I don't have enough context on this but this will be helpful to the next person who visits. I'll try to compile a list of issues we need to fix, and this will be on it.

@JasonBerry

This comment has been minimized.

Copy link

JasonBerry commented Jun 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment